Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Jan 26, 2018

The settings for the MonoPackaging build were removed inadvertently by @brettfo, this re-establishes them. These changes have been verified for Mono in http://github.com/fsharp/fsharp.

The build.sh for building on OSX and Linux was toast. This re-establishes that, at least for OSX. Some more work will be needed here to make this correctly build .NET Core on Linux and Mac (if that's even how we want to build things). It's really annoying to have duplication between build.cmd and build.sh but I've removed large chunks of VS-specific stuff from build.sh that we don't need.

CI is also updated to run build.sh debug and release on Linux.

Project("{F2A71F9B-5D33-465A-A702-920D77279786}") = "FSharp.Compiler.UnitTests", "tests\FSharp.Compiler.UnitTests\FSharp.Compiler.UnitTests.fsproj", "{A8D9641A-9170-4CF4-8FE0-6DB8C134E1B5}"
EndProject
Project("{F2A71F9B-5D33-465A-A702-920D77279786}") = "FSharp.Core.UnitTests", "tests\FSharp.Core.UnitTests\FSharp.Core.UnitTests.fsproj", "{88E2D422-6852-46E3-A740-83E391DC7973}"
Project("{F2A71F9B-5D33-465A-A702-920D77279786}") = "FSharp.Core.UnitTests", "tests\FSharp.Core.UnitTests\FSharp.Core.Unittests.fsproj", "{88E2D422-6852-46E3-A740-83E391DC7973}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good grief!!!! we should probably fix this casing by renaming the file on disk, rather than in the projects. No one is ever going to remember to lowercase the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this one was missed when I normalized the file names.

However I've had hell sometimes with git when changing casing of filenames, so for the moment lets leave this. We can fix it later

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It's hard to change casing on windows. It's nearly impossible to get it spread to all devs and not regress by someone.


</PropertyGroup>

<!--
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yes, checking this now

</PropertyGroup>
</Otherwise>
</Choose>
<PropertyGroup>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsyme I'm not clear how the signing settings end up for a proto build with this new factoring, I.e ... when Configuration='proto'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses the default settings used for all compiler binaries - delay signed on windows, test.snk signed on Mono

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings for non-Mono are in FSharpSource.Settings.targets. I'm not sure why, they were there before

  <PropertyGroup>
    <SkipSigning>false</SkipSigning>
    <UseOpenSourceSign>true</UseOpenSourceSign>
    <SignAssembly>true</SignAssembly>
    <AssemblyOriginatorKeyFile>$(FSharpSourcesRoot)\fsharp\msft.pubkey</AssemblyOriginatorKeyFile>
    <StrongNames>true</StrongNames>
    <DelaySign>true</DelaySign>
  </PropertyGroup>

packages.config Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KevinRansom These are needed when building the MonoPackaging: for better or worse the fsharp/fsharp build must be able to build using only packages from nuget.org.

The Microsoft.VisualFSharp.Msbuild.15.0 is a private package but I don't think we actually need rely on it even for the DevDiv build

If we could avoid using any private packages for the compiler/build tools themselves that would b much appreciated.

@dsyme
Copy link
Contributor Author

dsyme commented Jan 26, 2018

@KevinRansom Still working on this, will check final details later tonight

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be handled by the <InternalsVisibleTo /> elements in the project file.

Copy link
Contributor Author

@dsyme dsyme Jan 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brettfo That stuff doesn't work in the Mono build. Or perhaps it's using the wrong key? I only just noticed that you hardwire the key here:

      <_PublicKey>002400000480000094000000060200000024000052534131000400000100010007D1FA57C4AED9F0A32E84AA0FAEFD0DE9E8FD6AEC8F87FB03766C834C99921EB23BE79AD9D5DCC1DD9AD236132102900B723CF980957FC4E177108FC607774F29E8320E92EA05ECE4E821C0A5EFE8F1645C4C0C93C1AB99285D622CAA652C1DFAD63D745D6F2DE5F17E5EAF0FC4963D261C8A12436518206DC093344D5AD293</_PublicKey>

I'll check if making that conditional does the trick.

Copy link
Member

@brettfo brettfo Jan 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key can be overridden via this like so:

<ItemGroup Condition="not mono">
  <InternalsVisibleTo Include="fsc" /><!-- will use the hard-coded key -->
</ItemGroup>
<ItemGroup Condition="mono">
  <InternalsVisibleTo Include="fsc" Key="1234" />
</ItemGroup>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, will try that, thanks

@brettfo
Copy link
Member

brettfo commented Jan 26, 2018

@dsyme Before you merge this I'd like to do a full internal build to make sure we don't regress any assembly signing. I'll try to keep an eye on this and as soon as it's green I'll start the signed build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK these certificates are only needed on older versions of Mono. At least on OSX

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was preventing the project files from compiling on Mono

@dsyme
Copy link
Contributor Author

dsyme commented Jan 27, 2018

@brettfo Any reason not to use this for the global setting in FSharpSource.targets?

    <PropertyGroup Condition="'$(MonoPackaging)' != 'true'">
      <_PublicKey>002400000480000094000000060200000024000052534131000400000100010007D1FA57C4AED9F0A32E84AA0FAEFD0DE9E8FD6AEC8F87FB03766C834C99921EB23BE79AD9D5DCC1DD9AD236132102900B723CF980957FC4E177108FC607774F29E8320E92EA05ECE4E821C0A5EFE8F1645C4C0C93C1AB99285D622CAA652C1DFAD63D745D6F2DE5F17E5EAF0FC4963D261C8A12436518206DC093344D5AD293</_PublicKey>
    </PropertyGroup>
    <PropertyGroup Condition="'$(MonoPackaging)' == 'true'">
      <_PublicKey>002400000480000094000000060200000024000052534131000400000100010077d32e043d184cf8cebf177201ec6fad091581a3a639a0534f1c4ebb3ab847a6b6636990224a04cf4bd1aec51ecec44cf0c8922eb5bb2ee65ec3fb9baa87e141042c96ce414f98af33508c7e24dab5b068aa802f6693881537ee0efcb5d3f1c9aaf8215ac42e92ba9a5a02574d6890d07464cb2f338b043b1c4ffe98efe069ee</_PublicKey>
    </PropertyGroup>

@brettfo
Copy link
Member

brettfo commented Jan 27, 2018

That seems good to me, and will reduce the number of <InternalsVisibleTo /> elements.

@dsyme
Copy link
Contributor Author

dsyme commented Jan 27, 2018

This is ready to merge, subject to your checks @brettfo

@dsyme Before you merge this I'd like to do a full internal build to make sure we don't regress any assembly signing. I'll try to keep an eye on this and as soon as it's green I'll start the signed build.

I've done a careful check of the conditions listed. The logic is actually simpler now, especially for the Debug/Release build case for the regular build, where the default settings here https://github.com/Microsoft/visualfsharp/pull/4273/files#diff-88901e0c749aca7d233ea1ae8a4e5275R112 are used since none of the other overriding cases (Proto, MonoPackaging, StrongNames=false) apply.

@brettfo
Copy link
Member

brettfo commented Jan 27, 2018

Internal build started. I'll reply back as soon as it's done.

@brettfo
Copy link
Member

brettfo commented Jan 27, 2018

@dsyme Internal build is green, merge whenever you're ready.

@dsyme dsyme merged commit bca446d into dotnet:master Jan 29, 2018
@dsyme
Copy link
Contributor Author

dsyme commented Jan 29, 2018

@brettfo Thanks, done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants