Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use proto build on mac and linux #6380

Merged
merged 29 commits into from
Jun 4, 2019
Merged

use proto build on mac and linux #6380

merged 29 commits into from
Jun 4, 2019

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Mar 27, 2019

The build.sh was not correctly using a proto build on macOS and Linux.

This means PRs like #6325 were failing because the FSharp.Core.UnitTests must be built with an updated compiler.

The commit has been cherry picked into #6325 - if the mac and linux builds there start to pass then it shows that it's worked

@dsyme
Copy link
Contributor Author

dsyme commented Mar 28, 2019

Although this is green it is not yet working, the proto compiler is still not being used and #6325 is still failing. Working on it.

.vsts-pr.yaml Outdated Show resolved Hide resolved
eng/build.sh Outdated Show resolved Hide resolved
eng/build.sh Outdated Show resolved Hide resolved
Copy link
Member

@brettfo brettfo left a comment

Choose a reason for hiding this comment

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

See above comments.

eng/build.sh Outdated Show resolved Hide resolved
@@ -20,8 +20,4 @@
<DisableCompilerRedirection>true</DisableCompilerRedirection>
</PropertyGroup>

<!-- SDK targets override -->
<PropertyGroup>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe this is needed because DisableCompilerRedirection is true

src/fsharp/ExtensionTyping.fs Outdated Show resolved Hide resolved
@dsyme
Copy link
Contributor Author

dsyme commented Mar 29, 2019

@brettfo I've been having a hell of a time with this one. I think it's ok now, I just managed to complete a build.

(I'm only working on it because it blocks my other work, and a lot of my F# vNext PRs are failing because we don't use the proto build on Linux/macOS and they require it.)

@dsyme
Copy link
Contributor Author

dsyme commented Mar 29, 2019

Cool, well, things going better now, at least on Linux and macOS

@brettfo the problem was MSBuild node reuse when different MSBuild executions load different FSharp.Build.dll. I disabled node reuse in build.sh as a result (it's disabled for CI anyway)

@dsyme
Copy link
Contributor Author

dsyme commented Mar 29, 2019

@KevinRansom @brettfo OK, fixed another problem. I had a one character error fcs instead of fsc here

I have to say, the .NET Core execution of tests under tests\fsharp is fantastically, brutally complicated. It's a complete mishmash on top of the existing mishmash. Among other things we do this

  • compile the F# test project which then
  • creates a project file dynamically in temporary directory
  • uses baroque MSBuild XML to refer to Directory.Build.targets under visualfsharp\tests
  • implicitly load targets and properties further up the tree
  • loads a build DLL whose location is specified by very hidden MSBuild specifications
    and so on and so on.

I had to stare at MSBuild /v:diag output for ages to spot the one character error

After this experience I'm really tempted to suggest we junk all of the build infrastructure under tests\fsharp for .NET Core and start again with a set of .NET Core projects that exercise the different test.fsx and so on. It's just impossibly complicated as it stands.

@dsyme
Copy link
Contributor Author

dsyme commented Mar 30, 2019

@brettfo THis is finally ready. I reverted things to contain only the changes needed for bootstrapping

eng/Build.ps1 Show resolved Hide resolved
eng/build.sh Show resolved Hide resolved
@KevinRansom
Copy link
Member

@brettfo, is this still needed?

@dsyme
Copy link
Contributor Author

dsyme commented Apr 8, 2019

I'm pretty certain this is still needed

@dsyme
Copy link
Contributor Author

dsyme commented Apr 8, 2019

The one remaining question about how to get this "node_reuse" thing specified correctly.

@dsyme
Copy link
Contributor Author

dsyme commented May 20, 2019

@brettfo Could you re-review and approve this? I think I updated it as needed removing the verbosity flag changes. Among other things the lack of bootstrap on Linux and OSX is now hitting #6634

1 similar comment
@dsyme
Copy link
Contributor Author

dsyme commented May 20, 2019

@brettfo Could you re-review and approve this? I think I updated it as needed removing the verbosity flag changes. Among other things the lack of bootstrap on Linux and OSX is now hitting #6634

@dsyme
Copy link
Contributor Author

dsyme commented May 22, 2019

@brettfo ping :)

@brettfo
Copy link
Member

brettfo commented May 23, 2019

@dsyme I've been messing a bit with our build, can you either rebase or merge master in again just to make sure this has everything? Otherwise I'm satisfied.

@KevinRansom KevinRansom reopened this May 31, 2019
@dsyme dsyme closed this May 31, 2019
@dsyme dsyme reopened this May 31, 2019
@dsyme
Copy link
Contributor Author

dsyme commented Jun 3, 2019

Error in SourceBuild_Linux

/home/vsts/work/1/s/.packages/microsoft.sourcelink.common/1.0.0-beta2-18618-05/build/InitializeSourceControlInformation.targets(7,81): error MSB4022: The result "" of evaluating the value "$(_MicrosoftSourceLinkCommonAssemblyFile)" of the "AssemblyFile" attribute in element <UsingTask> is not valid. [/home/vsts/work/1/s/src/fsharp/fsi/fsi.fsproj]

@dsyme
Copy link
Contributor Author

dsyme commented Jun 3, 2019

Dodgy test:

Tests.LanguageService.QuickInfo.UsingProjectSystem.TypeProvider.XmlDocAttribute.Type.Comment

@dsyme
Copy link
Contributor Author

dsyme commented Jun 3, 2019

@brettfo I believe you still need to mark this as approved

@KevinRansom I can't see how the CI failures are related to the PR, though I suppose they might be - is CI running clean for us reliably?

@cartermp
Copy link
Contributor

cartermp commented Jun 3, 2019

@dsyme I get some issues with timeouts on downloading packages from time to time, but I don't have any reason to suspect CI not running cleanly as per my own recent PRs.

@dsyme dsyme mentioned this pull request Jun 3, 2019
22 tasks
@dsyme
Copy link
Contributor Author

dsyme commented Jun 3, 2019

@brettfo I don't understand the failure in SourceBuild_Linux - this is some new thing

/home/vsts/work/1/s/.packages/microsoft.sourcelink.common/1.0.0-beta2-18618-05/build/InitializeSourceControlInformation.targets(7,81): error MSB4022: The result "" of evaluating the value "$(_MicrosoftSourceLinkCommonAssemblyFile)" of the "AssemblyFile" attribute in element <UsingTask> is not valid. [/home/vsts/work/1/s/src/fsharp/fsi/fsi.fsproj]

It's something to do with the order of loading properties etc. for the Microsoft.SourceLink.Common package, but the process by which that is loaded is entirely obscure to me.

I can't work out why this would

  1. only affect the source build
  2. not affect fsc.fsproj

@dsyme dsyme dismissed brettfo’s stale review June 4, 2019 16:15

changes addressed, see #6380 (comment)

@dsyme
Copy link
Contributor Author

dsyme commented Jun 4, 2019

OK this is green, I'll merge this now.

@brettfo I resolved your review since the changes you wanted have been addressed

@dsyme dsyme merged commit c96481f into dotnet:master Jun 4, 2019
KevinRansom added a commit to KevinRansom/fsharp that referenced this pull request Jun 9, 2019
This reverts commit c96481f, reversing
changes made to 14be767.
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