Skip to content

Commit

Permalink
Remove support for adding non-exported transitions to Starlark attrib…
Browse files Browse the repository at this point in the history 
…utes.

Any native transitions must be exposed using ConfiguredTransitionApi.

Work towards composable starlark transitions: #22248.

PiperOrigin-RevId: 637945670
Change-Id: I712ead0e32ada463bbe024ab616ea96f50155f0d
  • Loading branch information
@katre @copybara-github
katre authored and copybara-github committed May 28, 2024
1 parent b7a1b21 commit b4d3b84
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 69 deletions.
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,6 @@ java_library(
":config/run_under",
":config/starlark_defined_config_transition",
":config/toolchain_type_requirement",
":config/transition_factories",
":config/transitions/composing_transition_factory",
":config/transitions/configuration_transition",
":config/transitions/no_transition",
Expand Down Expand Up @@ -420,6 +419,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/config:configuration_transition_api",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/core",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test",
Expand Down
118 changes: 69 additions & 49 deletions src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.analysis.config.TransitionFactories;
import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory.TransitionType;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
import com.google.devtools.build.lib.packages.Attribute.ImmutableAttributeFactory;
Expand All @@ -45,6 +45,7 @@
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.starlarkbuildapi.config.ConfigurationTransitionApi;
import com.google.devtools.build.lib.util.FileType;
import com.google.devtools.build.lib.util.FileTypeSet;
import java.util.List;
Expand Down Expand Up @@ -282,57 +283,28 @@ && containsNonNoneKey(arguments, ALLOW_SINGLE_FILE_ARG)) {

if (containsNonNoneKey(arguments, CONFIGURATION_ARG)) {
Object trans = arguments.get(CONFIGURATION_ARG);
boolean isSplit = false;
if (trans instanceof SplitTransition || trans instanceof StarlarkDefinedConfigTransition) {
isSplit = true;
} else if (trans instanceof TransitionFactory<?> tf) {
if (tf.isSplit()) {
isSplit = true;
}
}
TransitionFactory<AttributeTransitionData> transitionFactory = convertCfg(thread, trans);

// Check whether something is attempting an invalid late bound transition.
boolean isSplit = transitionFactory.isSplit();
if (isSplit && defaultValue instanceof StarlarkLateBoundDefault) {
throw Starlark.errorf(
"late-bound attributes must not have a split configuration transition");
}
// TODO(b/203203933): remove after removing --incompatible_disable_starlark_host_transitions.
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.createFactory());
} else if (trans.equals("exec")) {
builder.cfg(ExecutionTransitionFactory.createFactory());
} else if (trans instanceof ExecutionTransitionFactory executionTransitionFactory) {
builder.cfg(executionTransitionFactory);
} else if (trans instanceof SplitTransition) {
// TODO(jcater): remove TransitionFactories usage.
builder.cfg(TransitionFactories.of((SplitTransition) trans));
} else if (trans instanceof TransitionFactory) {
@SuppressWarnings("unchecked")
TransitionFactory<AttributeTransitionData> transitionFactory =
(TransitionFactory<AttributeTransitionData>) trans;
builder.cfg(transitionFactory);
} else if (trans instanceof StarlarkDefinedConfigTransition starlarkDefinedTransition) {
if (starlarkDefinedTransition.isForAnalysisTesting()) {
builder.hasAnalysisTestTransition();
} else {
builder.hasStarlarkDefinedTransition();
}
builder.cfg(new StarlarkAttributeTransitionProvider(starlarkDefinedTransition));
} else if (!trans.equals("target")) {
// We don't actively advertise the hard-coded but exposed transitions like
// android_split_transition because users of those transitions should already know about
// them.
throw Starlark.errorf(
"cfg must be either 'target', 'exec' or a starlark defined transition defined by the "
+ "exec() or transition() functions.");
}

// Check if this transition includes an analysis test or a Starlark transition.
transitionFactory.visit(
factory -> {
if (factory instanceof StarlarkAttributeTransitionProvider satp) {
if (satp.getStarlarkDefinedConfigTransitionForTesting().isForAnalysisTesting()) {
builder.hasAnalysisTestTransition();
} else {
builder.hasStarlarkDefinedTransition();
}
}
});

builder.cfg(transitionFactory);
}

if (containsNonNoneKey(arguments, ASPECTS_ARG)) {
Expand All @@ -345,6 +317,54 @@ && containsNonNoneKey(arguments, ALLOW_SINGLE_FILE_ARG)) {
return builder;
}

private static TransitionFactory<AttributeTransitionData> convertCfg(
StarlarkThread thread, @Nullable Object trans) throws EvalException {
// The most common case is no transition.
if (trans.equals("target") || trans.equals(Starlark.NONE)) {
return NoTransition.createFactory();
}
// TODO(b/203203933): remove after removing --incompatible_disable_starlark_host_transitions.
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.");
}
return ExecutionTransitionFactory.createFactory();
}
if (trans.equals("exec")) {
return ExecutionTransitionFactory.createFactory();
}
if (trans instanceof StarlarkDefinedConfigTransition starlarkDefinedTransition) {
return new StarlarkAttributeTransitionProvider(starlarkDefinedTransition);
}
if (trans instanceof ConfigurationTransitionApi cta) {
// Every ConfigurationTransitionApi must be a TransitionFactory instance to be usable.
if (cta instanceof TransitionFactory<?> tf) {
if (tf.transitionType().isCompatibleWith(TransitionType.ATTRIBUTE)) {
@SuppressWarnings("unchecked")
TransitionFactory<AttributeTransitionData> attrTransition =
(TransitionFactory<AttributeTransitionData>) tf;
return attrTransition;
}
} else {
throw new IllegalStateException(
"Every ConfigurationTransitionApi must be a TransitionFactory instance");
}
}

// We don't actively advertise the hard-coded but exposed transitions like
// android_split_transition because users of those transitions should already know about
// them.
throw Starlark.errorf(
"cfg must be either 'target', 'exec' or a starlark defined transition defined by the "
+ "exec() or transition() functions.");
}

/**
* Builds a list of sets of accepted providers from Starlark list {@code obj}. The list can either
* be a list of providers (in that case the result is a list with one set) or a list of lists of
Expand Down
19 changes: 13 additions & 6 deletions ...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkRuleFunctionsApi;
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkSubruleApi;
import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigurationTransitionApi;
import com.google.devtools.build.lib.util.FileTypeSet;
import com.google.devtools.build.lib.util.Pair;
import com.google.errorprone.annotations.FormatMethod;
Expand Down Expand Up @@ -847,12 +848,18 @@ private static TransitionFactory<RuleTransitionData> convertConfig(@Nullable Obj
// defined in Starlark via, cfg = transition
return new StarlarkRuleTransitionProvider(starlarkDefinedConfigTransition);
}
if (cfg instanceof TransitionFactory<?> tf) {
if (tf.transitionType().isCompatibleWith(TransitionType.RULE)) {
@SuppressWarnings("unchecked")
TransitionFactory<RuleTransitionData> ruleTransition =
(TransitionFactory<RuleTransitionData>) tf;
return ruleTransition;
if (cfg instanceof ConfigurationTransitionApi cta) {
// Every ConfigurationTransitionApi must be a TransitionFactory instance to be usable.
if (cta instanceof TransitionFactory<?> tf) {
if (tf.transitionType().isCompatibleWith(TransitionType.RULE)) {
@SuppressWarnings("unchecked")
TransitionFactory<RuleTransitionData> ruleTransition =
(TransitionFactory<RuleTransitionData>) tf;
return ruleTransition;
}
} else {
throw new IllegalStateException(
"Every ConfigurationTransitionApi must be a TransitionFactory instance");
}
}
throw Starlark.errorf(
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,7 @@ java_test(
"//src/main/java/com/google/devtools/build/lib/rules/java:java-compilation",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/config:configuration_transition_api",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp",
"//src/main/java/net/starlark/java/eval",
"//src/test/java/com/google/devtools/build/lib/analysis/util",
Expand Down
33 changes: 20 additions & 13 deletions src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory.TransitionType;
import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.cmdline.Label;
Expand All @@ -44,6 +45,7 @@
import com.google.devtools.build.lib.rules.java.JavaToolchainProvider;
import com.google.devtools.build.lib.starlark.util.BazelEvaluationTestCase;
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkSubruleApi;
import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigurationTransitionApi;
import com.google.devtools.build.lib.starlarkbuildapi.cpp.CppConfigurationApi;
import com.google.devtools.build.lib.testutil.TestConstants;
import net.starlark.java.eval.BuiltinFunction;
Expand Down Expand Up @@ -706,21 +708,26 @@ public void testSubruleAttrs_cannotHaveStarlarkTransitions() throws Exception {
")");
}

/**
* A test-only transition used to test native transitions on subrules. Must implement {@link
* ConfigurationTransitionApi} so that it is allowed by {@code rule}.
*/
private static final class NativeTransition
implements TransitionFactory<AttributeTransitionData>, ConfigurationTransitionApi {
@Override
public ConfigurationTransition create(AttributeTransitionData data) {
return null;
}

@Override
public TransitionType transitionType() {
return TransitionType.ATTRIBUTE;
}
}

@Test
public void testSubruleAttrs_cannotHaveNativeTransitions() throws Exception {
ev.update(
"native_transition",
new TransitionFactory<AttributeTransitionData>() {
@Override
public ConfigurationTransition create(AttributeTransitionData data) {
return null;
}

@Override
public TransitionType transitionType() {
return TransitionType.ATTRIBUTE;
}
});
ev.update("native_transition", new NativeTransition());
ev.checkEvalErrorContains(
"bad cfg for attribute '_foo': subrules may only have target/exec attributes.",
"_my_subrule = subrule(",
Expand Down

0 comments on commit b4d3b84

Please sign in to comment.