Collect logic related to extra Proguard outputs in one place.

--
MOS_MIGRATED_REVID=126120891

6 files changed, 144 insertions, 71 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
index c9530a6c69..0048c1565f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
@@ -412,6 +412,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
         applyProguard(
             ruleContext,
             androidCommon,
+            javaSemantics,
             binaryJar,
             filesBuilder,
             proguardSpecs,
@@ -678,7 +679,8 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
 
     if (proguardOutput.getMapping() != null) {
       builder.add(ProguardMappingProvider.class,
-          new ProguardMappingProvider(proguardOutput.getMapping()));
+          new ProguardMappingProvider(proguardOutput.getMapping(),
+                                      proguardOutput.getProtoMapping()));
     }
 
     return builder
@@ -884,6 +886,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
   private static ProguardOutput applyProguard(
       RuleContext ruleContext,
       AndroidCommon common,
+      JavaSemantics javaSemantics,
       Artifact deployJarArtifact,
       NestedSetBuilder<Artifact> filesBuilder,
       ImmutableList<Artifact> proguardSpecs,
@@ -898,7 +901,8 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
       // still have a Proguard jar implicit output, as it is impossible to tell what a select will
       // produce at the time of implicit output determination. As a result, this artifact must
       // always be created.
-      return createEmptyProguardAction(ruleContext, proguardOutputJar, deployJarArtifact);
+      return createEmptyProguardAction(ruleContext, javaSemantics, proguardOutputJar,
+                                       deployJarArtifact);
     }
 
     AndroidSdkProvider sdk = AndroidSdkProvider.fromRuleContext(ruleContext);
@@ -914,7 +918,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
         proguardMapping,
         libraryJars,
         proguardOutputJar,
-        ruleContext.attributes().get("proguard_generate_mapping", Type.BOOLEAN),
+        javaSemantics,
         getProguardOptimizationPasses(ruleContext));
     // Since Proguard is being run, add its output artifacts to the given filesBuilder
     result.addAllToSet(filesBuilder);
@@ -931,23 +935,22 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
   }
 
   private static ProguardOutput createEmptyProguardAction(RuleContext ruleContext,
-      Artifact proguardOutputJar, Artifact deployJarArtifact) throws InterruptedException {
-    ImmutableList.Builder<Artifact> failures = ImmutableList.<Artifact>builder()
-        .add(proguardOutputJar)
-        .add(ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_PROGUARD_CONFIG));
-    if (ruleContext.attributes().get("proguard_generate_mapping", Type.BOOLEAN)) {
-      failures.add(ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_PROGUARD_MAP));
-    }
+      JavaSemantics semantics, Artifact proguardOutputJar, Artifact deployJarArtifact)
+          throws InterruptedException {
+    NestedSetBuilder<Artifact> failures = NestedSetBuilder.<Artifact>stableOrder();
+    ProguardOutput outputs =
+        ProguardHelper.getProguardOutputs(proguardOutputJar, ruleContext, semantics);
+    outputs.addAllToSet(failures);
     JavaOptimizationMode optMode = getJavaOptimizationMode(ruleContext);
     ruleContext.registerAction(
         new FailAction(
             ruleContext.getActionOwner(),
             failures.build(),
-            String.format("Can't generate Proguard jar or mapping %s.",
+            String.format("Can't run Proguard %s",
                 optMode == JavaOptimizationMode.LEGACY
                     ? "without proguard_specs"
                     : "in optimization mode " + optMode)));
-    return new ProguardOutput(deployJarArtifact, null);
+    return new ProguardOutput(deployJarArtifact, null, null, null);
   }
 
   private static ResourceApk shrinkResources(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
index 0f2008b569..6d116f1970 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
@@ -49,7 +49,7 @@ import com.google.devtools.build.lib.rules.cpp.CppOptions;
 import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider;
 import com.google.devtools.build.lib.rules.java.JavaConfiguration;
 import com.google.devtools.build.lib.rules.java.JavaSemantics;
-import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.rules.java.ProguardHelper;
 import com.google.devtools.build.lib.util.FileType;
 
 import java.util.ArrayList;
@@ -259,42 +259,42 @@ public final class AndroidRuleClasses {
       "proto_library" // TODO(gregce): remove this line when no such dependencies exist
   };
 
+  public static final boolean hasProguardSpecs(AttributeMap rule) {
+    // The below is a hack to support configurable attributes (proguard_specs seems like
+    // too valuable an attribute to make nonconfigurable, and we don't currently
+    // have the ability to know the configuration when determining implicit outputs).
+    // An IllegalArgumentException gets triggered if the attribute instance is configurable.
+    // We assume, heuristically, that means every configurable value is a non-empty list.
+    //
+    // TODO(bazel-team): find a stronger approach for this. One simple approach is to somehow
+    // receive 'rule' as an AggregatingAttributeMapper instead of a RawAttributeMapper,
+    // check that all possible values are non-empty, and simply don't support configurable
+    // instances that mix empty and non-empty lists. A more ambitious approach would be
+    // to somehow determine implicit outputs after the configuration is known. A third
+    // approach is to refactor the Android rule logic to avoid these dependencies in the
+    // first place.
+    try {
+      return !rule.get("proguard_specs", LABEL_LIST).isEmpty();
+    } catch (IllegalArgumentException e) {
+      // We assume at this point the attribute instance is configurable.
+      return true;
+    }
+  }
+
   public static final SafeImplicitOutputsFunction ANDROID_BINARY_IMPLICIT_OUTPUTS =
       new SafeImplicitOutputsFunction() {
 
         @Override
         public Iterable<String> getImplicitOutputs(AttributeMap rule) {
-          boolean mapping = rule.get("proguard_generate_mapping", Type.BOOLEAN);
           List<SafeImplicitOutputsFunction> functions = Lists.newArrayList();
           functions.add(AndroidRuleClasses.ANDROID_BINARY_APK);
           functions.add(AndroidRuleClasses.ANDROID_BINARY_UNSIGNED_APK);
           functions.add(AndroidRuleClasses.ANDROID_BINARY_DEPLOY_JAR);
 
-          // The below is a hack to support configurable attributes (proguard_specs seems like
-          // too valuable an attribute to make nonconfigurable, and we don't currently
-          // have the ability to know the configuration when determining implicit outputs).
-          // An IllegalArgumentException gets triggered if the attribute instance is configurable.
-          // We assume, heuristically, that means every configurable value is a non-empty list.
-          //
-          // TODO(bazel-team): find a stronger approach for this. One simple approach is to somehow
-          // receive 'rule' as an AggregatingAttributeMapper instead of a RawAttributeMapper,
-          // check that all possible values are non-empty, and simply don't support configurable
-          // instances that mix empty and non-empty lists. A more ambitious approach would be
-          // to somehow determine implicit outputs after the configuration is known. A third
-          // approach is to refactor the Android rule logic to avoid these dependencies in the
-          // first place.
-          boolean hasProguardSpecs;
-          try {
-            hasProguardSpecs = !rule.get("proguard_specs", LABEL_LIST).isEmpty();
-          } catch (IllegalArgumentException e) {
-            // We assume at this point the attribute instance is configurable.
-            hasProguardSpecs = true;
-          }
-
-          if (hasProguardSpecs) {
+          if (hasProguardSpecs(rule)) {
             functions.add(AndroidRuleClasses.ANDROID_BINARY_PROGUARD_JAR);
             functions.add(JavaSemantics.JAVA_BINARY_PROGUARD_CONFIG);
-            if (mapping) {
+            if (ProguardHelper.genProguardMapping(rule)) {
               functions.add(JavaSemantics.JAVA_BINARY_PROGUARD_MAP);
             }
           }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ProguardMappingProvider.java b/src/main/java/com/google/devtools/build/lib/rules/android/ProguardMappingProvider.java
index 5ac576407f..68287cdeaa 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ProguardMappingProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ProguardMappingProvider.java
@@ -24,12 +24,18 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 public final class ProguardMappingProvider implements TransitiveInfoProvider {
 
   private final Artifact proguardMapping;
+  private final Artifact proguardProtoMapping;
 
-  public ProguardMappingProvider(Artifact proguardMapping) {
+  public ProguardMappingProvider(Artifact proguardMapping, Artifact proguardProtoMapping) {
     this.proguardMapping = proguardMapping;
+    this.proguardProtoMapping = proguardProtoMapping;
   }
 
   public Artifact getProguardMapping() {
     return proguardMapping;
   }
+
+  public Artifact getProguardProtoMapping() {
+    return proguardProtoMapping;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java
index 5551e2144f..4f83c0c2e2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java
@@ -43,6 +43,7 @@ import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CppHelper;
 import com.google.devtools.build.lib.rules.cpp.LinkerInput;
 import com.google.devtools.build.lib.rules.java.JavaCompilationArgs.ClasspathType;
+import com.google.devtools.build.lib.rules.java.ProguardHelper.ProguardOutput;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
@@ -245,7 +246,7 @@ public class JavaBinary implements RuleConfiguredTargetFactory {
     Artifact deployJar =
         ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_DEPLOY_JAR);
     boolean runProguard = applyProguardIfRequested(
-        ruleContext, deployJar, common.getBootClasspath(), mainClass, filesBuilder);
+        ruleContext, deployJar, common.getBootClasspath(), mainClass, semantics, filesBuilder);
 
     NestedSet<Artifact> filesToBuild = filesBuilder.build();
 
@@ -484,15 +485,15 @@ public class JavaBinary implements RuleConfiguredTargetFactory {
    * {@link ProguardHelper#applyProguardIfRequested}.
    */
   private static boolean applyProguardIfRequested(RuleContext ruleContext, Artifact deployJar,
-      ImmutableList<Artifact> bootclasspath, String mainClassName,
+      ImmutableList<Artifact> bootclasspath, String mainClassName, JavaSemantics semantics,
       NestedSetBuilder<Artifact> filesBuilder) throws InterruptedException {
     // We only support proguarding tests so Proguard doesn't try to proguard itself.
     if (!ruleContext.getRule().getRuleClass().endsWith("_test")) {
       return false;
     }
-    ProguardHelper.ProguardOutput output =
+    ProguardOutput output =
         JavaBinaryProguardHelper.INSTANCE.applyProguardIfRequested(
-            ruleContext, deployJar, bootclasspath, mainClassName);
+            ruleContext, deployJar, bootclasspath, mainClassName, semantics);
     if (output == null) {
       return false;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
index e6d1e90909..cecb24b515 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
@@ -70,6 +70,8 @@ public interface JavaSemantics {
       fromTemplates("%{name}_deploy.jar.unstripped");
   SafeImplicitOutputsFunction JAVA_BINARY_PROGUARD_MAP =
       fromTemplates("%{name}_proguard.map");
+  SafeImplicitOutputsFunction JAVA_BINARY_PROGUARD_PROTO_MAP =
+      fromTemplates("%{name}_proguard.pbmap");
   SafeImplicitOutputsFunction JAVA_BINARY_PROGUARD_CONFIG =
       fromTemplates("%{name}_proguard.config");
 
@@ -364,6 +366,13 @@ public interface JavaSemantics {
    */
   String getJavaBuilderMainClass();
 
+
+  /**
+   * @return An artifact representing the protobuf-format version of the
+   * proguard mapping, or null if the proguard version doesn't support this.
+   */
+  Artifact getProtoMapping(RuleContext ruleContext) throws InterruptedException;
+
   /**
    * @return Label of pseudo-cc_binary that tells Blaze a java target's JAVABIN is never to be
    * replaced by the contents of --java_launcher; only the JDK's launcher will ever be used.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/ProguardHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/ProguardHelper.java
index 70242be7cf..e4daee2693 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/ProguardHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/ProguardHelper.java
@@ -28,8 +28,10 @@ import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction.Builder;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.rules.java.JavaConfiguration.JavaOptimizationMode;
+import com.google.devtools.build.lib.syntax.Type;
 
 import javax.annotation.Nullable;
 
@@ -44,16 +46,23 @@ public abstract class ProguardHelper {
   public static final String PROGUARD_SPECS = "proguard_specs";
 
   /**
-   * Pair summarizing Proguard's output: a Jar file and an optional obfuscation mapping file.
+   * A class collecting Proguard output artifacts.
    */
   @Immutable
   public static final class ProguardOutput {
     private final Artifact outputJar;
     @Nullable private final Artifact mapping;
+    @Nullable private final Artifact protoMapping;
+    private final Artifact config;
 
-    public ProguardOutput(Artifact outputJar, @Nullable Artifact mapping) {
+    public ProguardOutput(Artifact outputJar,
+                          @Nullable Artifact mapping,
+                          @Nullable Artifact protoMapping,
+                          Artifact config) {
       this.outputJar = checkNotNull(outputJar);
       this.mapping = mapping;
+      this.protoMapping = protoMapping;
+      this.config = config;
     }
 
     public Artifact getOutputJar() {
@@ -65,12 +74,27 @@ public abstract class ProguardHelper {
       return mapping;
     }
 
+    @Nullable
+    public Artifact getProtoMapping() {
+      return protoMapping;
+    }
+
+    public Artifact getConfig() {
+      return config;
+    }
+
     /** Adds the output artifacts to the given set builder. */
     public void addAllToSet(NestedSetBuilder<Artifact> filesBuilder) {
       filesBuilder.add(outputJar);
       if (mapping != null) {
         filesBuilder.add(mapping);
       }
+      if (protoMapping != null) {
+        filesBuilder.add(protoMapping);
+      }
+      if (config != null) {
+        filesBuilder.add(config);
+      }
     }
   }
 
@@ -92,7 +116,8 @@ public abstract class ProguardHelper {
       RuleContext ruleContext,
       Artifact deployJar,
       ImmutableList<Artifact> bootclasspath,
-      String mainClassName) throws InterruptedException {
+      String mainClassName,
+      JavaSemantics semantics) throws InterruptedException {
     JavaOptimizationMode optMode = getJavaOptimizationMode(ruleContext);
     if (optMode == JavaOptimizationMode.NOOP || optMode == JavaOptimizationMode.LEGACY) {
       // For simplicity do nothing in LEGACY mode
@@ -111,7 +136,7 @@ public abstract class ProguardHelper {
     Artifact singleJar =
         ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_MERGED_JAR);
     return createProguardAction(ruleContext, proguard, singleJar, proguardSpecs, (Artifact) null,
-        bootclasspath, deployJar, /* mappingRequested */ false, /* optimizationPasses */ 3);
+        bootclasspath, deployJar, semantics, /* optimizationPases */ 3);
   }
 
   private ImmutableList<Artifact> collectProguardSpecs(
@@ -216,6 +241,37 @@ public abstract class ProguardHelper {
   }
 
   /**
+   * @return true if proguard_generate_mapping is specified.
+   */
+  public static final boolean genProguardMapping(AttributeMap rule) {
+      return rule.has("proguard_generate_mapping", Type.BOOLEAN)
+          && rule.get("proguard_generate_mapping", Type.BOOLEAN);
+  }
+
+  public static ProguardOutput getProguardOutputs(
+      Artifact outputJar, RuleContext ruleContext, JavaSemantics semantics)
+      throws InterruptedException {
+    JavaOptimizationMode optMode = getJavaOptimizationMode(ruleContext);
+    boolean mappingRequested = genProguardMapping(ruleContext.attributes());
+
+    Artifact proguardOutputMap = null;
+    Artifact proguardOutputProtoMap = null;
+    if (mappingRequested || optMode.alwaysGenerateOutputMapping()) {
+      // TODO(bazel-team): Verify that proguard spec files don't contain -printmapping directions
+      // which this -printmapping command line flag will override.
+      proguardOutputMap =
+          ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_PROGUARD_MAP);
+      proguardOutputProtoMap = semantics.getProtoMapping(ruleContext);
+    }
+
+    Artifact proguardConfigOutput =
+        ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_PROGUARD_CONFIG);
+
+    return new ProguardOutput(
+        outputJar, proguardOutputMap, proguardOutputProtoMap, proguardConfigOutput);
+  }
+
+  /**
    * Creates an action to run Proguard over the given {@code programJar} with various other given
    * inputs to produce {@code proguardOutputJar}.  If requested explicitly, or implicitly with
    * --java_optimization_mode, the action also produces a mapping file (which shows what methods and
@@ -240,26 +296,14 @@ public abstract class ProguardHelper {
       @Nullable Artifact proguardMapping,
       Iterable<Artifact> libraryJars,
       Artifact proguardOutputJar,
-      boolean mappingRequested,
+      JavaSemantics semantics,
       @Nullable Integer optimizationPasses) throws InterruptedException {
-
     JavaOptimizationMode optMode = getJavaOptimizationMode(ruleContext);
     Preconditions.checkArgument(optMode != JavaOptimizationMode.NOOP);
     Preconditions.checkArgument(optMode != JavaOptimizationMode.LEGACY || !proguardSpecs.isEmpty());
 
-    Artifact proguardOutputMap;
-    if (mappingRequested || optMode.alwaysGenerateOutputMapping()) {
-      // TODO(bazel-team): Verify that proguard spec files don't contain -printmapping directions
-      // which this -printmapping command line flag will override.
-      proguardOutputMap =
-          ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_PROGUARD_MAP);
-    } else {
-      proguardOutputMap = null;
-    }
-
-    Artifact proguardConfigOutput =
-        ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_PROGUARD_CONFIG);
-
+    ProguardOutput output = getProguardOutputs(proguardOutputJar, ruleContext, semantics);
+    
     if (optimizationPasses == null) {
       // Run proguard as a single step.
       Builder builder = makeBuilder(
@@ -268,16 +312,16 @@ public abstract class ProguardHelper {
           proguardSpecs,
           proguardMapping,
           libraryJars,
-          proguardOutputJar,
-          proguardOutputMap,
-          proguardConfigOutput)
+          output.getOutputJar(),
+          output.getMapping(),
+          output.getProtoMapping(),
+          output.getConfig())
           .setProgressMessage("Trimming binary with Proguard")
           .addOutput(proguardOutputJar);
 
       ruleContext.registerAction(builder.build(ruleContext));
     } else {
       // Optimization passes have been specified, so run proguard in multiple phases.
-
       Artifact lastStageOutput = getProguardTempArtifact(
           ruleContext, optMode.name().toLowerCase(), "proguard_preoptimization.jar");
       ruleContext.registerAction(
@@ -287,8 +331,9 @@ public abstract class ProguardHelper {
               proguardSpecs,
               proguardMapping,
               libraryJars,
-              proguardOutputJar,
+              output.getOutputJar(),
               /* proguardOutputMap */ null,
+              /* proguardOutputProtoMap */ null,
               /* proguardConfigOutput */ null)
               .setProgressMessage("Trimming binary with Proguard: Verification/Shrinking Pass")
               .addArgument("-runtype INITIAL")
@@ -306,8 +351,9 @@ public abstract class ProguardHelper {
                 proguardSpecs,
                 proguardMapping,
                 libraryJars,
-                proguardOutputJar,
+                output.getOutputJar(),
                 /* proguardOutputMap */ null,
+                /* proguardOutputProtoMap */ null,
                 /* proguardConfigOutput */ null)
                 .setProgressMessage("Trimming binary with Proguard: Optimization Pass " + (i + 1))
                 .addArgument("-runtype OPTIMIZATION")
@@ -325,9 +371,10 @@ public abstract class ProguardHelper {
           proguardSpecs,
           proguardMapping,
           libraryJars,
-          proguardOutputJar,
-          proguardOutputMap,
-          proguardConfigOutput)
+          output.getOutputJar(),
+          output.getMapping(),
+          output.getProtoMapping(),
+          output.getConfig())
           .setProgressMessage("Trimming binary with Proguard: Obfuscation and Final Ouput Pass")
           .addArgument("-runtype FINAL")
           .addArgument("-laststageoutput")
@@ -337,7 +384,7 @@ public abstract class ProguardHelper {
       ruleContext.registerAction(builder.build(ruleContext));
     }
 
-    return new ProguardOutput(proguardOutputJar, proguardOutputMap);
+    return output;
   }
 
   private static Builder makeBuilder(
@@ -348,6 +395,7 @@ public abstract class ProguardHelper {
       Iterable<Artifact> libraryJars,
       Artifact proguardOutputJar,
       @Nullable Artifact proguardOutputMap,
+      @Nullable Artifact proguardOutputProtoMap,
       @Nullable Artifact proguardConfigOutput) {
 
     Builder builder = new SpawnAction.Builder()
@@ -388,6 +436,12 @@ public abstract class ProguardHelper {
           .addOutputArgument(proguardOutputMap);
     }
 
+    if (proguardOutputProtoMap != null) {
+      builder
+          .addArgument("-protomapping")
+          .addOutputArgument(proguardOutputProtoMap);
+    }
+
     if (proguardConfigOutput != null) {
       builder
           .addArgument("-printconfiguration")