Skip to content
This repository has been archived by the owner on Nov 1, 2020. It is now read-only.

Update the repo to support VS2019 and to use C# 8.0 #7015

Merged
merged 9 commits into from
Feb 15, 2019
Merged

Update the repo to support VS2019 and to use C# 8.0 #7015

merged 9 commits into from
Feb 15, 2019

Conversation

tannergooding
Copy link
Member

CC. @jkotas

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.

Awesome. Thanks a lot!

Hope the CI won't hit problems with this ... this looks too easy to be true.

@tannergooding
Copy link
Member Author

Looks like jobs are failing with Unhandled Exception: System.IO.FileNotFoundException: Could not load file or assembly 'System.Runtime, Version=4.2.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified.

@tannergooding
Copy link
Member Author

tannergooding commented Feb 13, 2019

Failure looks to be because of BuildTools update also updated xunit.console, which depends on System.Runtime, Version=4.2.0.0.

Still trying to determine why it is failing now....

@tannergooding
Copy link
Member Author

Looks like RunTests.cmd changed from:

call %RUNTIME_PATH%\dotnet.exe xunit.console.dll ILCompiler.Compiler.Tests.dll  -xml testResults.xml -notrait category=nontests -notrait category=nonwindowstests  -notrait category=OuterLoop -notrait category=failing

to:

call xunit.console.dll ILCompiler.Compiler.Tests.dll  -xml testResults.xml -notrait category=nontests -notrait category=nonwindowstests  -notrait category=OuterLoop -notrait category=failing

@tannergooding
Copy link
Member Author

CC. @ViktorHofer who made most of the relevant changes to BuildTools here: dotnet/buildtools#2168.

What is the appropriate fix here? It looks like we are hitting: https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/tests.targets#L82

@ViktorHofer
Copy link
Member

ViktorHofer commented Feb 13, 2019

@tannergooding We don't tests.targets anymore in corefx. I don't think anyone besides coreclr is using buildtools/master. You can apply whatever fix works for you. Do you need help?

@tannergooding
Copy link
Member Author

Do you need help?

No, I think a workaround should be trivial enough. Looks like this might require another update to BuildTools, however (I didn't see any hooks that would allow me to fix this just locally).

@tannergooding
Copy link
Member Author

Actually, @ViktorHofer. It looks like just setting BuildingNETCoreAppVertical would do the trick. Would there be any concerns (or negative impact) from doing so?

It looks like it is only consumed from within the BuildTools tests.targets file.

@ViktorHofer
Copy link
Member

Yeah I had the same discussion with @trylek some months ago who tried to use the tests.targets in corert. Yes setting BuildingNETCoreAppVertical should do it as you tell the script that you want to use the .NET Core test profile.

@trylek
Copy link
Member

trylek commented Feb 13, 2019

FWIW, this is my PR from the end of last year where I tried to enable CoreRT build under VS2019:

#6617

I however didn't manage to make it fully work before I had to switch over to other work.

dir.props Outdated
<BuildingNETCoreAppVertical>true</BuildingNETCoreAppVertical>
<IncludeVSTestReferences>false</IncludeVSTestReferences>
<CopyConfigurationFiles>false</CopyConfigurationFiles>
<RuntimeConfigPath>$(PackagesDir)/xunit.runner.console/$(XUnitPackageVersion)/tools/netcoreapp2.0/xunit.console.runtimeconfig.json</RuntimeConfigPath>
Copy link
Member Author

Choose a reason for hiding this comment

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

These four (BuildingNETCoreAppVertical, IncludeVSTestReferences, CopyConfigurationFiles, and RuntimeConfigPath) are working around the test.targets from BuildTools.

It ensures that:

  • RunTests.cmd is generated to use dotnet.exe xunit.console.dll
  • xunit.console.runtimeconfig.json stays targeting netcoreapp2.0 (rather than v9.9.9)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sorry for all that stuff which isn't really needed outside of corefx. I guess at some point corert should either migrate away from tests.targets or clean it up...

@jkotas jkotas closed this Feb 13, 2019
@jkotas jkotas reopened this Feb 13, 2019
@tannergooding
Copy link
Member Author

No changes, just modified the timestamp on the last commit to try and retrigger the jobs. There were none actually pending in CI.

@tannergooding
Copy link
Member Author

Looks like some of the projects tried to use the desktop compiler and caused the "Requires net472" dialog box to show.

@tannergooding
Copy link
Member Author

tannergooding commented Feb 15, 2019

The CoreCLR tests jobs are failing because tests.targets is using xunit.runner.msbuild, which is Desktop only. However, LangVersion=8 requires net472 on Desktop.

I did try setting up just the CoreCLR tests to skip the toolset compiler and use the Desktop msbuild. That, however, fails with:

D:\j\w\corert\tests\CoreCLR\runtest\tests.targets(42,5): error : System.ArgumentNullException: Value cannot be null. [D:\j\w\corert\tests\CoreCLR\runtest\runtest.proj]
D:\j\w\corert\tests\CoreCLR\runtest\tests.targets(42,5): error : Parameter name: type [D:\j\w\corert\tests\CoreCLR\runtest\runtest.proj]

@tannergooding
Copy link
Member Author

Looks like the failures are because BuildTools significantly rewrote how the test logic works (and most of it is specific to CoreFX).

I'm trying a workaround that keeps the BuildTools version the same but adds a step to our own init-tools.cmd (the one that calls the build-tools init-tools.cmd) that restores the needed Toolset Compiler versions.

@@ -268,6 +270,14 @@
<IlasmToolPath Condition="'$(OSEnvironment)'=='Windows_NT'">%WINDIR%\Microsoft.NET\Framework\v4.0.30319\ilasm.exe</IlasmToolPath>
</PropertyGroup>

<PropertyGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed since we are no longer updating the BuildTools version. It configures the various roslyn toolset properties as appropriate.

@@ -81,6 +81,36 @@ if not [%INIT_TOOLS_ERRORLEVEL%]==[0] (
goto :error
)

:: Restore a custom RoslynToolset since we can't trivially update the BuildTools dependency in CoreRT
echo Configurating RoslynToolset...
Copy link
Member Author

Choose a reason for hiding this comment

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

This is basically a clone of the logic in "%BUILD_TOOLS_PATH%\init-tools.cmd", but restoring the required roslyn toolset version.

Copy link
Member Author

Choose a reason for hiding this comment

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

need to the corresponding logic to init-tools.sh as well...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -1,17 +1,14 @@
<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed because the tests/CoreCLR/runtests/tests.targets file uses xunit.runner.msbuild which is Desktop only.

The roslyn toolset we are restoring requires net472 for the desktop toolset, which is not installed on all the Jenkins build-machines.

@tannergooding
Copy link
Member Author

I believe this should be good now as well (only the Wasm jobs are left).

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 and has a "hack" to get the toolset compiler down, rather than pulling it in via BuildTools.

If people are okay with the above approach, I will log a bug tracking removal of the workaround and actually updating the BuildTools dependency (or maybe just dealing with it in the Arcade transition).

@tannergooding
Copy link
Member Author

Logged #7032 to track removal of the workaround once we can remove the desktop dependency from tests/CoreCLR/runtests/tests.targets.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants