From d052ececddf54587c576f876ce13d4b8f4aacb0b Mon Sep 17 00:00:00 2001 From: Philipp Schrader Date: Tue, 16 Mar 2021 08:11:30 -0700 Subject: [PATCH] Expand target_compatible_with to all non-workspace rules This patch makes it so the `target_compatible_with` attribute can be used with all non-workspace rules. The motivation here is to allow rules like `filegroup` and `cc_import` to also make use of incompatible target skipping. Even though these rules don't inherently generate files that are incompatible with the target platform, they can be used to provide incompatible files to other targets. Before this patch the user had to do something like this to create an incompatible pre-compiled library: cc_import( name = "foo_import", shared_library = "foo.so", ) cc_library( name = "foo", deps = [":foo_import"], target_compatible_with = ["//:foo_platform"], ) Now users can directly declare compatibility on the `cc_import` rule. cc_import( name = "foo_import", shared_library = "foo.so", target_compatible_with = ["//:foo_platform"], ) Unfortunately, This does mean that some rules like `cc_toolchain` now have a `target_compatible_with` that doesn't serve any purpose. Before the changes in `BaseRulesClasses.java` the test would error out with meesages like this: //target_skipping:some_precompiled_library: no such attribute 'target_compatible_with' in 'cc_import' rule //target_skipping:filegroup: no such attribute 'target_compatible_with' in 'filegroup' rule I also took this opportunity to update the documentation a bit. Fixes #12745 Closes #13170. PiperOrigin-RevId: 363185186 --- .../common/target_compatible_with.html | 13 +++-- .../build/lib/analysis/BaseRuleClasses.java | 12 +++-- .../RuleContextConstraintSemantics.java | 7 ++- .../target_compatible_with_test.sh | 53 +++++++++++++++++++ 4 files changed, 75 insertions(+), 10 deletions(-) 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 index 0e75da47c6437c..4557b6bff77eeb 100644 --- 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 @@ -10,9 +10,9 @@ 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 the target pattern is expanded -(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. +(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.

@@ -25,6 +25,13 @@ with all platforms.

+

+All rules other than [Workspace Rules](workspace.html) support this attribute. +For some rules this attribute has no effect. For example, specifying +target_compatible_with for a +cc_toolchain is not useful. +

+

See the Platforms 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 2797298788ae6a..f9c7e4c0cede0a 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 @@ -381,6 +381,13 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .add( attr("distribs", DISTRIBUTIONS) .nonconfigurable("Used in core loading phase logic with no access to configs")) + // Any rule that has provides its own meaning for the "target_compatible_with" attribute + // has to be excluded in `RuleContextConstraintSemantics.incompatibleConfiguredTarget()`. + .add( + attr(RuleClass.TARGET_RESTRICTED_TO_ATTR, LABEL_LIST) + .mandatoryProviders(ConstraintValueInfo.PROVIDER.id()) + // This should be configurable to allow for complex types of restrictions. + .allowedFileTypes(FileTypeSet.NO_FILE)) .build(); } @@ -441,11 +448,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .allowedFileTypes() .nonconfigurable("Used in toolchain resolution") .value(ImmutableList.of())) - .add( - attr(RuleClass.TARGET_RESTRICTED_TO_ATTR, LABEL_LIST) - .mandatoryProviders(ConstraintValueInfo.PROVIDER.id()) - // This should be configurable to allow for complex types of restrictions. - .allowedFileTypes(FileTypeSet.NO_FILE)) .build(); } 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 580a336d2b6c96..c2be9cd5251fce 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 @@ -907,8 +907,11 @@ public static ConfiguredTarget incompatibleConfiguredTarget( RuleContext ruleContext, OrderedSetMultimap prerequisiteMap) throws ActionConflictException, InterruptedException { - // The target (ruleContext) is incompatible if explicitly specified to be. - if (ruleContext.getRule().getRuleClassObject().useToolchainResolution() + // The target (ruleContext) is incompatible if explicitly specified to be. Any rule that + // provides its own meaning for the "target_compatible_with" attribute has to be excluded here. + // For example, the "toolchain" rule uses "target_compatible_with" for bazel's toolchain + // resolution. + if (!ruleContext.getRule().getRuleClass().equals("toolchain") && ruleContext.attributes().has("target_compatible_with")) { ImmutableList invalidConstraintValues = PlatformProviderUtils.constraintValues( diff --git a/src/test/shell/integration/target_compatible_with_test.sh b/src/test/shell/integration/target_compatible_with_test.sh index 375ca2a43561b9..d6e3219ef3c847 100755 --- a/src/test/shell/integration/target_compatible_with_test.sh +++ b/src/test/shell/integration/target_compatible_with_test.sh @@ -299,6 +299,59 @@ EOF expect_log 'Target //target_skipping:filegroup is incompatible and cannot be built' } +# Validates that filegroups and other rules that don't inherit from +# `NativeActionCreatingRule` can be marked with target_compatible_with. This is +# a regression test for https://github.com/bazelbuild/bazel/issues/12745. +function test_skipping_for_rules_that_dont_create_actions() { + # Create a fake shared library for cc_import. + echo > target_skipping/some_precompiled_library.so + + cat >> target_skipping/BUILD < "${TEST_log}" || fail "Bazel failed unexpectedly." + expect_log 'Target //target_skipping:some_precompiled_library was skipped' + expect_log 'Target //target_skipping:some_binary was skipped' + expect_log 'Target //target_skipping:filegroup was skipped' + + bazel build \ + --show_result=10 \ + --host_platform=@//target_skipping:foo1_bar1_platform \ + --platforms=@//target_skipping:foo1_bar1_platform \ + :filegroup &> "${TEST_log}" && fail "Bazel passed unexpectedly." + expect_log 'Target //target_skipping:filegroup is incompatible and cannot be built' +} + # 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