Skip to content

Commit

Permalink
Add safety checks for toolchain resolution in preparation for trimming.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
mstaib authored and copybara-github committed Apr 18, 2019
1 parent fc1ea45 commit 375f72b
Show file tree
Hide file tree
Showing 7 changed files with 100 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ public class ToolchainResolver {
// 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;
Expand Down Expand Up @@ -100,6 +101,17 @@ public ToolchainResolver setExecConstraintLabels(Set<Label> execConstraintLabels
return this;
}

/**
* 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.
Expand Down Expand Up @@ -187,7 +199,9 @@ private PlatformKeys loadPlatformKeys(

// 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();
}
Expand Down Expand Up @@ -245,7 +259,8 @@ private ImmutableList<ConfiguredTargetKey> filterAvailablePlatforms(
// 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();
}
Expand Down Expand Up @@ -367,7 +382,8 @@ private void determineToolchainImplementations(
Map<ConfiguredTargetKey, PlatformInfo> platforms =
PlatformLookupUtil.getPlatformInfo(
ImmutableList.of(selectedExecutionPlatformKey.get(), platformKeys.targetPlatformKey()),
environment);
environment,
shouldSanityCheckConfiguration);
if (platforms == null) {
throw new ValueMissingException();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,8 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc
new ToolchainResolver(env, configuredTargetKey.getConfigurationKey())
.setRequiredToolchainTypes(requiredToolchains)
.setExecConstraintLabels(execConstraintLabels)
.setShouldSanityCheckConfiguration(
configuration.trimConfigurationsRetroactively())
.resolve();
if (env.valuesMissing()) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -33,7 +37,7 @@ public class PlatformLookupUtil {

@Nullable
public static Map<ConfiguredTargetKey, PlatformInfo> getPlatformInfo(
Iterable<ConfiguredTargetKey> platformKeys, Environment env)
Iterable<ConfiguredTargetKey> platformKeys, Environment env, boolean sanityCheckConfiguration)
throws InterruptedException, InvalidPlatformException {

Map<
Expand All @@ -49,7 +53,7 @@ public static Map<ConfiguredTargetKey, PlatformInfo> getPlatformInfo(
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);
}
Expand All @@ -72,7 +76,8 @@ private static PlatformInfo findPlatformInfo(
ConfiguredTargetKey key,
ValueOrException3<
ConfiguredValueCreationException, NoSuchThingException, ActionConflictException>
valueOrException)
valueOrException,
boolean sanityCheckConfiguration)
throws InvalidPlatformException {

try {
Expand All @@ -82,6 +87,21 @@ private static PlatformInfo findPlatformInfo(
}

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());
Expand All @@ -99,12 +119,14 @@ private static PlatformInfo findPlatformInfo(

/** 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) {
Expand All @@ -113,12 +135,15 @@ public InvalidPlatformException(Label label, NoSuchThingException e) {
}

public InvalidPlatformException(Label label, ActionConflictException e) {
super(formatError(label), e);
super(formatError(label, DEFAULT_ERROR), e);
}

InvalidPlatformException(Label label, String error) {
super(formatError(label, error));
}

private static String formatError(Label label) {
return String.format(
"Target %s was referenced as a platform, but does not provide PlatformInfo", label);
private static String formatError(Label label, String error) {
return String.format("Target %s was referenced as a platform, but %s", label, error);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -92,7 +94,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)

// 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;
}
Expand Down Expand Up @@ -122,6 +125,7 @@ public static List<String> getWorkspaceExecutionPlatforms(Environment env)
private ImmutableList<ConfiguredTargetKey> configureRegisteredExecutionPlatforms(
Environment env,
BuildConfiguration configuration,
boolean sanityCheckConfiguration,
List<Label> labels)
throws InterruptedException, RegisteredExecutionPlatformsFunctionException {

Expand All @@ -145,6 +149,22 @@ private ImmutableList<ConfiguredTargetKey> configureRegisteredExecutionPlatforms
}
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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,23 @@ private ImmutableList<DeclaredToolchainInfo> configureRegisteredToolchains(
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) {
Expand Down Expand Up @@ -185,6 +200,10 @@ public InvalidToolchainLabelException(Label invalidLabel) {
"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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
key.toolchainTypeLabel(),
key.availableExecutionPlatformKeys(),
key.targetPlatformKey(),
configuration.trimConfigurationsRetroactively(),
toolchains.registeredToolchains(),
env,
debug ? env.getListener() : null);
Expand All @@ -97,6 +98,7 @@ private static SingleToolchainResolutionValue resolveConstraints(
Label toolchainTypeLabel,
List<ConfiguredTargetKey> availableExecutionPlatformKeys,
ConfiguredTargetKey targetPlatformKey,
boolean sanityCheckConfigurations,
ImmutableList<DeclaredToolchainInfo> toolchains,
Environment env,
@Nullable EventHandler eventHandler)
Expand All @@ -112,7 +114,8 @@ private static SingleToolchainResolutionValue resolveConstraints(
.add(targetPlatformKey)
.addAll(availableExecutionPlatformKeys)
.build(),
env);
env,
sanityCheckConfigurations);
if (env.valuesMissing()) {
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
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;
}
Expand Down

0 comments on commit 375f72b

Please sign in to comment.