Skip to content

Commit

Permalink
Expand target_compatible_with to all non-workspace rules
Browse files Browse the repository at this point in the history
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
  • Loading branch information
philsc authored and copybara-github committed Mar 16, 2021
1 parent ec17683 commit d052ece
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
rule type. If the target platform does not satisfy all listed constraints then
the target is considered <em>incompatible</em>. 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. <code>//...</code>, <code>:all</code>). When explicitly specified on the
command line, incompatible targets cause Bazel to print an error and cause a
build or test failure.
</p>

<p>
Expand All @@ -25,6 +25,13 @@
with all platforms.
<p>

<p>
All rules other than [Workspace Rules](workspace.html) support this attribute.
For some rules this attribute has no effect. For example, specifying
<code>target_compatible_with</code> for a
<code><a href="c-cpp.html#cc_toolchain">cc_toolchain</a></code> is not useful.
<p>

<p>
See the
<a href="../platforms.html#skipping-incompatible-targets">Platforms</a>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -907,8 +907,11 @@ public static ConfiguredTarget incompatibleConfiguredTarget(
RuleContext ruleContext,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> 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<ConstraintValueInfo> invalidConstraintValues =
PlatformProviderUtils.constraintValues(
Expand Down
53 changes: 53 additions & 0 deletions src/test/shell/integration/target_compatible_with_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 <<EOF
cc_import(
name = "some_precompiled_library",
shared_library = "some_precompiled_library.so",
target_compatible_with = [
":foo3",
],
)
cc_binary(
name = "some_binary",
deps = [
":some_precompiled_library",
],
)
filegroup(
name = "filegroup",
srcs = [
"some_precompiled_library.so",
],
target_compatible_with = [
":foo3",
],
)
EOF
cd target_skipping || fail "couldn't cd into workspace"
bazel build \
--show_result=10 \
--host_platform=@//target_skipping:foo1_bar1_platform \
--platforms=@//target_skipping:foo1_bar1_platform \
:all &> "${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
Expand Down

0 comments on commit d052ece

Please sign in to comment.