From 6c26d99a4268d09ed6e3fedb8a02f7753fbe95be Mon Sep 17 00:00:00 2001 From: Philipp Schrader Date: Tue, 9 Jun 2020 21:50:47 -0700 Subject: [PATCH] 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. --- .../build/lib/analysis/AnalysisResult.java | 17 + .../google/devtools/build/lib/analysis/BUILD | 12 + .../build/lib/analysis/BaseRuleClasses.java | 10 + .../build/lib/analysis/BuildView.java | 26 +- .../lib/analysis/ConfiguredTargetFactory.java | 47 +++ .../IncompatiblePlatformProvider.java | 28 ++ .../proto/build_event_stream.proto | 1 + .../lib/buildtool/AnalysisPhaseRunner.java | 14 + .../lib/buildtool/BuildResultPrinter.java | 14 +- .../build/lib/buildtool/ExecutionTool.java | 18 +- .../build/lib/packages/RuleClass.java | 3 + .../build/lib/rules/platform/Toolchain.java | 5 +- .../lib/runtime/AggregatingTestListener.java | 26 +- .../lib/runtime/BuildEventStreamerUtils.java | 2 + .../runtime/TerminalTestResultNotifier.java | 9 +- .../lib/runtime/TestResultAggregator.java | 17 + .../build/lib/runtime/UiEventHandler.java | 3 +- src/main/protobuf/failure_details.proto | 1 + src/main/protobuf/test_status.proto | 1 + src/test/shell/bazel/BUILD | 7 + .../bazel/target_compatible_with_test.sh | 301 ++++++++++++++++++ 21 files changed, 541 insertions(+), 21 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/IncompatiblePlatformProvider.java create mode 100755 src/test/shell/bazel/target_compatible_with_test.sh diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java index 3c63b346de8f85..5ca0e8050e08cc 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java @@ -35,6 +35,7 @@ public final class AnalysisResult { private final ImmutableSet targetsToBuild; @Nullable private final ImmutableList targetsToTest; private final ImmutableSet targetsToSkip; + private final ImmutableSet targetsToFail; @Nullable private final FailureDetail failureDetail; private final ActionGraph actionGraph; private final ArtifactsToOwnerLabels topLevelArtifactsToOwnerLabels; @@ -53,6 +54,7 @@ public final class AnalysisResult { ImmutableMap aspects, @Nullable ImmutableList targetsToTest, ImmutableSet targetsToSkip, + ImmutableSet targetsToFail, @Nullable FailureDetail failureDetail, ActionGraph actionGraph, ArtifactsToOwnerLabels topLevelArtifactsToOwnerLabels, @@ -68,6 +70,7 @@ public final class AnalysisResult { this.aspects = aspects; this.targetsToTest = targetsToTest; this.targetsToSkip = targetsToSkip; + this.targetsToFail = targetsToFail; this.failureDetail = failureDetail; this.actionGraph = actionGraph; this.topLevelArtifactsToOwnerLabels = topLevelArtifactsToOwnerLabels; @@ -120,6 +123,19 @@ public ImmutableSet getTargetsToSkip() { return targetsToSkip; } + /** + * Returns the configured targets that should not be executed, but were explicitly requested on + * the command line. + * + *

This set of targets is disjoint from {@code getTargetsToSkip()}. + * + *

For example: tests that aren't intended for the designated CPU, but were explicitly + * specified on the command line. + */ + public ImmutableSet getTargetsToFail() { + return targetsToFail; + } + public ArtifactsToOwnerLabels getTopLevelArtifactsToOwnerLabels() { return topLevelArtifactsToOwnerLabels; } @@ -176,6 +192,7 @@ public AnalysisResult withExclusiveTestsAsParallelTests() { aspects, targetsToTest, targetsToSkip, + targetsToFail, failureDetail, actionGraph, topLevelArtifactsToOwnerLabels, 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 4a976bc11276ce..0904272fccd4db 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -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", @@ -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", @@ -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", @@ -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"], diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java index 14becb7631c234..ebe1223a716f4e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java @@ -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; @@ -327,6 +328,15 @@ public static RuleClass.Builder commonCoreAndStarlarkAttributes(RuleClass.Builde .dontCheckConstraints() .nonconfigurable( "special logic for constraints and select: see ConstraintSemantics")) + /* + A list of constraint_values that must be satisfied by the target platform in + order for this toolchain to be selected for a target building for that platform. + */ + .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")) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index a8b8df5c1e93e2..2b46fc8ef90749 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -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; @@ -211,6 +210,7 @@ public AnalysisResult update( TargetPatternPhaseValue loadingResult, BuildOptions targetOptions, Set multiCpu, + List requestedTargetPatterns, List aspects, AnalysisOptions viewOptions, boolean keepGoing, @@ -428,12 +428,21 @@ public AnalysisResult update( logger.atInfo().log(msg); } - Set targetsToSkip = - new TopLevelConstraintSemantics( - skyframeExecutor.getPackageManager(), - input -> skyframeExecutor.getConfiguration(eventHandler, input), - eventHandler) - .checkTargetEnvironmentRestrictions(skyframeAnalysisResult.getConfiguredTargets()); + ImmutableSet.Builder badTargets = ImmutableSet.builder(); + ImmutableSet.Builder badButRequestedTargets = ImmutableSet.builder(); + + for (ConfiguredTarget target : skyframeAnalysisResult.getConfiguredTargets()) { + if (target.getProvider(IncompatiblePlatformProvider.class) != null) { + if (requestedTargetPatterns.contains(target.getLabel().toString())) { + badButRequestedTargets.add(target); + } else { + badTargets.add(target); + } + } + } + + Set targetsToSkip = ImmutableSet.copyOf(badTargets.build()); + Set targetsToFail = ImmutableSet.copyOf(badButRequestedTargets.build()); AnalysisResult result = createResult( @@ -445,6 +454,7 @@ public AnalysisResult update( viewOptions, skyframeAnalysisResult, targetsToSkip, + targetsToFail, topLevelTargetsWithConfigsResult); logger.atInfo().log("Finished analysis"); return result; @@ -459,6 +469,7 @@ private AnalysisResult createResult( AnalysisOptions viewOptions, SkyframeAnalysisResult skyframeAnalysisResult, Set targetsToSkip, + Set targetsToFail, TopLevelTargetsAndConfigsResult topLevelTargetsWithConfigs) throws InterruptedException { Set