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 architecture info to the 0141 warning #9547

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

grendello
Copy link
Contributor

Partial fix for #9544 to make
the warning more informative. Architecture information is necessary as
the nuget in question might have the same library for various
architectures.

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.

Do we have the full path to the .so file? It seems like we could simply put that in the error message and that's it?

If it's from a NuGet package, you will know what package and version from the file path.

@grendello
Copy link
Contributor Author

Do we have the full path to the .so file? It seems like we could simply put that in the error message and that's it?

If it's from a NuGet package, you will know what package and version from the file path.

I considered that, but I figured most people won't find it actionable anyway. They most likely don't maintain the nuget,
so the full path is just cognitive noise for them. With the library name + architecture then can, however, go to the nuget
authors and just let them know about the two dateails.

@jonathanpeppers
Copy link
Member

The full path to the file is more informative than <unknown>, though, right?

Comment on lines 69 to 75
log.LogCodedWarning ("XA0141", Properties.Resources.XA0141, packageId, packageVersion, Path.GetFileName (path));
log.LogCodedWarning ("XA0141", Properties.Resources.XA0141, packageId, packageVersion, Path.GetFileName (path), archName);
Copy link
Member

Choose a reason for hiding this comment

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

Could we use the overload for LogCodedWarning where you pass the file path in, and put 0 for line number:

This would include the file, and if you double-clicked the error in an IDE it would open the file.

@grendello
Copy link
Contributor Author

The full path to the file is more informative than <unknown>, though, right?

It is, but it also potentially leaks information about the user's environment, if shared (operating system, user login name), since it's going to be in the nuget cache.
The <unknown> part is something that needs a fix too, I imagine it should be possible to get that information from transient dependencies.

@grendello grendello force-pushed the dev/grendel/better-16kb-align-warning branch from 8aa5ef7 to e2ad080 Compare November 22, 2024 15:51
@jpobst
Copy link
Contributor

jpobst commented Nov 22, 2024

Can we also update the verbiage to be less scary:

warning XA0141: NuGet package 'Xamarin.GooglePlayServices.Vision.Face.Contour.Internal' version '116.1.0.19' contains a shared library 'arm64-v8a/libface_detector_v2_jni.so' which is not 16KB aligned. Google may require 16KB aligned libraries in the future. See https://developer.android.com/guide/practices/page-sizes for more details.

@grendello
Copy link
Contributor Author

@jpobst we can change the wording, but there's no "may" about it - they will require that alignment.

Partial fix for #9544 to make
the warning more informative.  Architecture information is necessary as
the nuget in question might have the same library for various
architectures.
@grendello grendello force-pushed the dev/grendel/better-16kb-align-warning branch from e2ad080 to 30b9c13 Compare November 22, 2024 19:55
@grendello
Copy link
Contributor Author

Linux build fails with

Workload installation failed: Version 35.99.0-ci.pr.gh9547.38 of package microsoft.android.sdk.linux is not found

@pjcollins @jonathanpeppers any idea what might be causing it?

@pjcollins
Copy link
Member

Linux build fails with

Workload installation failed: Version 35.99.0-ci.pr.gh9547.38 of package microsoft.android.sdk.linux is not found

@pjcollins @jonathanpeppers any idea what might be causing it?

This is a weird issue where the source in the target branch appears to have been updated between when the mac build ran and the linux build ran -- we have packages on the PR with different versions:
Screenshot 2024-11-25 at 10 36 12 AM

Did the linux build initially fail and re-run? I think we'll need to just kick off a new build

@pjcollins
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pjcollins
Copy link
Member

This failing check will continue to show on the PR from the old build since we're rebuilding the same commit, and it can be ignored in favor of the new build:

Xamarin.Android-PR (Linux Tests Linux > Tests > MSBuild)

@grendello
Copy link
Contributor Author

Did the linux build initially fail and re-run? I think we'll need to just kick off a new build

Yep, I restarted it two times I think. Let's hope it works fine now, thanks for looking into it :)

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