Extract java/javalite proto compilation args to command-line flags.

Addresses https://github.com/bazelbuild/bazel/issues/1718

--
MOS_MIGRATED_REVID=135274306

6 files changed, 174 insertions, 39 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index e00a95edef..18f6b93087 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.analysis.config;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
+import com.google.common.base.Splitter;
 import com.google.common.base.Verify;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ClassToInstanceMap;
@@ -217,6 +218,23 @@ public final class BuildConfiguration {
     }
   }
 
+  /** A converter from comma-separated strings to Label lists. */
+  public static class LabelListConverter implements Converter<List<Label>> {
+    @Override
+    public List<Label> convert(String input) throws OptionsParsingException {
+      ImmutableList.Builder result = ImmutableList.builder();
+      for (String label : Splitter.on(",").omitEmptyStrings().split(input)) {
+        result.add(convertLabel(label));
+      }
+      return result.build();
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "a build target label";
+    }
+  }
+
   /**
    * A converter that returns null if the input string is empty, otherwise it converts
    * the input to a label.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/proto/BazelJavaProtoAspect.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/proto/BazelJavaProtoAspect.java
index c2413e430f..ab062dd254 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/proto/BazelJavaProtoAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/proto/BazelJavaProtoAspect.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.bazel.rules.java.proto;
 
-import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.bazel.rules.java.BazelJavaSemantics;
@@ -31,9 +30,7 @@ public class BazelJavaProtoAspect extends JavaProtoAspect {
   public BazelJavaProtoAspect() {
     super(
         BazelJavaSemantics.INSTANCE,
-        ImmutableList.<String>of(),
         null, /* jacocoAttr */
-        ImmutableList.of("shared", "immutable"),
         new NoopRpcSupport());
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index 593254acaa..d8939e7bcb 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -1591,13 +1591,15 @@ public final class Attribute implements Comparable<Attribute> {
    */
   public abstract static class LateBoundLabelList<T> implements LateBoundDefault<T> {
     private final ImmutableList<Label> labels;
+    private final ImmutableSet<Class<?>> requiredConfigurationFragments;
 
     public LateBoundLabelList() {
-      this.labels = ImmutableList.of();
+      this(ImmutableList.<Label>of());
     }
 
-    public LateBoundLabelList(List<Label> labels) {
+    public LateBoundLabelList(List<Label> labels, Class<?>... requiredConfigurationFragments) {
       this.labels = ImmutableList.copyOf(labels);
+      this.requiredConfigurationFragments = ImmutableSet.copyOf(requiredConfigurationFragments);
     }
 
     @Override
@@ -1607,7 +1609,7 @@ public final class Attribute implements Comparable<Attribute> {
 
     @Override
     public ImmutableSet<Class<?>> getRequiredConfigurationFragments() {
-      return ImmutableSet.of();
+      return requiredConfigurationFragments;
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java
index cce0ab3735..5761dfe619 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java
@@ -30,10 +30,12 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMap;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.StrictDepsMode;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -42,7 +44,9 @@ import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.AspectDefinition;
 import com.google.devtools.build.lib.packages.AspectParameters;
 import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.NativeAspectClass;
+import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.rules.java.JavaCompilationArgs;
 import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider;
 import com.google.devtools.build.lib.rules.java.JavaConfiguration;
@@ -64,6 +68,21 @@ public class JavaLiteProtoAspect extends NativeAspectClass implements Configured
   public static final String LITE_PROTO_RUNTIME_ATTR = "$aspect_java_lib";
   public static final String LITE_PROTO_RUNTIME_LABEL = "//external:protobuf/javalite_runtime";
 
+  private static final Attribute.LateBoundLabel<BuildConfiguration> JAVA_LITE_PLUGIN =
+      new Attribute.LateBoundLabel<BuildConfiguration>((Label) null, ProtoConfiguration.class) {
+        @Override
+        public Label resolve(Rule rule, AttributeMap attributes, BuildConfiguration configuration) {
+          return configuration.getFragment(ProtoConfiguration.class).protoCompilerJavaLitePlugin();
+        }
+
+        @Override
+        public boolean useHostConfiguration() {
+          return true;
+        }
+      };
+  private static final String PROTO_COMPILER_JAVA_LITE_PLUGIN_ATTR =
+      ":proto_compiler_java_lite_plugin";
+
   private final JavaSemantics javaSemantics;
 
   @Nullable private final String jacocoLabel;
@@ -104,7 +123,12 @@ public class JavaLiteProtoAspect extends NativeAspectClass implements Configured
             .add(
                 attr(":java_toolchain", LABEL)
                     .allowedRuleClasses("java_toolchain")
-                    .value(JavaSemantics.JAVA_TOOLCHAIN));
+                    .value(JavaSemantics.JAVA_TOOLCHAIN))
+            .add(
+                attr(PROTO_COMPILER_JAVA_LITE_PLUGIN_ATTR, LABEL)
+                    .cfg(HOST)
+                    .exec()
+                    .value(JAVA_LITE_PLUGIN));
 
     Attribute.Builder<Label> jacocoAttr = attr("$jacoco_instrumentation", LABEL).cfg(HOST);
 
@@ -191,10 +215,18 @@ public class JavaLiteProtoAspect extends NativeAspectClass implements Configured
                   ruleContext, supportData, "Java", "java", ImmutableList.of(sourceJar))
               .allowServices(true)
               .setLangParameter(
-                  ProtoCompileActionBuilder.buildProtoArg(
-                      "java_out",
-                      sourceJar.getExecPathString(),
-                      ImmutableList.of("lite", "immutable", "no_enforce_api_compatibility")));
+                  String.format(
+                      ruleContext
+                          .getFragment(ProtoConfiguration.class, HOST)
+                          .protoCompilerJavaLiteFlags(),
+                      sourceJar.getExecPathString()));
+
+      FilesToRunProvider plugin =
+          ruleContext.getExecutablePrerequisite(PROTO_COMPILER_JAVA_LITE_PLUGIN_ATTR, Mode.HOST);
+      if (plugin != null) {
+        actionBuilder.setAdditionalTools(ImmutableList.of(plugin));
+      }
+
       ruleContext.registerAction(actionBuilder.build());
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java
index a9f61557f3..5045a5b12e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java
@@ -17,18 +17,16 @@ package com.google.devtools.build.lib.rules.java.proto;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.collect.Iterables.transform;
 import static com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode.TARGET;
-import static com.google.devtools.build.lib.cmdline.Label.PARSE_ABSOLUTE_UNCHECKED;
 import static com.google.devtools.build.lib.cmdline.Label.parseAbsoluteUnchecked;
 import static com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition.HOST;
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 import static com.google.devtools.build.lib.rules.java.proto.JavaCompilationArgsAspectProvider.GET_PROVIDER;
 import static com.google.devtools.build.lib.rules.java.proto.JavaProtoLibraryTransitiveFilesToBuildProvider.GET_JARS;
 
-import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
@@ -37,6 +35,7 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMap;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.StrictDepsMode;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -45,7 +44,10 @@ import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.AspectDefinition;
 import com.google.devtools.build.lib.packages.AspectParameters;
 import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
+import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.NativeAspectClass;
+import com.google.devtools.build.lib.packages.Rule;
 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.JavaLibraryHelper;
@@ -72,25 +74,36 @@ public class JavaProtoAspect extends NativeAspectClass implements ConfiguredAspe
    * The attribute name for holding a list of protos for which no code should be generated because
    * the proto-runtime already contains them.
    */
-  private static final String PROTO_SOURCE_FILE_BLACKLIST_ATTR = "$proto_source_file_blacklist";
+  private static final String PROTO_SOURCE_FILE_BLACKLIST_ATTR = ":proto_source_file_blacklist";
+
+  private static final Attribute.LateBoundLabelList<BuildConfiguration> BLACKLISTED_PROTOS =
+      new Attribute.LateBoundLabelList<BuildConfiguration>(
+          ImmutableList.<Label>of(), ProtoConfiguration.class) {
+        @Override
+        public List<Label> resolve(
+            Rule rule, AttributeMap attributes, BuildConfiguration configuration) {
+          return configuration
+              .getFragment(ProtoConfiguration.class)
+              .protoCompilerJavaBlacklistedProtos();
+        }
+
+        @Override
+        public boolean useHostConfiguration() {
+          return true;
+        }
+      };
 
   private final JavaSemantics javaSemantics;
-  private final ImmutableList<String> protoSourceFileBlacklistLabels;
 
   @Nullable private final String jacocoLabel;
-  private final ImmutableList<String> protoCompilerPluginOptions;
   private final RpcSupport rpcSupport;
 
   protected JavaProtoAspect(
       JavaSemantics javaSemantics,
-      ImmutableList<String> protoSourceFileBlacklistLabels,
       @Nullable String jacocoLabel,
-      ImmutableList<String> protoCompilerPluginOptions,
       RpcSupport rpcSupport) {
     this.javaSemantics = javaSemantics;
-    this.protoSourceFileBlacklistLabels = protoSourceFileBlacklistLabels;
     this.jacocoLabel = jacocoLabel;
-    this.protoCompilerPluginOptions = protoCompilerPluginOptions;
     this.rpcSupport = rpcSupport;
   }
 
@@ -113,9 +126,11 @@ public class JavaProtoAspect extends NativeAspectClass implements ConfiguredAspe
         new Impl(
                 ruleContext,
                 supportData,
-                protoCompilerPluginOptions,
+                ruleContext
+                    .getFragment(ProtoConfiguration.class, ConfigurationTransition.HOST)
+                    .protoCompilerJavaFlags(),
                 javaSemantics,
-            rpcSupport)
+                rpcSupport)
             .createProviders());
 
     return aspect.build();
@@ -132,9 +147,10 @@ public class JavaProtoAspect extends NativeAspectClass implements ConfiguredAspe
                 attr(SPEED_PROTO_RUNTIME_ATTR, LABEL)
                     .legacyAllowAnyFileType()
                     .value(parseAbsoluteUnchecked(SPEED_PROTO_RUNTIME_LABEL)))
-            .add(ProtoSourceFileBlacklist.blacklistFilegroupAttribute(
-                    PROTO_SOURCE_FILE_BLACKLIST_ATTR,
-                    transformToList(protoSourceFileBlacklistLabels, PARSE_ABSOLUTE_UNCHECKED)))
+            .add(
+                attr(PROTO_SOURCE_FILE_BLACKLIST_ATTR, LABEL_LIST)
+                    .cfg(HOST)
+                    .value(BLACKLISTED_PROTOS))
             .add(attr(":host_jdk", LABEL).cfg(HOST).value(JavaSemantics.HOST_JDK))
             .add(
                 attr(":java_toolchain", LABEL)
@@ -151,12 +167,6 @@ public class JavaProtoAspect extends NativeAspectClass implements ConfiguredAspe
     return result.add(jacocoAttr).build();
   }
 
-  /** Like Iterables.transform(), except it returns a List<> instead of an Iterable<>. */
-  private static <F, T> List<T> transformToList(
-      Iterable<F> fromIterable, Function<? super F, ? extends T> function) {
-    return Lists.<T>newArrayList(transform(fromIterable, function));
-  }
-
   private static class Impl {
 
     private final RuleContext ruleContext;
@@ -170,12 +180,12 @@ public class JavaProtoAspect extends NativeAspectClass implements ConfiguredAspe
      * Java compilation action.
      */
     private final JavaCompilationArgsProvider dependencyCompilationArgs;
-    private final ImmutableList<String> protoCompilerPluginOptions;
+    private final String protoCompilerPluginOptions;
 
     Impl(
         final RuleContext ruleContext,
         final SupportData supportData,
-        ImmutableList<String> protoCompilerPluginOptions,
+        String protoCompilerPluginOptions,
         JavaSemantics javaSemantics,
         RpcSupport rpcSupport) {
       this.ruleContext = ruleContext;
@@ -255,10 +265,8 @@ public class JavaProtoAspect extends NativeAspectClass implements ConfiguredAspe
                   ruleContext, supportData, "Java", "java", ImmutableList.of(sourceJar))
               .allowServices(true)
               .setLangParameter(
-                  ProtoCompileActionBuilder.buildProtoArg(
-                      "java_out", sourceJar.getExecPathString(), protoCompilerPluginOptions));
-      rpcSupport.mutateProtoCompileAction(
-          ruleContext, sourceJar, actionBuilder);
+                  String.format(protoCompilerPluginOptions, sourceJar.getExecPathString()));
+      rpcSupport.mutateProtoCompileAction(ruleContext, sourceJar, actionBuilder);
       ruleContext.registerAction(actionBuilder.build());
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java
index 650d7d412a..75ed40d883 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java
@@ -26,7 +26,6 @@ import com.google.devtools.build.lib.analysis.config.InvalidConfigurationExcepti
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.common.options.Option;
-
 import java.util.List;
 
 /**
@@ -63,6 +62,61 @@ public class ProtoConfiguration extends Fragment {
     )
     public Label protoCompiler;
 
+    // TODO(b/31775048): Replace with a toolchain
+    /** This is experimental, and is subject to change without warning. */
+    @Option(
+      name = "proto_compiler_java_flags",
+      defaultValue = "--java_out=shared,immutable:%s",
+      category = "experimental",
+      help = "The flags to pass to proto-compiler when generating Java protos."
+    )
+    public String protoCompilerJavaFlags;
+
+    // TODO(b/31775048): Replace with a toolchain
+    /** This is experimental, and is subject to change without warning. */
+    @Option(
+      name = "proto_compiler_java_blacklisted_protos",
+      defaultValue = "",
+      category = "experimental",
+      converter = BuildConfiguration.LabelListConverter.class,
+      help = "A label of a filegroup of .proto files that we shouldn't generate sources for."
+    )
+    public List<Label> protoCompilerJavaBlacklistedProtos;
+
+    // TODO(b/31775048): Replace with a toolchain
+    /** This is experimental, and is subject to change without warning. */
+    @Option(
+      name = "proto_compiler_javalite_flags",
+      defaultValue = "--javalite_out=%s",
+      category = "experimental",
+      help = "The flags to pass to proto-compiler when generating JavaLite protos."
+    )
+    public String protoCompilerJavaLiteFlags;
+
+    // TODO(b/31775048): Replace with a toolchain
+    /** This is experimental, and is subject to change without warning. */
+    @Option(
+      name = "proto_compiler_javalite_plugin",
+      defaultValue = "",
+      category = "experimental",
+      converter = BuildConfiguration.EmptyToNullLabelConverter.class,
+      help = "A label for the javalite proto-compiler plugin, if needed."
+    )
+    public Label protoCompilerJavaLitePlugin;
+
+    @Override
+    public FragmentOptions getHost(boolean fallback) {
+      Options host = (Options) super.getHost(fallback);
+      host.protoCompiler = protoCompiler;
+      host.protocOpts = protocOpts;
+      host.experimentalProtoExtraActions = experimentalProtoExtraActions;
+      host.protoCompiler = protoCompiler;
+      host.protoCompilerJavaFlags = protoCompilerJavaFlags;
+      host.protoCompilerJavaBlacklistedProtos = protoCompilerJavaBlacklistedProtos;
+      host.protoCompilerJavaLiteFlags = protoCompilerJavaLiteFlags;
+      host.protoCompilerJavaLitePlugin = protoCompilerJavaLitePlugin;
+      return host;
+    }
   }
 
   /**
@@ -89,11 +143,19 @@ public class ProtoConfiguration extends Fragment {
   private final boolean experimentalProtoExtraActions;
   private final ImmutableList<String> protocOpts;
   private final Label protoCompiler;
+  private final String protoCompilerJavaFlags;
+  private final List<Label> protoCompilerJavaBlacklistedProtos;
+  private final String protoCompilerJavaLiteFlags;
+  private final Label protoCompilerJavaLitePlugin;
 
   public ProtoConfiguration(Options options) {
     this.experimentalProtoExtraActions = options.experimentalProtoExtraActions;
     this.protocOpts = ImmutableList.copyOf(options.protocOpts);
     this.protoCompiler = options.protoCompiler;
+    this.protoCompilerJavaFlags = options.protoCompilerJavaFlags;
+    this.protoCompilerJavaLiteFlags = options.protoCompilerJavaLiteFlags;
+    this.protoCompilerJavaLitePlugin = options.protoCompilerJavaLitePlugin;
+    this.protoCompilerJavaBlacklistedProtos = options.protoCompilerJavaBlacklistedProtos;
   }
 
   public ImmutableList<String> protocOpts() {
@@ -112,4 +174,20 @@ public class ProtoConfiguration extends Fragment {
   public Label protoCompiler() {
     return protoCompiler;
   }
+
+  public String protoCompilerJavaFlags() {
+    return protoCompilerJavaFlags;
+  }
+
+  public String protoCompilerJavaLiteFlags() {
+    return protoCompilerJavaLiteFlags;
+  }
+
+  public Label protoCompilerJavaLitePlugin() {
+    return protoCompilerJavaLitePlugin;
+  }
+
+  public List<Label> protoCompilerJavaBlacklistedProtos() {
+    return protoCompilerJavaBlacklistedProtos;
+  }
 }