Revert "Remove --incompatible_blacklisted_protos_requires_proto_info"

This reverts commit fe4a680.
hrfuller · Dec 10, 2020 · e43825d · e43825d
1 parent 96a2551
commit e43825d
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 5 deletions.
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
@@ -168,6 +168,19 @@ public static class Options extends FragmentOptions {
         help = "If true, add --allowed_public_imports to the java compile actions.")
     public boolean experimentalJavaProtoAddAllowedPublicImports;
 
+    @Option(
+        name = "incompatible_blacklisted_protos_requires_proto_info",
+        defaultValue = "true",
+        documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+        effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+        metadataTags = {
+          OptionMetadataTag.INCOMPATIBLE_CHANGE,
+          OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+        },
+        help =
+            "If enabled, 'proto_lang_toolchain.blacklisted_protos' requires provider 'ProtoInfo'")
+    public boolean blacklistedProtosRequiresProtoInfo;
+
     @Override
     public FragmentOptions getHost() {
       Options host = (Options) super.getHost();
@@ -187,6 +200,7 @@ public FragmentOptions getHost() {
       host.experimentalJavaProtoAddAllowedPublicImports =
           experimentalJavaProtoAddAllowedPublicImports;
       host.generatedProtosInVirtualImports = generatedProtosInVirtualImports;
+      host.blacklistedProtosRequiresProtoInfo = blacklistedProtosRequiresProtoInfo;
       return host;
     }
   }
@@ -260,4 +274,8 @@ public boolean strictPublicImports() {
   public boolean generatedProtosInVirtualImports() {
     return options.generatedProtosInVirtualImports;
   }
+
+  public boolean blacklistedProtosRequiresProtoInfo() {
+    return options.blacklistedProtosRequiresProtoInfo;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java
@@ -19,12 +19,14 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
+import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.packages.Type;
 
@@ -34,9 +36,22 @@ public class ProtoLangToolchain implements RuleConfiguredTargetFactory {
   public ConfiguredTarget create(RuleContext ruleContext)
       throws InterruptedException, RuleErrorException, ActionConflictException {
     NestedSetBuilder<Artifact> blacklistedProtos = NestedSetBuilder.stableOrder();
-    for (ProtoInfo protoInfo :
-        ruleContext.getPrerequisites("blacklisted_protos", ProtoInfo.PROVIDER)) {
-      blacklistedProtos.addTransitive(protoInfo.getOriginalTransitiveProtoSources());
+    for (TransitiveInfoCollection protos : ruleContext.getPrerequisites("blacklisted_protos")) {
+      ProtoInfo protoInfo = protos.get(ProtoInfo.PROVIDER);
+      if (protoInfo == null
+          && ruleContext
+              .getFragment(ProtoConfiguration.class)
+              .blacklistedProtosRequiresProtoInfo()) {
+        ruleContext.ruleError(
+            "'" + ruleContext.getLabel() + "' does not have mandatory provider 'ProtoInfo'.");
+      }
+      if (protoInfo != null) {
+        blacklistedProtos.addTransitive(protoInfo.getOriginalTransitiveProtoSources());
+      } else {
+        // Only add files from FileProvider if |protos| is not a proto_library to avoid adding
+        // the descriptor_set of proto_library to the list of blacklisted files.
+        blacklistedProtos.addTransitive(protos.getProvider(FileProvider.class).getFilesToBuild());
+      }
     }
 
     return new RuleConfiguredTargetBuilder(ruleContext)

diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java
@@ -18,12 +18,14 @@
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
+import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
+import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.packages.RuleClass;
-import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier;
 import com.google.devtools.build.lib.packages.Type;
 
 /** Implements {code proto_lang_toolchain}. */
@@ -72,7 +74,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
         .add(
             attr("blacklisted_protos", LABEL_LIST)
                 .allowedFileTypes()
-                .mandatoryProviders(StarlarkProviderIdentifier.forKey(ProtoInfo.PROVIDER.getKey())))
+                .mandatoryBuiltinProviders(
+                    ImmutableList.<Class<? extends TransitiveInfoProvider>>of(FileProvider.class)))
         .requiresConfigurationFragments(ProtoConfiguration.class)
         .advertiseProvider(ProtoLangToolchainProvider.class)
         .removeAttribute("data")

diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainTest.java
@@ -140,6 +140,44 @@ public void protoToolchainBlacklistTransitiveProtos() throws Exception {
         getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
   }
 
+  @Test
+  public void protoToolchainMixedBlacklist() throws Exception {
+    // Tests legacy behaviour.
+    useConfiguration("--incompatible_blacklisted_protos_requires_proto_info=false");
+
+    scratch.file(
+        "third_party/x/BUILD",
+        TestConstants.LOAD_PROTO_LIBRARY,
+        "licenses(['unencumbered'])",
+        "cc_binary(name = 'plugin', srcs = ['plugin.cc'])",
+        "cc_library(name = 'runtime', srcs = ['runtime.cc'])",
+        "proto_library(name = 'metadata', srcs = ['metadata.proto'])",
+        "proto_library(",
+        "    name = 'descriptor',",
+        "    srcs = ['descriptor.proto'],",
+        "    strip_import_prefix = '/third_party')",
+        "filegroup(name = 'any', srcs = ['any.proto'])");
+
+    scratch.file(
+        "foo/BUILD",
+        TestConstants.LOAD_PROTO_LANG_TOOLCHAIN,
+        "proto_lang_toolchain(",
+        "    name = 'toolchain',",
+        "    command_line = 'cmd-line',",
+        "    plugin = '//third_party/x:plugin',",
+        "    runtime = '//third_party/x:runtime',",
+        "    blacklisted_protos = [",
+        "        '//third_party/x:metadata',",
+        "        '//third_party/x:descriptor',",
+        "        '//third_party/x:any']",
+        ")");
+
+    update(ImmutableList.of("//foo:toolchain"), false, 1, true, new EventBus());
+
+    validateProtoLangToolchain(
+        getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
+  }
+
   @Test
   public void optionalFieldsAreEmpty() throws Exception {
     scratch.file(