Update to target .NET5 framework by marek-safar · Pull Request #1471 · dotnet/linker

marek-safar · 2020-09-04T14:44:37Z

No description provided.

…ints case is duplicated The duplication is not "perfect", the warnings are slightly different (one reports the Activator.CreateInstance<T> method as the source, the other reports the generic parameter T of that method as the source), so we need two attributes in the test.

ViktorHofer · 2020-09-10T13:23:30Z

    <!-- Keep these in sync with _ILLinkTasksTFM in Sdk.props. -->
    <!-- Keep the netcoreapp TFM in sync with the Mono.Linker.csproj condition below. -->
-    <TargetFrameworks>netcoreapp3.0;net472</TargetFrameworks>
+    <TargetFrameworks>net5.0;net472</TargetFrameworks>


I recommend to use an msbuild property for the used .NETCoreApp version to keep changes as small as possible in the future.

Good idea - we should probably do that as a follow-up - the plan is to try to backport this to 5.0 release, so I would like to keep the changes to minimum.

ViktorHofer · 2020-09-10T13:23:55Z

 				var taskDirectory = Path.GetDirectoryName (Assembly.GetExecutingAssembly ().Location);
 				// The linker always runs on .NET Core, even when using desktop MSBuild to host ILLink.Tasks.
-				_illinkPath = Path.Combine (Path.GetDirectoryName (taskDirectory), "netcoreapp3.0", "illink.dll");
+				_illinkPath = Path.Combine (Path.GetDirectoryName (taskDirectory), "net5.0", "illink.dll");


The proposed msbuild property for the used .NETCoreApp version can also flow into this file by supplying the constant in the project file.

ViktorHofer · 2020-09-10T14:06:29Z

@nohwnd I spent some time trying to figure out why VSTest doesn't work when invoked via the VSTestTask, but without success. Can you please take a look? The SDK being used is 5.0 RC1 final. Here's the binlog:
msbuild.zip

nohwnd · 2020-09-10T15:46:43Z

That error is written because we don't write errors the MSBuild way, this PR is setting the appropriate option in MSBuild that suppresses the error: microsoft/vstest#2557, dotnet/msbuild#5701

But the option seems to be implemented incorrectly in MSBuild dotnet/msbuild#5701

marek-safar · 2020-09-10T15:54:17Z

@ViktorHofer I think the msbuild issue is a fallback from vstest not to be able to find the runtime which works fine with dotnet build so it looks to me like there is another issue happening before MSB4181.

ViktorHofer · 2020-09-14T13:43:59Z

@nohwnd any idea what the cause of the actual issue could be? The error in the pipeline suggests that a matching runtime can't be found.

nohwnd · 2020-09-14T15:15:25Z

@ViktorHofer ah sure, you don't have your PATH / DOTNET_ROOT set to the .dotnet directory where dotnet-install.ps1 installs your dotnet SDK, so when dotnet test tries to run it only finds the runtimes installed on the system, and there is no 5.0 runtime. I don't see why exactly, but adding the env variable manually should fix the issue.

ViktorHofer · 2020-09-14T15:18:28Z

@alexperovich seems like Arcade needs to have set DOTNET_ROOT or override the PATH to point to the local .dotnet folder when invoking the VSTest target.

ViktorHofer · 2020-09-14T15:23:13Z

OK this makes sense now. I see that the test project overrides the test target and routes it to the VSTest target: https://github.com/mono/linker/blob/395e24a5ccaf6c976feb4891756ee1f73a4672c9/test/Mono.Linker.Tests/Mono.Linker.Tests.csproj#L77-L81.

nohwnd · 2020-09-14T15:28:45Z

For the PATH. The dotnet-install script should do that automatically, but in old versions the script itself was doing exit 0 on success, so there are examples of using it in separate process, from which the PATH / DOTNET_ROOT won't propagate. Obtain script and install .NET Core CLI one-liner examples has that issue. https://docs.microsoft.com/cs-cz/dotnet/core/tools/dotnet-install-script

This would probably have also the same issue:
https://github.com/dotnet/arcade/blob/4873d157a8f34f8cc7e28b3f9938b32c642ef542/src/Microsoft.DotNet.Arcade.Sdk/tools/InstallDotNetCore.targets#L8

ViktorHofer · 2020-09-14T17:10:28Z

ah sure, you don't have your PATH / DOTNET_ROOT set to the .dotnet directory where dotnet-install.ps1 installs your dotnet SDK, so when dotnet test tries to run it only finds the runtimes installed on the system, and there is no 5.0 runtime. I don't see why exactly, but adding the env variable manually should fix the issue.

@nohwnd looking at the latest binlog, I see that PATH contains the local .dotnet SDK still the testhost fails to find the right SDK/runtime. Still I don't see DOTNET_PAT being set. Should this work with the correct %PATH% or do we need DOTNET_PATH as well?

ViktorHofer · 2020-09-14T17:25:03Z

@nohwnd also I see that the right dotnet invokes the test project:

D:\a\1\s\.dotnet\dotnet.exe test D:\a\1\s\artifacts\bin\ILLink.Tasks.Tests\Release\net5.0\ILLink.Tasks.Tests.dll --logger:"console;verbosity=normal" --logger:"trx;LogFileName=ILLink.Tasks.Tests_net5.0_x64.xml" "--ResultsDirectory:D:\a\1\s\artifacts\TestResults\Release" "--Framework:.NETCoreApp,Version=v5.0" > "D:\a\1\s\artifacts\log\Release\ILLink.Tasks.Tests_net5.0_x64.log" 2>&1

is it expected that PATH or DOTNET_PATH need to be set in such a case?

vitek-karas · 2020-09-15T10:37:49Z

Unless dotent test propagates its location into DOTNET_ROOT the testhost.exe will not find the runtime. Apphost (which testhost.exe is an instance of) does not look at PATH - instead it uses DOTNET_ROOT.

nohwnd · 2020-09-15T11:01:08Z

Ah okay, when running via dotnet test it will set DOTNET_ROOT to the directory of dotnet.exe, that is why testhost is able to start even if PATH is empty.

But this won't happen when dotnet vstest.console.dll is executed.

C:\t\UsePath> C:\d\dotnet.exe test
  Determining projects to restore...
  All projects are up-to-date for restore.
  You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
  UsePath -> C:\t\UsePath\bin\Debug\net5.0\UsePath.dll
Test run for C:\t\UsePath\bin\Debug\net5.0\UsePath.dll (.NETCoreApp,Version=v5.0)
Microsoft (R) Test Execution Command Line Tool Version 16.8.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.

Passed!  - Failed:     0, Passed:     1, Skipped:     0, Total:     1, Duration: 2 ms - UsePath.dll (net5.0)



C:\t\UsePath> C:\d\dotnet.exe "C:\d\sdk\5.0.100-rc.2.20465.1\vstest.console.dll"  C:\t\UsePath\bin\Debug\net5.0\UsePath.dll
Microsoft (R) Test Execution Command Line Tool Version 16.8.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
Testhost process exited with error: A fatal error occurred, the required library hostfxr.dll could not be found.
If this is a self-contained application, that library should exist in [C:\t\UsePath\bin\Debug\net5.0\].
If this is a framework-dependent application, install the runtime in the default location [C:\Program Files\dotnet] or use the DOTNET_ROOT environment variable to specify the runtime location.
. Please check the diagnostic logs for more information.

Test Run Aborted.

So yes what Vitek says, DOTNET_ROOT. needs to be set.

ViktorHofer · 2020-09-15T11:28:38Z

That makes sense now. Thanks to all who help root cause this. The fix likely involves setting DOTNET_PATH in the vstest.targets file in Arcade as it doesn't use dotnet test (IMO it should). Will submit as PR

nohwnd · 2020-09-15T11:32:55Z

@ViktorHofer do you mean DOTNET_ROOT? You said DOTNET_PATH.

ViktorHofer · 2020-09-15T11:41:51Z

Yeah sorry. DOTNET_ROOT.

ViktorHofer · 2020-09-15T14:41:03Z

Wait, Arcade's VSTest implementation is using dotnet test: https://github.com/dotnet/arcade/blob/5a630b52300710b806f22b34c079b2e6031d57c6/src/Microsoft.DotNet.Arcade.Sdk/tools/VSTest.targets#L22.

ViktorHofer · 2020-09-15T14:45:54Z

@nohwnd based on your commend and this code path I assumed that DOTNET_ROOT would be set when using dotnet test. Now I'm confused...

ViktorHofer · 2020-09-15T16:28:04Z

OK, found the reason. This is a bug in the SDK when invoking dotnet test: dotnet/sdk#13611

vitek-karas · 2020-09-15T18:49:36Z

Thanks a lot @ViktorHofer for figuring this out!

ViktorHofer · 2020-09-15T18:51:17Z

I will submit a quick fix in Arcade to unblock this after I submitted the product fix. Hopefully I'll have enough time for both today.

Workaround until tracking issue dotnet/sdk#13611 is fixed. Unblocks dotnet/linker#1471

ViktorHofer · 2020-09-15T21:29:23Z

Here's the product fix dotnet/sdk#13619 and here's the Arcade fix that should unblock this PR: dotnet/arcade#6182.

* Fix VSTest on Windows without globally installed SDK Workaround until tracking issue dotnet/sdk#13611 is fixed. Unblocks dotnet/linker#1471 * Update VSTest.targets

ViktorHofer · 2020-09-16T15:03:03Z

This should be unblocked as soon as a new Arcade.SDK package is produced which is currently blocked by a 16.8 dependency on Arcade. I expect bits to be flowing later today.

ViktorHofer · 2020-09-16T23:29:12Z

Nearly there. One more Arcade update needed tomorrow :)

ViktorHofer · 2020-09-17T08:35:02Z

et voila - this is now unblocked. @marek-safar feel free to merge.

Co-authored-by: vitek-karas <vitek.karas@microsoft.com> Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com> Commit migrated from dotnet/linker@cc5c0f9

marek-safar requested review from radekdoulik and sbomer as code owners September 4, 2020 14:44

marek-safar force-pushed the net5 branch from 83878bb to db2e47b Compare September 4, 2020 14:47

Update to target .NET5 framework

720c4d9

marek-safar force-pushed the net5 branch from db2e47b to 720c4d9 Compare September 4, 2020 14:49

akoeplinger reviewed Sep 4, 2020

View reviewed changes

Comment thread Directory.Build.props Outdated

sbomer reviewed Sep 4, 2020

View reviewed changes

Comment thread src/linker/Mono.Linker.csproj

marek-safar added 2 commits September 4, 2020 17:28

Review feedback

7ec2c57

Bump only to net5

d0a2640

sbomer reviewed Sep 9, 2020

View reviewed changes

Comment thread src/ILLink.Tasks/ILLink.Tasks.csproj

marek-safar added 3 commits September 10, 2020 09:51

Merge remote-tracking branch 'upstream/master' into net5

6054b96

Update sdk properties

d2bbb1b

Update tests to work with net5 sdk

89ebff9

dotnet deleted a comment from marek-safar Sep 10, 2020

ViktorHofer reviewed Sep 10, 2020

View reviewed changes

Fix tests on mono

f301889

ViktorHofer added 2 commits September 14, 2020 17:38

Use VSTest via arcade feature flag

4e99b2f

Merge branch 'net5' of https://github.com/marek-safar/linker into net5

3f763c7

ViktorHofer added a commit to dotnet/arcade that referenced this pull request Sep 15, 2020

Fix VSTest on Windows without globally installed SDK

c7d7e0e

Workaround until tracking issue dotnet/sdk#13611 is fixed. Unblocks dotnet/linker#1471

ViktorHofer mentioned this pull request Sep 15, 2020

Fix VSTest on Windows without globally installed SDK dotnet/arcade#6182

Merged

marek-safar added 2 commits September 16, 2020 15:47

Merge remote-tracking branch 'upstream/master' into net5

5ebfc8e

Add net5 TFM folder name

7541efe

ViktorHofer added 4 commits September 16, 2020 20:59

Update Arcade.SDK

74c2f45

Update Version.Details.xml

4bf51e1

Update global.json

b2730e3

Update Version.Details.xml

7a56194

ViktorHofer added 2 commits September 17, 2020 10:15

Update global.json

c0b34d8

Update Version.Details.xml

2704874

vitek-karas approved these changes Sep 17, 2020

View reviewed changes

ViktorHofer approved these changes Sep 17, 2020

View reviewed changes

marek-safar merged commit cc5c0f9 into dotnet:master Sep 17, 2020

marek-safar deleted the net5 branch September 17, 2020 08:35

Uh oh!

Conversation

marek-safar commented Sep 4, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ViktorHofer Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

vitek-karas Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Sep 10, 2020

Uh oh!

nohwnd commented Sep 10, 2020

Uh oh!

marek-safar commented Sep 10, 2020

Uh oh!

ViktorHofer commented Sep 14, 2020

Uh oh!

nohwnd commented Sep 14, 2020

Uh oh!

ViktorHofer commented Sep 14, 2020

Uh oh!

ViktorHofer commented Sep 14, 2020

Uh oh!

nohwnd commented Sep 14, 2020

Uh oh!

ViktorHofer commented Sep 14, 2020

Uh oh!

ViktorHofer commented Sep 14, 2020

Uh oh!

vitek-karas commented Sep 15, 2020

Uh oh!

nohwnd commented Sep 15, 2020

Uh oh!

ViktorHofer commented Sep 15, 2020

Uh oh!

nohwnd commented Sep 15, 2020

Uh oh!

ViktorHofer commented Sep 15, 2020

Uh oh!

ViktorHofer commented Sep 15, 2020

Uh oh!

ViktorHofer commented Sep 15, 2020

Uh oh!

ViktorHofer commented Sep 15, 2020

Uh oh!

vitek-karas commented Sep 15, 2020

Uh oh!

ViktorHofer commented Sep 15, 2020

Uh oh!

ViktorHofer commented Sep 15, 2020

Uh oh!

ViktorHofer commented Sep 16, 2020

Uh oh!

ViktorHofer commented Sep 16, 2020

Uh oh!

ViktorHofer commented Sep 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants