Default behavior of proto_library's strict-proto-deps is now determined by the value of a flag.

Used to be hardcoded to "yes, check for strict proto deps" because the value of the "strict_proto_deps" attributes was "true".

--
MOS_MIGRATED_REVID=140538461

5 files changed, 89 insertions, 8 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java
index b81e6e0a70..3557c5f95c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java
@@ -16,7 +16,7 @@ package com.google.devtools.build.lib.rules.proto;
 
 import static com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode.TARGET;
 import static com.google.devtools.build.lib.collect.nestedset.Order.STABLE_ORDER;
-import static com.google.devtools.build.lib.syntax.Type.BOOLEAN;
+import static com.google.devtools.build.lib.rules.proto.ProtoCommon.areDepsStrict;
 
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
@@ -52,9 +52,7 @@ public class BazelProtoLibrary implements RuleConfiguredTargetFactory {
             transitiveImports, transitiveImports, protoSources, checkDepsProtoSources);
 
     NestedSet<Artifact> protosInDirectDeps =
-        ruleContext.attributes().get("strict_proto_deps", BOOLEAN)
-            ? ProtoCommon.computeProtosInDirectDeps(ruleContext)
-            : null;
+        areDepsStrict(ruleContext) ? ProtoCommon.computeProtosInDirectDeps(ruleContext) : null;
 
     final SupportData supportData =
         SupportData.create(
@@ -69,7 +67,7 @@ public class BazelProtoLibrary implements RuleConfiguredTargetFactory {
 
     RuleConfiguredTargetBuilder result = new RuleConfiguredTargetBuilder(ruleContext);
 
-    if (checkDepsProtoSources.isEmpty()) {
+    if (checkDepsProtoSources.isEmpty() || !outputDescriptorSetFlagEnabled(ruleContext)) {
       result.setFilesToBuild(NestedSetBuilder.<Artifact>create(STABLE_ORDER));
     } else {
       Artifact descriptorSetOutput =
@@ -97,4 +95,11 @@ public class BazelProtoLibrary implements RuleConfiguredTargetFactory {
         .addSkylarkTransitiveInfo(ProtoSourcesProvider.SKYLARK_NAME, sourcesProvider)
         .build();
   }
+
+  private boolean outputDescriptorSetFlagEnabled(RuleContext ruleContext) {
+    return ruleContext
+        .getConfiguration()
+        .getFragment(ProtoConfiguration.class)
+        .outputDescriptorSet();
+  }
 }
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
index ee9a23daa3..a30a176310 100644
--- 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
@@ -18,7 +18,7 @@ import static com.google.devtools.build.lib.packages.Attribute.ConfigurationTran
 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.syntax.Type.BOOLEAN;
+import static com.google.devtools.build.lib.packages.BuildType.TRISTATE;
 
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
@@ -76,7 +76,7 @@ public final class BazelProtoLibraryRule implements RuleDefinition {
             attr("srcs", LABEL_LIST)
                 .direct_compile_time_input()
                 .allowedFileTypes(FileType.of(".proto"), FileType.of(".protodevel")))
-        .add(attr("strict_proto_deps", BOOLEAN).value(true).undocumented("for migration only"))
+        .add(attr("strict_proto_deps", TRISTATE).undocumented("for migration only"))
         .advertiseProvider(ProtoSourcesProvider.class)
         .build();
   }
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
index 626d7136ed..e1eef7f3f4 100644
--- 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
@@ -15,18 +15,23 @@
 package com.google.devtools.build.lib.rules.proto;
 
 import static com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode.TARGET;
+import static com.google.devtools.build.lib.packages.BuildType.TRISTATE;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.packages.BuildType;
+import com.google.devtools.build.lib.packages.TriState;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import javax.annotation.Nullable;
@@ -180,4 +185,40 @@ public class ProtoCommon {
     }
     return result.build();
   }
+
+  /**
+   * Decides whether this proto_library should check for strict proto deps.
+   *
+   * <p>Takes into account command-line flags, package-level attributes and rule attributes.
+   */
+  @VisibleForTesting
+  public static boolean areDepsStrict(RuleContext ruleContext) {
+    BuildConfiguration.StrictDepsMode flagValue =
+        ruleContext.getFragment(ProtoConfiguration.class).strictProtoDeps();
+    if (flagValue == BuildConfiguration.StrictDepsMode.OFF) {
+      return false;
+    }
+    if (flagValue == BuildConfiguration.StrictDepsMode.ERROR
+        || flagValue == BuildConfiguration.StrictDepsMode.WARN) {
+      return true;
+    }
+
+    TriState attrValue = ruleContext.attributes().get("strict_proto_deps", TRISTATE);
+    if (attrValue == TriState.NO) {
+      return false;
+    }
+    if (attrValue == TriState.YES) {
+      return true;
+    }
+
+    ImmutableSet<String> pkgFeatures = ruleContext.getRule().getPackage().getFeatures();
+    if (pkgFeatures.contains("disable_strict_proto_deps_NO")) {
+      return false;
+    }
+    if (pkgFeatures.contains("disable_strict_proto_deps_YES")) {
+      return true;
+    }
+
+    return false;
+  }
 }
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 ed10c4a564..827a3f8aa8 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
@@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration.StrictDepsMode;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment;
 import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
@@ -142,6 +143,25 @@ public class ProtoConfiguration extends Fragment {
     )
     public boolean useToolchainForJavaProto;
 
+    @Option(
+      name = "strict_proto_deps",
+      defaultValue = "error",
+      converter = BuildConfiguration.StrictDepsConverter.class,
+      category = "semantics",
+      help =
+          "If true, checks that a proto_library target explicitly declares all directly "
+              + "used targets as dependencies."
+    )
+    public StrictDepsMode strictProtoDeps;
+
+    @Option(
+      name = "output_descriptor_set",
+      defaultValue = "true",
+      category = "experimental",
+      help = "When true, a proto_library will produce a descriptor set proto in its outputs."
+    )
+    public boolean outputDescriptorSet;
+
     @Override
     public FragmentOptions getHost(boolean fallback) {
       Options host = (Options) super.getHost(fallback);
@@ -156,6 +176,8 @@ public class ProtoConfiguration extends Fragment {
       host.protoToolchainForJava = protoToolchainForJava;
       host.protoToolchainForJavaLite = protoToolchainForJavaLite;
       host.useToolchainForJavaProto = useToolchainForJavaProto;
+      host.strictProtoDeps = strictProtoDeps;
+      host.outputDescriptorSet = outputDescriptorSet;
       return host;
     }
   }
@@ -191,6 +213,8 @@ public class ProtoConfiguration extends Fragment {
   private final Label protoToolchainForJava;
   private final Label protoToolchainForJavaLite;
   private final boolean useToolchainForJavaProto;
+  private final StrictDepsMode strictProtoDeps;
+  private final boolean outputDescriptorSet;
 
   public ProtoConfiguration(Options options) {
     this.experimentalProtoExtraActions = options.experimentalProtoExtraActions;
@@ -203,6 +227,8 @@ public class ProtoConfiguration extends Fragment {
     this.protoToolchainForJava = options.protoToolchainForJava;
     this.protoToolchainForJavaLite = options.protoToolchainForJavaLite;
     this.useToolchainForJavaProto = options.useToolchainForJavaProto;
+    this.strictProtoDeps = options.strictProtoDeps;
+    this.outputDescriptorSet = options.outputDescriptorSet;
   }
 
   public ImmutableList<String> protocOpts() {
@@ -249,4 +275,12 @@ public class ProtoConfiguration extends Fragment {
   public boolean useToolchainForJavaProto() {
     return useToolchainForJavaProto;
   }
+
+  public StrictDepsMode strictProtoDeps() {
+    return strictProtoDeps;
+  }
+
+  public boolean outputDescriptorSet() {
+    return outputDescriptorSet;
+  }
 }
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
index 9fb22b92d8..03edbf5d60 100644
--- 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
@@ -119,7 +119,8 @@ public class BazelProtoLibraryTest extends BuildViewTestCase {
 
   @Test
   public void testDescriptorSetOutput_strictDeps_disabled() throws Exception {
-    scratch.file("x/BUILD", "proto_library(name='foo', srcs=['foo.proto'], strict_proto_deps=0)");
+    useConfiguration("--strict_proto_deps=off");
+    scratch.file("x/BUILD", "proto_library(name='foo', srcs=['foo.proto'])");
 
     for (String arg :
         getGeneratingSpawnAction(getDescriptorOutput("//x:foo")).getRemainingArguments()) {