-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Set PlatformTarget to AnyCPU for modern .NET #48599
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
base: main
Are you sure you want to change the base?
Conversation
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets
Outdated
Show resolved
Hide resolved
|
Thanks for starting this! It looks like the tests got removed in one of the more recent commits, though? |
| <!-- Determine PlatformTarget (if not already set) from runtime identifier. --> | ||
| <Choose> | ||
| <!-- For all targets other than .NETFramework, set $(PlatformTarget) to AnyCPU. --> | ||
| <When Condition="'$(PlatformTarget)' == '' and '$(TargetFrameworkIdentifier)' != '.NETFramework' and '$(UseNativeCode)' != 'true'"> |
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.
Is UseNativeCode a pre-existing property or is this a new property introduced by this PR?
If it is a new property introduced to disable the new behavior, it should have a specific name, something like UseSpecificPlatformTargetIfPossible.
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.
It is used in a test project which is failing on .NET Framework, so I added it here temporarily to check if that's the only issue. It turned out it's not the only problem there. Tests are written in a way that diverging behavior is tricky to assert.
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.
Given the latest conversation (#48599 (comment)), should the UseNativeCode condition be deleted here?
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets
Outdated
Show resolved
Hide resolved
Youssef1313
left a comment
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.
Did you verify the impact of this on VSTest?
For modern .NET, we determine which testhost will be copied to output directory based on that.
I think the usage in VSTest is bad, but we need to understand how we will approach handling this breaking change.
It's also passed to VSTestTask here:
https://github.com/microsoft/vstest/blob/dbcd471883202088496ab81d3ec2d3f3be0981b5/src/Microsoft.TestPlatform.Build/Microsoft.TestPlatform.targets#L53
But for the specific usage in this task, I don't know if it's .NET Framework only or if it also affects .NET Core. @nohwnd will be the best contact here.
|
Looks like the effect of this change is that we will copy always testhost.exe (the x64 testhost) to the output, and then when we see that the preference of the user is to run x86 tests, we will search for testhost.86.exe and won't find it, and will fall back to resolving dotnet for x86 architecture, and running x86/dotnet.exe testhost.dll. But who knows what other things we will see fail in the wild, we've seen users checking the process name to be testhost.exe and testhost.x86.exe, which was the reason we keep shipping those exes, but did not add one for arm64 or other architectures. If we go down this path it would be nice to detect that the fix is in place, and never copy the testhost.exe to output when new dotnet SDK is used, to unify how we run tests in the future, to always go through dotnet.exe. |
|
@Youssef1313 / @nohwnd if we target this for .NET 11, would that be a clear enough check for VSTest? If SDK version is at least 11 then don't copy testhost.exe? |
|
@Youssef1313 / @nohwnd Broadly, for SDK-style projects I'd say that |
|
Sounds good to me, I am okay with not shipping testhost.exe at all in the nuget package. At least we get compatible with how we run on non-windows. |
…imeIdentifierInference.targets Co-authored-by: Adeel Mujahid <[email protected]>
…imeIdentifierInference.targets Co-authored-by: Jan Kotas <[email protected]>
|
Rebased this to get a fresh build so we can see the test results - they'd expired since the last time this ran. |
|
Did a quick perusal of the test failures - most of them are explicitly validating that platform-specific binaries were used/provided/selected during the compilation/build/publish. We'll need to go into each of those scenarios in more detail and decide if the test is expressing an actual design constraint, or just pinning the behavior at that point in time and can be safely updated/removed/etc. |
Fixes #42558.
cc @jkotas, @baronfel