In CTF.K narrow ToolchainContextKey to execution platform Label. Update docs.
ConfigurationTargetFunction is very sensitive and should only be completed once per-build for a given Label, BuildConfiguration pair due to performance and correctness (considering unsharable action conflicts in cc rules).
Update the documentation of ConfiguredTargetKey to better explain this sensitivity and outline potential pain points.
In theory, using the narrower execution platform Label instead of a ToolchainContextKey reduces the chance that two different ConfigurationTargetKey map to the same (Label, BuildConfiguration) leading to the previously mentioned build issues. Thus, concern 3 in the ConfiguredTargetKey is even less likely. In practice, we do not think this issue has actually been observed in a real build.
PiperOrigin-RevId: 404639477
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
index cbaaa9a..b096dcb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -415,7 +415,6 @@
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_value_creation_exception",
"//src/main/java/com/google/devtools/build/lib/skyframe:package_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value",
- "//src/main/java/com/google/devtools/build/lib/skyframe:toolchain_context_key",
"//src/main/java/com/google/devtools/build/lib/skyframe:workspace_status_value",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
@@ -710,7 +709,6 @@
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
- "//src/main/java/com/google/devtools/build/lib/skyframe:toolchain_context_key",
"//third_party:auto_value",
"//third_party:guava",
"//third_party:jsr305",
@@ -725,7 +723,6 @@
":config/transitions/configuration_transition",
":dependency",
"//src/main/java/com/google/devtools/build/lib/cmdline",
- "//src/main/java/com/google/devtools/build/lib/skyframe:toolchain_context_key",
"//third_party:auto_value",
"//third_party:jsr305",
],
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Dependency.java b/src/main/java/com/google/devtools/build/lib/analysis/Dependency.java
index 936c47f..f958103 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/Dependency.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/Dependency.java
@@ -21,7 +21,6 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.AspectDescriptor;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
-import com.google.devtools.build.lib.skyframe.ToolchainContextKey;
import javax.annotation.Nullable;
/**
@@ -70,11 +69,10 @@
public abstract Builder setTransitionKeys(ImmutableList<String> keys);
/**
- * Sets the {@link ToolchainContextKey} that this dependency should use for toolchain
- * resolution.
+ * Sets the execution platform {@link Label} that this dependency should use as an override for
+ * toolchain resolution.
*/
- public abstract Builder setToolchainContextKey(
- @Nullable ToolchainContextKey toolchainContextKey);
+ public abstract Builder setExecutionPlatformLabel(@Nullable Label executionPlatformLabel);
// Not public.
abstract Dependency autoBuild();
@@ -150,18 +148,18 @@
public abstract ImmutableList<String> getTransitionKeys();
/**
- * Returns the {@link ToolchainContextKey} that this dependency should use for toolchain
- * resolution.
+ * Returns the execution platform {@link Label} that this dependency should use as an override for
+ * toolchain resolution.
*/
@Nullable
- public abstract ToolchainContextKey getToolchainContextKey();
+ public abstract Label getExecutionPlatformLabel();
/** Returns the ConfiguredTargetKey needed to fetch this dependency. */
public ConfiguredTargetKey getConfiguredTargetKey() {
ConfiguredTargetKey.Builder configuredTargetKeyBuilder =
ConfiguredTargetKey.builder().setLabel(getLabel()).setConfiguration(getConfiguration());
- if (getToolchainContextKey() != null) {
- configuredTargetKeyBuilder.setToolchainContextKey(getToolchainContextKey());
+ if (getExecutionPlatformLabel() != null) {
+ configuredTargetKeyBuilder.setExecutionPlatformLabel(getExecutionPlatformLabel());
}
return configuredTargetKeyBuilder.build();
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyKey.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyKey.java
index 7750772..5bf31b9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyKey.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyKey.java
@@ -16,7 +16,6 @@
import com.google.auto.value.AutoValue;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.skyframe.ToolchainContextKey;
import javax.annotation.Nullable;
/**
@@ -39,11 +38,11 @@
Builder setAspects(AspectCollection aspectCollection);
/**
- * Sets the {@link ToolchainContextKey} that this dependency should use for toolchain
- * resolution.
+ * Sets the execution platform {@link Label} that this dependency should use as an override for
+ * toolchain resolution.
*/
@Nullable
- Builder setToolchainContextKey(ToolchainContextKey toolchainContextKey);
+ Builder setExecutionPlatformLabel(Label executionPlatformLabel);
/** Returns the new instance. */
DependencyKey build();
@@ -64,15 +63,15 @@
public abstract AspectCollection getAspects();
/**
- * Returns the {@link ToolchainContextKey} that this dependency should use for toolchain
- * resolution.
+ * Returns the execution platform {@link Label} that this dependency should use as an override for
+ * toolchain resolution.
*/
@Nullable
- public abstract ToolchainContextKey getToolchainContextKey();
+ public abstract Label getExecutionPlatformLabel();
public Dependency.Builder getDependencyBuilder() {
return Dependency.builder()
.setLabel(getLabel())
- .setToolchainContextKey(getToolchainContextKey());
+ .setExecutionPlatformLabel(getExecutionPlatformLabel());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index e60897a..45554ad 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -58,7 +58,6 @@
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;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
import java.util.ArrayList;
import java.util.Collection;
@@ -124,7 +123,7 @@
abstract ImmutableList<Aspect> getPropagatingAspects();
@Nullable
- abstract ToolchainContextKey getToolchainContextKey();
+ abstract Label getExecutionPlatformLabel();
/** A Builder to create instances of PartiallyResolvedDependency. */
@AutoValue.Builder
@@ -135,7 +134,7 @@
abstract Builder setPropagatingAspects(List<Aspect> propagatingAspects);
- abstract Builder setToolchainContextKey(@Nullable ToolchainContextKey toolchainContextKey);
+ abstract Builder setExecutionPlatformLabel(@Nullable Label executionPlatformLabel);
abstract PartiallyResolvedDependency build();
}
@@ -148,7 +147,7 @@
public DependencyKey.Builder getDependencyKeyBuilder() {
return DependencyKey.builder()
.setLabel(getLabel())
- .setToolchainContextKey(getToolchainContextKey());
+ .setExecutionPlatformLabel(getExecutionPlatformLabel());
}
}
@@ -346,7 +345,7 @@
PartiallyResolvedDependency.builder()
.setLabel(toLabel)
.setTransition(NoTransition.INSTANCE)
- .setToolchainContextKey(toolchainContext.key())
+ .setExecutionPlatformLabel(toolchainContext.executionPlatform().label())
.build());
} else {
// Legacy approach: use a HostTransition.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Util.java b/src/main/java/com/google/devtools/build/lib/analysis/Util.java
index 5f9c8e5..b2dda41 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/Util.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/Util.java
@@ -109,7 +109,7 @@
ConfiguredTargetKey.builder()
.setLabel(toolchain)
.setConfiguration(targetConfiguration)
- .setToolchainContextKey(toolchainContext.key())
+ .setExecutionPlatformLabel(toolchainContext.executionPlatform().label())
.build());
} else {
maybeImplicitDeps.add(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index b3f319a..7bdd7a7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -275,7 +275,7 @@
// Determine what toolchains are needed by this target.
ComputedToolchainContexts result =
computeUnloadedToolchainContexts(
- env, ruleClassProvider, ctgValue, configuredTargetKey.getToolchainContextKey());
+ env, ruleClassProvider, ctgValue, configuredTargetKey.getExecutionPlatformLabel());
if (env.valuesMissing()) {
return null;
}
@@ -464,7 +464,7 @@
Environment env,
RuleClassProvider ruleClassProvider,
TargetAndConfiguration targetAndConfig,
- @Nullable ToolchainContextKey parentToolchainContextKey)
+ @Nullable Label parentExecutionPlatformLabel)
throws InterruptedException, ToolchainException {
if (!(targetAndConfig.getTarget() instanceof Rule)) {
return new ComputedToolchainContexts();
@@ -542,20 +542,10 @@
.execConstraintLabels(defaultExecConstraintLabels)
.debugTarget(debugTarget);
- if (parentToolchainContextKey != null) {
+ if (parentExecutionPlatformLabel != null) {
// Find out what execution platform the parent used, and force that.
- // This key should always be present, but check just in case.
- ToolchainContext parentToolchainContext =
- (ToolchainContext)
- env.getValueOrThrow(parentToolchainContextKey, ToolchainException.class);
- if (env.valuesMissing()) {
- return null;
- }
-
- Label execPlatform = parentToolchainContext.executionPlatform().label();
- if (execPlatform != null) {
- toolchainContextKeyBuilder.forceExecutionPlatform(execPlatform);
- }
+ // This should only be set for direct toolchain dependencies.
+ toolchainContextKeyBuilder.forceExecutionPlatform(parentExecutionPlatformLabel);
}
ToolchainContextKey toolchainContextKey = toolchainContextKeyBuilder.build();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
index c137ae0..0457e20 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
@@ -29,8 +29,36 @@
import javax.annotation.Nullable;
/**
- * A (Label, Configuration key) pair. Note that this pair may be used to look up the generating
- * action of an artifact.
+ * In simple form, a ({@link Label}, {@link BuildConfiguration}) pair used to trigger immediate
+ * dependency resolution and the rule analysis.
+ *
+ * <p>In practice, a ({@link Label}, canonical and post-transition {@link
+ * BuildConfigurationValue.Key}) pair plus a possible execution platform override {@link Label} with
+ * special constraints. To elaborate, in order of highest to lowest potential for concern:
+ *
+ * <p>1. The {@link BuildConfigurationValue.Key} must be post-transition and thus ready for
+ * immediate use in dependency resolution and analysis. In practice, this means that if the rule has
+ * an incoming-edge transition (cfg in {@link RuleClass}) or there are global trimming transitions,
+ * THOSE TRANSITIONS MUST ALREADY BE DONE before creating the key. Failure to do so will lead to
+ * build graphs with ConfiguredTarget that have seemingly impossible {@link BuildConfiguration} (due
+ * to the skipped transitions).
+ *
+ * <p>2. The {@link BuildConfigurationValue.Key} must be canonical. Multiple keys can correspond to
+ * the same {@link BuildConfiguration}. The canonical key is the one returned by {@link
+ * BuildConfigurationValue.key}. Failure to use a canonical key can result in build graphs with
+ * multiple seemingly-identical ConfiguredTarget that have the same ({@link Label}, {@link
+ * BuildConfiguration}) pair. This is non-performant in all cases and incorrect if those
+ * duplications lead to action conflicts due to unsharable actions.
+ *
+ * <p>3. A build should not request keys with equal ({@link Label}, {@link BuildConfiguration})
+ * pairs but different execution platform override {@link Label} if the invoked rule will register
+ * actions. (This is potentially OK if all outputs of all registered actions incorporate the
+ * execution platform in their name unless the build also requests keys without an override that
+ * happen to resolve to the same execution platform.) In practice, this issue has not been seen in
+ * any 'real' builds; however, pathologically failure could lead to multiple (potentially different)
+ * ConfiguredTarget that have the same ({@link Label}, {@link BuildConfiguration}) pair.
+ *
+ * <p>Note that this key may be used to look up the generating action of an artifact.
*/
@AutoCodec
public class ConfiguredTargetKey implements ActionLookupKey {
@@ -56,7 +84,7 @@
@AutoCodec.Instantiator
static ConfiguredTargetKey create(
Label label, @Nullable BuildConfigurationValue.Key configurationKey) {
- int hashCode = computeHashCode(label, configurationKey, /*toolchainContextKey=*/ null);
+ int hashCode = computeHashCode(label, configurationKey, /*executionPlatformLabel=*/ null);
return interner.intern(new ConfiguredTargetKey(label, configurationKey, hashCode));
}
@@ -64,7 +92,7 @@
return builder()
.setConfigurationKey(configurationKey)
.setLabel(label)
- .setToolchainContextKey(getToolchainContextKey());
+ .setExecutionPlatformLabel(getExecutionPlatformLabel());
}
@Override
@@ -83,7 +111,7 @@
}
@Nullable
- public ToolchainContextKey getToolchainContextKey() {
+ public Label getExecutionPlatformLabel() {
return null;
}
@@ -95,10 +123,11 @@
private static int computeHashCode(
Label label,
@Nullable BuildConfigurationValue.Key configurationKey,
- @Nullable ToolchainContextKey toolchainContextKey) {
+ @Nullable Label executionPlatformLabel) {
int configVal = configurationKey == null ? 79 : configurationKey.hashCode();
- int toolchainContextVal = toolchainContextKey == null ? 47 : toolchainContextKey.hashCode();
- return 31 * label.hashCode() + configVal + toolchainContextVal;
+ int executionPlatformLabelVal =
+ executionPlatformLabel == null ? 47 : executionPlatformLabel.hashCode();
+ return 31 * label.hashCode() + configVal + executionPlatformLabelVal;
}
@Override
@@ -113,7 +142,7 @@
return hashCode == other.hashCode
&& label.equals(other.label)
&& Objects.equals(configurationKey, other.configurationKey)
- && Objects.equals(getToolchainContextKey(), other.getToolchainContextKey());
+ && Objects.equals(getExecutionPlatformLabel(), other.getExecutionPlatformLabel());
}
public final String prettyPrint() {
@@ -130,44 +159,44 @@
// TODO(b/162809183): consider reverting to less verbose toString when bug is resolved.
MoreObjects.ToStringHelper helper =
MoreObjects.toStringHelper(this).add("label", label).add("config", configurationKey);
- if (getToolchainContextKey() != null) {
- helper.add("toolchainContextKey", getToolchainContextKey());
+ if (getExecutionPlatformLabel() != null) {
+ helper.add("executionPlatformLabel", getExecutionPlatformLabel());
}
return helper.toString();
}
@AutoCodec.VisibleForSerialization
@AutoCodec
- static class ConfiguredTargetKeyWithToolchainContext extends ConfiguredTargetKey {
- private static final Interner<ConfiguredTargetKeyWithToolchainContext>
- withToolchainContextInterner = BlazeInterners.newWeakInterner();
+ static class ToolchainDependencyConfiguredTargetKey extends ConfiguredTargetKey {
+ private static final Interner<ToolchainDependencyConfiguredTargetKey>
+ toolchainDependencyConfiguredTargetKeyInterner = BlazeInterners.newWeakInterner();
- private final ToolchainContextKey toolchainContextKey;
+ private final Label executionPlatformLabel;
- private ConfiguredTargetKeyWithToolchainContext(
+ private ToolchainDependencyConfiguredTargetKey(
Label label,
@Nullable BuildConfigurationValue.Key configurationKey,
int hashCode,
- ToolchainContextKey toolchainContextKey) {
+ Label executionPlatformLabel) {
super(label, configurationKey, hashCode);
- this.toolchainContextKey = checkNotNull(toolchainContextKey);
+ this.executionPlatformLabel = checkNotNull(executionPlatformLabel);
}
@AutoCodec.VisibleForSerialization
@AutoCodec.Instantiator
- static ConfiguredTargetKeyWithToolchainContext create(
+ static ToolchainDependencyConfiguredTargetKey create(
Label label,
@Nullable BuildConfigurationValue.Key configurationKey,
- ToolchainContextKey toolchainContextKey) {
- int hashCode = computeHashCode(label, configurationKey, toolchainContextKey);
- return withToolchainContextInterner.intern(
- new ConfiguredTargetKeyWithToolchainContext(
- label, configurationKey, hashCode, toolchainContextKey));
+ Label executionPlatformLabel) {
+ int hashCode = computeHashCode(label, configurationKey, executionPlatformLabel);
+ return toolchainDependencyConfiguredTargetKeyInterner.intern(
+ new ToolchainDependencyConfiguredTargetKey(
+ label, configurationKey, hashCode, executionPlatformLabel));
}
@Override
- public final ToolchainContextKey getToolchainContextKey() {
- return toolchainContextKey;
+ public final Label getExecutionPlatformLabel() {
+ return executionPlatformLabel;
}
}
@@ -180,7 +209,7 @@
public static final class Builder {
private Label label = null;
private BuildConfigurationValue.Key configurationKey = null;
- private ToolchainContextKey toolchainContextKey = null;
+ private Label executionPlatformLabel = null;
private Builder() {}
@@ -219,19 +248,19 @@
}
/**
- * Sets the {@link ToolchainContextKey} this configured target should use for toolchain
- * resolution. When present, this overrides the normally determined toolchain context.
+ * Sets the execution platform {@link Label} this configured target should use for toolchain
+ * resolution. When present, this overrides the normally determined execution platform.
*/
- public Builder setToolchainContextKey(@Nullable ToolchainContextKey toolchainContextKey) {
- this.toolchainContextKey = toolchainContextKey;
+ public Builder setExecutionPlatformLabel(@Nullable Label executionPlatformLabel) {
+ this.executionPlatformLabel = executionPlatformLabel;
return this;
}
/** Builds a new {@link ConfiguredTargetKey} based on the supplied data. */
public ConfiguredTargetKey build() {
- if (this.toolchainContextKey != null) {
- return ConfiguredTargetKeyWithToolchainContext.create(
- label, configurationKey, toolchainContextKey);
+ if (this.executionPlatformLabel != null) {
+ return ToolchainDependencyConfiguredTargetKey.create(
+ label, configurationKey, executionPlatformLabel);
}
return create(label, configurationKey);
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java
index 4a301a4..76c6a9a 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java
@@ -115,7 +115,7 @@
env,
stateProvider.lateBoundRuleClassProvider(),
key.targetAndConfiguration(),
- key.configuredTargetKey().getToolchainContextKey());
+ key.configuredTargetKey().getExecutionPlatformLabel());
if (env.valuesMissing()) {
return null;
}
@@ -457,19 +457,14 @@
"--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b");
ConfiguredTarget target = Iterables.getOnlyElement(update("//a").getTargetsToBuild());
- ToolchainContextKey parentKey =
- ToolchainContextKey.key()
- .configurationKey(target.getConfigurationKey())
- // Force the constraint label, to make the exec platform be local_platform_b.
- .execConstraintLabels(Label.parseAbsoluteUnchecked("//platforms:local_value_b"))
- .build();
ToolchainCollection<UnloadedToolchainContext> toolchainCollection =
getToolchainCollection(
target,
ConfiguredTargetKey.builder()
.setLabel(target.getOriginalLabel())
.setConfigurationKey(target.getConfigurationKey())
- .setToolchainContextKey(parentKey)
+ .setExecutionPlatformLabel(
+ Label.parseAbsoluteUnchecked("//platforms:local_platform_b"))
.build());
assertThat(toolchainCollection).isNotNull();
