Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Manually update the dependencies.props and move S.P.Corelib to use LangVersion=8.0 #22452

Merged
merged 7 commits into from
Feb 15, 2019
Merged

Manually update the dependencies.props and move S.P.Corelib to use LangVersion=8.0 #22452

merged 7 commits into from
Feb 15, 2019

Conversation

tannergooding
Copy link
Member

The dependencies.props were manually updated since the automatic sync is not currently working. A fix for that and more details on the issue are here: dotnet/versions#419

This simultaneously updates S.P.Corelib to use LangVersion=8.0 since the build-tools update makes that possible.

CC. @jkotas, @benaadams, @stephentoub

@tannergooding
Copy link
Member Author

Still looking into why building S.P.Corelib hangs. I am waiting on a machine to finish imaging so I can try to repro locally.

@tannergooding
Copy link
Member Author

@jkotas:
image

Looks like we need to either upgrade the Jenkins machines or wait until we can move fully onto coreclr-ci

@benaadams
Copy link
Member

Isn't there a .NET Core based compiler?

@tannergooding
Copy link
Member Author

Isn't there a .NET Core based compiler?

Yes. It would require additional work in BuildTools to enable, however.

I'm also not sure how close we are to fully switching onto the Azure DevOps based build-system, so I couldn't say if it is worth the effort.

@tannergooding
Copy link
Member Author

CC. @jashook on the above

@jashook
Copy link

jashook commented Feb 8, 2019

/cc @sbomer

@tannergooding
Copy link
Member Author

Rebased onto current head and moved LangVersion to be centrally managed in the dir.common.props.

@sbomer
Copy link
Member

sbomer commented Feb 12, 2019

If I follow, I think the above error would be fixed by building managed projects using "dotnet msbuild" instead of desktop msbuild on windows. If that's the case, you may be able to get it working by making https://github.com/dotnet/coreclr/blob/c4007d88a0c556182f70d7714e5b55cca92331a0/msbuild.cmd call "dotnet msbuild" using the SDK bootstrapped by buildtools. I believe you'd just need to update the native build on windows to continue using desktop msbuild explicitly.

@tannergooding
Copy link
Member Author

Still validating locally and in CI, but the latest commit adds a dotnet.cmd script (mirroring the existing dotnet.sh script). It then updates all the existing usages of msbuild.cmd except the native and packing sites to use dotnet.cmd msbuild.

@tannergooding
Copy link
Member Author

BuildTools on Windows only restores the Desktop Toolset Compiler. It needs to be updated to also restore the NetCore Toolset Compiler.

@tannergooding tannergooding mentioned this pull request Feb 13, 2019
Merged
@tannergooding
Copy link
Member Author

I put up a PR that updates BuildTools to also restore the .NETCore toolset on Windows: dotnet/buildtools#2222

@jkotas
Copy link
Member

jkotas commented Feb 13, 2019

  • @tannergooding Could you please also take care of updating the buildstools with these changes in CoreRT repo before starting to use any C# 8 features in the shared CoreLib partition?
  • @marek-safar Is it fine with you to start using C# 8 features in shared CoreLib partition?

@marek-safar
Copy link

Is it fine with you to start using C# 8 features in shared CoreLib partition?

Yes

@tannergooding
Copy link
Member Author

I have the CoreRT side here: dotnet/corert#7015
The BuildTools side fixup has been checked in and I am waiting on dotnet/versions#425 to be merged before updating this PR.

tannergooding
tannergooding commented Feb 13, 2019
View reviewed changes
dir.props
@@ -4,6 +4,10 @@

<Import Project="dir.common.props" />

<PropertyGroup>
<LangVersion>8.0</LangVersion>
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to move the LangVersion definition to here and tests\dir.props, since those are the only things that actually import $(RoslynPropsFile).

I think it would be nice to get this cleaned up (and using Directory.Build.props, etc). But that is a more complicated change and should be done separately. Doing so would allow this logic to be centrally managed, rather than duplicated in multiple places.

Copy link
Member

Choose a reason for hiding this comment

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

Right, that would be part of "Arcade migration".

tannergooding
tannergooding commented Feb 13, 2019
View reviewed changes
dir.props
@@ -4,6 +4,11 @@

<Import Project="dir.common.props" />

<PropertyGroup>
<LangVersion>8.0</LangVersion>
<UseSharedCompilation>true</UseSharedCompilation>
Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed a commit setting UseSharedCompilation=true, as we did on CoreFX.

When using a non-toolset compiler this is the default, but for Toolset builds the reverse is true and this is disabled. This should speed up the build and avoid the timeout that was getting hit.

@tannergooding
Copy link
Member Author

Looks like several jobs are hitting a hang here:

16:32:25   StringThrower -> D:\j\workspace\x64_checked_w---eac6a79c\bin\tests\Windows_NT.x64.Checked\baseservices\compilerservices\RuntimeWrappedException\StringThrower\StringThrower.dll

Trying to get a repro machine, but the repro button in Jenkins isn't working. CC. @dotnet/dnceng

@MattGal
Copy link
Member

MattGal commented Feb 14, 2019

@tannergooding we have our best Alex working on this, see https://github.com/dotnet/core-eng/issues/5203 for status.

@tannergooding
Copy link
Member Author

The project causing the stall looks to be an ilproj.

@MattGal
Copy link
Member

MattGal commented Feb 14, 2019

@tannergooding just a theory at the moment but if an ILProj is stalling build it can often be caused by Windows Defender / other AV things mistaking the output of such projects as malware. For the old Jenkins machines though I do not know how they have this configured.

@tannergooding
Copy link
Member Author

Found a couple of callsites that were just calling msbuild directly or via a subroutine in the batch file. Fixed those and did a double-check through all the cmd files in the repo. I managed to get it passing on the repro machine, so hopefully CI will do the same.

@tannergooding
Copy link
Member Author

Had to revert packages.builds to use Desktop msbuild on Windows. This should be fine since it is just packaging.

Some machines would otherwise fail with:

D:\repos\coreclr\Tools\Packaging.targets(1208,5): error : System.MissingMethodException: Method not found: 'Void NuGet.Packaging.Manifest.Save(System.IO.Stream, Boolean)'. [D:\repos\coreclr\src\.nuget\Microsoft.NETCore.Runtime.CoreCLR\Microsoft.NETCore.Runtime.CoreCLR.pkgproj]

@tannergooding
Copy link
Member Author

Looks like this should be good now (only 2 arm jobs and one of the CoreFX jobs remain).

It would be great if people who have already signed-off could give this a second glance, given it required a few more iterations to get correct.

jkotas
jkotas approved these changes Feb 15, 2019
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

stephentoub
stephentoub approved these changes Feb 15, 2019
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks, Tanner.

@tannergooding tannergooding merged commit cd9831c into dotnet:master Feb 15, 2019
@tannergooding
Copy link
Member Author

Just as a note, I believe I've resolved the CoreRT side as well, but we still won't want to use C# 8 features in the shared corelib code until after dotnet/corert#7015 is merged.

@RussKeldorph RussKeldorph mentioned this pull request Feb 16, 2019
Merged
@RussKeldorph RussKeldorph mentioned this pull request Jan 31, 2020
Closed
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@tannergooding
Manually update the dependencies.props and move S.P.Corelib to use La…
dbe4607 
…ngVersion=8.0 (dotnet/coreclr#22452)

* Update BuildTools to preview1-03713-01 (master)

* Updating CoreCLR to use LangVersion=8.0

* Moving the Windows scripts to default to `dotnet msbuild` for managed components

* Setting UseSharedCompilation=true

* Changing some additional callsites that were using msbuild to use dotnet msbuild

* Revert packages.builds to use Desktop msbuild on Windows

* Fixing runtest.cmd to always set DotNetCli


Commit migrated from dotnet/coreclr@cd9831c
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas approved these changes

@stephentoub stephentoub stephentoub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tannergooding @benaadams @jashook @sbomer @jkotas @marek-safar @MattGal @stephentoub