Remove incompatible_generated_protos_in_virtual_imports

The flag obscured bugs around generated proto files. Those were corrected in 6c6c196. proto_source_root is now set to path relative to output dir, no matter if the files are generated. This was always the case for a monorepo (consider a proto_library that mixes source file and generated file). The case was just corrected for the multi-repo world.

For backwards compatibility paths with _virtual_imports still contain bin or genfiles dir. This will be removed once proto_common.compile automatically computes output directory.

See the reasons for introducing the flag in #9215

PiperOrigin-RevId: 536654289
Change-Id: I61379a99ece4c5b0db7c222b1961c7dc9b87b137

Author: comius

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java

@@ -562,6 +562,15 @@ public static final class AllCommandGraveyardOptions extends OptionsBase {
         effectTags = {OptionEffectTag.NO_OP},
         help = "No-op.")
     public boolean useForkJoinPool;
+
+    @Option(
+        name = "incompatible_generated_protos_in_virtual_imports",
+        defaultValue = "true",
+        documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+        effectTags = {OptionEffectTag.NO_OP},
+        metadataTags = {OptionMetadataTag.DEPRECATED},
+        help = "No-op.")
+    public boolean generatedProtosInVirtualImports;
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java

@@ -44,16 +44,6 @@ public class ProtoConfiguration extends Fragment implements ProtoConfigurationAp
 
   /** Command line options. */
   public static class Options extends FragmentOptions {
-    @Option(
-        name = "incompatible_generated_protos_in_virtual_imports",
-        defaultValue = "true",
-        documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
-        effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
-        metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
-        help =
-            "If set, generated .proto files are put into a virtual import directory. For more "
-                + "information, see https://github.com/bazelbuild/bazel/issues/9215")
-    public boolean generatedProtosInVirtualImports;
 
     @Option(
         name = "protocopt",
@@ -196,7 +186,6 @@ public FragmentOptions getExec() {
       exec.strictPublicImports = strictPublicImports;
       exec.ccProtoLibraryHeaderSuffixes = ccProtoLibraryHeaderSuffixes;
       exec.ccProtoLibrarySourceSuffixes = ccProtoLibrarySourceSuffixes;
-      exec.generatedProtosInVirtualImports = generatedProtosInVirtualImports;
       return exec;
     }
   }
@@ -329,19 +318,4 @@ public List<String> ccProtoLibrarySourceSuffixesForStarlark(StarlarkThread threa
   public List<String> ccProtoLibrarySourceSuffixes() {
     return ccProtoLibrarySourceSuffixes;
   }
-
-
-  @StarlarkMethod(
-      name = "generated_protos_in_virtual_imports",
-      useStarlarkThread = true,
-      documented = false)
-  public boolean generatedProtosInVirtualImportsForStarlark(StarlarkThread thread)
-      throws EvalException {
-    ProtoCommon.checkPrivateStarlarkificationAllowlist(thread);
-    return generatedProtosInVirtualImports();
-  }
-
-  public boolean generatedProtosInVirtualImports() {
-    return options.generatedProtosInVirtualImports;
-  }
 }

src/main/starlark/builtins_bzl/common/proto/proto_library.bzl

@@ -118,11 +118,7 @@ def _process_srcs(ctx, srcs, import_prefix, strip_import_prefix):
     Returns:
       (str, [File]) A pair of proto_path and virtual_sources.
     """
-    generate_protos_in_virtual_imports = False
-    if ctx.fragments.proto.generated_protos_in_virtual_imports():
-        generate_protos_in_virtual_imports = any([not src.is_source for src in srcs])
-
-    if import_prefix != "" or strip_import_prefix != "" or generate_protos_in_virtual_imports:
+    if import_prefix != "" or strip_import_prefix != "":
         # Use virtual source roots
         return _symlink_to_virtual_imports(ctx, srcs, import_prefix, strip_import_prefix)
     else:

src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommonTest.java

@@ -388,7 +388,6 @@ public void generateCode_protocOpts() throws Exception {
    */
   @Test
   public void generateCode_directGeneratedProtos() throws Exception {
-    useConfiguration("--noincompatible_generated_protos_in_virtual_imports");
     scratch.file(
         "bar/BUILD",
         TestConstants.LOAD_PROTO_LIBRARY,
@@ -419,7 +418,6 @@ public void generateCode_directGeneratedProtos() throws Exception {
    */
   @Test
   public void generateCode_inDirectGeneratedProtos() throws Exception {
-    useConfiguration("--noincompatible_generated_protos_in_virtual_imports");
     scratch.file(
         "bar/BUILD",
         TestConstants.LOAD_PROTO_LIBRARY,
@@ -450,34 +448,18 @@ public void generateCode_inDirectGeneratedProtos() throws Exception {
    */
   @Test
   @TestParameters({
-    "{virtual: false, sibling: false, generated: false, expectedFlags:"
+    "{sibling: false, generated: false, expectedFlags:"
         + " ['--proto_path=external/foo','-Ie/E.proto=external/foo/e/E.proto']}",
-    "{virtual: false, sibling: false, generated: true, expectedFlags:"
+    "{sibling: false, generated: true, expectedFlags:"
         + " ['--proto_path=bl?azel?-out/k8-fastbuild/bin/external/foo',"
         + " '-Ie/E.proto=bl?azel?-out/k8-fastbuild/bin/external/foo/e/E.proto']}",
-    "{virtual: true, sibling: false, generated: false,expectedFlags:"
-        + " ['--proto_path=external/foo','-Ie/E.proto=external/foo/e/E.proto']}",
-    "{virtual: true, sibling: false, generated: true, expectedFlags:"
-        + " ['--proto_path=bl?azel?-out/k8-fastbuild/bin/external/foo/e/_virtual_imports/e',"
-        + " '-Ie/E.proto=bl?azel?-out/k8-fastbuild/bin/external/foo/e/_virtual_imports/e/e/E.proto']}",
-    "{virtual: true, sibling: true, generated: false,expectedFlags:"
-        + " ['--proto_path=../foo','-Ie/E.proto=../foo/e/E.proto']}",
-    "{virtual: true, sibling: true, generated: true, expectedFlags:"
-        + " ['--proto_path=bl?azel?-out/foo/k8-fastbuild/bin/e/_virtual_imports/e',"
-        + " '-Ie/E.proto=bl?azel?-out/foo/k8-fastbuild/bin/e/_virtual_imports/e/e/E.proto']}",
-    "{virtual: false, sibling: true, generated: false,expectedFlags:"
+    "{sibling: true, generated: false,expectedFlags:"
         + " ['--proto_path=../foo','-Ie/E.proto=../foo/e/E.proto']}",
-    "{virtual: false, sibling: true, generated: true, expectedFlags:"
+    "{sibling: true, generated: true, expectedFlags:"
         + " ['--proto_path=bl?azel?-out/foo/k8-fastbuild/bin','-Ie/E.proto=bl?azel?-out/foo/k8-fastbuild/bin/e/E.proto']}",
   })
   public void generateCode_externalProtoLibrary(
-      boolean virtual, boolean sibling, boolean generated, List<String> expectedFlags)
-      throws Exception {
-    if (virtual) {
-      useConfiguration("--incompatible_generated_protos_in_virtual_imports");
-    } else {
-      useConfiguration("--noincompatible_generated_protos_in_virtual_imports");
-    }
+      boolean sibling, boolean generated, List<String> expectedFlags) throws Exception {
     if (sibling) {
       setBuildLanguageOptions("--experimental_sibling_repository_layout");
     }

src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java

@@ -412,8 +412,7 @@ public void testStripImportPrefixWithDeps() throws Exception {
             ".");
   }
 
-  private void testExternalRepoWithGeneratedProto(
-      boolean siblingRepoLayout, boolean useVirtualImports) throws Exception {
+  private void testExternalRepoWithGeneratedProto(boolean siblingRepoLayout) throws Exception {
     if (!isThisBazel()) {
       return;
     }
@@ -423,9 +422,6 @@ private void testExternalRepoWithGeneratedProto(
     if (siblingRepoLayout) {
       setBuildLanguageOptions("--experimental_sibling_repository_layout");
     }
-    if (!useVirtualImports) {
-      useConfiguration("--noincompatible_generated_protos_in_virtual_imports");
-    }
     invalidatePackages();
 
     scratch.file("/foo/WORKSPACE");
@@ -445,12 +441,7 @@ private void testExternalRepoWithGeneratedProto(
                 siblingRepoLayout ? RepositoryName.create("foo") : RepositoryName.MAIN)
             .toString();
     String fooProtoRoot;
-    if (useVirtualImports) {
-      fooProtoRoot =
-          genfiles + (siblingRepoLayout ? "" : "/external/foo") + "/x/_virtual_imports/x";
-    } else {
-      fooProtoRoot = (siblingRepoLayout ? genfiles : genfiles + "/external/foo");
-    }
+    fooProtoRoot = (siblingRepoLayout ? genfiles : genfiles + "/external/foo");
     ConfiguredTarget a = getConfiguredTarget("//a:a");
     ProtoInfo aInfo = a.get(ProtoInfo.PROVIDER);
     assertThat(aInfo.getTransitiveProtoSourceRoots().toList()).containsExactly(".", fooProtoRoot);
@@ -462,24 +453,12 @@ private void testExternalRepoWithGeneratedProto(
 
   @Test
   public void testExternalRepoWithGeneratedProto_withSubdirRepoLayout() throws Exception {
-    testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ false, true);
+    testExternalRepoWithGeneratedProto(/* siblingRepoLayout= */ false);
   }
 
   @Test
   public void test_siblingRepoLayout_externalRepoWithGeneratedProto() throws Exception {
-    testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ true, true);
-  }
-
-  @Test
-  public void testExternalRepoWithGeneratedProto_withSubdirRepoLayoutAndNoVritualImports()
-      throws Exception {
-    testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ false, false);
-  }
-
-  @Test
-  public void test_siblingRepoLayout_externalRepoWithGeneratedProtoAndNoVritualImports()
-      throws Exception {
-    testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ true, false);
+    testExternalRepoWithGeneratedProto(/* siblingRepoLayout= */ true);
   }
 
   @Test
@@ -1076,35 +1055,7 @@ public void testExperimentalProtoDescriptorSetsIncludeSourceInfo() throws Except
   }
 
   @Test
-  public void testSourceAndGeneratedProtoFiles_Bazel() throws Exception {
-    if (!isThisBazel()) {
-      return;
-    }
-
-    scratch.file(
-        "a/BUILD",
-        TestConstants.LOAD_PROTO_LIBRARY,
-        "genrule(name='g', srcs=[], outs=['g.proto'], cmd = '')",
-        "proto_library(name='p', srcs=['s.proto', 'g.proto'])");
-
-    ImmutableList<String> commandLine =
-        allArgsForAction((SpawnAction) getDescriptorWriteAction("//a:p"));
-    String genfiles = getTargetConfiguration().getGenfilesFragment(RepositoryName.MAIN).toString();
-    assertThat(commandLine)
-        .containsAtLeast(
-            "-Ia/s.proto=" + genfiles + "/a/_virtual_imports/p/a/s.proto",
-            "-Ia/g.proto=" + genfiles + "/a/_virtual_imports/p/a/g.proto");
-  }
-
-  @Test
-  public void testSourceAndGeneratedProtoFiles_Blaze() throws Exception {
-    if (!isThisBazel()) {
-      return;
-    }
-
-    // Simulate behavoiur of Blaze's `proto_library` in Bazel.
-    useConfiguration("--incompatible_generated_protos_in_virtual_imports=false");
-
+  public void testSourceAndGeneratedProtoFiles() throws Exception {
     scratch.file(
         "a/BUILD",
         TestConstants.LOAD_PROTO_LIBRARY,
@@ -1210,43 +1161,12 @@ public void testProtoLibraryWithVirtualProtoSourceRoot() throws Exception {
         .containsExactly("foo/x/a.proto");
   }
 
-  @Test
-  public void testProtoLibraryWithGeneratedSources_Bazel() throws Exception {
-    if (!isThisBazel()) {
-      return;
-    }
-
-    useConfiguration("--incompatible_generated_protos_in_virtual_imports=true");
-
-    scratch.file(
-        "x/BUILD",
-        "genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')",
-        "proto_library(name='foo', srcs=['generated.proto'])");
-
-    String genfiles = getTargetConfiguration().getGenfilesFragment(RepositoryName.MAIN).toString();
-    ProtoInfo provider = getConfiguredTarget("//x:foo").get(ProtoInfo.PROVIDER);
-    assertThat(
-            Iterables.transform(
-                provider.getDirectSources(), s -> s.getSourceFile().getExecPath().getPathString()))
-        .containsExactly(genfiles + "/x/_virtual_imports/foo/x/generated.proto");
-    assertThat(
-            Iterables.transform(
-                provider.getDirectSources(), s -> s.getSourceRoot().getSafePathString()))
-        .containsExactly("x/_virtual_imports/foo");
-    assertThat(
-            Iterables.transform(
-                provider.getDirectSources(), s -> s.getImportPath().getPathString()))
-        .containsExactly("x/generated.proto");
-  }
-
   @Test
   public void testProtoLibraryWithGeneratedSources_Blaze() throws Exception {
     if (!isThisBazel()) {
       return;
     }
 
-    useConfiguration("--incompatible_generated_protos_in_virtual_imports=false");
-
     scratch.file(
         "x/BUILD",
         "genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')",
@@ -1268,45 +1188,12 @@ public void testProtoLibraryWithGeneratedSources_Blaze() throws Exception {
         .containsExactly("x/generated.proto");
   }
 
-  @Test
-  public void testProtoLibraryWithMixedSources_Bazel() throws Exception {
-    if (!isThisBazel()) {
-      return;
-    }
-
-    useConfiguration("--incompatible_generated_protos_in_virtual_imports=true");
-
-    scratch.file(
-        "x/BUILD",
-        "genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')",
-        "proto_library(name='foo', srcs=['generated.proto', 'a.proto'])");
-
-    String genfiles = getTargetConfiguration().getGenfilesFragment(RepositoryName.MAIN).toString();
-    ProtoInfo provider = getConfiguredTarget("//x:foo").get(ProtoInfo.PROVIDER);
-    assertThat(
-            Iterables.transform(
-                provider.getDirectSources(), s -> s.getSourceFile().getExecPath().getPathString()))
-        .containsExactly(
-            genfiles + "/x/_virtual_imports/foo/x/generated.proto",
-            genfiles + "/x/_virtual_imports/foo/x/a.proto");
-    assertThat(
-            Iterables.transform(
-                provider.getDirectSources(), s -> s.getSourceRoot().getSafePathString()))
-        .containsExactly("x/_virtual_imports/foo", "x/_virtual_imports/foo");
-    assertThat(
-            Iterables.transform(
-                provider.getDirectSources(), s -> s.getImportPath().getPathString()))
-        .containsExactly("x/generated.proto", "x/a.proto");
-  }
-
   @Test
   public void testProtoLibraryWithMixedSources_Blaze() throws Exception {
     if (!isThisBazel()) {
       return;
     }
 
-    useConfiguration("--incompatible_generated_protos_in_virtual_imports=false");
-
     scratch.file(
         "x/BUILD",
         "genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')",

src/test/shell/bazel/bazel_proto_library_test.sh

@@ -813,13 +813,13 @@ message H {
 }
 EOF
 
-  bazel build -s --noexperimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h >& $TEST_log || fail "failed"
-  bazel build -s --noexperimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h:h_cc_proto >& $TEST_log || fail "failed"
-  bazel build -s --noexperimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h:h_java_proto >& $TEST_log || fail "failed"
+  bazel build -s --noexperimental_sibling_repository_layout //h >& $TEST_log || fail "failed"
+  bazel build -s --noexperimental_sibling_repository_layout //h:h_cc_proto >& $TEST_log || fail "failed"
+  bazel build -s --noexperimental_sibling_repository_layout //h:h_java_proto >& $TEST_log || fail "failed"
 
-  bazel build -s --experimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h -s >& $TEST_log || fail "failed"
-  bazel build -s --experimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h:h_cc_proto -s >& $TEST_log || fail "failed"
-  bazel build -s --experimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports  //h:h_java_proto  -s >& $TEST_log || fail "failed"
+  bazel build -s --experimental_sibling_repository_layout //h -s >& $TEST_log || fail "failed"
+  bazel build -s --experimental_sibling_repository_layout //h:h_cc_proto -s >& $TEST_log || fail "failed"
+  bazel build -s --experimental_sibling_repository_layout //h:h_java_proto  -s >& $TEST_log || fail "failed"
 
   expect_not_log "warning: directory does not exist." # --proto_path is wrong