Skip to content

Commit

Permalink
Provide a default for RuleClass.transitionFactory.
Browse files Browse the repository at this point in the history
This removes a lot of useless null checks.

Work towards composable starlark transitions: #22248.

PiperOrigin-RevId: 639805169
Change-Id: I45b4f4b7cf71e568578956d6b7723fc999f5afc8
  • Loading branch information
katre authored and copybara-github committed Jun 3, 2024
1 parent 73892d5 commit a67dac5
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -235,13 +235,11 @@ private static void addRequiredFragmentsFromRuleTransitions(
ConfiguredAttributeMapper attributeMap,
BuildOptionDetails optionDetails,
@Nullable StarlarkAttributeTransitionProvider starlarkExecTransition) {
if (target.getRuleClassObject().getTransitionFactory() != null) {
target
.getRuleClassObject()
.getTransitionFactory()
.create(RuleTransitionData.create(target, configConditions.asProviders(), configHash))
.addRequiredFragments(requiredFragments, optionDetails);
}
target
.getRuleClassObject()
.getTransitionFactory()
.create(RuleTransitionData.create(target, configConditions.asProviders(), configHash))
.addRequiredFragments(requiredFragments, optionDetails);
// We don't set the execution platform in this data because a) that doesn't affect which
// fragments are required and b) it's one less parameter we have to pass to
// RequiredFragmenstUtil's public interface.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -466,13 +466,9 @@ public StateMachine computeTransition(Tasks tasks) {
configConditions.asProviders(),
preRuleTransitionKey.getConfigurationKey().getOptionsChecksum());

ConfigurationTransition transition = null;

TransitionFactory<RuleTransitionData> transitionFactory =
target.getAssociatedRule().getRuleClassObject().getTransitionFactory();
if (transitionFactory != null) {
transition = transitionFactory.create(transitionData);
}
ConfigurationTransition transition = transitionFactory.create(transitionData);

if (trimmingTransitionFactory != null) {
var trimmingTransition = trimmingTransitionFactory.create(transitionData);
Expand All @@ -483,11 +479,6 @@ public StateMachine computeTransition(Tasks tasks) {
}
}

if (transition == null) {
lookUpConfigurationValue(tasks);
return DONE;
}

this.ruleTransition = transition;

return new TransitionApplier(
Expand Down Expand Up @@ -695,18 +686,13 @@ public static DetailedExitCode createDetailedExitCode(String message) {

// Public for Cquery.
// TODO: @aranguyen keep cquery in sync with ConfiguredTargetFunction
@Nullable
public static ConfigurationTransition computeTransition(
Rule rule, @Nullable TransitionFactory<RuleTransitionData> trimmingTransitionFactory) {
var transitionData = RuleTransitionData.create(rule, /* configConditions= */ null, "");

ConfigurationTransition transition = null;

TransitionFactory<RuleTransitionData> transitionFactory =
rule.getRuleClassObject().getTransitionFactory();
if (transitionFactory != null) {
transition = transitionFactory.create(transitionData);
}
ConfigurationTransition transition = transitionFactory.create(transitionData);
boolean isAlias = rule.getAssociatedRule().getName().equals("alias");
if (trimmingTransitionFactory != null && !isAlias) {
var trimmingTransition = trimmingTransitionFactory.create(transitionData);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,7 @@ public static StarlarkRuleFunction createRule(
exposed.addToRuleFromStarlark(ruleDefinitionEnvironment, builder);
}
});
if (parent != null && parent.getTransitionFactory() != null) {
if (parent != null) {
transitionFactory =
ComposingTransitionFactory.of(transitionFactory, parent.getTransitionFactory());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
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.cmdline.Label;
Expand Down Expand Up @@ -721,7 +722,7 @@ public String toString() {
ImmutableList.builder();
private boolean ignoreLicenses = false;
private ImplicitOutputsFunction implicitOutputsFunction = SafeImplicitOutputsFunction.NONE;
@Nullable private TransitionFactory<RuleTransitionData> transitionFactory;
private TransitionFactory<RuleTransitionData> transitionFactory = NoTransition.getFactory();
private ConfiguredTargetFactory<?, ?, ?> configuredTargetFactory = null;
private final AdvertisedProviderSet.Builder advertisedProviders =
AdvertisedProviderSet.builder();
Expand Down Expand Up @@ -1141,7 +1142,8 @@ public Builder cfg(TransitionFactory<RuleTransitionData> transitionFactory) {
type != RuleClassType.ABSTRACT,
"Setting not inherited property (cfg) of abstract rule class '%s'",
name);
Preconditions.checkState(this.transitionFactory == null, "Property cfg has already been set");
Preconditions.checkState(
NoTransition.isInstance(this.transitionFactory), "Property cfg has already been set");
Preconditions.checkNotNull(transitionFactory);
Preconditions.checkArgument(
transitionFactory.transitionType().isCompatibleWith(TransitionType.RULE));
Expand Down Expand Up @@ -1674,7 +1676,7 @@ public Attribute.Builder<?> copy(String name) {
* A factory which will produce a configuration transition that should be applied on any edge of
* the configured target graph that leads into a target of this rule class.
*/
@Nullable private final TransitionFactory<RuleTransitionData> transitionFactory;
private final TransitionFactory<RuleTransitionData> transitionFactory;

/** The factory that creates configured targets from this rule. */
private final ConfiguredTargetFactory<?, ?, ?> configuredTargetFactory;
Expand Down Expand Up @@ -1782,7 +1784,7 @@ public Attribute.Builder<?> copy(String name) {
ImmutableList<AllowlistChecker> allowlistCheckers,
boolean ignoreLicenses,
ImplicitOutputsFunction implicitOutputsFunction,
@Nullable TransitionFactory<RuleTransitionData> transitionFactory,
TransitionFactory<RuleTransitionData> transitionFactory,
ConfiguredTargetFactory<?, ?, ?> configuredTargetFactory,
AdvertisedProviderSet advertisedProviders,
@Nullable StarlarkCallable configuredTargetFunction,
Expand Down Expand Up @@ -1892,7 +1894,6 @@ public ImplicitOutputsFunction getDefaultImplicitOutputsFunction() {
return implicitOutputsFunction;
}

@Nullable
public TransitionFactory<RuleTransitionData> getTransitionFactory() {
return transitionFactory;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,6 @@ private static String getRuleClassTransition(CqueryNode ct, Target target) {

TransitionFactory<RuleTransitionData> factory =
rule.getRuleClassObject().getTransitionFactory();
if (factory == null) {
return "";
}
return factory
.create(
RuleTransitionData.create(
Expand Down

0 comments on commit a67dac5

Please sign in to comment.