Skip to content

Normalize TFM platform versions when determining computed compatibility#9741

Merged
advay26 merged 6 commits into
devfrom
advay26-tfm-platformversionbug
Dec 19, 2023
Merged

Normalize TFM platform versions when determining computed compatibility#9741
advay26 merged 6 commits into
devfrom
advay26-tfm-platformversionbug

Conversation

@advay26

@advay26 advay26 commented Nov 29, 2023

Copy link
Copy Markdown
Member

Addresses #9711

Problem:

TFMs that contain platform monikers and versions, like net6.0-android31.0, were not showing the net7.0-android and net8.0-android TFMs as users expected.
https://www.nuget.org/packages/Xamarin.AndroidX.Lifecycle.Common/2.6.2.2#supportedframeworks-body-tab
image

If the TFM didn't have a platform version, say just net6.0-android, then the corresponding net7.0 and net8.0 TFMs show up as expected.
https://www.nuget.org/packages/Microsoft.CognitiveServices.Speech.Extension.ONNX.Runtime/1.33.0#supportedframeworks-body-tab
image

Solution:

Currently, if a TFM (.NET 5+) has a platform version, it does not show any computed compatible TFMs. Our compatibility matrix only includes TFMs without platform versions.

To fix this, if we see a TFM with a platform version:

  1. Create a normalized copy of the TFM, using an empty platform version
    net6.0-windows7.0 -> net6.0-windows
  2. Use this normalized TFM to find computed compatible TFMs
    net6.0-windows -> net6.0-windows, net7.0-windows, net8.0-windows
  3. Add the original TFM with its platform version (net6.0-windows7.0), and remove the normalized TFM (net6.0-windows) from the set
    net6.0-windows7.0, net7.0-windows, net8.0-windows

i.e. net6.0-windows7.0 -> net6.0-windows7.0, net7.0-windows, net8.0-windows

Similarly,

Asset TFM Computed TFMs
net6.0-windowsY.Z net6.0-windowsY.Z, net7.0-windows, net8.0-windows
net6.0-windows net6.0-windows, net7.0-windows, net8.0-windows
net6.0-windows7.0 net6.0-windows7.0, net7.0-windows, net8.0-windows
net6.0-windows99.0 net6.0-windows99.0, net7.0-windows, net8.0-windows
net7.0-windows7.0 net7.0-windows7.0, net8.0-windows
net6.0-ios net6.0-ios, net7.0-ios, net8.0-ios
net6.0-ios15.0 net6.0-ios15.0, net7.0-ios, net8.0-ios

image

@advay26
advay26 requested a review from a team as a code owner November 29, 2023 02:10
@advay26
advay26 requested a review from dannyjdev November 29, 2023 02:11
ryuyu
ryuyu previously approved these changes Nov 29, 2023

@ryuyu ryuyu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if my understanding is correct, this is change is made where it is because it would not be feasible for the CompatibilityMatrix to contain every possible iteration of a platform and version, right? :shipit:

@advay26

advay26 commented Nov 29, 2023

Copy link
Copy Markdown
Member Author

So if my understanding is correct, this is change is made where it is because it would not be feasible for the CompatibilityMatrix to contain every possible iteration of a platform and version, right? :shipit:

Yes, that's my understanding too.

Also, the CompatibilityMatrix uses a list of 'supported frameworks' that we maintain ourselves https://github.com/NuGet/NuGetGallery/blob/main/src/NuGetGallery.Core/Frameworks/SupportedFrameworks.cs#L66, so we would need to manually update this list with every platform and platform version combo in order to cover it that way.

@erdembayar erdembayar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just 2 questions.

Comment thread src/NuGetGallery.Core/Frameworks/FrameworkCompatibilityService.cs Outdated
erdembayar
erdembayar previously approved these changes Nov 29, 2023

@erdembayar erdembayar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 👏

ryuyu
ryuyu previously approved these changes Nov 29, 2023
@joelverhagen

Copy link
Copy Markdown
Member

This seems reasonable. Would it be possible to get a TFM compat expert on client to sign off on the concept here? It makes sense based on my understanding but I was not involved in the design or implementation of net5.0+ platform specific TFMs.

joelverhagen
joelverhagen previously approved these changes Dec 1, 2023
@nkolev92

nkolev92 commented Dec 1, 2023

Copy link
Copy Markdown
Member

If the TFM didn't have a platform version, say just net6.0-android, then the corresponding net7.0 and net8.0 TFMs show up as expected.

In the .NET world with platforms, TargetPlatformVersion is mandatory.
For the NuGetFramework type, no platform version means 0.0 (common pattern throughout), but it's restore and pack that prevent platform version-less packages/projects.

It is unusual that there are packages without a target platform in their package. In fact we have pack validation that should prevent that: https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu1012.

Packages that contain assets like that were probably manually edited, and since restore tries not to be too opinionated and break it can work.

I'm wondering if we should've blocked these packages from getting uploaded to nuget.org, but it's too late now I guess.

Now focusing on the issue is that we want to show net8.0-android as a computed framework for net7.0-android31, I'm not really familiar with what versions are used, and how often people downgrade the default SDK platform version etc.

In general, if the SDK defaults to net7.0-android31, for net8.0 it is likely the platform version is 32.0 or something like.
If someone manually specifies net8.0-android30.0, then it'll be incompatible, but I don't think there'd be as much confusion as those people likely understand the platform version.

The real concern is what is 31.0 wasn't the default, but a bumped version, and net8.0 defaults to a version 30.0, then this is a problem.

I'd involve .NET folks here, as while I can speak on the technical correctness, they're the better authorities on what people would expect.

Given net7.0-android31.0, I think showing net8.0-android31.0 is a good idea as a computed framework, which seems to what this PR is proposing.

The only downside is that eventually certain framework versions and platform versions are incompatible/can't build against, but idk if Nuget.org should try to guess that.

IE, there's a chance that net12.0-android31.0 simply cannot be targeted by a project and nuget.org might show that as a computed framework.

@JonDouglas

Copy link
Copy Markdown
Contributor

@nkolev92 i talked to daniel and immo on this. but while you point out these problems, the android team will never publish a TFM with a lesser value as a TPV. they build against the latest version of android APIs.

@JonDouglas

Copy link
Copy Markdown
Contributor

also i think uno or some other groups do the manual creation b/c they have the net8.0-android TFM:

https://www.nuget.org/packages?q=&tfms=net8.0-android&prerel=true&sortby=relevance

@JonDouglas

Copy link
Copy Markdown
Contributor

I'm wondering if we should've blocked these packages from getting uploaded to nuget.org, but it's too late now I guess.

Should we track this issue to ensure people are using proper tooling that packs TPV?

@JonDouglas

Copy link
Copy Markdown
Contributor

Some .NET responses earlier to help.

The implicit default is encoded in the SDK (or one of the workloads) and can change depending on which version of .NET you're targeting. So ideally the package should include the explicit platform version so that NuGet and other things don't have to try to figure out what the default is

The original design is that they are not supposed to increase if you are targeting the same version of .NET. So net8.0-android would always mean Android 34 for example, though net9.0-android could mean Android 35. However, I'm not sure if the ios/android/maui folks plan to always honor that, they may have reasons to want to change the implicit versions

@nkolev92

nkolev92 commented Dec 1, 2023

Copy link
Copy Markdown
Member

Does that feedback change the proposal in this PR?

Fwiw, while android does that rule, I think windows tends to be different. Do we want to apply the same rules there?

@JonDouglas

JonDouglas commented Dec 1, 2023

Copy link
Copy Markdown
Contributor

My only feedback here is to double check that the default major .NET version platform TPVs are right.

this:
image

i think it is perfectly fine to compute and normalize/simplify them too if we don't have a good way of getting mappings. they can lookup docs or similar.

dotnet/docs#38554

JonDouglas
JonDouglas previously approved these changes Dec 1, 2023

@JonDouglas JonDouglas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added comments for consideration

mariaghiondea
mariaghiondea previously approved these changes Dec 5, 2023
@jpobst

jpobst commented Dec 8, 2023

Copy link
Copy Markdown

Given net7.0-android31.0, I think showing net8.0-android31.0 is a good idea as a computed framework, which seems to what this PR is proposing.

This will not work, at least for Android. There is no such thing as net8.0-android31.0, and attempting to use it results in:

 error NETSDK1181: Error getting pack version: Pack 'Microsoft.Android.Ref.31' was not present in workload manifests.

You also cannot arbitrarily bump the platform version above what we have explicitly supported. For example, net6.0-android34.0 results in:

error NETSDK1181: Error getting pack version: Pack 'Microsoft.Android.Ref.34' was not present in workload manifests.

There are only a handful of valid combinations currently:

  • net6.0-android31.0 (net6.0-android)
  • net6.0-android32.0
  • net6.0-android33.0
  • net7.0-android33.0 (net7.0-android)
  • net8.0-android34.0 (net8.0-android)

I think for Android (and perhaps iOS is the same), you are going to have to know the explicitly valid combinations, and only show TFMs where both the .NET and Platform versions are >= to the package specified versions.

Alternatively, you could probably just ignore the platform version for Android and get "close enough" without maintaining an explicit list.

For example, a net6.0-android31.0 package would show net7.0-android and net8.0-android as computed frameworks. (We encourage users (via project templates and docs examples) to use the aliases that do not specify Android version.)

The Android platform version for each subsequent release of .NET will be >= to the previous .NET.

cc @jonathanpeppers @rolfbjarne

@rolfbjarne

Copy link
Copy Markdown

Note that for iOS, we're planning on supporting multi-targeting, where both of these would be valid (at the same time):

  • net8.0-ios17.0
  • net8.0-ios17.1

The Android platform version for each subsequent release of .NET will be >= to the previous .NET.

Same for iOS.

I think for Android (and perhaps iOS is the same), you are going to have to know the explicitly valid combinations

That's not really feasible for iOS, we ship new versions with updated TPVs all year, depending on what Apple does. I'd really like to avoid a solution where someone would have to (remember to) update NuGet's knowledge all the time about which .NET versions supports which iOS versions.

We'll also change the default TPV even though we're not supposed to according to the spec, because the alternative is worse (customers' projects will stop compiling because Xcode was auto-updated by Apple, and they'd have to add the TPV manually to their project files to use the new Xcode, and we don't want people to use explicit TPV because then they'd have to update the TPV again for the subsequent version of Xcode, and now the whole point of having a default value becomes useless because nobody can use it).

We encourage users (via project templates and docs examples) to use the aliases that do not specify Android version.

I've seen more projects with the TPV than I'd expect, so I've wondered if the IDEs have been writing it out when saving csprojs without customers explicitly choosing it?


I think there are a few alternatives here though.

  1. Given a netX.0-iosY.Z, then all versions of net(X+1).0-ios should support iOS Y.Z, so you don't need the Y.Z for X+1.

    In other words, given net6.0-ios15.4, it can be expanded to list net7.0-ios and net8.0-ios.

  2. The above solution isn't guaranteed (and I don't have a crystal ball), so a safer way to render it would be to somehow express that the version has a minimum:

    • net6.0-ios15.4 ->
      • net7.0-iosX [where X is at least 15.4, but the iOS SDK may require it to be higher]
      • net8.0-iosX [where X is at least 15.4, but the iOS SDK may require it to be higher]

    This is more accurate, but certainly wordier, if not straight out uglier.

@JonDouglas

JonDouglas commented Dec 14, 2023

Copy link
Copy Markdown
Contributor

@rolfbjarne

I've seen more projects with the TPV than I'd expect, so I've wondered if the IDEs have been writing it out when saving csprojs without customers explicitly choosing it?

Yeah I believe this was by design when packing w/ dotnet tooling.

What we will likely do in this case then @advay26 correct if wrong is that we will show:

net6.0-android<TPV> or net6.0-android and compute the normalized simplified compatible frameworks i.e. net7.0-android and net8.0-android. Similar story for iOS here. We will not add TPVs to computed compatible frameworks for now.

That way it is more likely dependent on the .NET SDK you are using at the time given TPVs can change. We can make a note of that in the docs somewhere.

@advay26

advay26 commented Dec 15, 2023

Copy link
Copy Markdown
Member Author

As @JonDouglas said, I've changed the proposal so that we're just showing the normalized compatible frameworks, rather than asserting a specific platform version. So a package targeting net6.0-androidY.Z will be mapped to net6.0-androidY.Z, net7.0-android, net8.0-android.

It would be difficult to continually keep track of all supported TPVs for each platform, so we'll instead try to point to docs somewhere once they're set up: dotnet/docs#38554.

@erdembayar erdembayar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants