Skip to content

Commit

Permalink
Add incompatible flag to disable cfg="host" from Starlark
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Yannic authored and copybara-github committed Apr 12, 2022
1 parent a32a0fd commit 6464f1c
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
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;
Expand Down Expand Up @@ -229,6 +230,15 @@ && containsNonNoneKey(arguments, ALLOW_SINGLE_FILE_ARG)) {
}
// TODO(b/203203933): Officially deprecate HOST transition and remove this.
if (trans.equals("host")) {
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());
Expand All @@ -255,8 +265,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.");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,17 @@ public final class BuildLanguageOptions extends OptionsBase {
+ " 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
Expand Down Expand Up @@ -599,6 +610,9 @@ public StarlarkSemantics toStarlarkSemantics() {
.setBool(
INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS,
incompatibleTopLevelAspectsRequireProviders)
.setBool(
INCOMPATIBLE_DISABLE_STARLARK_HOST_TRANSITIONS,
incompatibleDisableStarlarkHostTransitions)
.build();
return INTERNER.intern(semantics);
}
Expand Down Expand Up @@ -666,6 +680,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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,13 @@ public interface StarlarkAttrModuleApi extends StarlarkValue {
// TODO(b/151742236): Update when new Starlark-based configuration framework is implemented.
String CONFIGURATION_DOC =
"<a href=\"https://bazel.build/rules/rules#configurations\">"
+ "Configuration</a> of the attribute. It can be either <code>\"host\"</code>, "
+ "<code>\"exec\"</code>, or <code>\"target\"</code>.";
+ "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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,14 @@ public void testAttrCfgNoMoreHost() throws Exception {
assertThat(attr.getTransitionFactory().isTool()).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)");
Expand All @@ -791,7 +799,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
Expand Down

0 comments on commit 6464f1c

Please sign in to comment.