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

Update to target .NET5 framework #1471

Merged
merged 18 commits into from
Sep 17, 2020
Merged

Update to target .NET5 framework #1471

merged 18 commits into from
Sep 17, 2020

Conversation

marek-safar
Copy link
Contributor

No description provided.

Directory.Build.props Outdated Show resolved Hide resolved
@dotnet dotnet deleted a comment from marek-safar Sep 10, 2020
…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.
@@ -2,7 +2,7 @@
<PropertyGroup>
<!-- 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>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -235,7 +235,7 @@ public class ILLink : ToolTask

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");
Copy link
Member

Choose a reason for hiding this comment

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

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

@ViktorHofer
Copy link
Member

@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

image

@nohwnd
Copy link
Member

nohwnd commented Sep 10, 2020

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
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Member

nohwnd commented Sep 14, 2020

@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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

nohwnd commented Sep 14, 2020

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

nohwnd commented Sep 15, 2020

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
Copy link
Member

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
Copy link
Member

nohwnd commented Sep 15, 2020

@ViktorHofer do you mean DOTNET_ROOT? You said DOTNET_PATH.

@ViktorHofer
Copy link
Member

Yeah sorry. DOTNET_ROOT.

@ViktorHofer
Copy link
Member

@ViktorHofer
Copy link
Member

@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
Copy link
Member

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

@vitek-karas
Copy link
Member

Thanks a lot @ViktorHofer for figuring this out!

@ViktorHofer
Copy link
Member

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.

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

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

ViktorHofer added a commit to dotnet/arcade that referenced this pull request Sep 16, 2020
* 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
Copy link
Member

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
Copy link
Member

Nearly there. One more Arcade update needed tomorrow :)

@ViktorHofer
Copy link
Member

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

@marek-safar marek-safar merged commit cc5c0f9 into dotnet:master Sep 17, 2020
@marek-safar marek-safar deleted the net5 branch September 17, 2020 08:35
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
Co-authored-by: vitek-karas <vitek.karas@microsoft.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

Commit migrated from dotnet/linker@cc5c0f9
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.

6 participants