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.
  • Loading branch information
philsc committed Jul 12, 2020
1 parent 243c945 commit dd2e775
Show file tree
Hide file tree
Showing 20 changed files with 557 additions and 18 deletions.
12 changes: 12 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ java_library(
":extra/extra_action_info_file_write_action",
":extra_action_artifacts_provider",
":file_provider",
":incompatible_platform_provider",
":inconsistent_aspect_order_exception",
":label_and_location",
":label_expander",
Expand Down Expand Up @@ -373,6 +374,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
"//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
"//src/main/java/com/google/devtools/build/lib/analysis/platform",
"//src/main/java/com/google/devtools/build/lib/analysis/platform:utils",
"//src/main/java/com/google/devtools/build/lib/analysis/stringtemplate",
"//src/main/java/com/google/devtools/build/lib/bugreport",
"//src/main/java/com/google/devtools/build/lib/buildeventstream",
Expand Down Expand Up @@ -598,6 +600,7 @@ java_library(
":configured_target",
":constraints/top_level_constraint_semantics",
":extra_action_artifacts_provider",
":incompatible_platform_provider",
":make_environment_event",
":target_configured_event",
":test/coverage_report_action_factory",
Expand Down Expand Up @@ -767,6 +770,15 @@ java_library(
],
)

java_library(
name = "incompatible_platform_provider",
srcs = ["IncompatiblePlatformProvider.java"],
deps = [
":transitive_info_provider",
"//src/main/java/com/google/devtools/build/lib/concurrent",
],
)

java_library(
name = "inconsistent_aspect_order_exception",
srcs = ["InconsistentAspectOrderException.java"],
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.ConstraintConstants;
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 @@ -327,6 +328,15 @@ public static RuleClass.Builder commonCoreAndStarlarkAttributes(RuleClass.Builde
.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
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.ConfigurationResolver.TopLevelTargetsAndConfigsResult;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.constraints.TopLevelConstraintSemantics;
import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory;
import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory.CoverageReportActionsWrapper;
import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
Expand Down Expand Up @@ -211,6 +210,7 @@ public AnalysisResult update(
TargetPatternPhaseValue loadingResult,
BuildOptions targetOptions,
Set<String> multiCpu,
List<String> requestedTargetPatterns,
List<String> aspects,
AnalysisOptions viewOptions,
boolean keepGoing,
Expand Down Expand Up @@ -419,6 +419,40 @@ public AnalysisResult update(
skyframeBuildView.clearInvalidatedConfiguredTargets();
}

ImmutableSet.Builder<ConfiguredTarget> incompatibleTargets = ImmutableSet.builder();
ImmutableSet.Builder<ConfiguredTarget> incompatibleButRequestedTargets = ImmutableSet.builder();

for (ConfiguredTarget target : skyframeAnalysisResult.getConfiguredTargets()) {
if (target.getProvider(IncompatiblePlatformProvider.class) != null) {
String labelString = target.getLabel().toString();
if (requestedTargetPatterns.contains(labelString)) {
if (!keepGoing) {
throw new ViewCreationFailedException(
"Target "
+ labelString
+ " is incompatible and cannot be built, but was explicitly requested.");
}
eventHandler.handle(
Event.warn(
"Incompatible target "
+ labelString
+ " specifically requested: it will not be built"));
incompatibleButRequestedTargets.add(target);
} else {
incompatibleTargets.add(target);
}
}
}

Set<ConfiguredTarget> targetsToSkip = ImmutableSet.copyOf(incompatibleTargets.build());
Set<ConfiguredTarget> targetsToFail =
ImmutableSet.copyOf(incompatibleButRequestedTargets.build());

if (targetsToFail.size() > 0) {
skyframeAnalysisResult =
skyframeAnalysisResult.withAdditionalErroredTargets(ImmutableSet.copyOf(targetsToFail));
}

int numTargetsToAnalyze = topLevelTargetsWithConfigs.size();
int numSuccessful = skyframeAnalysisResult.getConfiguredTargets().size();
if (0 < numSuccessful && numSuccessful < numTargetsToAnalyze) {
Expand All @@ -428,13 +462,6 @@ public AnalysisResult update(
logger.atInfo().log(msg);
}

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

AnalysisResult result =
createResult(
eventHandler,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.devtools.build.lib.actions.FailAction;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.RuleContext.InvalidExecGroupException;
import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.Fragment;
Expand All @@ -35,6 +36,8 @@
import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget;
import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils;
import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleConfiguredTargetUtil;
import com.google.devtools.build.lib.analysis.test.AnalysisFailure;
import com.google.devtools.build.lib.analysis.test.AnalysisFailureInfo;
Expand Down Expand Up @@ -69,6 +72,7 @@
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
Expand Down Expand Up @@ -304,6 +308,76 @@ private ConfiguredTarget createRule(
prerequisiteMap.values()))
.build();

if (!"toolchain".equals(rule.getRuleClass())) {
boolean isIncompatible = false;

// This is incompatible if one of the dependencies is as well.
for (ConfiguredTargetAndData infoCollection : prerequisiteMap.values()) {
if (infoCollection.getConfiguredTarget().getProvider(IncompatiblePlatformProvider.class) != null) {
isIncompatible = true;
}
}

// This is incompatible if explicitly specified to be.
if (!isIncompatible && ruleContext.attributes().has("target_compatible_with")) {
Iterable<ConstraintValueInfo> constraintValues =
PlatformProviderUtils.constraintValues(
ruleContext.getPrerequisites("target_compatible_with", TransitionMode.DONT_CHECK));
for (ConstraintValueInfo constraintValue : constraintValues) {
if (!ruleContext.targetPlatformHasConstraint(constraintValue)) {
isIncompatible = true;
}
}
}
if (isIncompatible) {
NestedSetBuilder<Artifact> filesToBuildBuilder =
NestedSetBuilder.<Artifact>stableOrder();

ImmutableList<Artifact> outputArtifacts = ruleContext.getOutputArtifacts();

if (ruleContext.isTestTarget() && outputArtifacts.size() == 0) {
// TODO(philsc): Figure out how to avoid this. Right now test targets require the
// RunfilesSupport to be added to the RuleConfiguredTargetBuilder. Can we use an input for
// this? How do I get access to inputs?
outputArtifacts = ImmutableList.of(ruleContext.createOutputArtifact());
}

for (Artifact output : outputArtifacts) {
ruleContext.registerAction(
SymlinkAction.toAbsolutePath(
ruleContext.getActionOwner(),
PathFragment.create("/bin/false"),
output,
"Faking out " + ruleContext.getLabel()));
filesToBuildBuilder.add(output);
}

NestedSet<Artifact> filesToBuild = filesToBuildBuilder.build();

Runfiles.Builder runfilesBuilder =
new Runfiles.Builder(
ruleContext.getWorkspaceName(),
ruleContext.getConfiguration().legacyExternalRunfiles());
Runfiles runfiles =
runfilesBuilder
.addTransitiveArtifacts(filesToBuild)
.addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES)
.build();

RuleConfiguredTargetBuilder builder = new RuleConfiguredTargetBuilder(ruleContext);
builder.setFilesToBuild(filesToBuild);
builder.add(IncompatiblePlatformProvider.class, IncompatiblePlatformProvider.simple());
builder.add(RunfilesProvider.class, RunfilesProvider.simple(runfiles));
if (outputArtifacts.size() > 0) {
Artifact executable = outputArtifacts.get(0);
RunfilesSupport runfilesSupport =
RunfilesSupport.withExecutable(ruleContext, runfiles, executable);
builder.setRunfilesSupport(runfilesSupport, executable);
}
return builder.build();
}
}

List<NestedSet<AnalysisFailure>> analysisFailures = depAnalysisFailures(ruleContext);
if (!analysisFailures.isEmpty()) {
return erroredConfiguredTargetWithFailures(ruleContext, analysisFailures);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright 2020 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

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

import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;

@Immutable
public final class IncompatiblePlatformProvider implements TransitiveInfoProvider {
IncompatiblePlatformProvider() {
// Nothing to do.
}

public static IncompatiblePlatformProvider simple() {
return new IncompatiblePlatformProvider();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.Instantiator;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
import com.google.devtools.build.lib.syntax.Printer;
import javax.annotation.Nullable;

/** A ConfiguredTarget for an OutputFile. */
@AutoCodec
Expand Down Expand Up @@ -114,6 +115,14 @@ public boolean hasOutputLicenses() {
.hasOutputLicenses();
}

@Nullable
@Override
public <P extends TransitiveInfoProvider> P getProvider(Class<P> providerClass) {
// TODO(bazel-team): Should aspects be allowed to override providers on the configured target
// class?
return getProvider(providerClass, null);
}

/**
* Returns the corresponding provider from the generating rule, if it is non-null, or {@code
* defaultValue} otherwise.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,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 @@ -213,6 +213,7 @@ private static AnalysisResult runAnalysisPhase(
loadingResult,
targetOptions,
multiCpu,
request.getTargets(),
request.getAspects(),
request.getViewOptions(),
request.getKeepGoing(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,22 @@ void showBuildResult(
if (targetsToPrint.size() > request.getBuildOptions().maxResultTargets) {
return;
}
// Filter the targets we care about into two buckets:
// Filter the targets we care about into three buckets:
Collection<ConfiguredTarget> succeeded = new ArrayList<>();
Collection<ConfiguredTarget> failed = new ArrayList<>();
Collection<ConfiguredTarget> skipped = new ArrayList<>();
Collection<ConfiguredTarget> successfulTargets = result.getSuccessfulTargets();
for (ConfiguredTarget target : targetsToPrint) {
(successfulTargets.contains(target) ? succeeded : failed).add(target);
if (configuredTargetsToSkip.contains(target)) {
skipped.add(target);
} else {
(successfulTargets.contains(target) ? succeeded : failed).add(target);
}
}

// TODO(bazel-team): convert these to a new "SKIPPED" status when ready: b/62191890.
failed.addAll(configuredTargetsToSkip);
for (ConfiguredTarget target : skipped) {
outErr.printErr("Target " + target.getLabel() + " was skipped\n");
}

TopLevelArtifactContext context = request.getTopLevelArtifactContext();
for (ConfiguredTarget target : succeeded) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,8 @@ final class RuleErrorException extends Exception {
*/
public static final String COMPATIBLE_ENVIRONMENT_ATTR = "compatible_with";

public static final String TARGET_RESTRICTED_TO_ATTR = "target_compatible_with";

/**
* For Bazel's constraint system: the implicit attribute used to store rule class restriction
* defaults as specified by {@link Builder#restrictedTo}.
Expand Down Expand Up @@ -1405,6 +1407,7 @@ public <TYPE> Builder exemptFromConstraintChecking(String reason) {
this.supportsConstraintChecking = false;
attributes.remove(RuleClass.COMPATIBLE_ENVIRONMENT_ATTR);
attributes.remove(RuleClass.RESTRICTED_ENVIRONMENT_ATTR);
attributes.remove(RuleClass.TARGET_RESTRICTED_TO_ATTR);
return this;
}

Expand Down
Loading

0 comments on commit dd2e775

Please sign in to comment.