Mandatory cfg parameter for labels if executable=1 is provided

RELNOTES[INC]: All executable labels must also have a cfg parameter specified.

--
PiperOrigin-RevId: 144332992
MOS_MIGRATED_REVID=144332992

3 files changed, 43 insertions, 17 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
index 86d6542037..7ef502efb9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
@@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
@@ -159,8 +158,8 @@ public final class SkylarkAttr {
   }
 
   private static Attribute.Builder<?> createAttribute(
-      Type<?> type, SkylarkDict<String, Object> arguments, FuncallExpression ast, Environment env,
-      Location loc) throws EvalException, ConversionException {
+      Type<?> type, SkylarkDict<String, Object> arguments, FuncallExpression ast, Environment env)
+      throws EvalException, ConversionException {
     // We use an empty name now so that we can set it later.
     // This trick makes sense only in the context of Skylark (builtin rules should not use it).
     Attribute.Builder<?> builder = Attribute.attr("", type);
@@ -205,11 +204,11 @@ public final class SkylarkAttr {
     if (containsNonNoneKey(arguments, EXECUTABLE_ARG) && (Boolean) arguments.get(EXECUTABLE_ARG)) {
       builder.setPropertyFlag("EXECUTABLE");
       if (!containsNonNoneKey(arguments, CONFIGURATION_ARG)) {
-        String message = "Argument `cfg = \"host\"`, `cfg = \"data\"`, or `cfg = \"target\"` "
-            + "is required if `executable = True` is provided for a label. Please see "
+        throw new EvalException(
+            ast.getLocation(),
+            "cfg parameter is mandatory when executable=True is provided. Please see "
             + "https://www.bazel.build/versions/master/docs/skylark/rules.html#configurations "
-            + "for more details.";
-        env.handleEvent(Event.warn(loc, message));
+            + "for more details.");
       }
     }
 
@@ -325,7 +324,7 @@ public final class SkylarkAttr {
       SkylarkDict<String, Object> kwargs, Type<?> type, FuncallExpression ast, Environment env)
       throws EvalException {
     try {
-      return new Descriptor(createAttribute(type, kwargs, ast, env, ast.getLocation()));
+      return new Descriptor(createAttribute(type, kwargs, ast, env));
     } catch (ConversionException e) {
       throw new EvalException(ast.getLocation(), e.getMessage());
     }
@@ -356,7 +355,7 @@ public final class SkylarkAttr {
         Preconditions.checkNotNull(maybeGetNonConfigurableReason(type), type);
     try {
       return new Descriptor(
-          createAttribute(type, kwargs, ast, env, ast.getLocation())
+          createAttribute(type, kwargs, ast, env)
               .nonconfigurable(whyNotConfigurableReason));
     } catch (ConversionException e) {
       throw new EvalException(ast.getLocation(), e.getMessage());
@@ -622,8 +621,7 @@ public final class SkylarkAttr {
                     CONFIGURATION_ARG,
                     cfg),
                 ast,
-                env,
-                ast.getLocation());
+                env);
             ImmutableList<SkylarkAspect> skylarkAspects =
                 ImmutableList.copyOf(aspects.getContents(SkylarkAspect.class, "aspects"));
             return new Descriptor(attribute, skylarkAspects);
@@ -902,7 +900,7 @@ public final class SkylarkAttr {
                   cfg);
           try {
             Attribute.Builder<?> attribute =
-                createAttribute(BuildType.LABEL_LIST, kwargs, ast, env, ast.getLocation());
+                createAttribute(BuildType.LABEL_LIST, kwargs, ast, env);
             ImmutableList<SkylarkAspect> skylarkAspects =
                 ImmutableList.copyOf(aspects.getContents(SkylarkAspect.class, "aspects"));
             return new Descriptor(attribute, skylarkAspects);
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index 0872174ea0..47b4a98f41 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -1052,7 +1052,7 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
   }
 
   @Test
-  public void multipleExecutablesInTarget() throws Exception {
+  public void testMultipleExecutablesInTarget() throws Exception {
     scratch.file("foo/extension.bzl",
         "def _aspect_impl(target, ctx):",
         "   return struct()",
@@ -1061,8 +1061,8 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
         "   pass",
         "my_rule = rule(_main_rule_impl,",
         "   attrs = { ",
-        "      'exe1' : attr.label(executable = True, allow_files = True),",
-        "      'exe2' : attr.label(executable = True, allow_files = True),",
+        "      'exe1' : attr.label(executable = True, allow_files = True, cfg = 'host'),",
+        "      'exe2' : attr.label(executable = True, allow_files = True, cfg = 'host'),",
         "   },",
         ")"
     );
@@ -1081,7 +1081,6 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
     assertThat(analysisResultOfAspect.hasError()).isFalse();
   }
 
-
   @Test
   public void aspectFragmentAccessSuccess() throws Exception {
     getConfiguredTargetForAspectFragment(
@@ -1251,7 +1250,6 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
     };
   }
 
-
   @Test
   public void aspectOutputsToBinDirectory() throws Exception {
     scratch.file(
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 8c6df5b6d6..da072b93ec 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -56,7 +56,9 @@ import com.google.devtools.build.lib.util.FileTypeSet;
 import java.util.Collection;
 import org.junit.Assert;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
@@ -65,6 +67,7 @@ import org.junit.runners.JUnit4;
  */
 @RunWith(JUnit4.class)
 public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
+  @Rule public ExpectedException thrown = ExpectedException.none();
 
   @Before
   public final void createBuildFile() throws Exception  {
@@ -1083,6 +1086,7 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
   }
 
   @Test
+
   public void structsAsDeclaredProvidersTest() throws Exception {
     evalAndExport(
         "data = struct(x = 1)"
@@ -1115,4 +1119,30 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
         "   pass",
         "aspect(_impl, attr_aspects=['*', 'foo'])");
   }
+
+  @Test
+  public void testMandatoryConfigParameterForExecutableLabels() throws Exception {
+    scratch.file("third_party/foo/extension.bzl",
+      "def _main_rule_impl(ctx):",
+      "    pass",
+      "my_rule = rule(_main_rule_impl,",
+      "    attrs = { ",
+      "        'exe' : attr.label(executable = True, allow_files = True),",
+      "    },",
+      ")"
+    );
+    scratch.file("third_party/foo/BUILD",
+      "load('extension',  'my_rule')",
+      "my_rule(name = 'main', exe = ':tool.sh')"
+    );
+
+    try {
+      createRuleContext("//third_party/foo:main");
+      Assert.fail();
+    } catch (AssertionError e) {
+      assertThat(e.getMessage()).contains("cfg parameter is mandatory when executable=True is "
+          + "provided.");
+    }
+  }
 }
+