From 66f85e2acf373b6517b1bff177c0d002992066c6 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 support 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 target skipping based on the target platform. In a `BUILD` file you can now add constraints that the target platform must satisfy in order for the target to be built and/or tested. For example, use the following snippet to declare a target to be compatible with Windows platforms only: cc_binary( name = "bin", srcs = ["bin.cc"], target_compatible_with = [ "@platforms//os:windows", ], ) Builds triggered with `:all` or `...` on a non-Windows platform will simply skip the incompatible target. An appropriate note is shown on the command line if the `--show_result` threshold is high enough. Targets that transitively depend on incompatible targets are themselves considered incompatible and will also be skipped. Explicitly requesting an incompatible target on the command line is an error and will cause the build to fail. Bazel will print out an appropriate error message and inform the user what constraint could not be satisfied. See the new documentation in this patch for more information. In particular, https://docs.bazel.build/versions/master/platforms.html should be a good bit more informative. This implementation does not make any effort to support expressing compatibility with toolchains. It is possible that using `select()` (commented on below) already makes this possible, but it's not validated or explicitly supported in this patch. During implementation we noticed that `select()` can be quite powerful in combination with `target_compatible_with`. A basic summary of this is also documented on the Platforms page. It may be useful to create helper functions in, say, skylib to help make complex `select()` statements more readable. For example, we could replace the following: target_compatible_with = select({ "@platforms//os:linux": [], "@platforms//os:macos": [], "//conditions:default": [":not_compatible"], }) with something like: target_compatible_with = constraints.any_of([ "@platforms//os:linux", "@platforms//os:macos", ]) That, however, is work for follow-up patches. Many thanks to Austin Schuh (@AustinSchuh) and Greg Estren (@gregestren) for working on the proposal and helping a ton on this patch itself. Also thanks to many others who provided feedback on the implementation. RELNOTES: Bazel skips incompatible targets based on target platform and `target_compatible_with` contents. See https://docs.bazel.build/versions/master/platforms.html for more details. --- WORKSPACE | 2 + site/docs/platforms-intro.md | 2 + site/docs/platforms.md | 114 +++ .../build/docgen/PredefinedAttributes.java | 1 + .../common/target_compatible_with.html | 31 + .../google/devtools/build/lib/analysis/BUILD | 15 + .../build/lib/analysis/BaseRuleClasses.java | 5 + .../build/lib/analysis/BuildView.java | 29 +- .../lib/analysis/ConfiguredTargetFactory.java | 7 + .../IncompatiblePlatformProvider.java | 81 ++ .../RuleContextConstraintSemantics.java | 160 ++++ .../TopLevelConstraintSemantics.java | 119 +++ .../lib/buildtool/AnalysisPhaseRunner.java | 52 ++ .../lib/buildtool/BuildResultPrinter.java | 14 +- .../TestFilteringCompleteEvent.java | 13 +- .../build/lib/packages/RuleClass.java | 7 + .../lib/runtime/AggregatingTestListener.java | 32 +- .../runtime/TerminalTestResultNotifier.java | 4 +- .../lib/runtime/TestResultAggregator.java | 21 +- .../build/lib/runtime/TestSummary.java | 16 +- .../build/lib/runtime/TestSummaryPrinter.java | 32 +- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../lib/skyframe/SkyframeAnalysisResult.java | 20 + src/main/protobuf/failure_details.proto | 2 + .../lib/analysis/util/AnalysisTestCase.java | 23 +- .../analysis/util/BuildViewForTesting.java | 2 + .../lib/analysis/util/BuildViewTestCase.java | 16 +- .../lib/runtime/TestResultAggregatorTest.java | 3 +- .../lib/starlark/StarlarkRuleContextTest.java | 1 + src/test/shell/bazel/BUILD | 7 + .../bazel/target_compatible_with_test.sh | 696 ++++++++++++++++++ ...mpatible_with-to-internal_target_com.patch | 52 ++ 32 files changed, 1545 insertions(+), 35 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/docgen/templates/attributes/common/target_compatible_with.html 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 create mode 100644 third_party/bazel_toolchains/0001-Rename-target_compatible_with-to-internal_target_com.patch diff --git a/WORKSPACE b/WORKSPACE index 54b63d939643c6..e0a9703cb6e7cc 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -440,6 +440,8 @@ http_archive( "https://mirror.bazel.build/github.com/bazelbuild/bazel-toolchains/releases/download/3.1.0/bazel-toolchains-3.1.0.tar.gz", "https://github.com/bazelbuild/bazel-toolchains/releases/download/3.1.0/bazel-toolchains-3.1.0.tar.gz", ], + patches = ["@//third_party:bazel_toolchains/0001-Rename-target_compatible_with-to-internal_target_com.patch"], + patch_args = ["-p1"], ) load("@bazel_toolchains//rules:rbe_repo.bzl", "rbe_autoconfig") diff --git a/site/docs/platforms-intro.md b/site/docs/platforms-intro.md index ca35b23ac9c29c..f8a5e9d8adb1da 100644 --- a/site/docs/platforms-intro.md +++ b/site/docs/platforms-intro.md @@ -106,6 +106,8 @@ or opt in early depends on your specific value / cost needs: flags on the command line. * Simpler language design. All languages share a common API for defining toolchains, using toolchains, and selecting the right toolchain for a platform. +* Targets can be [skipped](platforms.html#skipping-incompatible-targets) in the + build and test phase if they are incompatible with the target platform. ### Costs * Dependent projects that don't yet support platforms might not automatically work diff --git a/site/docs/platforms.md b/site/docs/platforms.md index a6dfb112494b99..5dcc7636481806 100644 --- a/site/docs/platforms.md +++ b/site/docs/platforms.md @@ -113,3 +113,117 @@ command-line flags: * `--host_platform` - defaults to `@bazel_tools//platforms:host_platform` * `--platforms` - defaults to `@bazel_tools//platforms:target_platform` + +## Skipping incompatible targets + +When building for a specific target platform it is often desirable to skip +targets that will never work on that platform. For example, your Windows device +driver is likely going to generate lots of compiler errors when building on a +Linux machine with `//...`. Use the +[`target_compatible_with`](be/common-definitions.html#common.target_compatible_with) +attribute to tell bazel what target platform constraints your code has. + +The simplest use of this attribute restricts a target to a single platform. +The target will not be built for any platform that doesn't satisfy all of the +constraints. The following example restricts `win_driver_lib.cc` to 64-bit +Windows. + +```python +cc_library( + name = "win_driver_lib", + srcs = "win_driver_lib.cc", + target_compatible_with = [ + "@platforms//cpu:x86_64", + "@platforms//os:windows", + ], +) +``` + +When building for anything but 64-bit Windows we say that `:win_driver_lib` is +incompatible. Incompatibility is transitive. Any targets that transitively +depend on an incompatible target are themselves considered incompatible. + +### When are targets skipped? + +Targets are skipped when they are considered incompatible and included in the +build as part of a glob. For example, the following two invocations skip any +incompatible targets found in a glob expansion. + +```console +$ bazel build --platforms=//:myplatform //...` +``` + +```console +$ bazel build --platforms=//:myplatform //:all` +``` + +Explicitly specifying an incompatible target on the command line results in an +error message and a failed build. + +```console +$ bazel build --platforms=//:myplatform //:target_incompatible_with_myplatform +... +ERROR: Target //:target_incompatible_with_myplatform is incompatible and cannot be built, but was explicitly requested. +... +FAILED: Build did NOT complete successfully +``` + +### More expressive constraints + +For more flexibility in expressing constraints, create a user-defined +[`constraint_value`](platform.html#constraint_value) that no platform +satisfies. For example, Put the following somewhere in your project and change +`//:not_compatible` in the subsequent examples to match your location. + +```python +constraint_setting(name = "not_compatible_setting") + +constraint_value( + name = "not_compatible", + constraint_setting = ":not_compatible_setting", +) +``` + +Use [`select()`](functions.html#select) in combination with `:not_compatible` +to express more complicated restrictions. For example, use it to implement +basic OR logic. The following marks a library compatible with macOS and Linux, +but no other platforms. Note that an empty constraints list is equivalent to +"compatible with everything". + +```python +cc_library( + name = "unixish_lib", + srcs = "unixish_lib.cc", + target_compatible_with = select({ + "@platforms//os:osx": [], + "@platforms//os:linux": [], + "//conditions:default": ["//:not_compatible"], + ], +) +``` + +The above can be interpreted as follows: + +1. If we are targeting macOS, then this target has no constraints. +2. If we are targeting Linux, then this target has no constraints. +3. Otherwise the target has the `:not_compatible` constraint. Because + `:not_compatible` is not part of any platforms, the target is deemed + incompatible. + +To make your constraints more readable, use +[skylib](https://github.com/bazelbuild/bazel-skylib)'s +[`selects.with_or()`](https://github.com/bazelbuild/bazel-skylib/blob/master/docs/selects_doc.md#selectswith_or). + +You can express inverse compatibility in a similar way. The following example +describes a library that is compatible with everything _except_ for ARM. + +```python +cc_library( + name = "non_arm_lib", + srcs = "non_arm_lib.cc", + target_compatible_with = select({ + "@platforms//cpu:arm": ["//:not_compatible"], + "//conditions:default": [], + ], +) +``` diff --git a/src/main/java/com/google/devtools/build/docgen/PredefinedAttributes.java b/src/main/java/com/google/devtools/build/docgen/PredefinedAttributes.java index 2c6629fd9eb504..48dbbddcf4d070 100644 --- a/src/main/java/com/google/devtools/build/docgen/PredefinedAttributes.java +++ b/src/main/java/com/google/devtools/build/docgen/PredefinedAttributes.java @@ -54,6 +54,7 @@ public class PredefinedAttributes { "templates/attributes/common/licenses.html", "templates/attributes/common/restricted_to.html", "templates/attributes/common/tags.html", + "templates/attributes/common/target_compatible_with.html", "templates/attributes/common/testonly.html", "templates/attributes/common/toolchains.html", "templates/attributes/common/visibility.html"); diff --git a/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/target_compatible_with.html b/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/target_compatible_with.html new file mode 100644 index 00000000000000..7d46d00ef6c2b3 --- /dev/null +++ b/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/target_compatible_with.html @@ -0,0 +1,31 @@ +

+List of labels; optional; default +is the empty list +

+ +

+A list of +constraint_values +that must be present in the target platform for this target to be considered +"compatible". This is in addition to any constraints already set by the rule +type. If the target platform does not satisfy all listed constraints then the +target is considered "incompatible". Incompatible targets are skipped for +building and testing when globbed (e.g. `//...`, `:all`). When explicitly +specified on the command line, incompatible targets cause bazel to print an +error and cause a build or test failure. +

+ +

+Targets that transitively depend on incompatible targets are themselves +considered incompatible. They are also skipped for building and testing. +

+ +

+An empty list (which is the default) signifies that the target is compatible +with all platforms. +

+ +

+See the Platforms +page for more information about incompatible target skipping. +

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 1b80335f623587..c9e07a0e9eed8c 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", @@ -377,6 +378,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", @@ -775,6 +777,18 @@ java_library( ], ) +java_library( + name = "incompatible_platform_provider", + srcs = ["IncompatiblePlatformProvider.java"], + deps = [ + ":configured_target", + ":transitive_info_provider", + "//src/main/java/com/google/devtools/build/lib/analysis/platform", + "//src/main/java/com/google/devtools/build/lib/concurrent", + "//third_party:guava", + ], +) + java_library( name = "inconsistent_aspect_order_exception", srcs = ["InconsistentAspectOrderException.java"], @@ -1986,6 +2000,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 3eace72a2180af..12a54a0d5c68c1 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; @@ -334,6 +335,10 @@ public static RuleClass.Builder commonCoreAndStarlarkAttributes(RuleClass.Builde .dontCheckConstraints() .nonconfigurable( "special logic for constraints and select: see ConstraintSemantics")) + .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 7dd75d0467eece..48308d91756cde 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, + ImmutableSet explicitTargetPatterns, List aspects, AnalysisOptions viewOptions, boolean keepGoing, @@ -430,6 +431,24 @@ public AnalysisResult update( skyframeBuildView.clearInvalidatedConfiguredTargets(); } + TopLevelConstraintSemantics topLevelConstraintSemantics = + new TopLevelConstraintSemantics( + skyframeExecutor.getPackageManager(), + input -> skyframeExecutor.getConfiguration(eventHandler, input), + eventHandler); + + TopLevelConstraintSemantics.PlatformRestrictionsResult platformRestrictions = + topLevelConstraintSemantics.checkPlatformRestrictions( + skyframeAnalysisResult.getConfiguredTargets(), explicitTargetPatterns, keepGoing); + + if (platformRestrictions.targetsWithErrors.size() > 0) { + // If there are any errored targets (e.g. incompatible targets that are explicitly specified + // on the command line), remove them from the list of targets to be built. + skyframeAnalysisResult = + skyframeAnalysisResult.withAdditionalErroredTargets( + ImmutableSet.copyOf(platformRestrictions.targetsWithErrors)); + } + int numTargetsToAnalyze = topLevelTargetsWithConfigs.size(); int numSuccessful = skyframeAnalysisResult.getConfiguredTargets().size(); if (0 < numSuccessful && numSuccessful < numTargetsToAnalyze) { @@ -440,11 +459,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 52c24aaf9d28ae..ee92dce2ef4079 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,7 @@ 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.constraints.RuleContextConstraintSemantics; 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; @@ -322,6 +323,12 @@ private ConfiguredTarget createRule( prerequisiteMap.values())) .build(); + ConfiguredTarget incompatibleTarget = + RuleContextConstraintSemantics.incompatibleConfiguredTarget(ruleContext, prerequisiteMap); + if (incompatibleTarget != null) { + return incompatibleTarget; + } + List> analysisFailures = depAnalysisFailures(ruleContext); if (!analysisFailures.isEmpty()) { return erroredConfiguredTargetWithFailures(ruleContext, analysisFailures); 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..cb7e135b0df766 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/IncompatiblePlatformProvider.java @@ -0,0 +1,81 @@ +// 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.common.base.Preconditions; +import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo; +import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import javax.annotation.Nullable; + +/** + * Provider instance for the {@code target_compatible_with} attribute. + * + *

The presence of this provider is used to indicate that a target is incompatible with the + * current platform. Any target that provides this will automatically be excluded from {@link + * SkyframeAnalysisResult}'s list of configured targets. + * + *

This provider is able to keep track of _why_ the corresponding target is considered + * incompatible. If the target is incompatible because the target platform didn't satisfy one of the + * constraints in target_compatible_with, then the relevant constraint is accessible via {@code + * getConstraintResponsibleForIncompatibility()}. On the other hand, if the corresponding target is + * incompatible because one of its dependencies is incompatible, then the incompatible dependency is + * available via {@code getTargetResponsibleForIncompatibility()}. + */ +@Immutable +public final class IncompatiblePlatformProvider implements TransitiveInfoProvider { + private final ConfiguredTarget targetResponsibleForIncompatibility; + private final ConstraintValueInfo constraintResponsibleForIncompatibility; + + IncompatiblePlatformProvider( + ConfiguredTarget targetResponsibleForIncompatibility, + ConstraintValueInfo constraintResponsibleForIncompatibility) { + this.targetResponsibleForIncompatibility = targetResponsibleForIncompatibility; + this.constraintResponsibleForIncompatibility = constraintResponsibleForIncompatibility; + } + + public static IncompatiblePlatformProvider incompatibleDueToTarget( + ConfiguredTarget targetResponsibleForIncompatibility) { + Preconditions.checkNotNull(targetResponsibleForIncompatibility); + return new IncompatiblePlatformProvider(targetResponsibleForIncompatibility, null); + } + + public static IncompatiblePlatformProvider incompatibleDueToConstraint( + ConstraintValueInfo constraint) { + Preconditions.checkNotNull(constraint); + return new IncompatiblePlatformProvider(null, constraint); + } + + /** + * Returns the incompatible dependency that caused this provider to be present. + * + *

This may be null. If it is null, then {@code getConstraintResponsibleForIncompatibility()} + * is guaranteed to be non-null. + */ + @Nullable + public ConfiguredTarget getTargetResponsibleForIncompatibility() { + return this.targetResponsibleForIncompatibility; + } + + /** + * Returns the constraint that the target platform didn't satisfy. + * + *

This may be null. If it is null, then {@code getTargetResponsibleForIncompatibility()} is + * guaranteed to be non-null. + */ + @Nullable + public ConstraintValueInfo getConstraintResponsibleForIncompatibility() { + return this.constraintResponsibleForIncompatibility; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java index a6de0362a5fc74..e8bd5cd7285190 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java @@ -18,17 +18,31 @@ import com.google.common.base.Preconditions; import com.google.common.base.Verify; import com.google.common.collect.ImmutableCollection; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.FailAction; +import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.DependencyKind; +import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider; import com.google.devtools.build.lib.analysis.LabelAndLocation; +import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; import com.google.devtools.build.lib.analysis.RuleContext; +import com.google.devtools.build.lib.analysis.Runfiles; +import com.google.devtools.build.lib.analysis.RunfilesProvider; +import com.google.devtools.build.lib.analysis.RunfilesSupport; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget; import com.google.devtools.build.lib.analysis.constraints.EnvironmentCollection.EnvironmentWithGroup; 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.PlatformProviderUtils; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; +import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.BuildType; @@ -40,6 +54,9 @@ import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.packages.Type.LabelClass; import com.google.devtools.build.lib.packages.Type.LabelVisitor; +import com.google.devtools.build.lib.server.FailureDetails.FailAction.Code; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; +import com.google.devtools.build.lib.util.OrderedSetMultimap; import java.util.Collection; import java.util.HashMap; import java.util.LinkedHashSet; @@ -808,4 +825,147 @@ private static void addSelectValuesToSet(BuildType.Selector select, final Set type.visitLabels(visitor, value, /*context=*/ null); } } + + /** + * Creates an incompatible {@link ConfiguredTarget} if the corresponding rule is incompatible. + * + *

Returns null if the target is not incompatible. + * + *

"Incompatible" here means either: + * + *

    + *
  1. the corresponding target_compatible_with list contains a constraint that the + * current platform doesn't satisfy, or + *
  2. a transitive dependency is incompatible. + *
+ * + * @param ruleContext analysis context for the rule + * @param prerequisiteMap the dependencies of the rule + * @throws ActionConflictException if the underlying {@link RuleConfiguredTargetBuilder} + * encounters a problem when assembling a dummy action for the incompatible {@link + * ConfiguredTarget}. + */ + @Nullable + public static ConfiguredTarget incompatibleConfiguredTarget( + RuleContext ruleContext, + OrderedSetMultimap prerequisiteMap) + throws ActionConflictException { + if ("toolchain".equals(ruleContext.getRule().getRuleClass())) { + return null; + } + + // This is incompatible if explicitly specified to be. + if (ruleContext.attributes().has("target_compatible_with")) { + Iterable constraintValues = + PlatformProviderUtils.constraintValues( + ruleContext.getPrerequisites("target_compatible_with")); + for (ConstraintValueInfo constraintValue : constraintValues) { + if (!ruleContext.targetPlatformHasConstraint(constraintValue)) { + return createIncompatibleConfiguredTarget(ruleContext, null, constraintValue); + } + } + } + + // 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) { + return createIncompatibleConfiguredTarget(ruleContext, dependency, null); + } + } + + return null; + } + + /** + * A helper function for incompatibleConfiguredTarget() to actually create the incompatible {@link + * ConfiguredTarget}. + * + * @param ruleContext analysis context for the rule + * @param targetResponsibleForIncompatibility the target that is responsible this target's + * incompatibility. If null, that means that target is responsible for its own + * incompatibility. I.e. it has constraints in target_compatible_with that were not satisfied + * on the target platform. This must be null if violatedConstraint is set. This must be set if + * violatedConstraint is null. + * @param violatedConstraint the constraint that the target platform doesn't satisfy. This must be + * null if targetResponsibleForIncompatibility is set. + * @throws ActionConflictException if the underlying {@link RuleConfiguredTargetBuilder} + * encounters a problem when assembling a dummy action for the incompatible {@link + * ConfiguredTarget}. + */ + private static ConfiguredTarget createIncompatibleConfiguredTarget( + RuleContext ruleContext, + ConfiguredTarget targetResponsibleForIncompatibility, + ConstraintValueInfo violatedConstraint) + throws ActionConflictException { + // Create a dummy ConfiguredTarget that has the IncompatiblePlatformProvider set. + ImmutableList outputArtifacts = ruleContext.getOutputArtifacts(); + + if (ruleContext.isTestTarget() && outputArtifacts.size() == 0) { + // Test targets require RunfilesSupport to be added to the RuleConfiguredTargetBuilder + // which needs an "executable". Create one here if none exist already. + // + // It would be ideal if we could avoid this. Currently the problem is that some rules like + // sh_test only declare an output artifact in the corresponding ConfiguredTarget factory + // function (see ShBinary.java). Since this code path here replaces the factory function rules + // like sh_test never get a chance to declare an output artifact. + // + // On top of that, the rest of the code base makes the assumption that test targets provide an + // instance RunfilesSupport. This can be seen in the TestProvider, the TestActionBuilder, and + // the RuleConfiguredTargetBuilder classes. There might be a way to break this assumption, but + // it's currently too heavily baked in to work around it more nicely than this. + // + // Theoretically, this hack shouldn't be an issue because the corresponding actions will never + // get executed. They cannot be queried either. + 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); + + if (targetResponsibleForIncompatibility != null) { + builder.add( + IncompatiblePlatformProvider.class, + IncompatiblePlatformProvider.incompatibleDueToTarget( + targetResponsibleForIncompatibility)); + } else { + builder.add( + IncompatiblePlatformProvider.class, + IncompatiblePlatformProvider.incompatibleDueToConstraint(violatedConstraint)); + } + + 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.", + Code.CANT_BUILD_INCOMPATIBLE_TARGET)); + } + return builder.build(); + } } 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 f8ea49fd1043a4..b1bc03b25c3c77 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 @@ -14,6 +14,7 @@ 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.Verify; import com.google.common.collect.ArrayListMultimap; @@ -21,6 +22,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; @@ -60,6 +62,21 @@ public class TopLevelConstraintSemantics { private final Function configurationProvider; private final ExtendedEventHandler eventHandler; + /** Targets that have additional restrictions based on the current platform. */ + public class PlatformRestrictionsResult { + /** Targets that need be skipped. */ + public ImmutableSet targetsToSkip; + /** Targets that should be skipped, but were explicitly requested on the command line. */ + public ImmutableSet targetsWithErrors; + + public PlatformRestrictionsResult( + ImmutableSet targetsToSkip, + ImmutableSet targetsWithErrors) { + this.targetsToSkip = targetsToSkip; + this.targetsWithErrors = targetsWithErrors; + } + }; + /** * Constructor with helper classes for loading targets. * @@ -88,6 +105,108 @@ private MissingEnvironment(Label environment, RemovedEnvironmentCulprit culprit) } } + /** + * Checks that the all top-level targets are compatible with the target platform. + * + *

If any target doesn't support the target platform it will be either marked as "to be + * skipped" or marked as "errored". + * + *

Targets that are incompatible with the target platform and are not explicitly requested on + * the command line should be skipped. + * + *

Targets that are incompatible with the target platform and *are* explicitly requested on the + * command line are errored. Having one or more errored targets will cause the entire build to + * fail with an error message. + * + * @param topLevelTargets the build's top-level targets + * @param explicitTargetPatterns the set of explicit target patterns specified by the user on the + * command line. + * @return the set of to-be-skipped and errored top-level targets. + * @throws ViewCreationFailedException if any top-level target was explicitly requested on the + * command line. + */ + public PlatformRestrictionsResult checkPlatformRestrictions( + ImmutableList topLevelTargets, + ImmutableSet explicitTargetPatterns, + boolean keepGoing) + throws ViewCreationFailedException { + ImmutableSet.Builder incompatibleTargets = ImmutableSet.builder(); + ImmutableSet.Builder incompatibleButRequestedTargets = ImmutableSet.builder(); + + for (ConfiguredTarget target : topLevelTargets) { + IncompatiblePlatformProvider incompatibleProvider = + target.getProvider(IncompatiblePlatformProvider.class); + + // Move on to the next target if this one is compatible. + if (incompatibleProvider == null) { + continue; + } + + // We need the label in unambiguous form here. I.e. with the "@" prefix for targets in the + // main repository. Otherwise we risk comparing two unrelated targets. + String labelString = target.getLabel().getUnambiguousCanonicalForm(); + if (explicitTargetPatterns.contains(labelString)) { + if (!keepGoing) { + // Use the slightly simpler form for printing error messages. I.e. no "@" prefix for + // targets in the main repository. + String targetIncompatibleMessage = + ("Target " + + target.getLabel().toString() + + " is incompatible and cannot be built, but was explicitly requested."); + targetIncompatibleMessage += reportOnIncompatibility(target); + throw new ViewCreationFailedException( + targetIncompatibleMessage, + FailureDetail.newBuilder() + .setMessage(targetIncompatibleMessage) + .setAnalysis(Analysis.newBuilder().setCode(Code.INCOMPATIBLE_TARGET_REQUESTED)) + .build()); + } + this.eventHandler.handle( + Event.warn( + "Incompatible target " + + labelString + + " was explicitly requested: it will not be built")); + incompatibleButRequestedTargets.add(target); + } else { + // If this is not an explicitly requested target we can safely skip it. + incompatibleTargets.add(target); + } + } + + return new PlatformRestrictionsResult( + ImmutableSet.copyOf(incompatibleTargets.build()), + ImmutableSet.copyOf(incompatibleButRequestedTargets.build())); + } + + /** + * Assembles the explanation for a platform incompatibility. + * + *

This is useful when trying to explain to the user why an explicitly requested target on the + * command line is considered incompatible. The goal is to print out the dependency chain and the + * constraint that wasn't satisfied so that the user can immediately figure out what happened. + * + * @param target the incompatible target that was explicitly requested on the command line. + * @return the verbose error message to show to the user. + */ + private static String reportOnIncompatibility(ConfiguredTarget target) { + Preconditions.checkNotNull(target); + + String message = "\nDependency chain:"; + IncompatiblePlatformProvider provider = null; + + while (target != null) { + message += "\n " + target.getLabel().toString(); + provider = target.getProvider(IncompatiblePlatformProvider.class); + target = provider.getTargetResponsibleForIncompatibility(); + } + + message += + " <-- target platform didn't satisfy constraint " + + provider.getConstraintResponsibleForIncompatibility().label().toString(); + + return message; + } + /** * 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/buildtool/AnalysisPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java index b2b0c432166489..6270462693108d 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 @@ -34,6 +34,7 @@ import com.google.devtools.build.lib.buildtool.buildevent.TestFilteringCompleteEvent; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.TargetParsingException; +import com.google.devtools.build.lib.cmdline.TargetPattern; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.pkgcache.LoadingFailedException; @@ -54,6 +55,7 @@ import com.google.devtools.build.lib.util.RegexFilter; import com.google.devtools.common.options.OptionsParsingException; import java.util.Collection; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -203,6 +205,9 @@ private static AnalysisResult runAnalysisPhase( Stopwatch timer = Stopwatch.createStarted(); env.getReporter().handle(Event.progress("Loading complete. Analyzing...")); + ImmutableSet explicitTargetPatterns = + getExplicitTargetPatterns(env, request.getTargets()); + BuildView view = new BuildView( env.getDirectories(), @@ -214,6 +219,7 @@ private static AnalysisResult runAnalysisPhase( loadingResult, targetOptions, multiCpu, + explicitTargetPatterns, request.getAspects(), request.getViewOptions(), request.getKeepGoing(), @@ -259,6 +265,7 @@ private static AnalysisResult runAnalysisPhase( new TestFilteringCompleteEvent( analysisResult.getTargetsToBuild(), analysisResult.getTargetsToTest(), + analysisResult.getTargetsToSkip(), configurationMap)); return analysisResult; } @@ -294,4 +301,49 @@ private static void reportTargets(CommandEnvironment env, AnalysisResult analysi "Found " + targetCount + (targetCount == 1 ? " target..." : " targets..."))); } } + + /** + * Turns target patterns from the command line into parsed equivalents for single targets. + * + *

Globbing targets like ":all" and "..." are ignored here and will not be in the returned set. + * + * @param env the action's environment. + * @param requestedTargetPatterns the list of target patterns specified on the command line. + * @return the set of stringified labels of target patterns that represent single targets. The + * stringified labels are in the "unambiguous canonical form". + * @throws ViewCreationFailedException if a pattern fails to parse for some reason. + */ + private static ImmutableSet getExplicitTargetPatterns( + CommandEnvironment env, List requestedTargetPatterns) + throws ViewCreationFailedException { + ImmutableSet.Builder explicitTargetPatterns = ImmutableSet.builder(); + TargetPattern.Parser parser = new TargetPattern.Parser(env.getRelativeWorkingDirectory()); + + for (String requestedTargetPattern : requestedTargetPatterns) { + if (requestedTargetPattern.startsWith("-")) { + // Excluded patterns are by definition not explicitly requested so we can move on to the + // next target pattern. + continue; + } + + // Parse the pattern. This should always work because this is at least the second time we're + // doing it. The previous time is in runAnalysisPhase(). Still, if parsing does fail we + // propagate the exception up. + TargetPattern parsedPattern; + try { + parsedPattern = parser.parse(requestedTargetPattern); + } catch (TargetParsingException e) { + throw new ViewCreationFailedException( + "Failed to parse target pattern even though it was previously parsed successfully", + e.getDetailedExitCode().getFailureDetail(), + e); + } + + if (parsedPattern.getType() == TargetPattern.Type.SINGLE_TARGET) { + explicitTargetPatterns.add(parsedPattern.getSingleTargetPath()); + } + } + + return ImmutableSet.copyOf(explicitTargetPatterns.build()); + } } 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/buildtool/buildevent/TestFilteringCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/TestFilteringCompleteEvent.java index d8e8876493a186..459ba630d9ac56 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/TestFilteringCompleteEvent.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/TestFilteringCompleteEvent.java @@ -27,13 +27,14 @@ /** * This event is fired after test filtering. * - * The test filtering phase always expands test_suite rules, so - * the set of active targets should never contain test_suites. + *

The test filtering phase always expands test_suite rules, so the set of active targets should + * never contain test_suites. */ @Immutable public class TestFilteringCompleteEvent { private final Collection targets; private final Collection testTargets; + private final Collection skippedTests; private final Map configurationMap; /** @@ -41,14 +42,17 @@ public class TestFilteringCompleteEvent { * * @param targets The set of active targets that remain. * @param testTargets The collection of tests to be run. May be null. + * @param targetsToSkip The collection of tests that are to be skipped. * @param configurationMap A map from configuration keys of all targets to the configurations. */ public TestFilteringCompleteEvent( Collection targets, Collection testTargets, + Collection targetsToSkip, Map configurationMap) { this.targets = ImmutableList.copyOf(targets); this.testTargets = testTargets == null ? null : ImmutableList.copyOf(testTargets); + this.skippedTests = ImmutableList.copyOf(targetsToSkip); this.configurationMap = configurationMap; if (testTargets == null) { return; @@ -74,6 +78,11 @@ public Collection getTestTargets() { return testTargets; } + /** @return The set of tests that should be skipped. */ + public Collection getSkippedTests() { + return skippedTests; + } + public BuildConfiguration getConfigurationForTarget(ConfiguredTarget target) { return Preconditions.checkNotNull(configurationMap.get(target.getConfigurationKey())); } 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 715d6baf7aaa0a..5890fc2e5707cb 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,12 @@ public RuleErrorException(String message, Throwable cause) { */ public static final String COMPATIBLE_ENVIRONMENT_ATTR = "compatible_with"; + /** + * For Bazel's constraint system: the attribute that declares the list of constraints that the + * target must satisfy to be considered compatible. + */ + 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}. @@ -1410,6 +1416,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..63d8c5af0fe58c 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 @@ -110,7 +110,11 @@ public void populateTests(TestFilteringCompleteEvent event) { continue; } TestResultAggregator aggregator = - new TestResultAggregator(target, event.getConfigurationForTarget(target), policy); + new TestResultAggregator( + target, + event.getConfigurationForTarget(target), + policy, + event.getSkippedTests().contains(target)); TestResultAggregator oldAggregator = aggregators.put(asKey(target), aggregator); Preconditions.checkState( oldAggregator == null, "target: %s, values: %s %s", target, oldAggregator, aggregator); @@ -140,21 +144,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 +187,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/TerminalTestResultNotifier.java b/src/main/java/com/google/devtools/build/lib/runtime/TerminalTestResultNotifier.java index c51819432c1311..b72ca55b808896 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 @@ -317,7 +317,9 @@ private void printStats(TestResultStats stats) { 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", + 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..6c6452270a30c9 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 @@ -70,7 +70,10 @@ static final class AggregationPolicy { private final Map statusMap = new HashMap<>(); public TestResultAggregator( - ConfiguredTarget target, BuildConfiguration configuration, AggregationPolicy policy) { + ConfiguredTarget target, + BuildConfiguration configuration, + AggregationPolicy policy, + boolean skippedThisTest) { this.testTarget = target; this.policy = policy; @@ -83,6 +86,7 @@ public TestResultAggregator( this.summary.setConfiguration(configuration); } this.summary.setStatus(BlazeTestStatus.NO_STATUS); + this.summary.setSkipped(skippedThisTest); this.remainingRuns = new HashSet<>(TestProvider.getTestStatusArtifacts(target)); } @@ -150,6 +154,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.NO_STATUS); + + // 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/TestSummary.java b/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java index 6b2845ad79f635..fe0a79cef5b469 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java @@ -133,6 +133,12 @@ public Builder setStatus(BlazeTestStatus status) { return this; } + public Builder setSkipped(boolean skipped) { + checkMutation(skipped); + summary.skipped = skipped; + return this; + } + public Builder addCoverageFiles(List coverageFiles) { checkMutation(coverageFiles); summary.coverageFiles.addAll(coverageFiles); @@ -342,6 +348,7 @@ private void makeSummaryImmutable() { private ConfiguredTarget target; private BuildConfiguration configuration; private BlazeTestStatus status; + private boolean skipped; // Currently only populated if --runs_per_test_detects_flakes is enabled. private Multimap shardRunStatuses = ArrayListMultimap.create(); private int numCached; @@ -389,6 +396,10 @@ public BlazeTestStatus getStatus() { return status; } + public boolean isSkipped() { + return skipped; + } + /** * Whether or not any results associated with this test were cached locally or remotely. * @@ -525,7 +536,10 @@ public long getLastStopTimeMillis() { return lastStopTimeMillis; } - static Mode getStatusMode(BlazeTestStatus status) { + Mode getStatusMode() { + if (skipped) { + return Mode.WARNING; + } return status == BlazeTestStatus.PASSED ? Mode.INFO : (status == BlazeTestStatus.FLAKY ? Mode.WARNING : Mode.ERROR); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TestSummaryPrinter.java b/src/main/java/com/google/devtools/build/lib/runtime/TestSummaryPrinter.java index ce7052a1cc5032..adb74cefad0747 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/TestSummaryPrinter.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/TestSummaryPrinter.java @@ -60,8 +60,16 @@ public static void printCachedOutput( for (Path path : summary.getPassedLogs()) { allLogs.add(testLogPathFormatter.getPathStringToPrint(path)); } - printer.printLn("" + TestSummary.getStatusMode(summary.getStatus()) + summary.getStatus() + ": " - + Mode.DEFAULT + testName + " (see " + Joiner.on(' ').join(allLogs) + ")"); + printer.printLn( + "" + + summary.getStatusMode() + + summary.getStatus() + + ": " + + Mode.DEFAULT + + testName + + " (see " + + Joiner.on(' ').join(allLogs) + + ")"); printer.printLn(Mode.INFO + "INFO: " + Mode.DEFAULT + "From Testing " + testName); // Whether to output the target at all was checked by the caller. @@ -90,12 +98,18 @@ public static void printCachedOutput( } } - private static String statusString(BlazeTestStatus status) { - return status.toString().replace('_', ' '); + private static String statusString(TestSummary summary) { + if (summary.isSkipped()) { + // If the test was skipped then its status will be something like NO_STATUS. That's not + // informative enough to a user. Instead, return "SKIPPED" for skipped tests. + return "SKIPPED"; + } + return summary.getStatus().toString().replace('_', ' '); } /** * Prints summary status for a single test. + * * @param terminalPrinter The printer to print to */ public static void print( @@ -130,15 +144,19 @@ public static void print( || status == BlazeTestStatus.BLAZE_HALTED_BEFORE_TESTING) { return; } - String message = getCacheMessage(summary) + statusString(summary.getStatus()); + String message = getCacheMessage(summary) + statusString(summary); String targetName = summary.getLabel().toString(); if (withConfigurationName) { targetName += " (" + summary.getConfiguration().getMnemonic() + ")"; } terminalPrinter.print( Strings.padEnd(targetName, 78 - message.length(), ' ') - + " " + TestSummary.getStatusMode(summary.getStatus()) + message + Mode.DEFAULT - + (verboseSummary ? getAttemptSummary(summary) + getTimeSummary(summary) : "") + "\n"); + + " " + + summary.getStatusMode() + + message + + Mode.DEFAULT + + (verboseSummary ? getAttemptSummary(summary) + getTimeSummary(summary) : "") + + "\n"); if (printFailedTestCases && summary.getStatus() == BlazeTestStatus.FAILED) { if (summary.getFailedTestCasesStatus() == FailedTestCasesStatus.NOT_AVAILABLE) { 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 16261b97be986b..a5dc03710d5bc5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -254,6 +254,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/failure_details.proto b/src/main/protobuf/failure_details.proto index 698c222930ed12..07b9a563897caf 100644 --- a/src/main/protobuf/failure_details.proto +++ b/src/main/protobuf/failure_details.proto @@ -1040,6 +1040,7 @@ message FailAction { INCORRECT_TOOLCHAIN = 6 [(metadata) = { exit_code: 1 }]; FRAGMENT_CLASS_MISSING = 7 [(metadata) = { exit_code: 1 }]; reserved 8, 9; // For internal use + CANT_BUILD_INCOMPATIBLE_TARGET = 10 [(metadata) = { exit_code: 1 }]; } Code code = 1; @@ -1135,6 +1136,7 @@ message Analysis { INVALID_EXECUTION_PLATFORM = 16 [(metadata) = { exit_code: 1 }]; ASPECT_CREATION_FAILED = 17 [(metadata) = { exit_code: 1 }]; CONFIGURED_VALUE_CREATION_FAILED = 18 [(metadata) = { exit_code: 1 }]; + INCOMPATIBLE_TARGET_REQUESTED = 19 [(metadata) = { exit_code: 1}]; } Code code = 1; diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java index 797540b62c21b2..5b87ff938fe722 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMultiset; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.eventbus.EventBus; @@ -334,12 +335,14 @@ protected final void ensureUpdateWasCalled() { Preconditions.checkState(analysisResult != null, "You must run update() first!"); } - /** - * Update the BuildView: syncs the package cache; loads and analyzes the given labels. - */ + /** Update the BuildView: syncs the package cache; loads and analyzes the given labels. */ protected AnalysisResult update( - EventBus eventBus, FlagBuilder config, ImmutableList aspects, String... labels) - throws Exception { + EventBus eventBus, + FlagBuilder config, + ImmutableSet explicitTargetPatterns, + ImmutableList aspects, + String... labels) + throws Exception { Set flags = config.flags; LoadingOptions loadingOptions = optionsParser.getOptions(LoadingOptions.class); @@ -393,6 +396,7 @@ protected AnalysisResult update( loadingResult, buildOptions, multiCpu, + explicitTargetPatterns, aspects, viewOptions, keepGoing, @@ -408,9 +412,16 @@ protected AnalysisResult update( return analysisResult; } + protected AnalysisResult update( + EventBus eventBus, FlagBuilder config, ImmutableList aspects, String... labels) + throws Exception { + return update( + eventBus, config, /*explicitTargetPatterns=*/ ImmutableSet.of(), aspects, labels); + } + protected AnalysisResult update(EventBus eventBus, FlagBuilder config, String... labels) throws Exception { - return update(eventBus, config, /*aspects=*/ImmutableList.of(), labels); + return update(eventBus, config, /*aspects=*/ ImmutableList.of(), labels); } protected AnalysisResult update(FlagBuilder config, String... labels) throws Exception { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java index 8eb219e0bbc5e9..8154db43347f0f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java @@ -150,6 +150,7 @@ public AnalysisResult update( TargetPatternPhaseValue loadingResult, BuildOptions targetOptions, Set multiCpu, + ImmutableSet explicitTargetPatterns, List aspects, AnalysisOptions viewOptions, boolean keepGoing, @@ -162,6 +163,7 @@ public AnalysisResult update( loadingResult, targetOptions, multiCpu, + explicitTargetPatterns, aspects, viewOptions, keepGoing, diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index 7ab6d4db474d7b..b9cda2d6d8e1d0 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -1905,17 +1905,26 @@ protected void useLoadingOptions(String... options) throws OptionsParsingExcepti customLoadingOptions = Options.parse(LoadingOptions.class, options).getOptions(); } - protected AnalysisResult update(List targets, + protected AnalysisResult update( + List targets, boolean keepGoing, int loadingPhaseThreads, boolean doAnalysis, - EventBus eventBus) throws Exception { + EventBus eventBus) + throws Exception { return update( - targets, ImmutableList.of(), keepGoing, loadingPhaseThreads, doAnalysis, eventBus); + targets, + ImmutableSet.of(), + ImmutableList.of(), + keepGoing, + loadingPhaseThreads, + doAnalysis, + eventBus); } protected AnalysisResult update( List targets, + ImmutableSet explicitTargetPatterns, List aspects, boolean keepGoing, int loadingPhaseThreads, @@ -1947,6 +1956,7 @@ protected AnalysisResult update( loadingResult, targetConfig.getOptions(), /* multiCpu= */ ImmutableSet.of(), + explicitTargetPatterns, aspects, viewOptions, keepGoing, diff --git a/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java b/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java index 68719d359686a1..a582a9e3668582 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java @@ -64,7 +64,8 @@ public final void createMocks() throws Exception { new AggregationPolicy( new EventBus(), /*testCheckUpToDate=*/ false, - /*testVerboseTimeoutWarnings=*/ false)); + /*testVerboseTimeoutWarnings=*/ false), + /*skippedThisTest=*/ false); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java index de47e1dacb6c3a..0e25a180e63066 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java @@ -633,6 +633,7 @@ public void testGetRule() throws Exception { "generator_location", "features", "compatible_with", + "target_compatible_with", "restricted_to", "srcs", "tools", diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index 8ee8e245d03e00..e902fe6eba8769 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -1312,6 +1312,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..22dbdc941cde99 --- /dev/null +++ b/src/test/shell/bazel/target_compatible_with_test.sh @@ -0,0 +1,696 @@ +#!/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 + +if "$is_windows"; then + export MSYS_NO_PATHCONV=1 + export MSYS2_ARG_CONV_EXCL="*" +fi + +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" +# We're not validating visibility here. Let everything access these targets. +package(default_visibility = ["//visibility:public"]) + +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", +) + +constraint_setting(name = "bar_version") + +constraint_value( + name = "bar1", + constraint_setting = "bar_version", +) + +constraint_value( + name = "bar2", + constraint_setting = "bar_version", +) + +platform( + name = "foo1_bar1_platform", + parents = ["@local_config_platform//:host"], + constraint_values = [ + ":foo1", + ":bar1", + ], +) + +platform( + name = "foo2_bar1_platform", + parents = ["@local_config_platform//:host"], + constraint_values = [ + ":foo2", + ":bar1", + ], +) + +platform( + name = "foo1_bar2_platform", + parents = ["@local_config_platform//:host"], + constraint_values = [ + ":foo1", + ":bar2", + ], +) + +platform( + name = "foo3_platform", + parents = ["@local_config_platform//:host"], + constraint_values = [ + ":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 tear_down() { + bazel shutdown || : +} + +# Validates that the console log provides useful information to the user for +# builds. +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 + 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' +} + +# Validates that the console log provides useful information to the user for +# tests. +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 + 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 + 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 + 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' +} + +# Validates that incompatible target skipping errors behave nicely with +# --keep_going. In other words, even if there's an error in the target skipping +# (e.g. because the user explicitly requested an incompatible target) we still +# want bazel to finish the rest of the build when --keep_going is specified. +function test_failure_on_incompatible_top_level_target() { + cd target_skipping || fail "couldn't cd into workspace" + + # Validate a variety of ways to refer to the same target. + local -r -a incompatible_targets=( + :pass_on_foo1_bar2 + //:pass_on_foo1_bar2 + @//:pass_on_foo1_bar2 + ) + + for incompatible_target in "${incompatible_targets[@]}"; do + echo "Testing ${incompatible_target}" + + if bazel test --show_result=10 --host_platform=@//:foo1_bar1_platform \ + --platforms=@//:foo1_bar1_platform "${incompatible_target}" \ + --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.' + done + + # Run an additional (passing) test and make sure we still fail the build. + # This is intended to validate that --keep_going works as expected. + 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' +} + +# This is basically the same as test_failure_on_incompatible_top_level_target +# above, but with targets in an external repo. +function test_failure_on_incompatible_top_level_target_in_external_repo() { + cat >> target_skipping/WORKSPACE < target_skipping/third_party/test_repo/BUILD < target_skipping/third_party/test_repo/bin.cc < "${TEST_log}"; then + fail "Bazel passed unexpectedly." + fi + + expect_log 'ERROR: Target @test_repo//:bin 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 @test_repo//:bin is incompatible and cannot be built.' +} + +# Crudely validates that the build event protocol contains useful information +# when targets are skipped due to incompatibilities. +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 + 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\.' +} + +# Validates that incompatibilities are transitive. I.e. targets that depend on +# incompatible targets are themselves deemed incompatible and should therefore +# not be built. +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' +} + +# Validate that targets are skipped when the implementation is in Starlark +# instead of in Java. +function test_starlark_skipping() { + cat >> target_skipping/rules.bzl <> target_skipping/BUILD < "${TEST_log}"; then + fail "Bazel passed unexpectedly." + fi + expect_log 'ERROR: Target //:hello_world_bin is incompatible and cannot be built, but was explicitly requested' + expect_log 'FAILED: Build did NOT complete successfully' +} + +# Validates the same thing as test_non_top_level_skipping, but with a cc_test +# and adding one more level of dependencies. +function test_cc_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' + + # Validate that we get the dependency chain printed out. + expect_log '^Dependency chain:$' + expect_log '^ //:generated_test$' + expect_log '^ //:generate_with_tool$' + expect_log "^ //:generator_tool <-- target platform didn't satisfy constraint //:foo1$" + expect_log 'FAILED: Build did NOT complete successfully' +} + +# Validates that we can express targets being compatible with A _or_ B. +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' +} + +# Validates that we can express targets being compatible with everything _but_ +# A and B. +function test_inverse_logic() { + (cd target_skipping && setup_skylib_support) + + cat >> target_skipping/BUILD < "${TEST_log}"; then + fail "Bazel passed unexpectedly." + fi + expect_log 'ERROR: Target //:pass_on_everything_but_foo1_and_foo2 is incompatible and cannot be built, but was explicitly requested' + expect_log 'FAILED: Build did NOT complete successfully' + + # Try with :foo2. This should fail. + if bazel test --show_result=10 --host_platform=@//:foo2_bar1_platform \ + --platforms=@//:foo2_bar1_platform \ + //:pass_on_everything_but_foo1_and_foo2 \ + &> "${TEST_log}"; then + fail "Bazel passed unexpectedly." + fi + expect_log 'ERROR: Target //:pass_on_everything_but_foo1_and_foo2 is incompatible and cannot be built, but was explicitly requested' + expect_log 'FAILED: Build did NOT complete successfully' + + # Now with :foo3. This should pass. + if ! bazel test --show_result=10 --host_platform=@//:foo3_platform \ + --platforms=@//:foo3_platform //:pass_on_everything_but_foo1_and_foo2 \ + --nocache_test_results &> "${TEST_log}"; then + fail "Bazel failed unexpectedly." + fi + expect_log '^//:pass_on_everything_but_foo1_and_foo2 * PASSED in' +} + +# Validates that a tool compatible with the host platform, but incompatible +# with the target platform can still be used as a host tool. +function test_host_tool() { + # Create an arbitrary host tool. + cat > target_skipping/host_tool.cc < +int main() { + printf("Hello World\\n"); + return 0; +} +EOT + + cat >> target_skipping/BUILD < "${TEST_log}"; then + fail "Bazel passed unexpectedly." + fi + expect_log 'ERROR: Target //:host_tool is incompatible and cannot be built, but was explicitly requested' + expect_log 'FAILED: Build did NOT complete successfully' + + # Run with :foo1 in the host platform, but with :foo2 in the target platform. + # This should work fine because we're not asking for any constraints to be + # violated. + if ! bazel build --host_platform=@//:foo1_bar2_platform \ + --platforms=@//:foo2_bar1_platform //:host_tool_message.txt \ + &> "${TEST_log}"; then + fail "Bazel failed unexpectedly." + fi + expect_log ' bazel-bin/host_tool_message.txt$' + expect_log ' Build completed successfully, ' + + # Make sure that the contents of the file are what we expect. + cp bazel-bin/host_tool_message.txt "${TEST_log}" + expect_log '^Hello World$' +} + +# Validates that incompatible targets provide appropriate errors when queried +# explicitly or skipped when queried via a glob. +function test_queries() { + cat >> target_skipping/BUILD < "${TEST_log}"; then + fail "Bazel query failed unexpectedly." + fi + expect_log '^//:sh_foo1$' + expect_log '^//:genrule_foo1$' + + # Run a cquery on a target that is compatible. This should pass. + if ! bazel cquery --host_platform=@//:foo3_platform \ + --platforms=@//:foo3_platform \ + 'deps(//:some_foo3_target)' &> "${TEST_log}"; then + fail "Bazel cquery failed unexpectedly." + fi + + # Run a cquery with a glob. This should only pick up compatible targets and + # should therefore pass. + if ! bazel cquery --host_platform=@//:foo3_platform \ + --platforms=@//:foo3_platform 'deps(//...)' &> "${TEST_log}"; then + fail "Bazel cquery failed unexpectedly." + fi + expect_log '^//:some_foo3_target ' + expect_log '^//:sh_foo1 ' + expect_log '^//:genrule_foo1 ' + + # Run a cquery on an incompatible target. This should fail. + if bazel cquery --host_platform=@//:foo3_platform \ + --platforms=@//:foo3_platform \ + 'deps(//:sh_foo1)' &> "${TEST_log}"; then + fail "Bazel cquery passed unexpectedly." + fi + expect_log 'Target //:sh_foo1 is incompatible and cannot be built, but was explicitly requested' + expect_log "target platform didn't satisfy constraint //:foo1" + + # Run an aquery on a target that is compatible. This should pass. + if ! bazel aquery --host_platform=@//:foo3_platform \ + --platforms=@//:foo3_platform '//:some_foo3_target' &> "${TEST_log}"; then + fail "Bazel aquery failed unexpectedly." + fi + + # Run an aquery with a glob. This should only pick up compatible targets and + # should therefore pass. + if ! bazel aquery --host_platform=@//:foo3_platform \ + --platforms=@//:foo3_platform '//...' &> "${TEST_log}"; then + fail "Bazel aquery failed unexpectedly." + fi + + # Run an aquery on an incompatible target. This should fail. + if bazel aquery --host_platform=@//:foo3_platform \ + --platforms=@//:foo3_platform '//:sh_foo1' &> "${TEST_log}"; then + fail "Bazel aquery passed unexpectedly." + fi + expect_log 'Target //:sh_foo1 is incompatible and cannot be built, but was explicitly requested' + expect_log "target platform didn't satisfy constraint //:foo1" +} + +run_suite "target_compatible_with tests" diff --git a/third_party/bazel_toolchains/0001-Rename-target_compatible_with-to-internal_target_com.patch b/third_party/bazel_toolchains/0001-Rename-target_compatible_with-to-internal_target_com.patch new file mode 100644 index 00000000000000..c0960e9149b1d1 --- /dev/null +++ b/third_party/bazel_toolchains/0001-Rename-target_compatible_with-to-internal_target_com.patch @@ -0,0 +1,52 @@ +From 9141b40f4f155084eae7f3674ac1271f9552c4a9 Mon Sep 17 00:00:00 2001 +From: Philipp Schrader +Date: Sun, 13 Sep 2020 20:23:02 -0700 +Subject: [PATCH] Rename target_compatible_with to + internal_target_compatible_with in _rbe_config + +This is a backport of +https://github.com/bazelbuild/bazel-toolchains/pull/913/ to 3.1.0 +which is what's currently used in bazel. +--- + rules/rbe_repo.bzl | 4 ++-- + rules/rbe_repo/build_gen.bzl | 2 +- + 2 files changed, 3 insertions(+), 3 deletions(-) + +diff --git a/rules/rbe_repo.bzl b/rules/rbe_repo.bzl +index a386010..faec7e6 100644 +--- a/rules/rbe_repo.bzl ++++ b/rules/rbe_repo.bzl +@@ -817,7 +817,7 @@ _rbe_autoconfig = repository_rule( + "tag": attr.string( + doc = ("Optional. The tag of the image to pull, e.g. latest."), + ), +- "target_compatible_with": attr.string_list( ++ "internal_target_compatible_with": attr.string_list( + default = _RBE_UBUNTU_TARGET_COMPAT_WITH, + doc = ("The list of constraints that will be added to the " + + "toolchain in its target_compatible_with attribute. For " + +@@ -1174,7 +1174,7 @@ def rbe_autoconfig( + registry = registry, + repository = repository, + tag = tag, +- target_compatible_with = target_compatible_with, ++ internal_target_compatible_with = target_compatible_with, + use_checked_in_confs = use_checked_in_confs, + use_legacy_platform_definition = use_legacy_platform_definition, + ) +diff --git a/rules/rbe_repo/build_gen.bzl b/rules/rbe_repo/build_gen.bzl +index a4902cd..6aefd49 100644 +--- a/rules/rbe_repo/build_gen.bzl ++++ b/rules/rbe_repo/build_gen.bzl +@@ -165,7 +165,7 @@ def _create_platform(ctx, exec_properties, image_name, name, cc_toolchain_target + ("\",\n \"").join(ctx.attr.exec_compatible_with) + + "\",") + target_compatible_with = ("\"" + +- ("\",\n \"").join(ctx.attr.target_compatible_with) + ++ ("\",\n \"").join(ctx.attr.internal_target_compatible_with) + + "\",") + + platform_exec_properties = create_rbe_exec_properties_dict( +-- +2.20.1 +