Move --incompatible_disable_proto_source_root to the option graveyard.

And with it, the proto_library.proto_source_root attribute. Good riddance. Fixes #7153. RELNOTES: None. PiperOrigin-RevId: 262537130
bazelbuild · Aug 9, 2019 · 3e8364c · 3e8364c · meteorcloudy · Aug 9, 2019
1 parent 760e1dd
commit 3e8364c
Show file tree

Hide file tree

Showing 5 changed files with 16 additions and 76 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java
@@ -46,6 +46,18 @@
 public class BazelRulesModule extends BlazeModule {
   /** This is where deprecated options go to die. */
   public static class GraveyardOptions extends OptionsBase {
+    @Option(
+        name = "incompatible_disable_proto_source_root",
+        defaultValue = "true",
+        documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+        effectTags = {OptionEffectTag.NO_OP},
+        metadataTags = {
+          OptionMetadataTag.INCOMPATIBLE_CHANGE,
+          OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES,
+        },
+        help = "Deprecated no-op.")
+    public boolean disableProtoSourceRoot;
+
     @Option(
         name = "incompatible_do_not_emit_buggy_external_repo_import",
         defaultValue = "true",

diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryRule.java b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryRule.java
@@ -73,11 +73,6 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen
             attr("srcs", LABEL_LIST)
                 .direct_compile_time_input()
                 .allowedFileTypes(FileType.of(".proto"), FileType.of(".protodevel")))
-        /* <!-- #BLAZE_RULE(proto_library).ATTRIBUTE(proto_source_root) -->
-        Directory containing .proto files. If set, it must be equal to the package name. If not set,
-        the source root will be the workspace directory (default).
-        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
-        .add(attr("proto_source_root", STRING))
         /* <!-- #BLAZE_RULE(proto_library).ATTRIBUTE(exports) -->
         List of proto_library targets that can be referenced via "import public" in the proto
         source.

diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java
@@ -193,10 +193,8 @@ public String getSourceRoot() {
   /**
    * Returns the {@link Library} representing this <code>proto_library</code> rule.
    *
-   * <p>Assumes that <code>strip_import_prefix</code> and <code>import_prefix</code> are unset.
-   *
-   * <p>Build will fail if the {@code proto_source_root} of the current library is neither the
-   * package name nor the source root.
+   * <p>Assumes that <code>strip_import_prefix</code> and <code>import_prefix</code> are unset and
+   * that there are no generated .proto files that need to be compiled.
    */
   // TODO(lberki): This should really be a PathFragment. Unfortunately, it's on the Starlark API of
   // ProtoInfo so it's not an easy change :(
@@ -250,8 +248,6 @@ private static Library createLibraryWithVirtualSourceRootMaybe(
     PathFragment importPrefixAttribute = getPathFragmentAttribute(ruleContext, "import_prefix");
     PathFragment stripImportPrefixAttribute =
         getPathFragmentAttribute(ruleContext, "strip_import_prefix");
-    PathFragment protoSourceRootAttribute =
-        getPathFragmentAttribute(ruleContext, "proto_source_root");
     boolean hasGeneratedSources = false;
 
     if (generatedProtosInVirtualImports) {
@@ -265,20 +261,11 @@ private static Library createLibraryWithVirtualSourceRootMaybe(
 
     if (importPrefixAttribute == null
         && stripImportPrefixAttribute == null
-        && protoSourceRootAttribute == null
         && !hasGeneratedSources) {
       // Simple case, no magic required.
       return null;
     }
 
-    if (protoSourceRootAttribute != null
-        && (importPrefixAttribute != null || stripImportPrefixAttribute != null)) {
-      ruleContext.ruleError(
-          "the 'proto_source_root' attribute is incompatible with "
-              + "'strip_import_prefix' and 'import_prefix");
-      return null;
-    }
-
     PathFragment stripImportPrefix;
     PathFragment importPrefix;
 
@@ -308,30 +295,8 @@ private static Library createLibraryWithVirtualSourceRootMaybe(
         ruleContext.attributeError("import_prefix", "should be a relative path");
         return null;
       }
-    } else if (protoSourceRootAttribute != null) {
-      if (!ruleContext.getFragment(ProtoConfiguration.class).enableProtoSourceroot()) {
-        ruleContext.attributeError("proto_source_root", "this attribute is not supported anymore");
-        return null;
-      }
-
-      if (!ruleContext.getLabel().getPackageFragment().equals(protoSourceRootAttribute)) {
-        ruleContext.attributeError(
-            "proto_source_root",
-            "proto_source_root must be the same as the package name ("
-                + ruleContext.getLabel().getPackageName()
-                + ")."
-                + " not '"
-                + protoSourceRootAttribute
-                + "'.");
-
-        return null;
-      }
-
-      stripImportPrefix = ruleContext.getPackageDirectory();
-      importPrefix = PathFragment.EMPTY_FRAGMENT;
     } else {
-      // Has generated sources, but neither strip_import_prefix nor import_prefix nor
-      // proto_source_root.
+      // Has generated sources, but neither strip_import_prefix nor import_prefix
       stripImportPrefix =
           ruleContext.getLabel().getPackageIdentifier().getRepository().getPathUnderExecRoot();
 

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
@@ -169,20 +169,6 @@ public static class Options extends FragmentOptions {
                 + " 'dep.proto.' and it must be accessed by 'dep[ProtoInfo].")
     public boolean disableLegacyProvider;
 
-    @Option(
-        name = "incompatible_disable_proto_source_root",
-        defaultValue = "true",
-        documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
-        effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
-        metadataTags = {
-          OptionMetadataTag.INCOMPATIBLE_CHANGE,
-          OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
-        },
-        help =
-            "If true, proto_library will no longer allow the proto_source_root= attribute. It has "
-                + "been superseded by the strip_import_prefix= and import_prefix= attributes")
-    public boolean disableProtoSourceRoot;
-
     @Option(
         name = "incompatible_load_proto_rules_from_bzl",
         defaultValue = "false",
@@ -201,7 +187,6 @@ public static class Options extends FragmentOptions {
     public FragmentOptions getHost() {
       Options host = (Options) super.getHost();
       host.disableLegacyProvider = disableLegacyProvider;
-      host.disableProtoSourceRoot = disableProtoSourceRoot;
       host.loadProtoRulesFromBzl = loadProtoRulesFromBzl;
       host.protoCompiler = protoCompiler;
       host.protocOpts = protocOpts;
@@ -258,10 +243,6 @@ public boolean enableLegacyProvider() {
     return !options.disableLegacyProvider;
   }
 
-  public boolean enableProtoSourceroot() {
-    return !options.disableProtoSourceRoot;
-  }
-
   public ImmutableList<String> protocOpts() {
     return protocOpts;
   }

diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java
@@ -253,19 +253,6 @@ public void testDescriptorSetOutput_strictDeps_disabled() throws Exception {
     }
   }
 
-  @Test
-  public void testDisableProtoSourceRoot() throws Exception {
-    useConfiguration(
-        "--proto_compiler=//proto:compiler", "--incompatible_disable_proto_source_root");
-    scratch.file(
-        "x/BUILD",
-        ProtoTestHelper.LOAD_PROTO_LIBRARY,
-        "proto_library(name='x', srcs=['x.proto'], proto_source_root='x')");
-    reporter.removeHandler(failFastHandler);
-    getConfiguredTarget("//x:x");
-    assertContainsEvent("this attribute is not supported anymore");
-  }
-
   @Test
   public void testStripImportPrefixWithoutDeps() throws Exception {
     scratch.file(
@@ -290,7 +277,7 @@ public void testStripImportPrefixWithoutDeps() throws Exception {
   }
 
   @Test
-  public void testProtoSourceRootWithDepsDuplicate() throws Exception {
+  public void testStripImportPrefixWithDepsDuplicate() throws Exception {
     scratch.file(
         "third_party/x/foo/BUILD",
         ProtoTestHelper.LOAD_PROTO_LIBRARY,