Export ProGuard specs from java_import.

Background:
JAR files can bundle ProGuard specs under `META-INF/proguard/`
[See https://developer.android.com/studio/build/shrink-code]

Problem:
Bazel previously erroniously ignored these ProGuard specs, leading to failures with, for example, androidx.annotation.Keep. Bad times.

There was previously a parallel issue with aar_import.
[Fixed in bazelbuild#12749]

Solution:
This change causes the previously ignored, embedded proguards to be extracted, validated, and then bubbled up correctly via the ProguardSpecProvider.

There's also a minor fix to aar_import, adding proguard validation and slightly simplifying the resulting code. For reasoning behind why library proguards should be validated, see the module docstring of proguard_whitelister.py

Remaining issues:
JAR files brought down from Maven via rules_jvm_external bypass java_import in favor of rolling their own jvm_import, since java_import apparently been broken for Kotlin for years. That'll need a subsequent fix, since this only fixes java_import. For context on the Kotlin breakage, see bazelbuild#4549. For the status on fixes in rules_jvm_external, see bazel-contrib/rules_jvm_external#672

src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java

@@ -256,44 +256,22 @@ private static NestedSet<Artifact> getCompileTimeJarsFromCollection(
     return isDirect ? provider.getDirectCompileTimeJars() : provider.getTransitiveCompileTimeJars();
   }
 
-  /**
-   * Collect Proguard Specs from transitives and proguard.txt if it exists in the AAR file. In the
-   * case the proguard.txt file does exists, we need to extract it from the AAR file
-   */
+  /* Collects transitive and local ProGuard specs, including any embedded in the AAR. */
   private NestedSet<Artifact> extractProguardSpecs(RuleContext ruleContext, Artifact aar) {
-
-    NestedSet<Artifact> proguardSpecs =
-        new ProguardLibrary(ruleContext).collectProguardSpecs(ImmutableSet.of("deps", "exports"));
-
-    Artifact proguardSpecArtifact = createAarArtifact(ruleContext, PROGUARD_SPEC);
-
+    Artifact extractedSpec = createAarArtifact(ruleContext, PROGUARD_SPEC);
     ruleContext.registerAction(
-        createAarEmbeddedProguardExtractorActions(ruleContext, aar, proguardSpecArtifact));
-
-    NestedSetBuilder<Artifact> builder = NestedSetBuilder.naiveLinkOrder();
-    return builder.addTransitive(proguardSpecs).add(proguardSpecArtifact).build();
-  }
-
-  /**
-   * Creates action to extract embedded Proguard.txt from an AAR. If the file is not found, an empty
-   * file will be created
-   */
-  private static SpawnAction createAarEmbeddedProguardExtractorActions(
-      RuleContext ruleContext, Artifact aar, Artifact proguardSpecArtifact) {
-    return new SpawnAction.Builder()
-        .useDefaultShellEnvironment()
-        .setExecutable(
-            ruleContext.getExecutablePrerequisite(AarImportBaseRule.AAR_EMBEDDED_PROGUARD_EXTACTOR))
+        new SpawnAction.Builder().useDefaultShellEnvironment()
+        .setExecutable(ruleContext.getExecutablePrerequisite(
+            AarImportBaseRule.AAR_EMBEDDED_PROGUARD_EXTACTOR))
         .setMnemonic("AarEmbeddedProguardExtractor")
         .setProgressMessage("Extracting proguard.txt from %s", aar.getFilename())
         .addInput(aar)
-        .addOutput(proguardSpecArtifact)
-        .addCommandLine(
-            CustomCommandLine.builder()
-                .addExecPath("--input_aar", aar)
-                .addExecPath("--output_proguard_file", proguardSpecArtifact)
-                .build())
-        .build(ruleContext);
+        .addOutput(extractedSpec)
+        .addCommandLine(CustomCommandLine.builder()
+            .addExecPath("--input_aar", aar)
+            .addExecPath("--output_proguard_file", extractedSpec).build())
+        .build(ruleContext));
+    return new ProguardLibrary(ruleContext).collectProguardSpecs(extractedSpec);
   }
 
   private NestedSet<Artifact> getBootclasspath(RuleContext ruleContext) {

src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java

@@ -19,6 +19,8 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
+import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
+import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.OutputGroupInfo;
@@ -121,7 +123,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
               .build());
     }
 
-    NestedSet<Artifact> proguardSpecs = new ProguardLibrary(ruleContext).collectProguardSpecs();
+    NestedSet<Artifact> proguardSpecs = collectProguardSpecs(ruleContext, jars);
 
     JavaRuleOutputJarsProvider ruleOutputJarsProvider = ruleOutputJarsProviderBuilder.build();
     JavaSourceJarsProvider sourceJarsProvider =
@@ -219,4 +221,23 @@ private ImmutableList<Artifact> processWithIjarIfNeeded(
     }
     return interfaceJarsBuilder.build();
   }
+
+  /* Collects transitive and local ProGuard specs, including any embedded in the JARs. */
+  private NestedSet<Artifact> collectProguardSpecs(
+      RuleContext ruleContext, ImmutableList<Artifact> jars) {
+    Artifact extractedSpec = ruleContext.getUniqueDirectoryArtifact(
+        "_jar_import", "proguard.txt", ruleContext.getBinOrGenfilesDirectory());
+    ruleContext.registerAction(
+      new SpawnAction.Builder().useDefaultShellEnvironment()
+          .setMnemonic("JarEmbeddedProguardExtractor")
+          .setProgressMessage("Extracting ProGuard specs from JARs in %s",
+              ruleContext.getTarget().getLabel())
+          .setExecutable(ruleContext.getExecutablePrerequisite("$jar_embedded_proguard_extractor"))
+          .addCommandLine(
+              CustomCommandLine.builder().addExecPaths(jars).addExecPath(extractedSpec).build())
+          .addInputs(jars)
+          .addOutput(extractedSpec)
+          .build(ruleContext));
+    return new ProguardLibrary(ruleContext).collectProguardSpecs(extractedSpec);
+  }
 }

src/main/java/com/google/devtools/build/lib/rules/java/JavaImportBaseRule.java

@@ -21,6 +21,7 @@
 import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
 
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
+import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.packages.RuleClass;
@@ -63,6 +64,11 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
                 .orderIndependent()
                 .nonconfigurable(
                     "used in Attribute.validityPredicate implementations (loading time)"))
+        .add(
+            attr("$jar_embedded_proguard_extractor", LABEL)
+                .cfg(ExecutionTransitionFactory.create())
+                .exec()
+              .value(environment.getToolsLabel("//tools/jdk:jar_embedded_proguard_extractor")))
         .advertiseStarlarkProvider(StarlarkProviderIdentifier.forKey(JavaInfo.PROVIDER.getKey()))
         .build();
   }

src/main/java/com/google/devtools/build/lib/rules/java/ProguardLibrary.java

@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.BuildType;
 import java.util.Collection;
+import java.util.Collections;
 
 /**
  * Helpers for implementing rules which export Proguard specs.
@@ -51,19 +52,47 @@ public NestedSet<Artifact> collectProguardSpecs() {
     return collectProguardSpecs(DEPENDENCY_ATTRIBUTES);
   }
 
+  /** 
+   * Collects the validated proguard specs exported by this rule and its dependencies, adding an
+   * additional local spec from outside of the normal attributes, e.g., embedded in a JAR or AAR.
+   */
+  public NestedSet<Artifact> collectProguardSpecs(Artifact additionalLocalSpec) {
+    return collectProguardSpecs(DEPENDENCY_ATTRIBUTES, additionalLocalSpec);
+  }
+
   /**
    * Collects the validated proguard specs exported by this rule and its dependencies through the
    * given attributes.
    */
   public NestedSet<Artifact> collectProguardSpecs(Iterable<String> attributes) {
+    return collectProguardSpecs(attributes, Collections.emptySet());
+  }
+
+  /**
+   * Collects the validated proguard specs exported by this rule and its dependencies through the
+   * given attributes, adding an additional local spec from outside of the normal attributes, e.g.,
+   * embedded in a JAR or AAR.
+   */
+  public NestedSet<Artifact> collectProguardSpecs(
+      Iterable<String> attributes, Artifact additionalLocalSpec) {
+    return collectProguardSpecs(attributes, Collections.singleton(additionalLocalSpec));
+  }
+
+  /**
+   * Collects the validated proguard specs exported by this rule and its dependencies through the
+   * given attributes, adding additional local specs from outside of the normal attributes, e.g.,
+   * embedded in a JAR or AAR.
+   */
+  public NestedSet<Artifact> collectProguardSpecs(
+      Iterable<String> attributes, Iterable<Artifact> additionalLocalSpecs) {
     NestedSetBuilder<Artifact> specsBuilder = NestedSetBuilder.naiveLinkOrder();
 
     for (String attribute : attributes) {
       specsBuilder.addTransitive(collectProguardSpecsFromAttribute(attribute));
     }
 
     Collection<Artifact> localSpecs = collectLocalProguardSpecs();
-    if (!localSpecs.isEmpty()) {
+    if (!localSpecs.isEmpty() || additionalLocalSpecs.iterator().hasNext()) {
       // Pass our local proguard configs through the validator, which checks an allowlist.
       FilesToRunProvider proguardAllowlister =
           JavaToolchainProvider.from(ruleContext).getProguardAllowlister();
@@ -75,6 +104,9 @@ public NestedSet<Artifact> collectProguardSpecs(Iterable<String> attributes) {
       for (Artifact specToValidate : localSpecs) {
         specsBuilder.add(validateProguardSpec(ruleContext, proguardAllowlister, specToValidate));
       }
+      for (Artifact specToValidate : additionalLocalSpecs) {
+        specsBuilder.add(validateProguardSpec(ruleContext, proguardAllowlister, specToValidate));
+      }
     }
 
     return specsBuilder.build();

src/test/java/com/google/devtools/build/lib/rules/android/AarImportTest.java

@@ -166,30 +166,10 @@ public void proguardSpecsProvided() throws Exception {
                 .map(Artifact::getRootRelativePathString)
                 .collect(Collectors.toSet()))
         .containsExactly(
-            "a/_aar/bar/proguard.txt", "a/_aar/foo/proguard.txt", "a/_aar/baz/proguard.txt");
-  }
-
-  @Test
-  public void testProguardExtractor() throws Exception {
-    ConfiguredTarget target = getConfiguredTarget("//a:bar");
-    Artifact proguardSpecsAritfact =
-        target.get(ProguardSpecProvider.PROVIDER).getTransitiveProguardSpecs().toList().get(0);
-
-    Artifact aarProguardExtractor =
-        getDirectPrerequisite(
-                target,
-                ruleClassProvider.getToolsRepository()
-                    + "//tools/android:aar_embedded_proguard_extractor")
-            .getProvider(FilesToRunProvider.class)
-            .getExecutable();
-
-    assertThat(getGeneratingSpawnAction(proguardSpecsAritfact).getArguments())
-        .containsExactly(
-            aarProguardExtractor.getExecPathString(),
-            "--input_aar",
-            "a/bar.aar",
-            "--output_proguard_file",
-            proguardSpecsAritfact.getExecPathString());
+            "a/validated_proguard/bar/a/_aar/bar/proguard.txt_valid",
+            "java/validated_proguard/baz/java/_jar_import/baz/proguard.txt_valid",
+            "a/validated_proguard/baz/a/_aar/baz/proguard.txt_valid",
+            "a/validated_proguard/foo/a/_aar/foo/proguard.txt_valid");
   }
 
   @Test

tools/jdk/BUILD

@@ -20,6 +20,7 @@ filegroup(
         "jdk.BUILD",
         "local_java_repository.bzl",
         "nosystemjdk/README",
+        "jar_embedded_proguard_extractor.py",
         "proguard_whitelister.py",
         "proguard_whitelister_test.py",
         "proguard_whitelister_test_input.pgcfg",

tools/jdk/BUILD.tools

@@ -419,6 +419,11 @@ filegroup(
     visibility = ["//tools:__pkg__"],
 )
 
+py_binary(
+    name = "jar_embedded_proguard_extractor",
+    srcs = ["jar_embedded_proguard_extractor.py"],
+)
+
 py_binary(
     name = "proguard_whitelister",
     srcs = [

tools/jdk/jar_embedded_proguard_extractor.py

@@ -0,0 +1,40 @@
+# Copyright 2022 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""A tool for extracting proguard specs from JARs.
+
+Usage:
+Takes positional arguments: input_jar1 input_jar2 <...> output_proguard_spec
+
+Background: 
+A JAR may contain proguard specs under /META-INF/proguard/
+[https://developer.android.com/studio/build/shrink-code]
+
+This tool extracts them all into a single spec for easy usage."""
+
+
+import sys
+import zipfile
+
+
+if __name__ == "__main__":
+    with open(sys.argv[-1], 'wb') as output_proguard_spec:
+        for jar_path in sys.argv[1:-1]:
+            with zipfile.ZipFile(jar_path) as jar:
+                for entry in jar.namelist():
+                    if entry.startswith('META-INF/proguard'):
+                        # zip directories are empty and therefore ok to output
+                        output_proguard_spec.write(jar.read(entry))
+                        # gracefully handle any lack of trailing newline
+                        output_proguard_spec.write(b'\n')