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

dellis1972 · 2023-08-14T17:07:11Z

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

In fcd7cf8, 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.

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildWithLibraryTests.cs

jonathanpeppers

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

...id.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets

dellis1972 · 2023-08-16T16:44:24Z

/azp run

azure-pipelines · 2023-08-16T16:44:46Z

Azure Pipelines successfully started running 1 pipeline(s).

jonpryor · 2023-10-10T00:14:14Z

...id.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets

@@ -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>


What is the rationale for moving these properties?

There were in an Application Only Property Block, but we need them to be calculated in both apps and library projects. Which is why there were moved.

jonpryor · 2023-10-10T00:32:17Z

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 · 2023-10-10T00:40:28Z

@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.

dellis1972 · 2024-08-12T08:07:56Z

AndroidApplication

@jonpryor

AndroidApplication is calculated based on OutputType now. If its Exe we get an app.

jonpryor · 2024-09-10T16:50:46Z

/azp run

azure-pipelines · 2024-09-10T16:51:10Z

Azure Pipelines successfully started running 1 pipeline(s).

jonpryor · 2024-09-10T16:53:32Z

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildWithLibraryTests.cs

@@ -87,6 +95,7 @@ public void DotNetBuildLibrary (bool isRelease, bool duplicateAar, bool useDesig
 			using (var aar = ZipHelper.OpenZip (aarPath)) {
 				aar.AssertContainsEntry (aarPath, "assets/bar/bar.txt");
 				aar.AssertEntryEquals (aarPath, "proguard.txt", "# LibraryC");
+				aar.AssertContainsEntry (aarPath, "AndroidManifest.xml");


Should we also assert that the final app contains the <queries/> element from libC?

It is checked later on in the test when the appliction is built.

jonathanpeppers

This looks good, I just had some minor comments. 👍

src/Xamarin.Android.Build.Tasks/Tasks/CreateAar.cs

src/Xamarin.Android.Build.Tasks/Tasks/GetImportedLibraries.cs

…from class library Fixes dotnet#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.

) Fixes: #8262 Context: https://developer.android.com/studio/projects/android-library#aar-contents Context: fcd7cf8 In fcd7cf8, we stopped using `@(EmbeddedResource)` within assemblies to contain Android Resource, Assets, Libraries, etc., and instead began using 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 include 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.

dellis1972 requested review from jonathanpeppers and jonpryor as code owners August 14, 2023 17:07

dellis1972 mentioned this pull request Aug 14, 2023

Cannot merge Android Manifest elements from class library #8262

Closed

jonathanpeppers reviewed Aug 14, 2023

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildWithLibraryTests.cs Outdated Show resolved Hide resolved

jonathanpeppers approved these changes Aug 15, 2023

View reviewed changes

...id.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets Outdated Show resolved Hide resolved

dellis1972 force-pushed the Issue8262 branch from df81d10 to fa3b650 Compare August 16, 2023 13:49

dellis1972 force-pushed the Issue8262 branch from fa3b650 to 3af55a9 Compare August 21, 2023 14:55

dellis1972 force-pushed the Issue8262 branch 3 times, most recently from 482bcf4 to 9aeab85 Compare October 6, 2023 10:38

jonpryor reviewed Oct 10, 2023

View reviewed changes

dellis1972 force-pushed the Issue8262 branch 2 times, most recently from ccb1ebe to 27ed00a Compare October 11, 2023 13:38

dellis1972 force-pushed the Issue8262 branch from 27ed00a to 74a6039 Compare August 7, 2024 07:33

dellis1972 force-pushed the Issue8262 branch from 74a6039 to 8fe32dd Compare August 20, 2024 07:59

dellis1972 changed the title ~~[Xamarin.Android,Build.Tasks] Cannot merge Android Manifest elements from class library~~ [Xamarin.Android,Build.Tasks] Library .aar includes $(AndroidManifest) (#8273) Aug 20, 2024

dellis1972 force-pushed the Issue8262 branch from 8fe32dd to 0642581 Compare August 30, 2024 12:26

jonpryor reviewed Sep 10, 2024

View reviewed changes

dellis1972 force-pushed the Issue8262 branch from 0642581 to 9603f69 Compare September 12, 2024 07:51

dellis1972 requested review from jonathanpeppers and jonpryor September 12, 2024 18:03

jonathanpeppers approved these changes Sep 12, 2024

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tasks/CreateAar.cs Outdated Show resolved Hide resolved

src/Xamarin.Android.Build.Tasks/Tasks/GetImportedLibraries.cs Outdated Show resolved Hide resolved

dellis1972 added 5 commits September 18, 2024 08:40

Update tests and add a new warnings

471a01e

Fix tests

e3d1a12

Updated

32297ff

Add package attribute check for CreateAar

37f6371

Fix build error

1b84eb0

dellis1972 force-pushed the Issue8262 branch from eb3319c to 1b84eb0 Compare September 18, 2024 07:40

jonpryor merged commit 8050207 into dotnet:main Sep 30, 2024
55 of 57 checks passed

github-actions bot locked and limited conversation to collaborators Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dellis1972 commented Aug 14, 2023 •

edited

Loading

jonathanpeppers left a comment

dellis1972 commented Aug 16, 2023

azure-pipelines bot commented Aug 16, 2023

jonpryor Oct 10, 2023

dellis1972 Aug 7, 2024

jonpryor commented Oct 10, 2023

jonpryor commented Oct 10, 2023

dellis1972 commented Aug 12, 2024

jonpryor commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

jonpryor Sep 10, 2024

dellis1972 Sep 12, 2024

jonathanpeppers left a comment

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

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

Conversation

dellis1972 commented Aug 14, 2023 • edited Loading

jonathanpeppers left a comment

Choose a reason for hiding this comment

dellis1972 commented Aug 16, 2023

azure-pipelines bot commented Aug 16, 2023

jonpryor Oct 10, 2023

Choose a reason for hiding this comment

dellis1972 Aug 7, 2024

Choose a reason for hiding this comment

jonpryor commented Oct 10, 2023

jonpryor commented Oct 10, 2023

dellis1972 commented Aug 12, 2024

jonpryor commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

jonpryor Sep 10, 2024

Choose a reason for hiding this comment

dellis1972 Sep 12, 2024

Choose a reason for hiding this comment

jonathanpeppers left a comment

Choose a reason for hiding this comment

dellis1972 commented Aug 14, 2023 •

edited

Loading