-
Notifications
You must be signed in to change notification settings - Fork 347
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
Remove TargetFrameworkMonikers.Mono and replace with runtime detection #4799
Conversation
The Mono TFM is a leftover from some earlier plans and doesn't apply anymore with .NET 5. It just creates confusion so remove it and replace with runtime detection.
@safern @ViktorHofer ping |
} | ||
|
||
if (DiscovererHelpers.TestPlatformApplies(platforms)) | ||
if (DiscovererHelpers.TestPlatformApplies(platforms) && DiscovererHelpers.TestRuntimeApplies(runtimes)) |
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.
If no TFM is provided and the runtime applies, this will return the “activeissue” trait and I don’t believe we ignore that trait in all our scripts. Could we update the last yield return to return failing instead?
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.
What is the activeissue
trait for actually? We never catch that. Should we instead just always return failing if none else applies?
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.
Actually since we default frameworks to 0 it would return failing, but I would instead always return failing as @ViktorHofer suggested.
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.
Never-mind, we just need to remove the whole yield return at the end that returns the activeissue trait.
@@ -11,7 +11,6 @@ public enum TargetFrameworkMonikers | |||
{ | |||
Netcoreapp = 0x1, | |||
NetFramework = 0x2, | |||
Uap = 0x4, | |||
Mono = 0x8 | |||
Uap = 0x4 |
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.
We could also remove Uap.
@akoeplinger I also removed uap in your PR. I hope you don't mind :) |
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.
You missed to remove UAP from the TFMs enum 😄
* Update dependencies from https://github.com/dotnet/arcade build 20200210.11 - Microsoft.DotNet.XUnitExtensions - 5.0.0-beta.20110.11 - Microsoft.DotNet.VersionTools.Tasks - 5.0.0-beta.20110.11 - Microsoft.DotNet.ApiCompat - 5.0.0-beta.20110.11 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20110.11 - Microsoft.DotNet.Build.Tasks.Configuration - 5.0.0-beta.20110.11 - Microsoft.DotNet.Build.Tasks.Feed - 5.0.0-beta.20110.11 - Microsoft.DotNet.Build.Tasks.Packaging - 5.0.0-beta.20110.11 - Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk - 5.0.0-beta.20110.11 - Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk - 5.0.0-beta.20110.11 - Microsoft.DotNet.CodeAnalysis - 5.0.0-beta.20110.11 - Microsoft.DotNet.GenAPI - 5.0.0-beta.20110.11 - Microsoft.DotNet.XUnitConsoleRunner - 2.5.1-beta.20110.11 - Microsoft.DotNet.GenFacades - 5.0.0-beta.20110.11 - Microsoft.DotNet.Helix.Sdk - 5.0.0-beta.20110.11 - Microsoft.DotNet.RemoteExecutor - 5.0.0-beta.20110.11 * Update dependencies from https://github.com/dotnet/arcade build 20200211.3 - Microsoft.DotNet.XUnitExtensions - 5.0.0-beta.20111.3 - Microsoft.DotNet.VersionTools.Tasks - 5.0.0-beta.20111.3 - Microsoft.DotNet.ApiCompat - 5.0.0-beta.20111.3 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20111.3 - Microsoft.DotNet.Build.Tasks.Configuration - 5.0.0-beta.20111.3 - Microsoft.DotNet.Build.Tasks.Feed - 5.0.0-beta.20111.3 - Microsoft.DotNet.Build.Tasks.Packaging - 5.0.0-beta.20111.3 - Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk - 5.0.0-beta.20111.3 - Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk - 5.0.0-beta.20111.3 - Microsoft.DotNet.CodeAnalysis - 5.0.0-beta.20111.3 - Microsoft.DotNet.GenAPI - 5.0.0-beta.20111.3 - Microsoft.DotNet.XUnitConsoleRunner - 2.5.1-beta.20111.3 - Microsoft.DotNet.GenFacades - 5.0.0-beta.20111.3 - Microsoft.DotNet.Helix.Sdk - 5.0.0-beta.20111.3 - Microsoft.DotNet.RemoteExecutor - 5.0.0-beta.20111.3 * Update dependencies from https://github.com/dotnet/arcade build 20200211.5 - Microsoft.DotNet.XUnitExtensions - 5.0.0-beta.20111.5 - Microsoft.DotNet.VersionTools.Tasks - 5.0.0-beta.20111.5 - Microsoft.DotNet.ApiCompat - 5.0.0-beta.20111.5 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20111.5 - Microsoft.DotNet.Build.Tasks.Configuration - 5.0.0-beta.20111.5 - Microsoft.DotNet.Build.Tasks.Feed - 5.0.0-beta.20111.5 - Microsoft.DotNet.Build.Tasks.Packaging - 5.0.0-beta.20111.5 - Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk - 5.0.0-beta.20111.5 - Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk - 5.0.0-beta.20111.5 - Microsoft.DotNet.CodeAnalysis - 5.0.0-beta.20111.5 - Microsoft.DotNet.GenAPI - 5.0.0-beta.20111.5 - Microsoft.DotNet.XUnitConsoleRunner - 2.5.1-beta.20111.5 - Microsoft.DotNet.GenFacades - 5.0.0-beta.20111.5 - Microsoft.DotNet.Helix.Sdk - 5.0.0-beta.20111.5 - Microsoft.DotNet.RemoteExecutor - 5.0.0-beta.20111.5 * React to changes from dotnet/arcade#4799 * Downgrade compiler version * Update dependencies from https://github.com/dotnet/arcade build 20200211.11 - Microsoft.DotNet.XUnitExtensions - 5.0.0-beta.20111.11 - Microsoft.DotNet.VersionTools.Tasks - 5.0.0-beta.20111.11 - Microsoft.DotNet.ApiCompat - 5.0.0-beta.20111.11 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20111.11 - Microsoft.DotNet.Build.Tasks.Configuration - 5.0.0-beta.20111.11 - Microsoft.DotNet.Build.Tasks.Feed - 5.0.0-beta.20111.11 - Microsoft.DotNet.Build.Tasks.Packaging - 5.0.0-beta.20111.11 - Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk - 5.0.0-beta.20111.11 - Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk - 5.0.0-beta.20111.11 - Microsoft.DotNet.CodeAnalysis - 5.0.0-beta.20111.11 - Microsoft.DotNet.GenAPI - 5.0.0-beta.20111.11 - Microsoft.DotNet.XUnitConsoleRunner - 2.5.1-beta.20111.11 - Microsoft.DotNet.GenFacades - 5.0.0-beta.20111.11 - Microsoft.DotNet.Helix.Sdk - 5.0.0-beta.20111.11 - Microsoft.DotNet.RemoteExecutor - 5.0.0-beta.20111.11 * Disable serialization test on Mono Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com> Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@akoeplinger, @safern I see this PR was going to omit Mono and its dependencies with runtime detection. What I'm looking for is the reason for taking the UAP out. Is it only because it had zero reference or it was something else like having a better way for its detection? |
Yep, it wasn't used anymore. |
The Mono TFM is a leftover from some earlier plans and doesn't apply anymore with .NET 5.
It just creates confusion so remove it and replace with runtime detection.
Noticed this while looking at dotnet/runtime#31920.