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

Remove some version hard-coding in the OSPlatformTranslator #4577

Open
stevenaw opened this issue Dec 3, 2023 · 8 comments
Open

Remove some version hard-coding in the OSPlatformTranslator #4577

stevenaw opened this issue Dec 3, 2023 · 8 comments
Assignees
Milestone

Comments

@stevenaw
Copy link
Member

stevenaw commented Dec 3, 2023

In #4574 and #4569 we added some hard-coded version numbers in the OSPlatformTranslator to fix a few things in NUnit 4.0.1.
It should be alright in the short-term as new major versions of Windows are relatively infrequent, however we should see if we can remove some of the hard-codings to minimize the chance that a new version of Windows will require a new version of the framework to work with it.

We should also ensure that the mapping code can account for any changes in the SDK for how a TFM such as {version}-windows may map to a compiler-generated SupportOSPlatform attribute that may come out of this issue: dotnet/sdk#37220

One option could be to see if we can make use of [Platform("Win")] .

@OsirisTerje OsirisTerje added this to the 4.1 milestone Dec 3, 2023
@OsirisTerje
Copy link
Member

Can we get this in for 4.1 ?

@manfred-brands
Copy link
Member

@stevenaw We cannot simply use Win for all. If the SupportedOSPlatform say Windows11 we should not try to run tests on Windows7 or Windows10.

As with new versions of Windows, when Windows10 eventually comes out we would need to add that, including how to detect this.

There seems no logic in MS version numbers. One only has to look at OS platform where Windows10 is 10.0.22000, nobody knows what Windows12 will identify as.

One option would be to drop the translation and convert the MS attributes in a modified new platform attribute which only checks if the version returned by MS is larger than the specified one. That way we can check minor versions as well and will be future proof.

@stevenaw
Copy link
Member Author

stevenaw commented Dec 7, 2023

Quite right @manfred-brands . The issue title was a bit hastily-worded, but we can't get rid of the hard-coding entirely in OSPlatformTranslator.

I'd been thinking though, that for the common case where NUnit encounters [SupportedOSPlatform("Windows7.0")] that we may be able to translate that to [Platform("Win")] as I don't think it likely NUnit 4 on net462 could be run on an even older OS. The net6 build certainly won't, NET6 is only Windows 10+.

Based on the discussion in the linked SDK issue it also sounds like [SupportedOSPlatform("Windows7.0")] can't be targeted directly in the currently tooling and is then, I think, likely to only be produced when using a TFM of the form {platform}-windows without any other build number info. If we can safely translate some cases to [Platform("Win")] then it also covers us for future Windows OSes

@manfred-brands @OsirisTerje I think I'm connecting the right dots here but I'd welcome a second read of dotnet/sdk#37220 or opinion on if there's any simplification on the NUnit side here.

@manfred-brands
Copy link
Member

manfred-brands commented Dec 7, 2023

@stevenaw I did read that article and they say Windows7 doesn't even work as that operating system identifies itself as 6.1.
Only very specific minor version of Windows10 seem to be allowed

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 documentation2

Hence my suggestion to decode the version numbers and compare that to the running OS version.
I think that will have a longer future than translating to the Platform attribute.
Unlike MS, I can add the exceptions that for major version < 10: 7 => > 6.1, 8 => > 6.2.

If you agree, I can work on that.

@stevenaw
Copy link
Member Author

stevenaw commented Dec 8, 2023

Oh ok thanks for explaining @manfred-brands I may've missed that. That sounds like a better idea if you think it'll work 😀

@OsirisTerje
Copy link
Member

@manfred-brands @stevenaw Should this be moved to e.g. 4.2 ?

@stevenaw
Copy link
Member Author

I'm alright with that.
@manfred-brands I'm going to take the liberty of changing the milestone here but feel free to move it back if you disagree

@stevenaw stevenaw modified the milestones: 4.1, 4.2 Feb 16, 2024
@manfred-brands
Copy link
Member

No problem moving milestone. I actually forgot about the ticket.

@manfred-brands manfred-brands self-assigned this Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants