AAR that contains R.class has that class overwritten by Buck assumptions

[jhansche]

The scenario here is an AAR file that has no resources in its res/ directory, and has no R.txt file. The AAR has a classes.jar that includes an R.class that was created as a result of what appears to be an obfuscation pass (proguard or otherwise) -- that assumption is made based on the fact that there are several other adjacent classes with single-character names: Q.class, R.class, S.class, etc. Ordinarily, if there is no R.txt, then no resources-related R.class should be generated for it (and this works correctly in a Gradle build, for example: there is no build/generated/source/r/debug/com/crashlytics/android/R.java file, because the AAR contains no R.txt).

Buck appears to be assuming that all AAR files require an R.class, even if it has an empty res/ and missing R.txt, so it ends up generating an empty R.class for that package. However, in this case, that class ends up overwriting the R class that already exists in classes.jar. That causes an error with the caching system, reporting an error due to "Multiple entries with the same key":

Multiple entries with same key: com/crashlytics/android/R=3040495ca6cb51bd19a9878a5a73ae8093cdb0fd and com/crashlytics/android/R=2dd42ddb490ce969a59fcfd0e692f3ee1accafad

Checking the buck-out/ directory shows that the entries come from two different places:

buck-out//gen/aars__crashlytics-2.0.3#aar_prebuilt_jar.classes.txt:com/crashlytics/android/R 2dd42ddb490ce969a59fcfd0e692f3ee1accafad
buck-out//bin/__app#aapt_package_rdotjava_classes__/classes.txt:com/crashlytics/android/R 3040495ca6cb51bd19a9878a5a73ae8093cdb0fd

Presumably, the one in #aar_prebuilt_jar.classes.txt is the true R.class that exists in the classes.jar of the AAR. The one in #aapt_package_rdotjava_classes__/classes.txt appears to be the automatically generated R.class as a result of the aapt resources processing.

In an effort to get past this error just to get the project to compile, I deleted the entry from the #aapt_package_ version, and that allowed it to continue to the smart_dex step. At this point I hit a GC overhead limit exceeded error (this is caused by dx not having a large enough heap size: my project requires a 3GB heap for dex to succeed). But that's a separate issue.

After adjusting the heap size in the dx shell script, the dex tool itself then hit an error with:

java.lang.IllegalArgumentException: already added: Lcom/crashlytics/android/R;

It's already added because it is still trying to add both the real (obfuscated) R.class and the generated (R.txt) R.class.

I'm not sure if this should be reported as an issue to the Crashlytics team (since having an R.class in the library's own package scope can be seen as risky and should probably be avoided). However, since all other build tools seem to work perfectly fine with this scenario, I'm going to assume that it's a bug in buck in that it generates the resources R.class for an AAR that does not want one.

An AAR that exhibits this behavior is the Crashlytics SDK (both versions 1.2.0 and 2.0.3). The URL to download the AAR from their Maven repository: crashlytics-2.0.3.aar.

[jhansche]

It looks like this is done intentionally in MergeAndroidResourcesStep#writeEmptyRDotJavaForPackages(). It seems the only reason this is done, however, is because the JavacStep has already been created (expecting the R.java paths to exist for each package that ultimately has no resources), before MergeAndroidResourcesStep can conclude that R.txt was empty to begin with:

    for (String rDotJavaPackage : rDotJavaPackages) {
      Path outputFile = getPathToRDotJava(rDotJavaPackage);
      filesystem.mkdirs(outputFile.getParent());
      filesystem.writeContentsToPath(
          String.format("package %s;\n\npublic class R {}\n", rDotJavaPackage),
          outputFile);
    }

If, however, that bogus R.java file is simply an empty file, everything works fine; the cache does not collide, the bogus/empty R.class is not created, and proguard and dex do not complain about duplicate classes:

    for (String rDotJavaPackage : rDotJavaPackages) {
      Path outputFile = getPathToRDotJava(rDotJavaPackage);
      filesystem.mkdirs(outputFile.getParent());
      filesystem.writeContentsToPath(
          String.format(""),
          outputFile);
    }

[bolinfest]

@jhansche Your assessment of the source of the bug looks correct to me. Again, I feel like we're getting unfairly punished since Google published a spec that says R.txt is required: http://tools.android.com/tech-docs/new-build-system/aar-format. In that sense, whether it's a bug in Buck versus a de facto standard in Gradle is a bit academic, and obviously unhelpful.

We know that Crashlytics is an important use case, so we just need to make this work. Thanks for the detailed report!

[jhansche]

Google published a spec that says R.txt is required: http://tools.android.com/tech-docs/new-build-system/aar-format.

Hrmm, indeed... You even pointed out that same format in my report about the unpacked res/ folder being missing, but I missed the fact that R.txt is also deemed mandatory!

I'll go ahead and report the issue to Twitter/Crashlytics team as well, given the deviation from the mandates of the AAR format. But their use case (obfuscation leading to an inevitable overlap of the R class) does not seem like a difficult one to be in, and should probably be something that even Proguard itself should avoid.

It does seem, however, that the other build systems seem to be quite lenient on the R.txt mandate, because other AAR libraries in my project that do not contain any res/ files or an R.txt file simply end up having no R class generated for them. Of course, that makes the library fragile as once they do add a resource of any kind, their library will then become broken.

This does highlight another difference between Buck and other build systems though: It seems that other build tools tend to completely ignore the R class in the AAR library's package, and instead all those library resources get repackaged into the main/host app's R class. For example, the appcompat-v7 library contains many resources, and in previous build tools (gradle), I could reference those resources using R.id.home, for example. With Buck, that no longer seems to be the case: the AAR's resources are not made available in the host app's R class, but I have to reference them by the library's package: android.support.v7.appcompat.R.id.home. In my opinion, that is much cleaner, and I'd prefer to have all those "borrowed" resources namespaced to represent what library the resource is being borrowed from.

[joshzana]

Bringing this up again - this is currently breaking my code. I want a single codebase that is buildable by both gradle and buck users.

My code depends on appCompat-v7 and consumes the following resource:

android.support.v7.appcompat.R.attr.colorPrimary

If I fully specify it like that, buck is happy, but gradle is not:

Error:(38, 55) error: package android.support.v7.appcompat.R does not exist

For the gradle build, I have to have:

com.mypackagename.R.attr.colorPrimary

But then buck is unhappy:
error: cannot find symbol
int[] attrs = { com.mypackagename.R.attr.colorPrimary, android.R.attr.actionBarSize };
^

Which I suspect is because in gradle land this resource actually gets merged into my own project's package.

This issue has been open for a while. Can we get an update? Any plans to address this?

[sdwilsh]

It hasn't been a priority for the core team, to be perfectly honest. If someone wanted to work on a PR, I'd be help to help get it merged in.

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

[sean-kenny]

@joshzana you may want to take a look at #643, we also had the same requirement of getting something to work in buck and bazel and were having a terrible time with letting go of resource conventions from gradle, specifically the simplicity of the resource merging it provides. So, we made a change that allowed an options for merging R.txt contents for android resources and deps. Using this approach we were able to build a very large app (with many many related android aar/project libraries), and still support both buck and gradle simultaneously with the same R naming conventions.

[v-jizhang]

Sorry we haven't update this issue for more than 3 years. At this point, we are closing this because it is quite out of date and it is not clear if the issue still exists. Please create a new one if you still see the problem. Thank you.