Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new data class for rule transition factories. #13749

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@
import com.google.devtools.build.lib.graph.Node;
import com.google.devtools.build.lib.packages.BazelStarlarkContext;
import com.google.devtools.build.lib.packages.NativeAspectClass;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.Builder.ThirdPartyLicenseExistencePolicy;
import com.google.devtools.build.lib.packages.RuleTransitionData;
import com.google.devtools.build.lib.packages.SymbolGenerator;
import com.google.devtools.build.lib.starlarkbuildapi.core.Bootstrap;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
Expand Down Expand Up @@ -150,7 +150,7 @@ public static class Builder implements RuleDefinitionEnvironment {
private final Map<Class<? extends RuleDefinition>, RuleClass> ruleMap = new HashMap<>();
private final Digraph<Class<? extends RuleDefinition>> dependencyGraph = new Digraph<>();
private final List<Class<? extends Fragment>> universalFragments = new ArrayList<>();
@Nullable private TransitionFactory<Rule> trimmingTransitionFactory = null;
@Nullable private TransitionFactory<RuleTransitionData> trimmingTransitionFactory = null;
@Nullable private PatchTransition toolchainTaggedTrimmingTransition = null;
private OptionsDiffPredicate shouldInvalidateCacheForOptionDiff =
OptionsDiffPredicate.ALWAYS_INVALIDATE;
Expand Down Expand Up @@ -363,7 +363,7 @@ public Builder setThirdPartyLicenseExistencePolicy(ThirdPartyLicenseExistencePol
* feature flags, and support for this transition factory will likely be removed at some point
* in the future (whenever automatic trimming is sufficiently workable).
*/
public Builder addTrimmingTransitionFactory(TransitionFactory<Rule> factory) {
public Builder addTrimmingTransitionFactory(TransitionFactory<RuleTransitionData> factory) {
Preconditions.checkNotNull(factory);
Preconditions.checkArgument(!factory.isSplit());
if (trimmingTransitionFactory == null) {
Expand All @@ -389,7 +389,8 @@ public Builder setToolchainTaggedTrimmingTransition(PatchTransition transition)
* @see #addTrimmingTransitionFactory(TransitionFactory)
*/
@VisibleForTesting(/* for testing trimming transition factories without relying on prod use */ )
public Builder overrideTrimmingTransitionFactoryForTesting(TransitionFactory<Rule> factory) {
public Builder overrideTrimmingTransitionFactoryForTesting(
TransitionFactory<RuleTransitionData> factory) {
trimmingTransitionFactory = null;
return this.addTrimmingTransitionFactory(factory);
}
Expand Down Expand Up @@ -629,7 +630,7 @@ public String getToolsRepository() {
private final Map<String, Class<? extends Fragment>> optionsToFragmentMap;

/** The transition factory used to produce the transition that will trim targets. */
@Nullable private final TransitionFactory<Rule> trimmingTransitionFactory;
@Nullable private final TransitionFactory<RuleTransitionData> trimmingTransitionFactory;

/** The transition to apply to toolchain deps for manual trimming. */
@Nullable private final PatchTransition toolchainTaggedTrimmingTransition;
Expand Down Expand Up @@ -682,7 +683,7 @@ private ConfiguredRuleClassProvider(
ImmutableList<Class<? extends FragmentOptions>> configurationOptions,
FragmentClassSet configurationFragmentClasses,
FragmentClassSet universalFragments,
@Nullable TransitionFactory<Rule> trimmingTransitionFactory,
@Nullable TransitionFactory<RuleTransitionData> trimmingTransitionFactory,
PatchTransition toolchainTaggedTrimmingTransition,
OptionsDiffPredicate shouldInvalidateCacheForOptionDiff,
PrerequisiteValidator prerequisiteValidator,
Expand Down Expand Up @@ -830,7 +831,7 @@ public Class<? extends Fragment> getConfigurationFragmentForOption(String requir
* trimming is sufficiently workable
*/
@Nullable
public TransitionFactory<Rule> getTrimmingTransitionFactory() {
public TransitionFactory<RuleTransitionData> getTrimmingTransitionFactory() {
return trimmingTransitionFactory;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import com.google.devtools.build.lib.packages.PackageGroup;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleTransitionData;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.skyframe.ToolchainContextKey;
Expand Down Expand Up @@ -182,7 +183,7 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
@Nullable ToolchainCollection<ToolchainContext> toolchainContexts,
boolean useToolchainTransition,
@Nullable TransitionFactory<Rule> trimmingTransitionFactory)
@Nullable TransitionFactory<RuleTransitionData> trimmingTransitionFactory)
throws Failure, InterruptedException, InconsistentAspectOrderException {
NestedSetBuilder<Cause> rootCauses = NestedSetBuilder.stableOrder();
OrderedSetMultimap<DependencyKind, DependencyKey> outgoingEdges =
Expand Down Expand Up @@ -240,7 +241,7 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
@Nullable ToolchainCollection<ToolchainContext> toolchainContexts,
boolean useToolchainTransition,
NestedSetBuilder<Cause> rootCauses,
@Nullable TransitionFactory<Rule> trimmingTransitionFactory)
@Nullable TransitionFactory<RuleTransitionData> trimmingTransitionFactory)
throws Failure, InterruptedException, InconsistentAspectOrderException {
Target target = node.getTarget();
BuildConfiguration config = node.getConfiguration();
Expand Down Expand Up @@ -446,7 +447,7 @@ private OrderedSetMultimap<DependencyKind, DependencyKey> fullyResolveDependenci
OrderedSetMultimap<DependencyKind, PartiallyResolvedDependency> partiallyResolvedDeps,
Map<Label, Target> targetMap,
BuildConfiguration originalConfiguration,
@Nullable TransitionFactory<Rule> trimmingTransitionFactory)
@Nullable TransitionFactory<RuleTransitionData> trimmingTransitionFactory)
throws InconsistentAspectOrderException {
OrderedSetMultimap<DependencyKind, DependencyKey> outgoingEdges = OrderedSetMultimap.create();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy;
import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleTransitionData;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.util.ClassName;
import java.util.Collection;
Expand Down Expand Up @@ -202,7 +203,11 @@ private static ImmutableList<ConfigurationTransition> getTransitions(
String configHash) {
ImmutableList.Builder<ConfigurationTransition> transitions = ImmutableList.builder();
if (target.getRuleClassObject().getTransitionFactory() != null) {
transitions.add(target.getRuleClassObject().getTransitionFactory().create(target));
transitions.add(
target
.getRuleClassObject()
.getTransitionFactory()
.create(RuleTransitionData.create(target)));
}
// 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.devtools.build.lib.analysis.config.transitions.NullTransition;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleTransitionData;
import com.google.devtools.build.lib.packages.Target;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -52,7 +53,7 @@ public static ConfigurationTransition evaluateTransition(
BuildConfiguration fromConfig,
ConfigurationTransition baseTransition,
Target toTarget,
@Nullable TransitionFactory<Rule> trimmingTransitionFactory) {
@Nullable TransitionFactory<RuleTransitionData> trimmingTransitionFactory) {

// I. The null configuration always remains the null configuration. We could fold this into
// (III), but NoTransition doesn't work if the source is the null configuration.
Expand Down Expand Up @@ -93,7 +94,7 @@ public static ConfigurationTransition evaluateTransition(
private static ConfigurationTransition applyRuleTransition(
ConfigurationTransition currentTransition, Target toTarget) {
Rule associatedRule = toTarget.getAssociatedRule();
TransitionFactory<Rule> transitionFactory =
TransitionFactory<RuleTransitionData> transitionFactory =
associatedRule.getRuleClassObject().getTransitionFactory();
return applyTransitionFromFactory(currentTransition, toTarget, transitionFactory);
}
Expand All @@ -106,10 +107,11 @@ private static ConfigurationTransition applyRuleTransition(
private static ConfigurationTransition applyTransitionFromFactory(
ConfigurationTransition currentTransition,
Target toTarget,
@Nullable TransitionFactory<Rule> transitionFactory) {
@Nullable TransitionFactory<RuleTransitionData> transitionFactory) {
if (transitionFactory != null) {
return ComposingTransition.of(
currentTransition, transitionFactory.create(toTarget.getAssociatedRule()));
currentTransition,
transitionFactory.create(RuleTransitionData.create(toTarget.getAssociatedRule())));
}
return currentTransition;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,14 @@
import com.google.devtools.build.lib.packages.PackageFactory.PackageContext;
import com.google.devtools.build.lib.packages.PredicateWithMessage;
import com.google.devtools.build.lib.packages.Provider;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
import com.google.devtools.build.lib.packages.RuleClass.ToolchainTransitionMode;
import com.google.devtools.build.lib.packages.RuleFactory;
import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException;
import com.google.devtools.build.lib.packages.RuleFunction;
import com.google.devtools.build.lib.packages.RuleTransitionData;
import com.google.devtools.build.lib.packages.StarlarkAspect;
import com.google.devtools.build.lib.packages.StarlarkCallbackHelper;
import com.google.devtools.build.lib.packages.StarlarkDefinedAspect;
Expand Down Expand Up @@ -404,7 +404,8 @@ public StarlarkCallable rule(
builder.cfg((PatchTransition) cfg);
} else if (cfg instanceof TransitionFactory) {
@SuppressWarnings("unchecked")
TransitionFactory<Rule> transitionFactory = (TransitionFactory<Rule>) cfg;
TransitionFactory<RuleTransitionData> transitionFactory =
(TransitionFactory<RuleTransitionData>) cfg;
builder.cfg(transitionFactory);
} else {
// This is not technically true: it could also be a native transition, but this is the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.RawAttributeMapper;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleTransitionData;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.packages.StructProvider;
import java.util.LinkedHashMap;
Expand All @@ -50,7 +51,7 @@
*
* <p>For starlark-defined attribute transitions, see {@link StarlarkAttributeTransitionProvider}.
*/
public class StarlarkRuleTransitionProvider implements TransitionFactory<Rule> {
public class StarlarkRuleTransitionProvider implements TransitionFactory<RuleTransitionData> {

private final StarlarkDefinedConfigTransition starlarkDefinedConfigTransition;

Expand Down Expand Up @@ -115,7 +116,7 @@ public int hashCode() {
Caffeine.newBuilder().softValues().build();

@Override
public PatchTransition create(Rule rule) {
public PatchTransition create(RuleTransitionData ruleData) {
// This wouldn't be safe if rule transitions could read attributes with select(), in which case
// the rule alone isn't sufficient to define the transition's semantics (both the rule and its
// configuration are needed). Rule transitions can't read select()s, so this is a non-issue.
Expand All @@ -125,8 +126,8 @@ public PatchTransition create(Rule rule) {
// transition never reads {@code attr}. If we had a way to formally identify such transitions,
// we wouldn't need {@code rule} in the cache key.
return cache.get(
new CacheKey(starlarkDefinedConfigTransition, rule),
unused -> new FunctionPatchTransition(starlarkDefinedConfigTransition, rule));
new CacheKey(starlarkDefinedConfigTransition, ruleData.rule()),
unused -> new FunctionPatchTransition(starlarkDefinedConfigTransition, ruleData.rule()));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,16 @@
import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleTransitionData;
import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.common.options.Options;

/**
* Trimming transition factory which removes the test config fragment when entering a non-test rule.
*/
public final class TestTrimmingTransitionFactory implements TransitionFactory<Rule> {
public final class TestTrimmingTransitionFactory implements TransitionFactory<RuleTransitionData> {

private static final ImmutableSet<String> TEST_OPTIONS =
ImmutableSet.copyOf(Options.getDefaults(TestOptions.class).asMap().keySet());
Expand Down Expand Up @@ -102,18 +102,18 @@ public BuildOptions patch(BuildOptionsView originalOptions, EventHandler eventHa
}

@Override
public PatchTransition create(Rule rule) {
RuleClass ruleClass = rule.getRuleClassObject();
public PatchTransition create(RuleTransitionData ruleData) {
RuleClass ruleClass = ruleData.rule().getRuleClassObject();
if (ruleClass
.getConfigurationFragmentPolicy()
.isLegalConfigurationFragment(TestConfiguration.class)
|| TargetUtils.isAlias(rule)) {
|| TargetUtils.isAlias(ruleData.rule())) {
// If Test rule, no need to trim here.
// If Alias rule, might point to test rule so don't trim yet.
return NoTransition.INSTANCE;
}

for (String referencedOptions : ruleClass.getOptionReferenceFunction().apply(rule)) {
for (String referencedOptions : ruleClass.getOptionReferenceFunction().apply(ruleData.rule())) {
if (TEST_OPTIONS.contains(referencedOptions)) {
// Test-option-referencing config_setting; no need to trim here.
return NoTransition.INSTANCE;
Expand All @@ -124,7 +124,7 @@ public PatchTransition create(Rule rule) {
// Use an attribute mapper to ensure testonly is resolved to an actual boolean value.
// It is expected all rules should have a boolean testonly value so the `has` check is only
// there as an over-abundance of caution.
NonconfigurableAttributeMapper attrs = NonconfigurableAttributeMapper.of(rule);
NonconfigurableAttributeMapper attrs = NonconfigurableAttributeMapper.of(ruleData.rule());
if (attrs.has("testonly", Type.BOOLEAN) && attrs.get("testonly", Type.BOOLEAN)) {
return TestTrimmingTransition.TESTONLY_TRUE;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ java_library(
deps = [
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/composing_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
"//src/main/java/com/google/devtools/build/lib/bazel/rules/cpp:bazel_cpp_semantics",
"//src/main/java/com/google/devtools/build/lib/packages",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransitionFactory;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
import com.google.devtools.build.lib.packages.ImplicitOutputsFunction;
import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SafeImplicitOutputsFunction;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleTransitionData;
import com.google.devtools.build.lib.rules.config.ConfigFeatureFlagTransitionFactory;
import com.google.devtools.build.lib.rules.objc.AppleCrosstoolTransition;
import com.google.devtools.build.lib.rules.objc.AppleStaticLibraryBaseRule;
Expand All @@ -48,7 +50,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.setImplicitOutputsFunction(ImplicitOutputsFunction.fromFunctions(LIPO_ARCHIVE))
.cfg(
ComposingTransitionFactory.of(
(rule) -> AppleCrosstoolTransition.APPLE_CROSSTOOL_TRANSITION,
(TransitionFactory<RuleTransitionData>)
(unused) -> AppleCrosstoolTransition.APPLE_CROSSTOOL_TRANSITION,
new ConfigFeatureFlagTransitionFactory("feature_flags")))
.build();
}
Expand Down
Loading