-
Notifications
You must be signed in to change notification settings - Fork 127
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
Conversation
…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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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.
@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: |
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 |
@ViktorHofer I think the msbuild issue is a fallback from vstest not to be able to find the runtime which works fine with |
@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. |
@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. |
@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. |
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. |
For the PATH. The dotnet-install script should do that automatically, but in old versions the script itself was doing This would probably have also the same 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? |
@nohwnd also I see that the right dotnet invokes the test project:
is it expected that PATH or DOTNET_PATH need to be set in such a case? |
Unless |
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, |
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 |
@ViktorHofer do you mean DOTNET_ROOT? You said DOTNET_PATH. |
Yeah sorry. DOTNET_ROOT. |
Wait, Arcade's VSTest implementation is using |
@nohwnd based on your commend and this code path I assumed that DOTNET_ROOT would be set when using |
OK, found the reason. This is a bug in the SDK when invoking |
Thanks a lot @ViktorHofer for figuring this out! |
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
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
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. |
Nearly there. One more Arcade update needed tomorrow :) |
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
No description provided.