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

Add NugetPackageId Metadata to TaskItems #9559

Merged
merged 11 commits into from
Jan 9, 2025
Merged

Conversation

dellis1972
Copy link
Contributor

@dellis1972 dellis1972 commented Nov 26, 2024

Figuring out how to fix a XA0141 error is almost impossible for an end user at this time. The default error message is

NuGet package '' version '' contains a shared library 'foo.so' which is not correctly aligned. See https://developer.android.com/guide/practices/page-sizes for more details

the assumption was that the NuGet package and Version would be available as metadata items on the
MSBuild Items. Unfortunately that was not the case, so the data the user gets is empty.

This commit adds the required Metadata to all NuGet Package references resolved by the project. This will allow us to propagate that data throughout the build Items as they are transformed. This will in the end mean we can provide a decent error message to the user.

Shared library 'foo.so' must use 16 KB page sizes. Please inform the authors of the NuGet package 'Foo.Bar' version '1.2.3.4' which contains 'lib/arm64-v8a/foo.so'. See https://developer.android.com/guide/practices/page-sizes for more details.

@dellis1972 dellis1972 force-pushed the dev/dellis1972/nugetmetadata branch from faea51c to 9bc94c4 Compare December 3, 2024 09:15
@jonpryor jonpryor mentioned this pull request Dec 9, 2024
@dellis1972 dellis1972 force-pushed the dev/dellis1972/nugetmetadata branch from 9bc94c4 to eaf23a3 Compare January 6, 2025 15:06
@dellis1972 dellis1972 marked this pull request as ready for review January 6, 2025 15:07
@dellis1972
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

Is there a test that checks XA0141? Maybe it could assert the new message?

@dellis1972
Copy link
Contributor Author

XA0141

It doesn't look like there is an existing test. I'm not sure how we could trigger the error unless we have a Nuget with a non 16bit aligned native library. Maybe an old version of Skia will work.

@dellis1972
Copy link
Contributor Author

This version should trigger the error https://github.com/mono/SkiaSharp/releases/tag/v3.116.0

@dellis1972 dellis1972 force-pushed the dev/dellis1972/nugetmetadata branch from 2fe790f to ee42973 Compare January 7, 2025 09:04
@dellis1972
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dellis1972 dellis1972 requested a review from jonpryor January 7, 2025 09:20
@dellis1972 dellis1972 added the do-not-merge PR should not be merged. label Jan 7, 2025
@dellis1972
Copy link
Contributor Author

The unit test has thrown up some issues.

@dellis1972 dellis1972 removed the do-not-merge PR should not be merged. label Jan 7, 2025
@dellis1972
Copy link
Contributor Author

/azp run

@dellis1972
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Dictionary initializers for the win!
@jonpryor
Copy link
Member

jonpryor commented Jan 7, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Member

jonpryor commented Jan 8, 2025

Draft commit message:

Fixes: https://github.com/dotnet/android/issues/9544

Context: 64bb147f2ead5bd403a823d27b33f590197477fa

[Google announced][0] that future versions of Android would require
that native libraries use 16 KB page sizes on arm64.  At present, the
timeline for *when* 16 KB page sizes will be required is unknown,
though we assume it will be with Android 16 or later.

In order to get ahead of this, .NET 9 added an XA0141 warning in
64bb147f.

The problem is, this warning is not entirely actionable:

	dotnet new android
	dotnet add package Xamarin.GooglePlayServices.Vision.Face.Contour.Internal --version 116.1.0.19
	dotnet build -c Release

results in:

	warning XA0141: NuGet package '<unknown>' version '<unknown>' contains a shared library 'libface_detector_v2_jni.so'
	  which is not correctly aligned. See https://developer.android.com/guide/practices/page-sizes for more details
	warning XA0141: NuGet package '<unknown>' version '<unknown>' contains a shared library 'libface_detector_v2_jni.so'
	  which is not correctly aligned. See https://developer.android.com/guide/practices/page-sizes for more details

What are customers supposed to *do* with this?

The original assumption was that the NuGet package and Version would
be available as metadata items on the MSBuild Items.  Unfortunately
that was not the case, so the data the user gets is empty. 

Add the required metadata to all NuGet Package references resolved by
the project.  This will allow us to propagate that data throughout
the build Items as they are transformed.  This means we can provide a
decent error message to the user:

	warning XA0141: Android 16 will require 16 KB page sizes, Shared library 'libface_detector_v2_jni.so' does not have a 16 KB page size.
	  Please inform the authors of the NuGet package 'Xamarin.GooglePlayServices.Vision.Face.Contour.Internal' version '116.1.0.19'
	  which contains '/Users/xxx/.nuget/packages/xamarin.googleplayservices.vision.face.contour.internal/116.1.0.19/lib/net8.0-android34.0/play-services-vision-face-contour-internal.aar'.
	  See https://developer.android.com/guide/practices/page-sizes for more details.

[0]: https://android-developers.googleblog.com/2024/08/adding-16-kb-page-size-to-android.html

@jonpryor
Copy link
Member

jonpryor commented Jan 8, 2025

Re-reading the message "Shared library 'libface_detector_v2_jni.so' must use 16 KB page sizes" with "fresher" eyes implies that this must be fixed now, which isn't quite true. It's fine now; it won't be fine in some future Android version, currently unknown though assumed to be after Android 16.

Perhaps we should do:

Future versions of Android will require 16 KB page sizes, which shared library 'libface_detector_v2_jni.so' does not use. …

@dellis1972
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dellis1972
Copy link
Contributor Author

dellis1972 commented Jan 8, 2025

For the example project this is the new error message

 warning XA0141: Android 16 will require 16 KB page sizes, Shared library 'libface_detector_v2_jni.so' does not have a 16 KB page size. Please inform the authors of the NuGet package 'Xamarin.GooglePlayServices.Vision.Face.Contour.Internal' version '116.1.0.19' which contains '/Users/xxx/.nuget/packages/xamarin.googleplayservices.vision.face.contour.internal/116.1.0.19/lib/net8.0-android34.0/play-services-vision-face-contour-internal.aar'. See https://developer.android.com/guide/practices/page-sizes for more details.

@dellis1972
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Member

jonpryor commented Jan 8, 2025

Looks like Android 16 may be requiring 16KB page sizes, e.g. API-35 emulators are requiring 16KB page size: mono/SkiaSharp#3025 (comment)

@jonpryor
Copy link
Member

jonpryor commented Jan 8, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor jonpryor merged commit d18c306 into main Jan 9, 2025
61 checks passed
@jonpryor jonpryor deleted the dev/dellis1972/nugetmetadata branch January 9, 2025 00:14
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.

4 participants