Manifest merger warning confusing, child package conflicting with parent [166471969]

Fixed

Bug

Status Update

No update yet.

Description

je...@google.com

created issue #1

Aug 26, 2020 06:32PM

I'm observing when building AndroidX that running processDebugAndroidTestManifest is generating some warnings. For example:

./gradlew :ads-identifier:processDebugAndroidTestManifest

generates this output:

Configuration on demand is an incubating feature.

> Task :ads-identifier:processDebugAndroidTestManifest
/usr/local/google/workspace/aosp-androidx-git/simplify-tmp/bestResults/ads/ads-identifier/src/androidTest/AndroidManifest.xml Warning:
        Package name 'androidx.ads.identifier.test' used in: AndroidManifest.xml, manifestMerger17468703757590145208.xml.

After some investigation, it seems that the minimal amount of code required to reproduce this problem is:

./src/androidTest/AndroidManifest.xml:

<manifest xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:tools="http://schemas.android.com/tools"
    package="androidx.ads.identifier.test">

</manifest>

./src/main/AndroidManifest.xml:

<?xml version="1.0" encoding="utf-8"?>
<manifest package="androidx.ads.identifier"
    xmlns:android="http://schemas.android.com/apk/res/android">
</manifest>

./build.gradle:

plugins {
    id("com.android.library")
}

android {
    compileSdkVersion "android-30"
    compileOptions {
        sourceCompatibility JavaVersion.VERSION_1_7
        targetCompatibility JavaVersion.VERSION_1_7
    }
}

It seems to me that there are a few confusing aspects of this output:

The fact that it is unexpected to find the same package in multiple manifests could be made more clear by adding the word "multiple", perhaps: "Package $package used in multiple manifests: $manifest1, $manifest2"
The warning says "AndroidManifest.xml, manifestMerger17468703757590145208.xml." but it's difficult to identify where manifestMerger17468703757590145208.xml comes from.
It seems to me that although the reported conflict is androidx.ads.identifier.test, that it would be easier to interpret if the conflict were instead reported as androidx.ads.identifier. That would make it easy for a user to run grep -r "androidx.ads.identifier" and immediately see both conflicting instances.
Could the warning be made to explain why it is undesirable to have these partially overlapping package names? I'm unsure whether I should file a bug on corresponding library owners, asking them to change the package names.

Thanks!

Gradle version: 6.5.1

Android Plugin Version: 4.2.0-alpha06

Module Compile Sdk Version: android-30

Module Build Tools Version: 29.0.3

Comments

au...@google.com <au...@google.com> Aug 26, 2020 06:53PM

Assigned to im...@google.com.

au...@google.com <au...@google.com> #2Oct 15, 2020 08:13PM

imorlowska@ can you comment if this is a KI?

je...@google.com <je...@google.com> #3Oct 19, 2020 09:18PM

AndroidX is probably going to start ignoring warnings that look like this,

https://buganizer.corp.google.com/issues/171245332#comment1

im...@google.com <im...@google.com> #4Oct 20, 2020 09:52AM

Reassigned to sp...@google.com.

Generally these warnings are important, but I think with test packages we are too overzealous. I think if the specified test package matches the one we would generate by default (appending ".test" to the tested module's package) we give a warning when we should not.
Scott, could you please take a look? I'm OOO for the next 2 weeks but can have a look too once I'm back.

sp...@google.com <sp...@google.com> #5Oct 27, 2020 05:30PM

I agree that the warning message needs improvement, but would it be possible to use package="androidx.ads.identifier" in your AndroidTest manifest, instead of package="androidx.ads.identifier.test"?

The package will be set to "androidx.ads.identifier.test" in the AndroidTest APK either way, but you won't get a warning with the former.

au...@google.com <au...@google.com> #6Oct 27, 2020 05:48PM

That seems confusing. Don't we want the package to be androidx.ads.identifier.test for tests?

Are you simply proposing a workaround until AGP fixes this issue?

sp...@google.com <sp...@google.com> #7Oct 27, 2020 05:52PM

~~AGP automatically appends .test to the package in the AndroidTest APK's AndroidManifest.xml.~~

~~Is there a reason you also need it to be androidx.ads.identifier.test in the source AndroidManifest.xml?~~

Correction: AGP uses the tested app's applicationId + ".test" for the AndroidTest APK's package name. See here.

Message last modified on Oct 27, 2020 06:36PM

au...@google.com <au...@google.com> #8Oct 27, 2020 05:55PM

Ideally, looking at the AndroidManifest.xml you see the final test package, which includes .test. It seems a bit odd to have androidx.ads.identifier which is the package of the main library, which then silently gets replaced with androidx.ads.identifier.test.

sp...@google.com <sp...@google.com> #9Oct 27, 2020 06:31PM

Okay, I think the correct workaround is to set android.defaultConfig.testApplicationId "androidx.ads.identifier.test" and remove the package attribute entirely from the AndroidTest AndroidManifest.xml.

Currently, the package attribute of the AndroidTest AndroidManifest.xml has no impact on the package in the merged manifest.

I'm not sure what the desired behavior is here. +android-gradle@

au...@google.com <au...@google.com> #10Oct 27, 2020 09:07PM

We'd like to have consistency, so that library package is androidx.foo, and test has androidx.foo.test. Is there a programmatic way to do that? I'd prefer we don't hardcode all of these values in build.gradle

sp...@google.com <sp...@google.com> #11Oct 27, 2020 09:12PM

Yes, if you remove the package attribute from the AndroidTest AndroidManifest.xml, then the package names will use that exact convention.

sp...@google.com <sp...@google.com> #12Oct 27, 2020 09:27PM

I discussed this with jedo@ and xav@ to see what the desired behavior is.

Long-term, specifying a package in the AndroidManifest.xml should be an error, and there should instead be a way to specify it via the DSL (both for the app/library and for the androidTest).

AFAICT, the package attribute of the AndroidTest AndroidManifest.xml hasn't impacted the build for quite a while, so I don't think we'll add that functionality now.

We need to do the following:

(1) Add package and testPackage to AGP's DSL.

(2) Move users' package values from AndroidManifest.xml to the DSL via the upgrade wizard.

(3) Eventually warn users if they set package in the AndroidManifest.xml. And eventually make this an error.

sj...@google.com <sj...@google.com> #13Oct 28, 2020 03:36PM

Removing the package attribute from the androidTest AndroidManifext.xml causes build failure for me with the changes in this CL

Here's the build_error.log

The stacktrace is:

* Exception is:
org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':heifwriter:heifwriter:processDebugAndroidTestManifest'.
	at org.gradle...
Caused by: java.lang.RuntimeException: Manifest merger failed : Main AndroidManifest.xml at AndroidManifest.xml manifest:package attribute is not declared
	at com.android.build.gradle.tasks.ProcessTestManifest.handleMergingResult(ProcessTestManifest.kt:315)
	at com.android.build.gradle.tasks.ProcessTestManifest.mergeManifestsForTestVariant(ProcessTestManifest.kt:266)
	at com.android.build.gradle.tasks.ProcessTestManifest.doFullTaskAction(ProcessTestManifest.kt:106)
	at com.android.build.gradle.internal.tasks.IncrementalTask.handleIncrementalInputs(IncrementalTask.kt:110)
	at com.android.build.gradle.internal.tasks.IncrementalTask.access$handleIncrementalInputs(IncrementalTask.kt:65)
	at com.android.build.gradle.internal.tasks.IncrementalTask$taskAction$$inlined$recordTaskAction$1.invoke(BaseTask.kt:73)
	at com.android.build.gradle.internal.tasks.IncrementalTask$taskAction$$inlined$recordTaskAction$1.invoke(BaseTask.kt:36)
	at com.android.build.gradle.internal.tasks.Blocks.recordSpan(Blocks.java:91)
	at com.android.build.gradle.internal.tasks.IncrementalTask.taskAction$gradle(IncrementalTask.kt:137)
	at jdk.internal.reflect.GeneratedMethodAccessor862.invoke(Unknown Source)
	at java.base...

This is with gradle 6.7 and AGP version 4.2.0-alpha06

je...@google.com <je...@google.com> #14Oct 28, 2020 05:13PM

yes this will be a change of behavior, first we need to add DSL elements so you can set it there and then we will stop failing loading manifest files that do not have a package declaration.

sp...@google.com <sp...@google.com> #15Oct 28, 2020 05:45PM

Ah, I hit the same error with AGP 4.2.0-alpha06, but I don't hit it with 4.2.0-alpha13.

It looks like the reason is because http://ag/12514624 was merged in 4.2.0-alpha13.

sp...@google.com <sp...@google.com> #16Nov 4, 2020 07:00PM

I filed Issue 172361895 for comment #12.

au...@google.com <au...@google.com> #17Nov 7, 2020 01:03AM

Verified that with 4.2.0-alpha15 we are able to remove the manifest package, however, I do think this should be a clearer warning or it should have no warning.

sp...@google.com <sp...@google.com> #18Nov 9, 2020 05:30PM

sp...@google.com <sp...@google.com> May 5, 2021 06:02PM

Reassigned to ji...@google.com.

ji...@google.com <ji...@google.com> May 7, 2021 08:33PM

Marked as fixed.

au...@google.com <au...@google.com> #19May 7, 2021 08:49PM

When should the fix ship? Will it be in the next 7.0.0-* release? can we make sure to cherrypick it?