Add incompatible flag to disable cfg="host" from Starlark (bazelbuild#15436)

ckolli5 · Yannic · web-flow · commit ddbac4ad0c86 · 2022-05-09T18:16:06.000-05:00
Host transitions are problematic where the host and exec environment are different (e.g., with Remote Execution). This change adds an incompatible flag that fails analysis if Starlark rules use `cfg = "host"`. Migrating is pretty straigt forward (replace `host` with `exec`) and should be automatable by buildifier (although I haven't tried to do so). Closes bazelbuild#14383. PiperOrigin-RevId: 441253060 Co-authored-by: Yannic <yannic@engflow.com>
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java
@@ -41,6 +41,8 @@
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.packages.Type.ConversionException;
 import com.google.devtools.build.lib.packages.Type.LabelClass;
+import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
+import com.google.devtools.build.lib.starlarkbuildapi.NativeComputedDefaultApi;
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkAttrModuleApi;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.FileTypeSet;
@@ -226,7 +228,16 @@ && containsNonNoneKey(arguments, ALLOW_SINGLE_FILE_ARG)) {
             "late-bound attributes must not have a split configuration transition");
       }
       if (trans.equals("host")) {
-        builder.cfg(HostTransition.createFactory());
+        boolean disableStarlarkHostTransitions =
+            thread
+                .getSemantics()
+                .getBool(BuildLanguageOptions.INCOMPATIBLE_DISABLE_STARLARK_HOST_TRANSITIONS);
+        if (disableStarlarkHostTransitions) {
+          throw new EvalException(
+              "'cfg = \"host\"' is deprecated and should no longer be used. Please use "
+                  + "'cfg = \"exec\"' instead.");
+        }
+        builder.cfg(ExecutionTransitionFactory.create());
       } else if (trans.equals("exec")) {
         builder.cfg(ExecutionTransitionFactory.create());
       } else if (trans instanceof ExecutionTransitionFactory) {
@@ -252,8 +263,8 @@ && containsNonNoneKey(arguments, ALLOW_SINGLE_FILE_ARG)) {
         // android_split_transition because users of those transitions should already know about
         // them.
         throw Starlark.errorf(
-            "cfg must be either 'host', 'target', 'exec' or a starlark defined transition defined"
-                + " by the exec() or transition() functions.");
+            "cfg must be either 'target', 'exec' or a starlark defined transition defined by the "
+                + "exec() or transition() functions.");
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java
@@ -548,6 +548,17 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {
               + " providers of the aspect.")
   public boolean incompatibleTopLevelAspectsRequireProviders;
 
+  @Option(
+      name = "incompatible_disable_starlark_host_transitions",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      help =
+          "If set to true, rule attributes cannot set 'cfg = \"host\"'. Rules should set "
+              + "'cfg = \"exec\"' instead.")
+  public boolean incompatibleDisableStarlarkHostTransitions;
+
   /**
    * An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should
    * never accumulate a large number of these and being able to shortcut on object identity makes a
@@ -615,6 +626,9 @@ public StarlarkSemantics toStarlarkSemantics() {
             .setBool(
                 INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS,
                 incompatibleTopLevelAspectsRequireProviders)
+            .setBool(
+                INCOMPATIBLE_DISABLE_STARLARK_HOST_TRANSITIONS,
+                incompatibleDisableStarlarkHostTransitions)
             .build();
     return INTERNER.intern(semantics);
   }
@@ -683,6 +697,8 @@ public StarlarkSemantics toStarlarkSemantics() {
       "-incompatible_visibility_private_attributes_at_definition";
   public static final String INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS =
       "-incompatible_top_level_aspects_require_providers";
+  public static final String INCOMPATIBLE_DISABLE_STARLARK_HOST_TRANSITIONS =
+      "-incompatible_disable_starlark_host_transitions";
 
   // non-booleans
   public static final StarlarkSemantics.Key<String> EXPERIMENTAL_BUILTINS_BZL_PATH =
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkAttrModuleApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkAttrModuleApi.java
@@ -92,9 +92,14 @@ public interface StarlarkAttrModuleApi extends StarlarkValue {
   String CONFIGURATION_ARG = "cfg";
   // TODO(b/151742236): Update when new Starlark-based configuration framework is implemented.
   String CONFIGURATION_DOC =
-      "<a href=\"https://docs.bazel.build/versions/main/skylark/rules.html#configurations\">"
-          + "Configuration</a> of the attribute. It can be either <code>\"host\"</code>, "
-          + "<code>\"exec\"</code>, or <code>\"target\"</code>.";
+      "<a href=\"https://bazel.build/rules/rules#configurations\">"
+          + "Configuration</a> of the attribute. It can be either <code>\"exec\"</code>, which "
+          + "indicates that the dependency is built for the <code>execution platform</code>, or "
+          + "<code>\"target\"</code>, which indicates that the dependency is build for the "
+          + "<code>target platform</code>. A typical example of the difference is when building "
+          + "mobile apps, where the <code>target platform</code> is <code>Android</code> or "
+          + "<code>iOS</code> while the <code>execution platform</code> is <code>Linux</code>, "
+          + "<code>macOS</code>, or <code>Windows</code>.";
 
   String DEFAULT_ARG = "default";
   // A trailing space is required because it's often prepended to other sentences
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
@@ -732,6 +732,14 @@ public void testAttrCfg() throws Exception {
     assertThat(attr.getTransitionFactory().isHost()).isTrue();
   }
 
+  @Test
+  public void testAttrCfgHostDisabled() throws Exception {
+    setBuildLanguageOptions("--incompatible_disable_starlark_host_transitions");
+
+    EvalException ex = assertThrows(EvalException.class, () -> ev.eval("attr.label(cfg = 'host')"));
+    assertThat(ex).hasMessageThat().contains("Please use 'cfg = \"exec\"' instead");
+  }
+
   @Test
   public void testAttrCfgTarget() throws Exception {
     Attribute attr = buildAttribute("a1", "attr.label(cfg = 'target', allow_files = True)");
@@ -742,7 +750,7 @@ public void testAttrCfgTarget() throws Exception {
   public void incompatibleDataTransition() throws Exception {
     EvalException expected =
         assertThrows(EvalException.class, () -> ev.eval("attr.label(cfg = 'data')"));
-    assertThat(expected).hasMessageThat().contains("cfg must be either 'host', 'target'");
+    assertThat(expected).hasMessageThat().contains("cfg must be either 'target', 'exec'");
   }
 
   @Test
