Add NU1703 warning for packages using deprecated MonoAndroid framework#7229
Add NU1703 warning for packages using deprecated MonoAndroid framework#7229
Conversation
Emit NU1704 when a project targeting net11.0-android or later references a package that resolves MonoAndroid framework assets instead of the modern net6.0-android+ TFMs. The warning is gated on SdkAnalysisLevel >= 11.0.100 and only applies to Android-targeting projects with framework version >= 11. The detection inspects compile-time and runtime assembly paths in the lock file target library for 'monoandroid' folder segments, following the pattern of the previously removed MaccatalystFallback helper.
# Conflicts: # src/NuGet.Core/NuGet.Common/PublicAPI.Unshipped.txt
|
NU1704 doesn't seem to be documented at https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings do I assume it's a new warning for NuGet 7/.NET 11? The PR title or description wasn't making it obvious if the warning itself is new or just the scenario... Since I couldn't find any docs for it I will ask this question. The PR description mentions lockfiles so will using lockfiles be a pre-condition to get this warning? Could a warning be issued without it or could some docs be further clarified? |
|
NU1704 is a new warning I'm introducing. It's similar to the NU1703 for xamarin.ios (before it was removed in #4362 because we decided xamarin.ios would not be supported). I'll add a docs PR too. |
|
Hey @sbomer Thanks for getting this conversation going. While the implementation is correct, we normally start with an https://github.com/nuget/home/issues and sometimes a spec for changes like this. https://github.com/NuGet/Home/blob/dev/meta/template.md The reasoning being that adding a new warning is considered disruptive and a breaking change as well meaning as it may be. We'd also probably be curious about the blast radius, so how many projects would be affected, but I doubt that's something we can easily figure out right now. I'll tag the PR as a breaking change and bot will follow with more links. |
| } | ||
| } | ||
|
|
||
| // Log NU1704 warning if the package uses the deprecated MonoAndroid framework |
There was a problem hiding this comment.
breaking change mechanics aside, i wonder if we should just re-use Nu1703.
It existed for a very very short time and it's unlikely anyone is specifying it their project file based on some of the comments we've made when we reverted it.
There was a problem hiding this comment.
I considered this but noticed we did document NU1703 with text that references xamarin.ios, so I went with a new warning. Happy to use NU1703 instead if we are ok with changing that doc.
There was a problem hiding this comment.
I'll let someone else from the team chip in too before you go and make that change. I am comfortable with changing that documentation too.
Oh there's this thing I meant to link to but I forgot in the previous comment: https://github.com/NuGet/NuGet.Client/blob/dev/docs/feature-guide.md#warnings-and-defaults
So whatever we add, we'll need to gate on SDKAnalysisLevel.
There was a problem hiding this comment.
/cc @rolfbjarne
I think it would be OK to reuse the code and just include Xamarin.Android and Xamarin.iOS both in the docs.
I think this is false anyway:
While net6.0-maccatalyst (and higher .NET versions) support using Xamarin.iOS dependencies, this is not guaranteed to be 100% compatible.
I don't think they would work at all?
However, Xamarin.Android class libraries are more in the "not guaranteed to be 100% compatible", but could actually work.
There was a problem hiding this comment.
While net6.0-maccatalyst (and higher .NET versions) support using Xamarin.iOS dependencies, this is not guaranteed to be 100% compatible.
I don't think they would work at all?
No, this doesn't work.
In fact Xamarin.iOS.dll never worked in .NET, so I'm fine with reusing the warning number, nobody should have seen NU1703 in the first place.
Per PR feedback on NuGet/NuGet.Client#7229, reuse the NU1703 warning code for the MonoAndroid deprecation scenario. The previous NU1703 for Xamarin.iOS never shipped, so the code is safe to repurpose. Remove NU1704 doc and references from TOC and index.
| /// </summary> | ||
| /// <param name="library">The lock file target library to check.</param> | ||
| /// <returns>True if the library uses MonoAndroid framework assets.</returns> | ||
| internal static bool UsesMonoAndroidFramework(LockFileTargetLibrary library) |
There was a problem hiding this comment.
I wonder if there are cheaper ways to determine this, that doesn't involve re-parsing.
Do you know how much overhead this is adding?
I guess the fact that it's only on -android frameworks makes it not a hot path.
It's also using span to avoid allocating, but there may be some ways we can utilize the details from the asset selection logic that already parses this.
There was a problem hiding this comment.
I tried another approach that reuses the asset selection results, PTAL!
…g paths Instead of re-parsing assembly paths (e.g. lib/monoandroid10.0/a.dll) to detect MonoAndroid usage, read the NuGetFramework already computed by FindBestItemGroup from ContentItemGroup.Properties["tfm"] during asset selection. Extend CreateLockFileTargetLibrary and LockFileBuilderCache to propagate the compile and runtime asset frameworks, and check IsMonoAndroidFramework in LockFileBuilder where the NU1703 warning is emitted. This removes the path-parsing helpers and simplifies tests.
|
@nkolev92 PTAL |
| return StringComparer.OrdinalIgnoreCase.Equals(framework.Framework, FrameworkConstants.FrameworkIdentifiers.NetCoreApp) | ||
| && framework.Version.Major >= 11 | ||
| && framework.HasPlatform | ||
| && framework.Platform.Equals("android", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
I'm wondering if NuGet is the right place to check for this. Can this logic be moved to the Android workload instead? It's probably easiest to do in NuGet, but this is code that will run for every restore, even when the Android workload isn't installed.
When I take a binlog of a normal build, the .NET SDK reads NuGet's assets file in the ResolvePackageAssets task. The RuntimeCopyLocalItems and ResolvedCompileFileDefinitions items have PathInPackage metadata, which can be searched for monoandroid, for example a regex something like [lib|ref]/monoandroid*. Checking the runtime assets will be tricker to validate the subdirectory that corresponds to the TFM.
It's not as precise as checking the NuGetFramework for the selected asset, but the question is if it's good enough.
Another design idea is to change the assets file to emit a "framework" metadata for every asset, and change the SDK's ResolvePackageAssets to copy that metadata into the MSBuild item, which can then be checked by the android workload's msbuild targets. However, I don't like this idea as the increased assets file size and additional metadata on every item will cause additional memory allocations and therefore more GC and will probably have worse perf impact than the small benefit in moving the check from NuGet to the android workload.
Given restore almost never breaks backwards compatibility, I just want to make sure that NuGet isn't stuck with this "forever" when it's fundamentally a non-NuGet concern. So, trying to do due diligence in checking if the owning component can feasibly implement the check, rather than NuGet.
There was a problem hiding this comment.
I'm not familiar enough with the design of the workloads to know whether it would make sense to handle it there. @jonathanpeppers what do you think?
My instinct is that this should warn during restore, not during build, which is why I thought it made the most sense to handle it in NuGet. Plus there was precedent in the (now removed) Xamarin.iOS warning.
There was a problem hiding this comment.
The design says to emit this warning here in NuGet (but note the code was NU1701 back then):
I'm not aware of us running any MSBuild logic during restore from a workload.
If we had to do it from a workload, it would be easiest to do during a build -- which is maybe not what we'd want.
Emit NU1704 when a project targeting net11.0-android or later references a package that resolves MonoAndroid framework assets instead of the modern net6.0-android+ TFMs. The warning is gated on SdkAnalysisLevel >= 11.0.100 and only applies to Android-targeting projects with framework version >= 11.
The detection inspects compile-time and runtime assembly paths in the lock file target library for 'monoandroid' folder segments, following the pattern of the previously removed MaccatalystFallback helper.
Bug
Adds the warning mentioned in dotnet/android#11028.
Description
PR Checklist