Skip to content

Commit

Permalink
Add basic incompatible target skipping
Browse files Browse the repository at this point in the history
This patch aims to implement a basic version of incompatible target
skipping outlined here:
https://docs.google.com/document/d/12n5QNHmFSkuh5yAbdEex64ot4hRgR-moL1zRimU7wHQ/edit?usp=sharing

The implementation in this patch supports only "top level" target
skipping. In other words we only filter out targets if they are
globbed as part of an invocation like "bazel build //...".

The following features are not yet implemented:
- Filtering targets based on transitive dependencies. For example, if
  //a:b depends on //c:d an invocation of "bazel build //a/..." will
  still build //c:d even if it is incompatible with the current
  platform.
- Being able to select() on constraint values. For example, it's not
  possible to select() based on whether or not GCC is being used.
  Discussion of this aspect of the proposal is ongoing:
  https://groups.google.com/d/msg/bazel-dev/xK7diubpljQ/s3KSRwTWAQAJ

The goal is to implement the missing features in follow-up patches.

Austin Schuh ([email protected]) provided me with the majority of
the logic in TopLevelConstraintSemantics.java. I added the remaining
error reporting logic and the tests.

The patch largely re-uses the existing support for skipping targets
based on incompatible CPU constraints. There are a few enhancements to
the error reporting so that it's a little more obvious on the command
line when a target was skipped. To a lesser extent the Build Event
Stream should also contain less misleading information now. I did have
to introduce the notion of to-be-failed targets determined during the
analysis phase. For now these are targets that are incompatible with
the current platform, but were explicitly requested on the command
line. The list of targets ("targetsToFail" in AnalysisResult) is used
to affect the outcome of the build. I.e. if the list is non-empty,
then the build is considered to have failed.
  • Loading branch information
philsc committed Mar 11, 2020
1 parent f17062d commit 4a6f40d
Show file tree
Hide file tree
Showing 18 changed files with 557 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public final class AnalysisResult {
private final ImmutableSet<ConfiguredTarget> targetsToBuild;
@Nullable private final ImmutableList<ConfiguredTarget> targetsToTest;
private final ImmutableSet<ConfiguredTarget> targetsToSkip;
private final ImmutableSet<ConfiguredTarget> targetsToFail;
@Nullable private final String error;
private final ActionGraph actionGraph;
private final ArtifactsToOwnerLabels topLevelArtifactsToOwnerLabels;
Expand All @@ -51,6 +52,7 @@ public final class AnalysisResult {
ImmutableSet<AspectValue> aspects,
@Nullable ImmutableList<ConfiguredTarget> targetsToTest,
ImmutableSet<ConfiguredTarget> targetsToSkip,
ImmutableSet<ConfiguredTarget> targetsToFail,
@Nullable String error,
ActionGraph actionGraph,
ArtifactsToOwnerLabels topLevelArtifactsToOwnerLabels,
Expand All @@ -66,6 +68,7 @@ public final class AnalysisResult {
this.aspects = aspects;
this.targetsToTest = targetsToTest;
this.targetsToSkip = targetsToSkip;
this.targetsToFail = targetsToFail;
this.error = error;
this.actionGraph = actionGraph;
this.topLevelArtifactsToOwnerLabels = topLevelArtifactsToOwnerLabels;
Expand Down Expand Up @@ -123,6 +126,14 @@ public ImmutableSet<ConfiguredTarget> getTargetsToSkip() {
return targetsToSkip;
}

/**
* Returns the incompatible targets that were explicitly requested on the command line.
*/
// TODO(phil): Document this properly.
public ImmutableSet<ConfiguredTarget> getTargetsToFail() {
return targetsToFail;
}

public ArtifactsToOwnerLabels getTopLevelArtifactsToOwnerLabels() {
return topLevelArtifactsToOwnerLabels;
}
Expand Down Expand Up @@ -180,6 +191,7 @@ public AnalysisResult withExclusiveTestsAsParallelTests() {
aspects,
targetsToTest,
targetsToSkip,
targetsToFail,
error,
actionGraph,
topLevelArtifactsToOwnerLabels,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.analysis.config.RunUnder;
import com.google.devtools.build.lib.analysis.constraints.EnvironmentRule;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.analysis.test.TestConfiguration;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.Attribute;
Expand Down Expand Up @@ -329,6 +330,15 @@ public static RuleClass.Builder commonCoreAndSkylarkAttributes(RuleClass.Builder
.dontCheckConstraints()
.nonconfigurable(
"special logic for constraints and select: see ConstraintSemantics"))
/* <!-- #BLAZE_RULE(toolchain).ATTRIBUTE(target_compatible_with) -->
A list of <code>constraint_value</code>s that must be satisfied by the target platform in
order for this toolchain to be selected for a target building for that platform.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.add(
attr("target_compatible_with", LABEL_LIST)
.mandatoryProviders(ConstraintValueInfo.PROVIDER.id())
.allowedFileTypes()
.nonconfigurable("Logic for target compatibility"))
.add(
attr(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE, LABEL_LIST)
.nonconfigurable("stores configurability keys"))
Expand Down
43 changes: 31 additions & 12 deletions src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ public AnalysisResult update(
TargetPatternPhaseValue loadingResult,
BuildOptions targetOptions,
Set<String> multiCpu,
List<String> requestedTargets,
List<String> aspects,
AnalysisOptions viewOptions,
boolean keepGoing,
Expand Down Expand Up @@ -388,18 +389,18 @@ public AnalysisResult update(

getArtifactFactory().noteAnalysisStarting();
SkyframeAnalysisResult skyframeAnalysisResult;
try {
Supplier<Map<BuildConfigurationValue.Key, BuildConfiguration>> configurationLookupSupplier =
() -> {
Map<BuildConfigurationValue.Key, BuildConfiguration> result = new HashMap<>();
for (TargetAndConfiguration node : topLevelTargetsWithConfigs) {
if (node.getConfiguration() != null) {
result.put(
BuildConfigurationValue.key(node.getConfiguration()), node.getConfiguration());
}
Supplier<Map<BuildConfigurationValue.Key, BuildConfiguration>> configurationLookupSupplier =
() -> {
Map<BuildConfigurationValue.Key, BuildConfiguration> result = new HashMap<>();
for (TargetAndConfiguration node : topLevelTargetsWithConfigs) {
if (node.getConfiguration() != null) {
result.put(
BuildConfigurationValue.key(node.getConfiguration()), node.getConfiguration());
}
return result;
};
}
return result;
};
try {
skyframeAnalysisResult =
skyframeBuildView.configureTargets(
eventHandler,
Expand All @@ -425,10 +426,16 @@ public AnalysisResult update(

Set<ConfiguredTarget> targetsToSkip =
new TopLevelConstraintSemantics(
skyframeBuildView,
skyframeExecutor.getPackageManager(),
input -> skyframeExecutor.getConfiguration(eventHandler, input),
eventHandler)
.checkTargetEnvironmentRestrictions(skyframeAnalysisResult.getConfiguredTargets());
.checkTargetEnvironmentRestrictions(
configurationLookupSupplier,
keepGoing,
loadingPhaseThreads,
eventBus,
skyframeAnalysisResult.getConfiguredTargets());

AnalysisResult result =
createResult(
Expand All @@ -440,6 +447,7 @@ public AnalysisResult update(
viewOptions,
skyframeAnalysisResult,
targetsToSkip,
requestedTargets,
topLevelTargetsWithConfigsResult);
logger.atInfo().log("Finished analysis");
return result;
Expand All @@ -454,13 +462,23 @@ private AnalysisResult createResult(
AnalysisOptions viewOptions,
SkyframeAnalysisResult skyframeAnalysisResult,
Set<ConfiguredTarget> targetsToSkip,
List<String> requestedTargets,
TopLevelTargetsAndConfigsResult topLevelTargetsWithConfigs)
throws InterruptedException {
Set<Label> testsToRun = loadingResult.getTestsToRunLabels();
Set<ConfiguredTarget> configuredTargets =
Sets.newLinkedHashSet(skyframeAnalysisResult.getConfiguredTargets());
ImmutableSet<AspectValue> aspects = ImmutableSet.copyOf(skyframeAnalysisResult.getAspects());

// Targets that should be skipped and were requested on the command line
// will be marked as failed.
ImmutableSet.Builder<ConfiguredTarget> failedTargets = ImmutableSet.builder();
for (ConfiguredTarget topLevelTarget : targetsToSkip) {
if (requestedTargets.contains(topLevelTarget.getLabel().toString())) {
failedTargets.add(topLevelTarget);
}
}

Set<ConfiguredTarget> allTargetsToTest = null;
if (testsToRun != null) {
// Determine the subset of configured targets that are meant to be run as tests.
Expand Down Expand Up @@ -545,6 +563,7 @@ public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) {
aspects,
allTargetsToTest == null ? null : ImmutableList.copyOf(allTargetsToTest),
ImmutableSet.copyOf(targetsToSkip),
ImmutableSet.copyOf(failedTargets.build()),
error,
actionGraph,
artifactsToOwnerLabelsBuilder.build(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,26 @@

package com.google.devtools.build.lib.analysis.constraints;

import com.google.common.base.Preconditions;
import com.google.common.base.Predicates;
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
import com.google.common.base.Verify;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Multimap;
import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.PlatformConfiguration;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget;
import com.google.devtools.build.lib.analysis.constraints.SupportedEnvironmentsProvider.RemovedEnvironmentCulprit;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
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;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
Expand All @@ -35,7 +43,11 @@
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.pkgcache.PackageManager;
import com.google.devtools.build.lib.rules.AliasConfiguredTarget;
import com.google.devtools.build.lib.skyframe.BuildConfigurationValue.Key;
import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey;
import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.skyframe.SkyframeAnalysisResult;
import com.google.devtools.build.lib.skyframe.SkyframeBuildView;
import java.util.Collection;
import java.util.List;
import java.util.Map;
Expand All @@ -55,8 +67,9 @@
*/
public class TopLevelConstraintSemantics {
private final PackageManager packageManager;
private final Function<Key, BuildConfiguration> configurationProvider;
private final Function<BuildConfigurationValue.Key, BuildConfiguration> configurationProvider;
private final ExtendedEventHandler eventHandler;
private final SkyframeBuildView skyframeBuildView;

/**
* Constructor with helper classes for loading targets.
Expand All @@ -65,9 +78,11 @@ public class TopLevelConstraintSemantics {
* @param eventHandler the build's event handler
*/
public TopLevelConstraintSemantics(
SkyframeBuildView skyframeBuildView,
PackageManager packageManager,
Function<Key, BuildConfiguration> configurationProvider,
Function<BuildConfigurationValue.Key, BuildConfiguration> configurationProvider,
ExtendedEventHandler eventHandler) {
this.skyframeBuildView = skyframeBuildView;
this.packageManager = packageManager;
this.configurationProvider = configurationProvider;
this.eventHandler = eventHandler;
Expand Down Expand Up @@ -105,6 +120,10 @@ private MissingEnvironment(Label environment, RemovedEnvironmentCulprit culprit)
* environment declared through {@link CoreOptions#targetEnvironments}
*/
public Set<ConfiguredTarget> checkTargetEnvironmentRestrictions(
Supplier<Map<BuildConfigurationValue.Key, BuildConfiguration>> configurationLookupSupplier,
boolean keepGoing,
int loadingPhaseThreads,
EventBus eventBus,
ImmutableList<ConfiguredTarget> topLevelTargets)
throws ViewCreationFailedException, InterruptedException {
ImmutableSet.Builder<ConfiguredTarget> badTargets = ImmutableSet.builder();
Expand Down Expand Up @@ -136,6 +155,83 @@ public Set<ConfiguredTarget> checkTargetEnvironmentRestrictions(
continue; // This target doesn't participate in constraints.
}

Object target_compatible_with_object =
target.getAssociatedRule().getAttributeContainer().getAttr("target_compatible_with");

// Get the target platform key.
ConfiguredTargetKey targetPlatformKey =
ConfiguredTargetKey.of(
Preconditions.checkNotNull(config.getFragment(PlatformConfiguration.class))
.getTargetPlatform(),
config);

ImmutableList.Builder<ConfiguredTargetKey> builder = ImmutableList.builder();

builder.add(targetPlatformKey);
if (target_compatible_with_object instanceof ImmutableList) {
ImmutableList<Label> target_compatible_with_list =
(ImmutableList<Label>) target_compatible_with_object;

for (Label label : target_compatible_with_list) {
Target constraintTarget = null;
try {
constraintTarget = packageManager.getTarget(eventHandler, label);
} catch (NoSuchPackageException | NoSuchTargetException e) {
eventHandler.handle(
Event.error(
"Unable to get target from package when checking environment restrictions. "
+ e));
continue;
}

builder.add(ConfiguredTargetKey.of(label, config));
}
}

SkyframeAnalysisResult skyframeAnalysisResult;

ImmutableList<ConfiguredTargetKey> targets = builder.build();
try {
skyframeAnalysisResult =
skyframeBuildView.configureTargets(
eventHandler,
targets,
ImmutableList.<AspectValueKey>of(),
Suppliers.memoize(configurationLookupSupplier),
eventBus,
keepGoing,
loadingPhaseThreads);
} finally {
skyframeBuildView.clearInvalidatedConfiguredTargets();
}

if (skyframeAnalysisResult.getConfiguredTargets().size() != targets.size()) {
throw new ViewCreationFailedException("found wrong number of targets");
}

// Grab the platform info.
ConfiguredTarget platformTarget = skyframeAnalysisResult.getConfiguredTargets().get(0);
PlatformInfo platformInfo = PlatformProviderUtils.platform(platformTarget);
if (platformInfo == null) {
throw new ViewCreationFailedException("Couldn't find platform info");
}

boolean targetIsBad = false;
for (int i = 1; i < targets.size(); ++i) {
ConstraintValueInfo constraintValue =
PlatformProviderUtils.constraintValue(
skyframeAnalysisResult.getConfiguredTargets().get(i));
if (!platformInfo.constraints().containsAll(ImmutableList.<ConstraintValueInfo>of(constraintValue))) {
// Violated constraint, skipping this target.
targetIsBad = true;
break;
}
}
if (targetIsBad) {
badTargets.add(topLevelTarget);
continue;
}

// Check explicitly expected environments.
exceptionInducingTargets.putAll(topLevelTarget, // This is a no-op on empty collections.
getMissingEnvironments(topLevelTarget, config.getTargetEnvironments()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,7 @@ enum TestStatus {
REMOTE_FAILURE = 6;
FAILED_TO_BUILD = 7;
TOOL_HALTED_BEFORE_TESTING = 8;
SKIPPED = 9;
}

// Payload on events reporting about individual test action.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,23 @@ public static AnalysisResult execute(
env.getSkyframeExecutor()
.getConfiguration(env.getReporter(), target.getConfigurationKey());
Label label = target.getLabel();
env.getEventBus()
.post(
new AbortedEvent(
BuildEventId.targetCompleted(label, config.getEventId()),
AbortReason.SKIPPED,
String.format("Target %s build was skipped.", label),
label));
if (analysisResult.getTargetsToFail().contains(target)) {
env.getEventBus()
.post(
new AbortedEvent(
BuildEventId.targetCompleted(label, config.getEventId()),
AbortReason.ANALYSIS_FAILURE,
String.format("Target %s cannot be built for this platform.", label),
label));
} else {
env.getEventBus()
.post(
new AbortedEvent(
BuildEventId.targetCompleted(label, config.getEventId()),
AbortReason.SKIPPED,
String.format("Target %s build was skipped.", label),
label));
}
}
} else {
env.getReporter().handle(Event.progress("Loading complete."));
Expand Down Expand Up @@ -214,6 +224,7 @@ private static AnalysisResult runAnalysisPhase(
loadingResult,
targetOptions,
multiCpu,
request.getTargets(),
request.getAspects(),
request.getViewOptions(),
request.getKeepGoing(),
Expand Down
Loading

0 comments on commit 4a6f40d

Please sign in to comment.