Add safety checks for toolchain resolution in preparation for trimming.
Trimming has to make assumptions about what configuration is used in the
process of calculating toolchains. These checks enforce those assumptions.
Progress on #6524.
PiperOrigin-RevId: 244258819
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ToolchainResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/ToolchainResolver.java
index ead2d52..04e548a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ToolchainResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ToolchainResolver.java
@@ -66,6 +66,7 @@
// Optional data.
private ImmutableSet<Label> requiredToolchainTypeLabels = ImmutableSet.of();
private ImmutableSet<Label> execConstraintLabels = ImmutableSet.of();
+ private boolean shouldSanityCheckConfiguration = false;
// Determined during execution.
private boolean debug = false;
@@ -101,6 +102,17 @@
}
/**
+ * Sets whether the experimental retroactive trimming mode is in use. This determines whether
+ * sanity checks regarding the fragments in use for the configurations of platforms and toolchains
+ * are used - specifically, whether platforms use only the PlatformConfiguration, and toolchains
+ * do not use any configuration at all.
+ */
+ public ToolchainResolver setShouldSanityCheckConfiguration(boolean useSanityChecks) {
+ this.shouldSanityCheckConfiguration = useSanityChecks;
+ return this;
+ }
+
+ /**
* Determines the specific toolchains that are required, given the requested toolchain types,
* target platform, and configuration.
*
@@ -187,7 +199,9 @@
// Load the host and target platforms early, to check for errors.
PlatformLookupUtil.getPlatformInfo(
- ImmutableList.of(hostPlatformKey, targetPlatformKey), environment);
+ ImmutableList.of(hostPlatformKey, targetPlatformKey),
+ environment,
+ shouldSanityCheckConfiguration);
if (environment.valuesMissing()) {
throw new ValueMissingException();
}
@@ -245,7 +259,8 @@
// platform is the host platform), Skyframe will return the correct results immediately without
// need of a restart.
Map<ConfiguredTargetKey, PlatformInfo> platformInfoMap =
- PlatformLookupUtil.getPlatformInfo(platformKeys, environment);
+ PlatformLookupUtil.getPlatformInfo(
+ platformKeys, environment, shouldSanityCheckConfiguration);
if (platformInfoMap == null) {
throw new ValueMissingException();
}
@@ -367,7 +382,8 @@
Map<ConfiguredTargetKey, PlatformInfo> platforms =
PlatformLookupUtil.getPlatformInfo(
ImmutableList.of(selectedExecutionPlatformKey.get(), platformKeys.targetPlatformKey()),
- environment);
+ environment,
+ shouldSanityCheckConfiguration);
if (platforms == null) {
throw new ValueMissingException();
}
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 854ffe3..5c6481b 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
@@ -313,6 +313,8 @@
new ToolchainResolver(env, configuredTargetKey.getConfigurationKey())
.setRequiredToolchainTypes(requiredToolchains)
.setExecConstraintLabels(execConstraintLabels)
+ .setShouldSanityCheckConfiguration(
+ configuration.trimConfigurationsRetroactively())
.resolve();
if (env.valuesMissing()) {
return null;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
index d9f998b..4c1c306 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
@@ -14,8 +14,12 @@
package com.google.devtools.build.lib.skyframe;
+import static com.google.common.base.Predicates.equalTo;
+import static com.google.common.base.Predicates.not;
+
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.PlatformConfiguration;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils;
import com.google.devtools.build.lib.cmdline.Label;
@@ -33,7 +37,7 @@
@Nullable
public static Map<ConfiguredTargetKey, PlatformInfo> getPlatformInfo(
- Iterable<ConfiguredTargetKey> platformKeys, Environment env)
+ Iterable<ConfiguredTargetKey> platformKeys, Environment env, boolean sanityCheckConfiguration)
throws InterruptedException, InvalidPlatformException {
Map<
@@ -49,7 +53,7 @@
boolean valuesMissing = env.valuesMissing();
Map<ConfiguredTargetKey, PlatformInfo> platforms = valuesMissing ? null : new HashMap<>();
for (ConfiguredTargetKey key : platformKeys) {
- PlatformInfo platformInfo = findPlatformInfo(key, values.get(key));
+ PlatformInfo platformInfo = findPlatformInfo(key, values.get(key), sanityCheckConfiguration);
if (!valuesMissing && platformInfo != null) {
platforms.put(key, platformInfo);
}
@@ -72,7 +76,8 @@
ConfiguredTargetKey key,
ValueOrException3<
ConfiguredValueCreationException, NoSuchThingException, ActionConflictException>
- valueOrException)
+ valueOrException,
+ boolean sanityCheckConfiguration)
throws InvalidPlatformException {
try {
@@ -82,6 +87,21 @@
}
ConfiguredTarget configuredTarget = ctv.getConfiguredTarget();
+ BuildConfigurationValue.Key configurationKey = configuredTarget.getConfigurationKey();
+ // This check is necessary because trimming for other rules assumes that platform resolution
+ // uses the platform fragment and _only_ the platform fragment. Without this check, it's
+ // possible another fragment could slip in without us realizing, and thus break this
+ // assumption.
+ if (sanityCheckConfiguration
+ && configurationKey.getFragments().stream()
+ .anyMatch(not(equalTo(PlatformConfiguration.class)))) {
+ // Only the PlatformConfiguration fragment may be present on a platform rule in retroactive
+ // trimming mode.
+ throw new InvalidPlatformException(
+ configuredTarget.getLabel(),
+ "has fragments other than PlatformConfiguration, "
+ + "which is forbidden in retroactive trimming mode");
+ }
PlatformInfo platformInfo = PlatformProviderUtils.platform(configuredTarget);
if (platformInfo == null) {
throw new InvalidPlatformException(configuredTarget.getLabel());
@@ -99,12 +119,14 @@
/** Exception used when a platform label is not a valid platform. */
public static final class InvalidPlatformException extends ToolchainException {
+ private static final String DEFAULT_ERROR = "does not provide PlatformInfo";
+
InvalidPlatformException(Label label) {
- super(formatError(label));
+ super(formatError(label, DEFAULT_ERROR));
}
InvalidPlatformException(Label label, ConfiguredValueCreationException e) {
- super(formatError(label), e);
+ super(formatError(label, DEFAULT_ERROR), e);
}
public InvalidPlatformException(Label label, NoSuchThingException e) {
@@ -113,12 +135,15 @@
}
public InvalidPlatformException(Label label, ActionConflictException e) {
- super(formatError(label), e);
+ super(formatError(label, DEFAULT_ERROR), e);
}
- private static String formatError(Label label) {
- return String.format(
- "Target %s was referenced as a platform, but does not provide PlatformInfo", label);
+ InvalidPlatformException(Label label, String error) {
+ super(formatError(label, error));
+ }
+
+ private static String formatError(Label label, String error) {
+ return String.format("Target %s was referenced as a platform, but %s", label, error);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java
index 9348cec..255fe0e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java
@@ -14,6 +14,8 @@
package com.google.devtools.build.lib.skyframe;
+import static com.google.common.base.Predicates.equalTo;
+import static com.google.common.base.Predicates.not;
import static com.google.common.collect.ImmutableList.toImmutableList;
import com.google.auto.value.AutoValue;
@@ -92,7 +94,8 @@
// Load the configured target for each, and get the declared execution platforms providers.
ImmutableList<ConfiguredTargetKey> registeredExecutionPlatformKeys =
- configureRegisteredExecutionPlatforms(env, configuration, platformLabels);
+ configureRegisteredExecutionPlatforms(
+ env, configuration, configuration.trimConfigurationsRetroactively(), platformLabels);
if (env.valuesMissing()) {
return null;
}
@@ -122,6 +125,7 @@
private ImmutableList<ConfiguredTargetKey> configureRegisteredExecutionPlatforms(
Environment env,
BuildConfiguration configuration,
+ boolean sanityCheckConfiguration,
List<Label> labels)
throws InterruptedException, RegisteredExecutionPlatformsFunctionException {
@@ -145,6 +149,22 @@
}
ConfiguredTarget target =
((ConfiguredTargetValue) valueOrException.get()).getConfiguredTarget();
+ // This check is necessary because trimming for other rules assumes that platform resolution
+ // uses the platform fragment and _only_ the platform fragment. Without this check, it's
+ // possible another fragment could slip in without us realizing, and thus break this
+ // assumption.
+ if (sanityCheckConfiguration
+ && target.getConfigurationKey().getFragments().stream()
+ .anyMatch(not(equalTo(PlatformConfiguration.class)))) {
+ // Only the PlatformConfiguration fragment may be present on a platform rule in
+ // retroactive trimming mode.
+ throw new RegisteredExecutionPlatformsFunctionException(
+ new InvalidPlatformException(
+ target.getLabel(),
+ "has fragments other than PlatformConfiguration, "
+ + "which is forbidden in retroactive trimming mode"),
+ Transience.PERSISTENT);
+ }
PlatformInfo platformInfo = PlatformProviderUtils.platform(target);
if (platformInfo == null) {
throw new RegisteredExecutionPlatformsFunctionException(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java
index 69ac1b7..7d7b054 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java
@@ -145,8 +145,23 @@
valuesMissing = true;
continue;
}
+
ConfiguredTarget target =
((ConfiguredTargetValue) valueOrException.get()).getConfiguredTarget();
+ if (configuration.trimConfigurationsRetroactively()
+ && !target.getConfigurationKey().getFragments().isEmpty()) {
+ // No fragment may be present on a toolchain rule in retroactive trimming mode.
+ // This is because trimming expects that platform and toolchain resolution uses only
+ // the platform configuration. In theory, this means toolchains could use platforms, but
+ // the current expectation is that toolchains should not use anything at all, so better
+ // to go with the stricter expectation for now.
+ throw new RegisteredToolchainsFunctionException(
+ new InvalidToolchainLabelException(
+ toolchainLabel,
+ "this toolchain uses configuration, which is forbidden in retroactive trimming "
+ + "mode"),
+ Transience.PERSISTENT);
+ }
DeclaredToolchainInfo toolchainInfo = target.getProvider(DeclaredToolchainInfo.class);
if (toolchainInfo == null) {
@@ -185,6 +200,10 @@
"target does not provide the DeclaredToolchainInfo provider"));
}
+ public InvalidToolchainLabelException(Label invalidLabel, String reason) {
+ super(formatMessage(invalidLabel.getCanonicalForm(), reason));
+ }
+
public InvalidToolchainLabelException(TargetPatternUtil.InvalidTargetPatternException e) {
this(e.getInvalidPattern(), e.getTpe());
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java
index 2313d70..1e37fe4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java
@@ -82,6 +82,7 @@
key.toolchainTypeLabel(),
key.availableExecutionPlatformKeys(),
key.targetPlatformKey(),
+ configuration.trimConfigurationsRetroactively(),
toolchains.registeredToolchains(),
env,
debug ? env.getListener() : null);
@@ -97,6 +98,7 @@
Label toolchainTypeLabel,
List<ConfiguredTargetKey> availableExecutionPlatformKeys,
ConfiguredTargetKey targetPlatformKey,
+ boolean sanityCheckConfigurations,
ImmutableList<DeclaredToolchainInfo> toolchains,
Environment env,
@Nullable EventHandler eventHandler)
@@ -112,7 +114,8 @@
.add(targetPlatformKey)
.addAll(availableExecutionPlatformKeys)
.build(),
- env);
+ env,
+ sanityCheckConfigurations);
if (env.valuesMissing()) {
return null;
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java
index cf3c590..a851e8b 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java
@@ -175,7 +175,7 @@
GetPlatformInfoKey key = (GetPlatformInfoKey) skyKey;
try {
Map<ConfiguredTargetKey, PlatformInfo> platforms =
- PlatformLookupUtil.getPlatformInfo(key.platformKeys(), env);
+ PlatformLookupUtil.getPlatformInfo(key.platformKeys(), env, false);
if (env.valuesMissing()) {
return null;
}
