Add special case for Xamarin assets to nuget precedence rules

[akoeplinger]

ABANDONED, see #222 (comment)

---

While working on .NET 6 support for Xamarin we hit a build break in a test project when referencing the System.ComponentModel.Composition v4.7.0 nuget package.

xamarin-macios/tests/linker/ios/link all/MEFTests.cs(3,29): error CS0234: The type or namespace name 'Composition' does not exist in the namespace 'System.ComponentModel' (are you missing an assembly reference?)

We've tracked it down to the nuget restore storing this in obj/project.assets.json:

      "System.ComponentModel.Composition/4.7.0": {
        "type": "package",
        "compile": {
          "ref/xamarinios10/_._": {}
        },
        "runtime": {
          "lib/xamarinios10/_._": {}
        }
      },

I.e. it wants to pick the assembly from the GAC/framework, which is what we want for the legacy Xamarin (since System.ComponentModel.Composition shipped in the framework there) but not for the new .NET6-based one since it is an out-of-band package there and doesn't ship in the framework.

We're proposing an addition to the nuget precedence rules to skip the Xamarin TFM asset in this case, so we fallback to the netcoreapp2.0 assembly.

The other alternative would be to add an explicit net6.0 asset to the package, however this would only apply to new releases since we can't fix the existing nugets.

Note: we discovered additional issues while working on this, see #222 (comment) so we're also proposing to change the precedence rules to prefer netstandard assets before net5.0

/cc @marek-safar @ericstj @rolfbjarne

[mhutch]

Does this affect just this assembly or are there other cases where Xamarin frameworks included out-of-band assemblies in-box?

I don't think it would be good to set a precedent of special-casing individual assemblies in the precedence rules. Perhaps we could generalize this by having the net6.0-* precedence rules skip _._ assets for Xamarin frameworks?

If we choose to go the route of updating the affected packages, we should consider adding a warning when affected projects directly or indirectly reference the old versions of the packages.

[akoeplinger]

Does this affect just this assembly or are there other cases where Xamarin frameworks included out-of-band assemblies in-box?

It affects these assemblies/packages:

System.ComponentModel.Composition
System.Data.SqlClient
System.Diagnostics.PerformanceCounter
System.Drawing.Common
System.Json
System.Runtime.Caching
System.Runtime.InteropServices.WindowsRuntime
System.Security.Cryptography.ProtectedData
System.ServiceModel.Duplex
System.ServiceModel.Http
System.ServiceModel.NetTcp
System.ServiceModel.Primitives
System.ServiceModel.Security
System.Xml.XPath.XmlDocument

I don't think it would be good to set a precedent of special-casing individual assemblies in the precedence rules. Perhaps we could generalize this by having the net6.0-* precedence rules skip _._ assets for Xamarin frameworks?

Sorry if this wasn't clear but my proposal was about making this a generalized rule like you described.

[akoeplinger]

Another thing that we noticed which would not be fixed by this proposal is that with the current precedence rules you also get transitive dependencies for the Xamarin.iOS group instead of e.g. netcoreapp3.1.

This manifests itself when referencing e.g. System.Text.Json 5.0.2, now you get this in project.assets.json:

"System.Text.Json/5.0.2": {
    "type": "package",
    "dependencies": {
        "Microsoft.Bcl.AsyncInterfaces": "5.0.0",
        "System.Buffers": "4.5.1",
        "System.Runtime.CompilerServices.Unsafe": "5.0.0",
        "System.Text.Encodings.Web": "5.0.1"
    },
    "compile": {
        "lib/netcoreapp3.0/System.Text.Json.dll": {}
    },
    "runtime": {
        "lib/netcoreapp3.0/System.Text.Json.dll": {}
    }
}

i.e. unnecessary Microsoft.Bcl.AsyncInterfaces while a normal net5.0 project gets no dependency nugets.

[danmoseley]

cc @joperezr since he was on the email.

[mhutch]

So NuGet selects the TFM for dependencies independently of selecting the TFM for assets? I don't know how hard it would be to change that, either in the general case or for this specific pattern. @nkolev92 ?

Maybe it's okay to special case the package names, given it's a small and limited set and this is an exceptional scenario.

[akoeplinger]

Here's another case where we run into issues now due to nuget picking netcoreapp assets instead of netstandard: xunit/xunit#2314

I'm starting to wonder whether we should tweak the rules to prefer netstandard assets before netcoreapp <= 5 assets, since these earlier netcoreapp assets could've justifiably made the assumption that they're running on "desktop" .net core.

[nkolev92]

So NuGet selects the TFM for dependencies independently of selecting the TFM for assets? I don't know how hard it would be to change that, either in the general case or for this specific pattern. @nkolev92 ?

This is a known behavior. There's a lot of packages that author their assets correctly, but don't use dependency groups for their dependencies.
We have not messed with it, because we haven't gotten a lot of feedback around this topic and it would be a breaking change.

I'd really prefer us to update the packages, as that change would be error prone and pretty difficult to follow.

cc @zkat @zivkan @aortiz-msft @JonDouglas

[akoeplinger]

So to summarize I think there are three issues right now:

1. Using the _._ GAC assembly which doesn't exist for .NET6-based Xamarin (tentative fix in this proposal)
2. Preferring netcoreapp assets over netstandard assets even though they might be assuming "desktop" .NET behavior (not fixed by this proposal)
3. Transitive dependencies via Xamarin.iOS dependency group are not ideal (not fixed by this proposal, though unclear how bad this is)

[ViktorHofer]

The other alternative would be to add an explicit net6.0 asset to the package, however this would only apply to new releases since we can't fix the existing nugets.

FWIW I'm doing this in dotnet/runtime#53439 but that will only cover packages that we build live.

cc @ericstj

[ericstj]

FWIW I'm doing this in dotnet/runtime#53439 but that will only cover packages that we build live.

And only those that happen to have a NetCoreAppCurrent TargetFramework already. I'm more inclined to remove the placeholders in the latest packages instead; so long as all existing customers who want to consume the new packages will be on net6.0-based SDKs.

[akoeplinger]

Existing Xamarin will still be supported for quite some time (https://dotnet.microsoft.com/platform/support/policy/xamarin) so removing the placeholders would break the packages for them.

[ericstj]

so removing the placeholders would break the packages for them.

Unless conflict resolution worked for them like it does for .NETCore and .NETFramework (where we have packages that overlap with shared framework and "best" wins). Not sure what the state of this is: do you know?

[JonDouglas]

Just so this PR contains all the suggested changes talked about, should we also introduce the ideal precedence order into the PR as it sounded like @Redth and @mattleibow saw some inconsistencies with this already?

Is there a proposal on how to resolve the precedence issues here? If netcoreapp3.1-1.0 assets exist in a package and are preferred with current rules, is that going to break too many things? Do we need to adjust the current proposal to skip certain generations to bring back the desired precedence?

You can use a tool like https://nugettoolsdev.azurewebsites.net/5.10.0-preview.2.7185/framework-precedence to check out existing precedence rules.

/cc @davidortinau

[mhutch]

Unless conflict resolution worked for them like it does for .NETCore and .NETFramework (where we have packages that overlap with shared framework and "best" wins). Not sure what the state of this is: do you know?

@ericstj how exactly does this work? I can't think why it should be any different for ios/android,

[ericstj]

@ericstj how exactly does this work? I can't think why it should be any different for ios/android

When there are two sources of the same binary it tries to decide which is better based on higher version. Reasons it could be different: if the build for these frameworks doesn't use these targets, if the files are not visible to the build (like the runtime packs in core that have to be represented in manifests passed to the resolver), or if for some reason the framework files should be better (eg more API, specialized implementation) but have a lower version than the package. You can try this out pretty easily by modifying the packages locally (deleting the placeholders in both the nupkg) and seeing how it works.

[mhutch]

Ah, it's a related problem but kinda the inverse?

The problem here is that the fallbacks are causing the package to resolve to a TFM where the assets are in the framework, but in the current TFM they aren't in the framework and instead need to come from the package. So no assets are resolved at all and there is no conflict to be resolved.

[ericstj]

Ah, it's a related problem but kinda the inverse? ... So no assets are resolved at all and there is no conflict to be resolved.

I'm discussing what can be done to fix those no assets cases: we can just remove placeholders from the packages and let things apply. Conflict resolution can make those additional assets a non-issue on the frameworks where they exists (Xamarin* frameworks). IOW: conflict resolution works better and replaces the uses of placeholder files.

[mhutch]

Oh, I wasn't aware the conflict resolution targets were available for non-SDK style projects. It looks they get conditionally imported for .NETFramework projects in Microsoft.NET.Build.Extensions.targets, which is present in the MSBuild shipped inside Visual Studio on Windows.

Unfortunately it doesn't look like Microsoft.NET.Build.Extensions.targets is present on Mac in either Mono or VSMac, so we'll have to fix that.

Assuming we can make this work, it does seem like the best option. We should maybe consider guard rails for folks updating their NuGet references on older versions of Xamarin?

[Redth]

@JonDouglas here's what I believe the precedence rules should really look like if we want to support the cases where packages expected net5.0/netcoreapp3.1-1.0 to run on desktop/coreclr and that netstandard2.1-1.0 would be selected in xamarin/mobile scenarios:

net6.0-ios|net6.0-android|net6.0-maccatalyst|net6.0-tvos|net6.0-macos
net6.0
xamarin.ios
netstandard2.1-1.0
net5.0 (warning)
netcoreapp3.1-1.0 (warning)
net4.x-1.0 (warning)

[akoeplinger]

@ericstj @mhutch if I understand your suggestion correctly it'd still only apply to newer versions of the packages right? At that point I'd rather add net6.0 assets to the packages so we don't need to touch legacy mono/Xamarin to fix conflict resolution.

[ViktorHofer]

Do we have the full list of affected packages? Presumably most of those don't exist in dotnet/runtime's main branch anymore or don't generate a package anymore as configurations were removed.

[akoeplinger]

@ViktorHofer I posted the list of packages that run into the _._ problem here: #222 (comment)

I'll update the proposal to include the precedence of netcoreapp vs. netstandard changes as those likely affect a lot more packages.

[ViktorHofer]

@ViktorHofer I posted the list of packages that run into the . problem here: #222 (comment)

Asking you mentioned that this list might not be complete:

It should affect these assemblies/packages (based on dotnet/runtime main, so might be missing older packages we don't build there anymore):

[JonDouglas]

I asked @Redth about precedence here based on the second point in this comment.

#222 (comment)

[mhutch]

@akoeplinger @ericstj as I see it we have three options:

1. Ship updated versions of these packages that include net6.0 assets. Add logic to the net6.0-* targets that detects older versions of these packages and emits an error telling the user to update the package reference.

2. Ship updated versions of the packages that remove the _._ for Xamarin frameworks. Ship updated versions of the Xamarin frameworks that enable conflict resolution. Ship the conflict resolution targets on Mac. Add logic to the Xamarin and net6.0-* targets that detects older versions of these packages and emits an error telling the user to update the package reference.

3. Special case these packages in NuGet.

[akoeplinger]

@mhutch yup, that's true for solving the _._ issue. There's another issue which is about the precedence rules that is unrelated to that one.

[mhutch]

I really think the precedence discussion belongs in another issue. However, here's my read of the situation:

AFAICT the only advantage of preferring netstandard assets over net5.0/netcoreapp* assets is that it may fix "Type 1" packages, for which all of the following are true:

• do not have xamarin assets
• have netstandard assets that are portable
• have net5.0/netcoreapp* assets that assume they're running on desktop

However, it will result in inferior assets being selected for "Type 2" packages, for which all the following are true:

• do not have xamarin assets
• have netstandard assets that are portable
• have net5.0/netcoreapp* assets that don't assume they're running on desktop

And it will break valid "Type 3" packages for which all the following are true:

• do not have xamarin assets
• netstandard asset is reference assembly (i.e. bait and switch) and is in lib
• net5.0/netcoreapp* assets have concrete impl

We don't know the relative numbers of these different types, nor do we know what proportion of type 1 packages would actually be fixed by the precedence change.

It would also introduce a massive inconsistency into the precedence.

[filipnavara]

I've to strongly agree with @mhutch here that it would be very weird if changing TFM from net6.0 to net6.0-macos (ie. to consume some Cocoa API) would suddenly start pulling in netstandard2.1 packages instead of net5.0/netcoreapp3.1 ones.

[filipnavara]

Ship updated versions of the Xamarin frameworks that enable conflict resolution. Ship the conflict resolution targets on Mac.

@mhutch Actually there is some conflict resolution logic already. It was recently broken too (dotnet/macios#10928). Look for *FrameworkList.xml.in files in https://github.com/xamarin/xamarin-macios/. This is already used for System.Buffers/System.Memory and similar assemblies to let the framework implementation win over the NuGet one.

[Redth]

I actually tend to agree that the precedence rules are correct as currently designed, and not the ones I suggested in my last comment to collect more evidence of the correct decision either way.

The unfortunate reality with the current rules is that many packages will need updating. Some of these are more impactful due to their prevalence in the ecosystem (xUnit for instance).

Is there anything else we can do for the user experience here when this is encountered? Is there a way for consumers of packages in this scenario to work around it?

I want to be sure this scenario is well understood and messaged appropriately /cc @terrajobst @richlander @davidortinau

[mhutch]

@Redth re. third party libraries, maybe NuGet should emit a warning when resolving netcoreapp*/net5.0 packages from net6.0-(ios|android|etc), like you suggested, to make the developer aware that there may be issues. i don't love this idea though, as it will result in many false positives and will be unactionable.

there isn't really much else we can do. we could statically determine some broken cases but that would be too expensive to do on restore or build and wouldn't catch everything. perhaps we could have the linker validate the platform checks and warn if unavailable APIs are found to be reachable? it would be a bit late in the dev cycle (especially for android, where linking may not happen till publish) but better than nothing. it would also be a generalized feature that would be useful long after any of this ceases to be an issue.

AFAIK there's no way to override the framework NuGet resolves for a package. it would definitely be a nice feature to have and would make problems like this actionable. i image it would mesh nicely with a generalized graph override capability.

re. the framework assemblies that moved out of box, I think all we can do is detect the unfixed versions of these specific packages and emit a build error instructing the developer to update those packages. maybe we could have a nuget feature where the targets could instruct NuGet to force package references to a minimum version if found anywhere in the graph e.g. something like <ForceUpgradePackage Id="SomeId" MinVersion="SomeVersion" />?

[mhutch]

@filipnavara @ericstj ah, my bad, looks like we do in fact ship the conflict resolution targets on Mac and have enabled them unconditionally for all Xamarin frameworks since late 2018.

We'd have to dig up some older versions of Xamarin and test it works correctly for all these packages, but maybe it's safe to just remove all those _._ files.

We'd still need the warning when using non-updated versions of those packages from net6.0-(ios|android|etc).

[akoeplinger]

I think for fixing the _._ issue I'd prefer adding net6.0 assets to the 14 packages mentioned in #222 (comment) rather than relying on conflict resolution, seems much easier to understand and requires no testing on old Xamarin frameworks.

Warning when restoring netcoreapp*/net5.0 packages indeed sounds like it will cause a lot of warnings so probably not something we want to do.

Is there anything else we can do for the user experience here when this is encountered? Is there a way for consumers of packages in this scenario to work around it?

Yeah I think giving users an escape hatch to override might be a good compromise, basically something similar to AssetTargetFallback (https://docs.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#assettargetfallback) which you put on the PackageReference.

[akoeplinger]

Having the "override asset" feature would actually allow us to solve the _._ issue for existing packages too since then we could just ship a hardcoded list of the 14 bad packages and do something like this in the iOS/Android SDKs:

<PackageReference Update="System.ComponentModel.Composition" Condition="Version == 5.0.0">
  <AssetTargetOverride>netcoreapp3.1</AssetTargetOverride>
</PackageReference>

[JonDouglas]

Just to circle back and re-iterate on a potential consensus of the initial 3 issues & what's been discussed thus far:

1. We believe fixing the libraries themselves is the best path forward. Whether that's removing the _._ or adding net6.0 assets.
2. We believe the precedence rules as implemented today are ideal and keep the precedence integrity for the future.
   • There are concerns around mobile/MAUI user expectations & UX of previous experiences & new experiences until the ecosystem broadly supports net6.0-(ios|android|etc) assets.
3. We believe this is a known behavior & do not have plans to resolve it at this time.

Does this sound right? If so, I propose we find some owners for the various items above & work to close out this PR if there's nothing in addendum to precedence rules.

[akoeplinger]

It sounds good to me, however we'd need the asset override nuget feature to make it a good experience.

@JonDouglas @nkolev92 how hard do you think that would be to implement?

[nkolev92]

It's non trivial as NuGet does asset selection per asset group + the fact that we'd need to consider what this means upstream for consumers of the project in question, be it as a projref or packref.

[mhutch]

Having the "override asset" feature would actually allow us to solve the _._ issue for existing packages too since then we could just ship a hardcoded list of the 14 bad packages and do something like this in the iOS/Android SDKs:

<PackageReference Update="System.ComponentModel.Composition" Condition="Version == 5.0.0">
  <AssetTargetOverride>netcoreapp3.1</AssetTargetOverride>
</PackageReference>

Conditionally applying metadata to PackageReference items wouldn't work for transitive refs.

I think my <ForceUpgradePackage Id="SomeId" MinVersion="SomeVersion" /> suggestion would be pretty straightforward? The idea is to clamp the version of any reference in the graph matching the ID.

[zivkan]

I think my <ForceUpgradePackage Id="SomeId" MinVersion="SomeVersion" /> suggestion would be pretty straightforward?

The NuGet Client team had been working on a Central Package Version Management feature, when we had fewer urgent tasks, but currently it doesn't make the cut when prioritized given our resources. Anyway, it contains a "transitive package pinning" feature, which we should use, once implemented, rather than providing another syntax. However, it took months to prototype, and once released, library authors immediately hated it given it's impact on pack & package dependencies, plus it broke features such as PrivateAssets. So, I would not say it's straightforward to implement. As a result, I don't imagine this could be delivered in time for net6.0.

[akoeplinger]

Ok so it seems the consensus is that we shouldn't do this 😃

We'll be doing this instead:

• Change the libraries mentioned in Add special case for Xamarin assets to nuget precedence rules #222 (comment) to add net6.0 assets which is tracked by Add NetCoreAppCurrent configurations to every library that targets .NETStandard or .NETCoreApp runtime#54012
• Work with affected nuget package owners to fix their packages, e.g. Changes in TFM hierarchy for .NET 6 on Android/iOS/etc xunit/xunit#2314
• Find out whether we can have some kind of escape hatch for unfixed packages to override the asset: Override asset selection in a package NuGet/Home#10937

[ViktorHofer]

Change the libraries mentioned in #222 (comment) to add net6.0 assets which is tracked by dotnet/runtime#54012

Correct me if I'm wrong but dotnet/runtime#54012 won't help with this in any way. None of the libraries mentioned above are part of the work tracked in that issue.

[akoeplinger]

Talked with Viktor offline, here's the status for the packages:

• Already fixed in dotnet/runtime or fixed shortly:

System.Drawing.Common
System.Diagnostics.PerformanceCounter
System.ComponentModel.Composition
System.Runtime.Caching
System.Security.Cryptography.ProtectedData

• Owned by dotnet/wcf, adding the net6.0 assets is tracked by Add net6.0 TFM to packages wcf#4237

System.ServiceModel.Duplex
System.ServiceModel.Http
System.ServiceModel.NetTcp
System.ServiceModel.Primitives
System.ServiceModel.Security

---

The following packages however are not building in dotnet/runtime main anymore and are basically deprecated:

System.Data.SqlClient (replaced by Microsoft.Data.SqlClient)
System.Runtime.InteropServices.WindowsRuntime (never ran on Xamarin)
System.Json
System.Xml.XPath.XmlDocument

From talking to @ViktorHofer we don't service these packages anymore so we need to work with package owners to remove their dependencies on these packages.