sdk icon indicating copy to clipboard operation
sdk copied to clipboard

SDK is incorrectly emitted SupportedOSPlatform("Windows7.0")

Open tannergooding opened this issue 2 years ago • 11 comments

As per the title, several tests are using windows7.0 as part of various TFM checks: https://github.com/search?q=repo%3Adotnet%2Fsdk%20windows7.0&type=code

However, the underlying OSPlatform checks do not operate on "brand names" and there is no such thing as windows7.0. The OSPlatform checks instead operate on the reported version given by EnvironmentOSVersion.Version in which case it is:

  • Windows 7: windows6.1 (full version is 6.1.7601.0)
  • Windows 8: windows6.2 (full version is 6.2.9200.0)
  • Windows 8.1: windows6.3 (full version is 6.3.9600.0)
  • Windows 10: windows10.0 (full version is 10.0.10240.0)
  • ... (various interim versions of Windows 10)
  • Windows 11: windows10.0.22000.0
  • ... (various interim versions of Windows 11)

Additionally, it looks like targeting net8.0-windows emits [SupportedOSPlatform("Windows7.0")] and [TargetPlatform("Windows7.0")] (it may also do this on older TFMs).

For additional context:

  • IsOSVersionAtLeast calls Environment.OSVersion.Version
  • OSVersion populates itself via GetOSVersion()
  • GetOSVersion populates itself via RtlGetVersionEx. The structure definition for OSVERSIONINFOEXW then covers that the major/minor version returned matches the kernel version and that Windows 7 is 6.1

tannergooding avatar Nov 29 '23 16:11 tannergooding

A repro showing the following program:

using System.Runtime.Versioning;

Console.WriteLine($"Environment.OSVersion: {Environment.OSVersion}");

Console.WriteLine("Checking OperatingSystem.IsWindowsVersionAtLeast(7, 0)");

if (OperatingSystem.IsWindowsVersionAtLeast(7, 0))
{
    CheckSucceeded();
}
else
{
    CheckFailed();
}

[SupportedOSPlatform("Windows7.0")]
static void CheckSucceeded() => Console.WriteLine("Check succeeded");

static void CheckFailed() => Console.WriteLine("Check failed");

Gives the following output:

Environment.OSVersion: Microsoft Windows NT 6.1.7601 Service Pack 1
Checking OperatingSystem.IsWindowsVersionAtLeast(7, 0)
Check failed

image

tannergooding avatar Nov 29 '23 20:11 tannergooding

Tagging @buyaa-n @dsplaisted @JonDouglas

I can repro the behavior. Building for net8.0-windows produces a binary with [assembly: SupportedOSPlatform("Windows7.0")] and [assembly: TargetPlatform("Windows7.0")]. I believe we have also hard coded the expansion that net8.0-windows means net8.0-windows7.0 into NuGet as well (@JonDouglas please check me on this).

The intent was to express that Windows 7 is the minimum OS that .NET 5+ runs on. It looks like we never considered that the actual OS version is 6.1, so we should probably fix that.

In terms of version numbers: I don't think we need to map the marketing versions to underlying OS version; the intent for OS checks was that the developer gets the version number they are interested in from the API documentation (or in case of iOS/Android from the generated bindings).

So I think the only thing we need to fix is our expansion of windows to windows7.0 as the Windows 7 version is actually 6.1.

terrajobst avatar Nov 29 '23 23:11 terrajobst

For context the TFMs are like this too image

MichalPetryka avatar Nov 29 '23 23:11 MichalPetryka

Oops. FYI @baronfel

Is this actually worth fixing though? It might be a non-trivial breaking change, and Windows 7 and 8 are no longer supported by .NET.

dsplaisted avatar Nov 29 '23 23:11 dsplaisted

and 8

Server 2012 is still supported and that corresponds to Windows 8.

MichalPetryka avatar Nov 29 '23 23:11 MichalPetryka

.NET 6 does still support Windows 7 as well, and is in support for at least another 12 months, it also has this issue.

I think at the very least we should "fix" this so that, moving forward, -windows emits just [SupportedOSPlatform("windows")]. We shouldn't be prescribing a minimum to what Windows is, as we don't do that for any other platforms (AFAICT). -- Notably the analyzer doesn't treat if (OSPlatform.IsWindows()) as doing a 7.0 check. It treats it as any windows version, so prescribing a minimum in the SDK just makes it inconsistent and can potentially introduce bugs over time.

Likewise, we should probably ensure that any version can be specified and not error. Us not supporting or recognizing something shouldn't mean it should block outright (warning should still be fine if its unexpected or seems incorrect). For example, no one can specify just net8.0-windows10 today, even though that is technically valid from the [SupportedOSPlatform] perspective. Likewise, no one can manually opt to targeting windows6.1 (7), windows6.2 (8), or windows6.3 (8.1) today because we force error. Not erroring at least gives users a manual workaround

Users could be targeting a specialized scenario or a different runtime where this support does exist and is valid. They could even be running the app under a compat context in which case the version reported to the app is different from the actual supported OS it is running on.

We should probably consider whether or not windows7.0 and windows8.0 need special handling given the bug has existed for 3+ years. This impacts WinForms, WPF, arbitrary net*-windows tfms, etc. Users can hit this for roll forward and other compat checks.

tannergooding avatar Nov 29 '23 23:11 tannergooding

Marked it for team triage. Do we have any customers who have been impacted by this? Without customer impact, it'll be hard to justify changing the behavior in servicing for 6 and as Daniel points out, it is technically breaking. Not sure how a customer could be relying on the existing behavior though.

marcpopMSFT avatar Dec 09 '23 00:12 marcpopMSFT

After discussion, this is too risky to change in servicing. Given there haven't been customer complaints, we're also wary about rushing to change it for net9 targeting net9 (we likely wouldn't want too change it for downlevel targeting either) as we don't know what the side-effects of not setting a version or setting it to 6.1 would be.

We're going to hold off for now and see if we get additional customer feedback. If we did change it, it'd probably be better to do 6.1 than nothing as there is potentially some unknown impact to not setting a minimum. Note that customers can set the Supported version outside of the TFM today.

marcpopMSFT avatar Dec 13 '23 21:12 marcpopMSFT

With respect to setting arbitrary supported versions, you can use the SupportedOSPlatformVersion property to control what goes into the SupportedOSPlatform property.

dsplaisted avatar Dec 13 '23 21:12 dsplaisted

I'm running into this issue on windows 7. I am unable to set the Supported OS version in Visual Studio 2022 to below 7.0. For the time being I'm just ignoring the warnings

#pragma warning disable CA1416

lutzklai avatar Jan 08 '24 17:01 lutzklai

This is a bit of a CI blocker for me as I go through and update our projects that target windows to net 8. Doesn't seem like NUnit has a fix for the default OS targeting output being Windows 7.0, so I'm not able to update the package. It would be preferable to not have to use workarounds to get things working.

I'd recommend updating the OS support docs if there are indeed no plans to fix this.

pyrabt avatar Feb 14 '24 22:02 pyrabt

This is a bit of a CI blocker for m

Why is this a CI blocker?

terrajobst avatar Mar 12 '24 21:03 terrajobst