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

[Xamarin.Android,Build.Tasks] Cannot merge Android Manifest elements from class library #8273

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dellis1972
Copy link
Contributor

Fixes #8262

So if a user wants to distribute an AndroidManifest.xml file along with a NuGet the file is currently NOT included in the library .aar file. So lets include it if one is present. This will allow NuGet authors to set default permissions if need be by simply adding the file to the project directory.

This also fixes an issue in GenerateLibraryResources where if a manifest file does NOT have a package name.

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.

Overall looks good, just had the one thought/comment. 👍

@dellis1972
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -83,8 +83,6 @@
<!-- Prefer $(RuntimeIdentifiers) plural -->
<RuntimeIdentifiers Condition=" '$(RuntimeIdentifier)' == '' And '$(RuntimeIdentifiers)' == '' ">android-arm;android-arm64;android-x86;android-x64</RuntimeIdentifiers>
<RuntimeIdentifier Condition=" '$(RuntimeIdentifiers)' != '' And '$(RuntimeIdentifier)' != '' " />
<AndroidManifest Condition=" '$(AndroidManifest)' == '' and Exists ('Properties\AndroidManifest.xml') and !Exists ('AndroidManifest.xml') ">Properties\AndroidManifest.xml</AndroidManifest>
Copy link
Member

Choose a reason for hiding this comment

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

What is the rationale for moving these properties?

@jonpryor
Copy link
Member

Draft commit message:

[Xamarin.Android,Build.Tasks] Library .aar includes $(AndroidManifest) (#8273)

Fixes: https://github.com/xamarin/xamarin-android/issues/8262

Context: https://developer.android.com/studio/projects/android-library#aar-contents
Context: fcd7cf8f4200ccae92e333c920613bfec260973b

In fcd7cf8f, we stopped using `@(EmbeddedResource)` within assemblies
to contain Android Resource, Assets, Libraries, etc., and instead
began a `.aar` file.

One of the things never supported in the Classic system was to allow
Library projects to contain `AndroidManifest.xml` files, and package
`AndroidManifest.xml` files into the library `.aar` file for inclusion
in NuGet packages.

Add `$(AndroidManifest)` to `@(_CreateAarInputs)`, so that Library
projects can now specify an `AndroidManifest.xml` file for inclusion
in the library `.aar` file (which in turn should be included in NuGet
packages).

This allows NuGet authors to e.g. set default permissions if need be
by simply adding `AndroidManifest.xml` to the library project dir.

Also, fix an issue in `<GenerateLibraryResources/>` when a manifest
file does NOT have a package name.  Such files should be ignored.

@jonpryor
Copy link
Member

@dellis1972: do we have any idea what, if anything, this will do when done broadly?

On the one hand, AndroidManifest.xml is the one required file in .aar files: https://developer.android.com/studio/projects/android-library#aar-contents

The only mandatory entry is /AndroidManifest.xml.

On the other hand, this is still A Change, and I find it hard to know if this could possibly break anything. For example, if someone starts with an App project (dotnet new android), then "changes their mind" and starts using it as a library project. (Is that even a thing? We used to have $(AndroidApplication), but I don't see that in our current template, so how do we know if we're building an App or a Library anymore?)

Said "app now library" project will contain an AndroidManifest.xml, quite possibly the default:

https://github.com/xamarin/xamarin-android/blob/1a309b63345d64356a68805c2e20f1c4fd794e42/src/Microsoft.Android.Templates/android/AndroidManifest.xml

What happens if an app starts getting multiple copies of our default AndroidManifest.xml? Is that a problem?

I'm wary of including a change this close to release when I can't foresee how it will impact the ecosystem.

…from class library

Fixes xamarin#8262

So if a user wants to distribute an AndroidManifest.xml file along with
a NuGet the file is currently NOT included in the library .aar file.
So lets include it if one is present. This will allow NuGet authors
to set default permissions if need be by simply adding the file to the
project directory.

This also fixes an issue in GenerateLibraryResources where if a manifest
file does NOT have a package name.
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.

Cannot merge Android Manifest elements from class library
3 participants