Skip to content

Commit

Permalink
Enable rule transition to inspect configurable attributes' value
Browse files Browse the repository at this point in the history
Fixes #15157

PiperOrigin-RevId: 558262406
Change-Id: I514c95c0470a2d171a5b276f25b65a4adddf17e9
  • Loading branch information
Googler authored and copybara-github committed Aug 18, 2023
1 parent c9d7ff9 commit ceddfb1
Show file tree
Hide file tree
Showing 29 changed files with 862 additions and 113 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ public ImmutableCollection<String> getStarlarkFragmentNames() {
return starlarkVisibleFragments.keySet();
}

private BuildEventId getEventId() {
public BuildEventId getEventId() {
return BuildEventIdUtil.configurationId(checksum());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,11 @@ public static ConfigMatchingProvider create(
/** The target's label. */
public abstract Label label();

abstract ImmutableMultimap<String, String> settingsMap();
public abstract ImmutableMultimap<String, String> settingsMap();

abstract ImmutableMap<Label, String> flagSettingsMap();
public abstract ImmutableMap<Label, String> flagSettingsMap();

abstract ImmutableSet<Label> constraintValuesSetting();
public abstract ImmutableSet<Label> constraintValuesSetting();

/**
* Whether or not the configuration criteria defined by this target match its actual
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,12 @@ public static RequiredConfigFragmentsProvider getRuleRequiredFragmentsIfEnabled(
.addRuleImplSpecificRequiredConfigFragments(requiredFragments, attributes, configuration);
}
addRequiredFragmentsFromRuleTransitions(
requiredFragments, target, attributes, configuration.getBuildOptionDetails());
configConditions,
configuration.checksum(),
requiredFragments,
target,
attributes,
configuration.getBuildOptionDetails());

// We consider build settings (which are both targets and configuration) to require themselves.
if (target.isBuildSetting()) {
Expand Down Expand Up @@ -216,6 +221,8 @@ private static RequiredConfigFragmentsProvider.Builder getRequiredFragments(
* because the child's properties determine that dependency.
*/
private static void addRequiredFragmentsFromRuleTransitions(
ConfigConditions configConditions,
String configHash,
RequiredConfigFragmentsProvider.Builder requiredFragments,
Rule target,
ConfiguredAttributeMapper attributeMap,
Expand All @@ -224,7 +231,7 @@ private static void addRequiredFragmentsFromRuleTransitions(
target
.getRuleClassObject()
.getTransitionFactory()
.create(RuleTransitionData.create(target))
.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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import com.google.devtools.build.lib.packages.BazelStarlarkContext;
import com.google.devtools.build.lib.packages.BazelStarlarkContext.Phase;
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.SymbolGenerator;
import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigurationTransitionApi;
Expand Down Expand Up @@ -95,7 +96,7 @@ public enum Settings {

// The values in this cache should always be instances of StarlarkTransition, but referencing that
// here results in a circular dependency.
private final transient Cache<Rule, PatchTransition> ruleTransitionCache =
private final transient Cache<RuleTransitionData, PatchTransition> ruleTransitionCache =
Caffeine.newBuilder().weakKeys().build();

private StarlarkDefinedConfigTransition(
Expand Down Expand Up @@ -216,7 +217,8 @@ public final Location getLocation() {
/**
* Returns a cache that can be used to ensure that this {@link StarlarkDefinedConfigTransition}
* results in at most one {@link
* com.google.devtools.build.lib.analysis.starlark.StarlarkTransition} instance per {@link Rule}.
* com.google.devtools.build.lib.analysis.starlark.StarlarkTransition} instance per {@link
* RuleTransitionData}.
*
* <p>The cache uses {@link Caffeine#weakKeys} to permit collection of transition objects when the
* corresponding {@link Rule} is collectable. As a consequence, it uses identity comparison for
Expand All @@ -231,7 +233,7 @@ public final Location getLocation() {
* practice to have few or even one transition invoke multiple times over multiple configured
* targets.
*/
public final Cache<Rule, PatchTransition> getRuleTransitionCache() {
public final Cache<RuleTransitionData, PatchTransition> getRuleTransitionCache() {
return ruleTransitionCache;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ java_library(
name = "producers",
srcs = glob(["*.java"]),
deps = [
"//third_party:guava",
"//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:aspect_collection",
Expand All @@ -23,10 +24,13 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_conditions",
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:config/configuration_transition_event",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",
"//src/main/java/com/google/devtools/build/lib/analysis:config/starlark_transition_cache",
"//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/composing_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_collector",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
Expand All @@ -52,10 +56,12 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
"//src/main/java/com/google/devtools/build/lib/causes",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper",
"//src/main/java/com/google/devtools/build/lib/packages:exec_group",
#"//src/main/java/com/google/devtools/build/lib/rules/config",
"//src/main/java/com/google/devtools/build/lib/skyframe:aspect_creation_exception",
"//src/main/java/com/google/devtools/build/lib/skyframe:aspect_key_creator",
"//src/main/java/com/google/devtools/build/lib/skyframe:build_configuration",
Expand All @@ -78,7 +84,6 @@ java_library(
"//src/main/java/net/starlark/java/syntax",
"//src/main/protobuf:failure_details_java_proto",
"//third_party:auto_value",
"//third_party:guava",
"//third_party:jsr305",
],
)
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.InconsistentNullConfigException;
import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
import com.google.devtools.build.lib.analysis.TransitiveDependencyState;
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
Expand All @@ -28,6 +27,7 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.skyframe.ConfiguredValueCreationException;
Expand All @@ -47,7 +47,9 @@ interface ResultSink {
}

// -------------------- Input --------------------
private final TargetAndConfiguration targetAndConfiguration;
private final Label targetLabel;
private final Target target;
private final BuildConfigurationKey buildConfigurationKey;
@Nullable private final PlatformInfo targetPlatformInfo;
private final TransitiveDependencyState transitiveState;

Expand All @@ -66,18 +68,22 @@ interface ResultSink {
private DetailedExitCode mostImportantExitCode;

ConfigConditionsProducer(
TargetAndConfiguration targetAndConfiguration,
Target target,
Label targetLabel,
BuildConfigurationKey buildConfigurationKey,
@Nullable PlatformInfo targetPlatformInfo,
TransitiveDependencyState transitiveState,
ResultSink sink,
StateMachine runAfter) {
this.targetAndConfiguration = targetAndConfiguration;
this.targetLabel = targetLabel;
this.target = target;
this.buildConfigurationKey = buildConfigurationKey;
this.targetPlatformInfo = targetPlatformInfo;
this.transitiveState = transitiveState;
this.sink = sink;
this.runAfter = runAfter;

this.configLabels = computeConfigLabels(targetAndConfiguration.getTarget());
this.configLabels = computeConfigLabels(target);
this.prerequisites =
configLabels == null ? null : new ConfiguredTargetAndData[configLabels.size()];
}
Expand All @@ -98,7 +104,7 @@ public StateMachine step(Tasks tasks) {
new ConfiguredTargetAndDataProducer(
ConfiguredTargetKey.builder()
.setLabel(configLabels.get(i))
.setConfiguration(targetAndConfiguration.getConfiguration())
.setConfigurationKey(buildConfigurationKey)
.build(),
/* transitionKeys= */ ImmutableList.of(),
transitiveState,
Expand Down Expand Up @@ -147,16 +153,14 @@ private StateMachine constructConfigConditions(Tasks tasks) {
asConfigConditions.put(
label, ConfigConditions.fromConfiguredTarget(prerequisite, targetPlatformInfo));
} catch (ConfigConditions.InvalidConditionException e) {
var targetLabel = targetAndConfiguration.getLabel();
String message =
String.format(
"%s is not a valid select() condition for %s.\n",
prerequisite.getTargetLabel(), targetLabel)
+ String.format(
"To inspect the select(), run: bazel query --output=build %s.\n", targetLabel)
+ "For more help, see https://bazel.build/reference/be/functions#select.\n\n";
sink.acceptConfigConditionsError(
new ConfiguredValueCreationException(targetAndConfiguration, message));
sink.acceptConfigConditionsError(new ConfiguredValueCreationException(target, message));
return runAfter;
}
}
Expand Down Expand Up @@ -200,9 +204,7 @@ private void emitErrorIfMostImportant(@Nullable DetailedExitCode newExitCode) {
// The precise error is reported by the dependency that failed to load.
// TODO(gregce): beautify this error: https://github.com/bazelbuild/bazel/issues/11984.
new ConfiguredValueCreationException(
targetAndConfiguration,
"errors encountered resolving select() keys for "
+ targetAndConfiguration.getLabel()));
target, "errors encountered resolving select() keys for " + targetLabel));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.devtools.build.lib.analysis.TransitiveDependencyState;
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
import com.google.devtools.build.lib.skyframe.ConfiguredValueCreationException;
import com.google.devtools.build.lib.skyframe.toolchains.ToolchainException;
import com.google.devtools.build.lib.skyframe.toolchains.UnloadedToolchainContext;
Expand Down Expand Up @@ -56,6 +57,7 @@ public interface ResultSink {
// -------------------- Input --------------------
private final UnloadedToolchainContextsInputs unloadedToolchainContextsInputs;
private final TargetAndConfiguration targetAndConfiguration;
private final BuildConfigurationKey buildConfigurationKey;
private final TransitiveDependencyState transitiveState;

// -------------------- Output --------------------
Expand All @@ -70,9 +72,12 @@ public interface ResultSink {
public DependencyContextProducer(
UnloadedToolchainContextsInputs unloadedToolchainContextsInputs,
TargetAndConfiguration targetAndConfiguration,
BuildConfigurationKey buildConfigurationKey,
TransitiveDependencyState transitiveState,
ResultSink sink) {
this.unloadedToolchainContextsInputs = unloadedToolchainContextsInputs;
this.buildConfigurationKey = buildConfigurationKey;
this.unloadedToolchainContexts = null;
this.targetAndConfiguration = targetAndConfiguration;
this.transitiveState = transitiveState;
this.sink = sink;
Expand Down Expand Up @@ -104,7 +109,9 @@ private StateMachine computeConfigConditions(Tasks tasks) {
}

return new ConfigConditionsProducer(
targetAndConfiguration,
targetAndConfiguration.getTarget(),
targetAndConfiguration.getTarget().getLabel(),
buildConfigurationKey,
unloadedToolchainContexts == null ? null : unloadedToolchainContexts.getTargetPlatform(),
transitiveState,
(ConfigConditionsProducer.ResultSink) this,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ public StateMachine step(Tasks tasks) {
// If `defaultToolchainContextKey` is null, there's no platform info, incompatibility check
// or toolchain resolution. Short-circuits and computes only the ConfigConditions.
return new ConfigConditionsProducer(
targetAndConfiguration,
targetAndConfiguration.getTarget(),
targetAndConfiguration.getTarget().getLabel(),
configuredTargetKey.getConfigurationKey(),
/* targetPlatformInfo= */ null,
transitiveState,
(ConfigConditionsProducer.ResultSink) this,
Expand Down Expand Up @@ -119,7 +121,9 @@ private StateMachine computeConfigConditions(Tasks tasks) {
}

return new ConfigConditionsProducer(
targetAndConfiguration,
targetAndConfiguration.getTarget(),
targetAndConfiguration.getTarget().getLabel(),
configuredTargetKey.getConfigurationKey(),
targetPlatformInfo,
transitiveState,
(ConfigConditionsProducer.ResultSink) this,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -376,9 +376,10 @@ private StateMachine postEvent(Tasks tasks) {
DependencyError.of(
new DependencyEvaluationException(
new ConfiguredValueCreationException(
parameters.target(),
parameters.location(),
message,
toLabel,
parameters.label(),
parameters.eventId(),
/* rootCauses= */ null,
/* detailedExitCode= */ null),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ public void acceptConfiguredAspectError(DuplicateException error) {
sink.acceptPrerequisitesAspectError(
new DependencyEvaluationException(
new ConfiguredValueCreationException(
parameters.target(),
parameters.location(),
error.getMessage(),
parameters.label(),
Expand Down
Loading

0 comments on commit ceddfb1

Please sign in to comment.