Repository: bazelbuild/bazel
Issue: proto_lang_toolchain and blacklisted_protos with ProtoInfo not propagated correctly
Labels: team-Rules-Server
untriaged

### Description of the problem / feature request:

With protobuf's `proto_lang_toolchain`, after this change https://github.com/protocolbuffers/protobuf/commit/7b28278c7d4f4175e70aef2f89d304696eb85ae3 it seems the `blacklisted_protos` are no longer working correctly and removing that field has no effect on the build.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

See https://github.com/protocolbuffers/protobuf/issues/7046 for a repro case

### What operating system are you running Bazel on?

macOS

### What's the output of `bazel info release`?

release 1.2.0 (I also tried with 2.0.0 with no change)

### Have you found anything relevant by searching the web?

The protobuf commit linked above referenced this bazel change https://github.com/bazelbuild/bazel/commit/a5ee2c4d979441e663cf53b2ace4daa05e5033d8

I'm also wondering if the issue is with protobuf's use of `strip_import_prefix` on the `proto_library` targets they define: https://github.com/protocolbuffers/protobuf/commit/a03d332aca5d33c5d4b2cd25037c9e37d57eff02

Issue comments:
cc @cushon 
Suspecting a combination of https://github.com/bazelbuild/bazel/commit/a5ee2c4d979441e663cf53b2ace4daa05e5033d8 and https://github.com/bazelbuild/bazel/commit/46c95dcbe2889e515a2b44b9dec00d6c13f7e9fa (/cc @lberki).

Submitted #10493 to fix this for now.
Longer-term, we should make `ProtoInfo` mandatory for `blacklisted_protos`.

Pull request review comments:
Using direct instead of transitive sources un-does a change made in 01df1e5c3aaf709e9388293514ae978e59ae2cbb that I think is going to break some uses of `proto_lang_toolchain.blacklisted_protos`.
Yep, this is exactly what I found out after debugging the aforementioned test failures. Adding _both_ here isn't very nice so I'll try adding the set of transitive original sources (I'd totally ask Yannic to do so, but since the tests that break are internal, he'd be flying blind...)
...and it blends! I'm sending this out (+my changes) for internal review.

Patch that resolved the issue:
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
index ea124b56ee52cb..5f011d81bb5d24 100644
--- 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
@@ -44,8 +44,12 @@ public ConfiguredTarget create(RuleContext ruleContext)
       ProtoInfo protoInfo = protos.get(ProtoInfo.PROVIDER);
       // TODO(cushon): it would be nice to make this mandatory and stop adding files to build too
       if (protoInfo != null) {
-        blacklistedProtos.addTransitive(protoInfo.getTransitiveProtoSources());
+        // TODO(yannic): Switch to getTransitiveProtoSources() and remove references to
+        // original proto sources when ProtoInfo is mandatory.
+        blacklistedProtos.addAll(protoInfo.getOriginalDirectProtoSources());
       } 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());
       }
     }


Test patch:
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
index dd09ca1189405c..ad2ab914537e03 100644
--- 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
@@ -39,6 +39,19 @@ public void setUp() throws Exception {
     invalidatePackages();
   }
 
+  private void validateProtoLangToolchain(ProtoLangToolchainProvider toolchain) throws Exception {
+    assertThat(toolchain.commandLine()).isEqualTo("cmd-line");
+    assertThat(toolchain.pluginExecutable().getExecutable().getRootRelativePathString())
+        .isEqualTo("x/plugin");
+
+    TransitiveInfoCollection runtimes = toolchain.runtime();
+    assertThat(runtimes.getLabel())
+        .isEqualTo(Label.parseAbsolute("//x:runtime", ImmutableMap.of()));
+
+    assertThat(prettyArtifactNames(toolchain.blacklistedProtos()))
+        .containsExactly("x/metadata.proto", "x/descriptor.proto", "x/any.proto");
+  }
+
   @Test
   public void protoToolchain() throws Exception {
     scratch.file(
@@ -62,19 +75,63 @@ public void protoToolchain() throws Exception {
 
     update(ImmutableList.of("//foo:toolchain"), false, 1, true, new EventBus());
 
-    ProtoLangToolchainProvider toolchain =
-        getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class);
+    validateProtoLangToolchain(
+        getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
+  }
 
-    assertThat(toolchain.commandLine()).isEqualTo("cmd-line");
-    assertThat(toolchain.pluginExecutable().getExecutable().getRootRelativePathString())
-        .isEqualTo("x/plugin");
+  @Test
+  public void protoToolchainBlacklistProtoLibraries() throws Exception {
+    scratch.file(
+        "x/BUILD",
+        TestConstants.LOAD_PROTO_LIBRARY,
+        "cc_binary(name = 'plugin', srcs = ['plugin.cc'])",
+        "cc_library(name = 'runtime', srcs = ['runtime.cc'])",
+        "proto_library(name = 'descriptors', srcs = ['metadata.proto', 'descriptor.proto'])",
+        "proto_library(name = 'any', srcs = ['any.proto'], import_prefix = 'bar')");
 
-    TransitiveInfoCollection runtimes = toolchain.runtime();
-    assertThat(runtimes.getLabel())
-        .isEqualTo(Label.parseAbsolute("//x:runtime", ImmutableMap.of()));
+    scratch.file(
+        "foo/BUILD",
+        TestConstants.LOAD_PROTO_LANG_TOOLCHAIN,
+        "proto_lang_toolchain(",
+        "    name = 'toolchain',",
+        "    command_line = 'cmd-line',",
+        "    plugin = '//x:plugin',",
+        "    runtime = '//x:runtime',",
+        "    blacklisted_protos = ['//x:descriptors', '//x:any']",
+        ")");
 
-    assertThat(prettyArtifactNames(toolchain.blacklistedProtos()))
-        .containsExactly("x/metadata.proto", "x/descriptor.proto", "x/any.proto");
+    update(ImmutableList.of("//foo:toolchain"), false, 1, true, new EventBus());
+
+    validateProtoLangToolchain(
+        getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
+  }
+
+  @Test
+  public void protoToolchainMixedBlacklist() throws Exception {
+    scratch.file(
+        "x/BUILD",
+        TestConstants.LOAD_PROTO_LIBRARY,
+        "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'], import_prefix = 'bar')",
+        "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 = '//x:plugin',",
+        "    runtime = '//x:runtime',",
+        "    blacklisted_protos = ['//x:metadata', '//x:descriptor', '//x:any']",
+        ")");
+
+    update(ImmutableList.of("//foo:toolchain"), false, 1, true, new EventBus());
+
+    validateProtoLangToolchain(
+        getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
   }
 
   @Test