From e4df959db954df2869f53f4bc6749b649c03c832 Mon Sep 17 00:00:00 2001 From: Philipp Schrader Date: Tue, 9 Jun 2020 21:50:47 -0700 Subject: [PATCH] Add basic incompatible target skipping 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. --- .../google/devtools/build/lib/analysis/BUILD | 12 + .../build/lib/analysis/BaseRuleClasses.java | 9 + .../build/lib/analysis/BuildView.java | 27 +- .../lib/analysis/ConfiguredTargetFactory.java | 91 ++++ .../IncompatiblePlatformProvider.java | 29 ++ .../TopLevelConstraintSemantics.java | 50 +++ .../proto/build_event_stream.proto | 1 + .../lib/buildtool/AnalysisPhaseRunner.java | 1 + .../lib/buildtool/BuildResultPrinter.java | 14 +- .../build/lib/packages/RuleClass.java | 3 + .../lib/runtime/AggregatingTestListener.java | 26 +- .../lib/runtime/BuildEventStreamerUtils.java | 2 + .../runtime/TerminalTestResultNotifier.java | 9 +- .../lib/runtime/TestResultAggregator.java | 15 + .../build/lib/runtime/UiEventHandler.java | 3 +- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../lib/skyframe/SkyframeAnalysisResult.java | 20 + src/main/protobuf/test_status.proto | 1 + src/test/shell/bazel/BUILD | 7 + .../bazel/target_compatible_with_test.sh | 390 ++++++++++++++++++ 20 files changed, 696 insertions(+), 15 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/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index a1cb8b660ebc93..7781ae6fa82bae 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -328,6 +328,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", @@ -378,6 +379,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:fileset_output_symlink", "//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", @@ -778,6 +780,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"], @@ -1982,6 +1993,7 @@ java_library( srcs = ["constraints/TopLevelConstraintSemantics.java"], deps = [ ":analysis_cluster", + ":incompatible_platform_provider", ":config/build_configuration", ":configured_target", ":constraints/constraint_semantics", 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 586d9cc716b404..f0f35719e91c78 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,14 @@ 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(FileTypeSet.NO_FILE)) .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 74f0b84b62a85f..046adba4aa8627 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 @@ -212,6 +212,7 @@ public AnalysisResult update( TargetPatternPhaseValue loadingResult, BuildOptions targetOptions, Set multiCpu, + List requestedTargetPatterns, List aspects, AnalysisOptions viewOptions, boolean keepGoing, @@ -420,6 +421,22 @@ public AnalysisResult update( skyframeBuildView.clearInvalidatedConfiguredTargets(); } + TopLevelConstraintSemantics topLevelConstraintSemantics = + new TopLevelConstraintSemantics( + skyframeExecutor.getPackageManager(), + input -> skyframeExecutor.getConfiguration(eventHandler, input), + eventHandler); + + TopLevelConstraintSemantics.PlatformRestrictionsResult platformRestrictions = + topLevelConstraintSemantics.checkPlatformRestrictions( + skyframeAnalysisResult.getConfiguredTargets(), requestedTargetPatterns, keepGoing); + + if (platformRestrictions.targetsWithErrors.size() > 0) { + skyframeAnalysisResult = + skyframeAnalysisResult.withAdditionalErroredTargets( + ImmutableSet.copyOf(platformRestrictions.targetsWithErrors)); + } + int numTargetsToAnalyze = topLevelTargetsWithConfigs.size(); int numSuccessful = skyframeAnalysisResult.getConfiguredTargets().size(); if (0 < numSuccessful && numSuccessful < numTargetsToAnalyze) { @@ -430,11 +447,11 @@ public AnalysisResult update( } Set targetsToSkip = - new TopLevelConstraintSemantics( - skyframeExecutor.getPackageManager(), - input -> skyframeExecutor.getConfiguration(eventHandler, input), - eventHandler) - .checkTargetEnvironmentRestrictions(skyframeAnalysisResult.getConfiguredTargets()); + ImmutableSet.copyOf( + Sets.union( + topLevelConstraintSemantics.checkTargetEnvironmentRestrictions( + skyframeAnalysisResult.getConfiguredTargets()), + platformRestrictions.targetsToSkip)); AnalysisResult result = createResult( diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index 5d3d61fc5e48aa..ad43b4c7cd7a94 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java @@ -35,6 +35,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; @@ -318,6 +320,12 @@ private ConfiguredTarget createRule( prerequisiteMap.values())) .build(); + ConfiguredTarget incompatibleTarget = + incompatibleConfiguredTarget(ruleContext, prerequisiteMap); + if (incompatibleTarget != null) { + return incompatibleTarget; + } + List> analysisFailures = depAnalysisFailures(ruleContext); if (!analysisFailures.isEmpty()) { return erroredConfiguredTargetWithFailures(ruleContext, analysisFailures); @@ -372,6 +380,89 @@ private ConfiguredTarget createRule( } } + // TODO(phil): Document this. + // TODO(phil): Move this into ConstraintSemantics somehow. + @Nullable + private ConfiguredTarget incompatibleConfiguredTarget( + RuleContext ruleContext, + OrderedSetMultimap prerequisiteMap) + throws ActionConflictException { + if (!"toolchain".equals(ruleContext.getRule().getRuleClass())) { + boolean isIncompatible = false; + + // This is incompatible if one of the dependencies is as well. + for (ConfiguredTargetAndData infoCollection : prerequisiteMap.values()) { + ConfiguredTarget dependency = infoCollection.getConfiguredTarget(); + if (dependency instanceof OutputFileConfiguredTarget) { + // For generated files, we want to query the generating rule for providers. genrule() for + // example doesn't attach providers like IncompatiblePlatformProvider to its outputs. + dependency = ((OutputFileConfiguredTarget)dependency).getGeneratingRule(); + } + if (dependency.getProvider(IncompatiblePlatformProvider.class) != null) { + isIncompatible = true; + } + } + + // This is incompatible if explicitly specified to be. + if (!isIncompatible && ruleContext.attributes().has("target_compatible_with")) { + Iterable constraintValues = + PlatformProviderUtils.constraintValues( + ruleContext.getPrerequisites("target_compatible_with", TransitionMode.DONT_CHECK)); + for (ConstraintValueInfo constraintValue : constraintValues) { + if (!ruleContext.targetPlatformHasConstraint(constraintValue)) { + isIncompatible = true; + } + } + } + + if (isIncompatible) { + // Create a dummy ConfiguredTarget that has the IncompatiblePlatformProvider set. + ImmutableList outputArtifacts = ruleContext.getOutputArtifacts(); + + if (ruleContext.isTestTarget() && outputArtifacts.size() == 0) { + // TODO(philsc): Figure out how to avoid this. Right now some test targets require the + // RunfilesSupport to be added to the RuleConfiguredTargetBuilder. E.g. sh_test requires + // this while cc_test does not. + outputArtifacts = ImmutableList.of(ruleContext.createOutputArtifact()); + } + + NestedSet filesToBuild = + NestedSetBuilder.stableOrder().addAll(outputArtifacts).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); + + ruleContext.registerAction( + new FailAction( + ruleContext.getActionOwner(), + outputArtifacts, + "Can't build this. This target is incompatible.")); + } + return builder.build(); + } + } + + // No incompatibilities found. + return null; + } + private List> depAnalysisFailures(RuleContext ruleContext) { if (ruleContext.getConfiguration().allowAnalysisFailures()) { ImmutableList.Builder> analysisFailures = ImmutableList.builder(); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/IncompatiblePlatformProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/IncompatiblePlatformProvider.java new file mode 100644 index 00000000000000..70cb92163397e2 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/IncompatiblePlatformProvider.java @@ -0,0 +1,29 @@ +// 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; + +// TODO(phil): Document this. +@Immutable +public final class IncompatiblePlatformProvider implements TransitiveInfoProvider { + IncompatiblePlatformProvider() { + // Nothing to do. + } + + public static IncompatiblePlatformProvider simple() { + return new IncompatiblePlatformProvider(); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java index bbeffe7f2658bb..be83f24edea865 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Multimap; import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider; 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; @@ -57,6 +58,19 @@ public class TopLevelConstraintSemantics { private final Function configurationProvider; private final ExtendedEventHandler eventHandler; + // TODO(phil): Document this. + public class PlatformRestrictionsResult { + public ImmutableSet targetsToSkip; + public ImmutableSet targetsWithErrors; + + public PlatformRestrictionsResult( + ImmutableSet targetsToSkip, + ImmutableSet targetsWithErrors) { + this.targetsToSkip = targetsToSkip; + this.targetsWithErrors = targetsWithErrors; + } + }; + /** * Constructor with helper classes for loading targets. * @@ -85,6 +99,42 @@ private MissingEnvironment(Label environment, RemovedEnvironmentCulprit culprit) } } + // TODO(phil): Document this. + public PlatformRestrictionsResult checkPlatformRestrictions( + ImmutableList topLevelTargets, + List requestedTargetPatterns, + boolean keepGoing) + throws ViewCreationFailedException { + ImmutableSet.Builder incompatibleTargets = ImmutableSet.builder(); + ImmutableSet.Builder incompatibleButRequestedTargets = ImmutableSet.builder(); + + for (ConfiguredTarget target : topLevelTargets) { + 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."); + } + this.eventHandler.handle( + Event.warn( + "Incompatible target " + + labelString + + " specifically requested: it will not be built")); + incompatibleButRequestedTargets.add(target); + } else { + incompatibleTargets.add(target); + } + } + } + + return new PlatformRestrictionsResult( + ImmutableSet.copyOf(incompatibleTargets.build()), + ImmutableSet.copyOf(incompatibleButRequestedTargets.build())); + } + /** * Checks that if this is an environment-restricted build, all top-level targets support expected * top-level environments. Expected top-level environments can be declared explicitly through diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto index 29642df0b7f961..806fa51e4856bb 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto @@ -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. diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java index 5118fe42128145..9bcc21c236bb2e 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java @@ -213,6 +213,7 @@ private static AnalysisResult runAnalysisPhase( loadingResult, targetOptions, multiCpu, + request.getTargets(), request.getAspects(), request.getViewOptions(), request.getKeepGoing(), diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java index d4a0e89ba553e8..f3bcdbd59f6ab7 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java @@ -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 succeeded = new ArrayList<>(); Collection failed = new ArrayList<>(); + Collection skipped = new ArrayList<>(); Collection 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) { diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java index e2657e8c39f19d..be910ba9c9142b 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java @@ -264,6 +264,8 @@ public RuleErrorException(String message, Throwable cause) { */ 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}. @@ -1411,6 +1413,7 @@ public 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; } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java b/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java index 62be54b78015d7..5d1b1321f9eae4 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java @@ -140,21 +140,40 @@ private void targetFailure(ConfiguredTargetKey configuredTargetKey) { } } + private void targetSkipped(ConfiguredTargetKey configuredTargetKey) { + TestResultAggregator aggregator = aggregators.get(configuredTargetKey); + if (aggregator != null) { + aggregator.targetSkipped(); + } + } + @VisibleForTesting void buildComplete( - Collection actualTargets, Collection successfulTargets) { + Collection actualTargets, + Collection skippedTargets, + Collection successfulTargets) { if (actualTargets == null || successfulTargets == null) { return; } + ImmutableSet nonSuccessfulTargets = + Sets.difference(ImmutableSet.copyOf(actualTargets), ImmutableSet.copyOf(successfulTargets)) + .immutableCopy(); for (ConfiguredTarget target : Sets.difference( - ImmutableSet.copyOf(actualTargets), ImmutableSet.copyOf(successfulTargets))) { + ImmutableSet.copyOf(nonSuccessfulTargets), ImmutableSet.copyOf(skippedTargets))) { if (isAlias(target)) { continue; } targetFailure(asKey(target)); } + + for (ConfiguredTarget target : skippedTargets) { + if (isAlias(target)) { + continue; + } + targetSkipped(asKey(target)); + } } @Subscribe @@ -164,7 +183,8 @@ public void buildCompleteEvent(BuildCompleteEvent event) { blazeHalted = true; } skipTargetsOnFailure = result.getStopOnFirstFailure(); - buildComplete(result.getActualTargets(), result.getSuccessfulTargets()); + buildComplete( + result.getActualTargets(), result.getSkippedTargets(), result.getSuccessfulTargets()); } @Subscribe diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamerUtils.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamerUtils.java index fc0f161e5a145c..8b15084bb843e1 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamerUtils.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamerUtils.java @@ -45,6 +45,8 @@ public static TestStatus bepStatus(BlazeTestStatus status) { return BuildEventStreamProtos.TestStatus.REMOTE_FAILURE; case BLAZE_HALTED_BEFORE_TESTING: return BuildEventStreamProtos.TestStatus.TOOL_HALTED_BEFORE_TESTING; + case SKIPPED: + return BuildEventStreamProtos.TestStatus.SKIPPED; default: // Not used as the above is a complete case distinction; however, by the open // nature of protobuf enums, we need the clause to convice java, that we always diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TerminalTestResultNotifier.java b/src/main/java/com/google/devtools/build/lib/runtime/TerminalTestResultNotifier.java index e05d7622a17136..bc6dba2acdcd4d 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/TerminalTestResultNotifier.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/TerminalTestResultNotifier.java @@ -56,6 +56,7 @@ private static class TestResultStats { int failedRemotelyCount; int failedLocallyCount; int noStatusCount; + int skippedCount; int numberOfExecutedTargets; boolean wasUnreportedWrongSize; @@ -213,6 +214,8 @@ public void notify(Set summaries, int numberOfExecutedTargets) { stats.passCount++; } else if (summary.getStatus() == BlazeTestStatus.NO_STATUS) { stats.noStatusCount++; + } else if (summary.getStatus() == BlazeTestStatus.SKIPPED) { + stats.skippedCount++; } else if (summary.getStatus() == BlazeTestStatus.FAILED_TO_BUILD) { stats.failedToBuildCount++; } else if (summary.ranRemotely()) { @@ -311,8 +314,10 @@ private void printStats(TestResultStats stats) { addFailureToErrorList(results, "to build", stats.failedToBuildCount); addFailureToErrorList(results, "locally", stats.failedLocallyCount); addFailureToErrorList(results, "remotely", stats.failedRemotelyCount); - addToErrorList(results, "was", "were", "skipped", stats.noStatusCount); - printer.print(String.format("\nExecuted %d out of %d %s: %s.\n", + addToErrorList(results, "was", "were", "skipped", stats.noStatusCount + stats.skippedCount); + printer.print( + String.format( + "\nExecuted %d out of %d %s: %s.\n", stats.numberOfExecutedTargets, stats.numberOfTargets, stats.numberOfTargets == 1 ? "test" : "tests", diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TestResultAggregator.java b/src/main/java/com/google/devtools/build/lib/runtime/TestResultAggregator.java index 6650bcb2ec1a17..0d757b30ef2323 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/TestResultAggregator.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/TestResultAggregator.java @@ -150,6 +150,21 @@ synchronized void targetFailure(boolean blazeHalted, boolean skipTargetsOnFailur policy.eventBus.post(summary.build()); } + synchronized void targetSkipped() { + if (remainingRuns.isEmpty()) { + // Blaze does not guarantee that BuildResult.getSuccessfulTargets() and posted TestResult + // events are in sync. Thus, it is possible that a test event was posted, but the target is + // not present in the set of successful targets. + return; + } + + summary.setStatus(BlazeTestStatus.SKIPPED); + + // These are never going to run; removing them marks the target complete. + remainingRuns.clear(); + policy.eventBus.post(summary.build()); + } + /** Returns the known aggregate results for the given target at the current moment. */ synchronized TestSummary.Builder getCurrentSummaryForTesting() { return summary; diff --git a/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java index ba1d490c98c607..2af4e55e8515b8 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java @@ -679,7 +679,8 @@ private boolean testSummaryProvidesNewInformation(TestSummary summary) { BlazeTestStatus.PASSED, BlazeTestStatus.FAILED_TO_BUILD, BlazeTestStatus.BLAZE_HALTED_BEFORE_TESTING, - BlazeTestStatus.NO_STATUS); + BlazeTestStatus.NO_STATUS, + BlazeTestStatus.SKIPPED); if (statusToIgnore.contains(summary.getStatus())) { return false; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index e542c3e7d33eb2..fd9e8f2f8321c3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -256,6 +256,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:dependency_kind", "//src/main/java/com/google/devtools/build/lib/analysis:duplicate_exception", "//src/main/java/com/google/devtools/build/lib/analysis:inconsistent_aspect_order_exception", + "//src/main/java/com/google/devtools/build/lib/analysis:incompatible_platform_provider", "//src/main/java/com/google/devtools/build/lib/analysis:platform_options", "//src/main/java/com/google/devtools/build/lib/analysis:resolved_toolchain_context", "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_collection", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java index 47e88064df102f..c1cd9527c7bbb1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java @@ -15,6 +15,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.PackageRoots; import com.google.devtools.build.lib.analysis.ConfiguredAspect; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -82,4 +84,22 @@ public ImmutableMap getAspects() { public PackageRoots getPackageRoots() { return packageRoots; } + + /** + * Returns an equivalent {@link SkyframeAnalysisResult}, except with errored targets removed from + * the configured target list. + */ + public SkyframeAnalysisResult withAdditionalErroredTargets( + ImmutableSet erroredTargets) { + return new SkyframeAnalysisResult( + hasLoadingError, + /*hasAnalaysiError=*/ true, + hasActionConflicts, + Sets.difference(ImmutableSet.copyOf(configuredTargets), erroredTargets) + .immutableCopy() + .asList(), + walkableGraph, + aspects, + packageRoots); + } } diff --git a/src/main/protobuf/test_status.proto b/src/main/protobuf/test_status.proto index 72cb268607f0c0..95f9a2775aea63 100644 --- a/src/main/protobuf/test_status.proto +++ b/src/main/protobuf/test_status.proto @@ -41,6 +41,7 @@ enum BlazeTestStatus { REMOTE_FAILURE = 6; FAILED_TO_BUILD = 7; BLAZE_HALTED_BEFORE_TESTING = 8; + SKIPPED = 9; }; // TestCase contains detailed data about all tests (cases/suites/decorators) diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index 6fa630566cda67..76e05b3e449655 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -1296,6 +1296,13 @@ sh_test( tags = ["no_windows"], ) +sh_test( + name = "target_compatible_with_test", + srcs = ["target_compatible_with_test.sh"], + data = [":test-deps"], + deps = ["@bazel_tools//tools/bash/runfiles"], +) + sh_test( name = "resource_compiler_toolchain_test", srcs = ["resource_compiler_toolchain_test.sh"], diff --git a/src/test/shell/bazel/target_compatible_with_test.sh b/src/test/shell/bazel/target_compatible_with_test.sh new file mode 100755 index 00000000000000..cd56353518090d --- /dev/null +++ b/src/test/shell/bazel/target_compatible_with_test.sh @@ -0,0 +1,390 @@ +#!/bin/bash + +# --- begin runfiles.bash initialization --- +set -euo pipefail +if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + if [[ -f "$0.runfiles_manifest" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest" + elif [[ -f "$0.runfiles/MANIFEST" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST" + elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + export RUNFILES_DIR="$0.runfiles" + fi +fi +if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash" +elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \ + "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)" +else + echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash" + exit 1 +fi +# --- end runfiles.bash initialization --- + +source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +# `uname` returns the current platform, e.g "MSYS_NT-10.0" or "Linux". +# `tr` converts all upper case letters to lower case. +# `case` matches the result if the `uname | tr` expression to string prefixes +# that use the same wildcards as names do in Bash, i.e. "msys*" matches strings +# starting with "msys", and "*" matches everything (it's the default case). +case "$(uname -s | tr [:upper:] [:lower:])" in +msys*) + # As of 2019-01-15, Bazel on Windows only supports MSYS Bash. + declare -r is_windows=true + ;; +*) + declare -r is_windows=false + ;; +esac + +function set_up() { + mkdir -p target_skipping || fail "couldn't create directory" + cat < target_skipping/pass.sh || fail "couldn't create pass.sh" +#!/bin/bash +exit 0 +EOT + chmod +x target_skipping/pass.sh + + cat < target_skipping/fail.sh || fail "couldn't create fail.sh" +#!/bin/bash +exit 1 +EOT + chmod +x target_skipping/fail.sh + + cat < target_skipping/BUILD || fail "couldn't create BUILD file" +constraint_setting(name = "not_compatible_setting") + +constraint_value( + name = "not_compatible", + constraint_setting = ":not_compatible_setting", +) + +constraint_setting(name = "foo_version") + +constraint_value( + name = "foo1", + constraint_setting = ":foo_version", +) + +constraint_value( + name = "foo2", + constraint_setting = ":foo_version", +) + +constraint_value( + name = "foo3", + constraint_setting = ":foo_version", +) + +config_setting( + name = "foo1_selectable", + constraint_values = [":foo1"], +) + +config_setting( + name = "foo2_selectable", + constraint_values = [":foo2"], +) + +constraint_setting(name = "bar_version") + +constraint_value( + name = "bar1", + constraint_setting = "bar_version", +) + +constraint_value( + name = "bar2", + constraint_setting = "bar_version", +) + +# TODO(phil): Use more generic constraints for os and cpu. +platform( + name = "foo1_bar1_platform", + constraint_values = [ + "@platforms//os:linux", + "@platforms//cpu:x86_64", + ":foo1", + ":bar1", + ], +) + +platform( + name = "foo2_bar1_platform", + constraint_values = [ + "@platforms//os:linux", + "@platforms//cpu:x86_64", + ":foo2", + ":bar1", + ], +) + +platform( + name = "foo1_bar2_platform", + constraint_values = [ + "@platforms//os:linux", + "@platforms//cpu:x86_64", + ":foo1", + ":bar2", + ], +) + +platform( + name = "foo3_platform", + constraint_values = [ + "@platforms//os:linux", + "@platforms//cpu:x86_64", + ":foo3", + ], +) + +sh_test( + name = "pass_on_foo1", + srcs = [":pass.sh"], + target_compatible_with = [":foo1"], +) + +sh_test( + name = "fail_on_foo2", + srcs = [":fail.sh"], + target_compatible_with = [":foo2"], +) + +sh_test( + name = "pass_on_foo1_bar2", + srcs = [":pass.sh"], + target_compatible_with = [ + ":foo1", + ":bar2", + ], +) + +sh_binary( + name = "some_foo3_target", + srcs = [":pass.sh"], + target_compatible_with = [ + ":foo3", + ], +) +EOT + + cat < target_skipping/WORKSPACE || fail "couldn't create WORKSPACE" +EOT +} + +function test_console_log_for_builds() { + cd target_skipping || fail "couldn't cd into workspace" + + if ! bazel build --show_result=10 --host_platform=@//:foo3_platform \ + --platforms=@//:foo3_platform ... &> "${TEST_log}"; then + cat "${TEST_log}" + fail "Bazel failed the test." + fi + + cp "${TEST_log}" "${TEST_UNDECLARED_OUTPUTS_DIR}"/ + + expect_log 'Target //:some_foo3_target up-to-date' + expect_log 'Target //:fail_on_foo2 was skipped' + expect_log 'Target //:pass_on_foo1 was skipped' + expect_log 'Target //:pass_on_foo1_bar2 was skipped' + expect_not_log 'Target //:fail_on_foo2 failed to build' + expect_not_log 'Target //:pass_on_foo1 failed to build' + expect_not_log 'Target //:pass_on_foo1_bar2 failed to build' + expect_not_log 'Target //:fail_on_foo2 up-to-date' + expect_not_log 'Target //:pass_on_foo1 up-to-date' + expect_not_log 'Target //:pass_on_foo1_bar2 up-to-date' +} + +function test_console_log_for_tests() { + cd target_skipping || fail "couldn't cd into workspace" + + # Get repeatable results from this test. + bazel clean --expunge + + if ! bazel test --host_platform=@//:foo1_bar1_platform \ + --platforms=@//:foo1_bar1_platform ... &> "${TEST_log}"; then + cat "${TEST_log}" + fail "Bazel failed unexpectedly." + fi + expect_log 'Executed 1 out of 3 tests: 1 test passes and 2 were skipped' + expect_log '^//:pass_on_foo1 * PASSED in' + expect_log '^//:fail_on_foo2 * SKIPPED$' + expect_log '^//:pass_on_foo1_bar2 * SKIPPED$' + + if bazel test --host_platform=@//:foo2_bar1_platform \ + --platforms=@//:foo2_bar1_platform ... &> "${TEST_log}"; then + cat "${TEST_log}" + fail "Bazel passed unexpectedly." + fi + expect_log 'Executed 1 out of 3 tests: 1 fails locally and 2 were skipped.' + expect_log '^//:pass_on_foo1 * SKIPPED$' + expect_log '^//:fail_on_foo2 * FAILED in' + expect_log '^//:pass_on_foo1_bar2 * SKIPPED$' + + # Use :all for this one to validate similar behaviour. + if ! bazel test --host_platform=@//:foo1_bar2_platform \ + --platforms=@//:foo1_bar2_platform :all &> "${TEST_log}"; then + cat "${TEST_log}" + fail "Bazel failed unexpectedly." + fi + expect_log 'Executed 2 out of 3 tests: 2 tests pass and 1 was skipped' + expect_log '^//:pass_on_foo1 * PASSED in' + expect_log '^//:fail_on_foo2 * SKIPPED$' + expect_log '^//:pass_on_foo1_bar2 * PASSED in' +} + +function test_failure_on_incompatible_top_level_target() { + cd target_skipping || fail "couldn't cd into workspace" + + if bazel test --show_result=10 --host_platform=@//:foo1_bar1_platform \ + --platforms=@//:foo1_bar1_platform //:pass_on_foo1_bar2 \ + --build_event_text_file="${TEST_log}".build.json \ + &> "${TEST_log}"; then + fail "Bazel passed unexpectedly." + fi + + expect_log 'ERROR: Target //:pass_on_foo1_bar2 is incompatible and cannot be built' + expect_log '^FAILED: Build did NOT complete successfully' + + # Now look at the build event log. + mv "${TEST_log}".build.json "${TEST_log}" + + expect_log '^ name: "PARSING_FAILURE"$' + expect_log 'Target //:pass_on_foo1_bar2 is incompatible and cannot be built.' + + # Run an additional (passing) test and make sure we still fail the build. + if bazel test --show_result=10 --host_platform=@//:foo1_bar1_platform \ + --platforms=@//:foo1_bar1_platform //:pass_on_foo1_bar2 //:pass_on_foo1 \ + --keep_going \ + &> "${TEST_log}"; then + fail "Bazel passed unexpectedly." + fi + + expect_log '^//:pass_on_foo1 * PASSED in' + expect_log '^ERROR: command succeeded, but not all targets were analyzed' + expect_log '^FAILED: Build did NOT complete successfully' +} + +function test_build_event_protocol() { + cd target_skipping || fail "couldn't cd into workspace" + + if ! bazel test --show_result=10 --host_platform=@//:foo1_bar1_platform \ + --build_event_text_file=$TEST_log \ + --platforms=@//:foo1_bar1_platform ...; then + cat "${TEST_log}" + fail "Bazel failed unexpectedly." + fi + expect_not_log 'Target //:pass_on_foo1 build was skipped\.' + expect_log_n 'reason: SKIPPED\>' 3 + expect_log 'Target //:fail_on_foo2 build was skipped\.' + expect_log 'Target //:pass_on_foo1_bar2 build was skipped\.' +} + +function test_non_top_level_skipping() { + cat >> target_skipping/BUILD < "${TEST_log}"; then + fail "Bazel passed unexpectedly." + fi + expect_log 'ERROR: Target //:sh_foo2 is incompatible and cannot be built, but was explicitly requested' + expect_log 'FAILED: Build did NOT complete successfully' +} + +function test_py_test() { + cat > target_skipping/generator_tool.cc < +int main() { + printf("int main() { return 1; }\\n"); + return 0; +} +EOT + + cat >> target_skipping/BUILD < "${TEST_log}"; then + fail "Bazel passed unexpectedly." + fi + expect_log 'ERROR: Target //:generated_test is incompatible and cannot be built, but was explicitly requested' + expect_log 'FAILED: Build did NOT complete successfully' +} + +function test_or_logic() { + cat >> target_skipping/BUILD < "${TEST_log}"; then + fail "Bazel failed unexpectedly." + fi + expect_log '^//:pass_on_foo1_or_foo2_but_not_on_foo3 * PASSED in' + + if ! bazel test --show_result=10 --host_platform=@//:foo2_bar1_platform \ + --platforms=@//:foo2_bar1_platform //:pass_on_foo1_or_foo2_but_not_on_foo3 \ + --nocache_test_results &> "${TEST_log}"; then + fail "Bazel failed unexpectedly." + fi + expect_log '^//:pass_on_foo1_or_foo2_but_not_on_foo3 * PASSED in' + + if bazel test --show_result=10 --host_platform=@//:foo3_platform \ + --platforms=@//:foo3_platform //:pass_on_foo1_or_foo2_but_not_on_foo3 \ + &> "${TEST_log}"; then + fail "Bazel passed unexpectedly." + fi + + expect_log 'ERROR: Target //:pass_on_foo1_or_foo2_but_not_on_foo3 is incompatible and cannot be built, but was explicitly requested' + expect_log 'FAILED: Build did NOT complete successfully' +} + +run_suite "target_compatible_with tests"