Skip to content

Commit

Permalink
Deduplicate target_compatible_with labels
Browse files Browse the repository at this point in the history
The latest refactor unintentionally made it so you can list the same
constraint multiple times in the `target_compatible_with` attribute.

It was unintentional, but actually greatly simplifies a discussion
point on bazelbuild/bazel-skylib#381. That skylib patch aims to make
it easier to write non-trivial `target_compatible_with` expressions.
For example, to express that something is compatible with everything
except for Windows, one can use the following:

    foo_binary(
        name = "bin",
        target_compatible_with = select({
            "@platforms//os:windows": ["@platforms//:incomaptible"],
            "//conditions:default: [],
        }),
    )

The skylib patch aims to reduce that to:

    foo_binary(
        name = "bin",
        target_compatible_with = compatibility.none_of("@platforms//os:windows"),
    )

This works fine on its own, but a problem arises when these
expressions are composed. For example, a macro author might write the
following:

    def foo_wrapped_binary(name, target_compatible_with = [], **kwargs):
        foo_binary(
            name = name,
            target_compatible_with = target_compatible_with + select({
                "@platforms//os:none": ["@platforms//:incompatible"],
                "//conditions:default": [],
            }),
        )

A macro author should be able to express their own incompatibility
while also honouring user-defined incompatibility.

It turns out that my latest refactor (bazelbuild#14096) unintentionally made
this work. This happened because we now check for incompatibility
before we perform a lot of sanity checks. That's also one of the
reasons that visibility restrictions are not honoured for incomaptible
targets at the moment (bazelbuild#16044).

Without the deduplicating behaviour, macro authors are stuck with
either not allowing composition, or having to create unique
incompatible constraints for every piece in a composed
`target_compatible_with` expression.

This patch adds a test to validate the deduplicating behaviour to
cement it as a feature.

Unfortunately, this means that `target_compatible_with` behaves
differently from other label list attributes. For other label list
attributes, bazel complains when labels are duplicated. For example,
the following BUILD file:

    py_library(
        name = "lib",
    )

    py_binary(
        name = "bin",
        srcs = ["bin.py"],
        deps = [
        |   ":lib",
        |   ":lib",
        ],
    )

will result in the following error:

    $ bazel build :bin
    INFO: Invocation ID: 3d8345a4-2e76-493e-8343-c96a36185b52
    ERROR: /home/jenkins/repos/test_repo/BUILD:41:10: Label '//:lib' is duplicated in the 'deps' attribute of rule 'bin'
    ERROR: error loading package '': Package '' contains errors
    INFO: Elapsed time: 2.514s
    INFO: 0 processes.
    FAILED: Build did NOT complete successfully (1 packages loaded)
  • Loading branch information
philsc committed Sep 14, 2022
1 parent a06dca5 commit 8a10c8e
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
Expand Down Expand Up @@ -69,7 +70,14 @@ public static IncompatiblePlatformProvider incompatibleDueToConstraints(
@Nullable Label targetPlatform, ImmutableList<ConstraintValueInfo> constraints) {
Preconditions.checkNotNull(constraints);
Preconditions.checkArgument(!constraints.isEmpty());
return new AutoValue_IncompatiblePlatformProvider(targetPlatform, null, constraints);

// Deduplicate and sort the list of constraints.
ImmutableSet<ConstraintValueInfo> filteredConstraints = ImmutableSet.copyOf(constraints);
ImmutableList<ConstraintValueInfo> filteredAndSortedConstraints = ImmutableList.sortedCopyOf(
(ConstraintValueInfo a, ConstraintValueInfo b) -> b.label().compareTo(a.label()),
filteredConstraints);

return new AutoValue_IncompatiblePlatformProvider(targetPlatform, null, filteredAndSortedConstraints);
}

@Override
Expand All @@ -95,6 +103,8 @@ public boolean isImmutable() {
*
* <p>This may be null. If it is null, then {@code getTargetsResponsibleForIncompatibility()} is
* guaranteed to be non-null. It will have at least one element in it if it is not null.
*
* <p>The list is sorted based on the toString() result of each constraint.
*/
@Nullable
public abstract ImmutableList<ConstraintValueInfo> constraintsResponsibleForIncompatibility();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,12 +325,8 @@ private static String reportOnIncompatibility(ConfiguredTarget target) {

message += "s [";

// Print out a sorted list to make the output reproducible.
boolean first = true;
for (ConstraintValueInfo constraintValueInfo :
ImmutableList.sortedCopyOf(
(ConstraintValueInfo a, ConstraintValueInfo b) -> b.label().compareTo(a.label()),
provider.constraintsResponsibleForIncompatibility())) {
for (ConstraintValueInfo constraintValueInfo : provider.constraintsResponsibleForIncompatibility()) {
if (first) {
first = false;
} else {
Expand Down
47 changes: 47 additions & 0 deletions src/test/shell/integration/target_compatible_with_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,15 @@ platform(
],
)
platform(
name = "foo3_bar2_platform",
parents = ["${default_host_platform}"],
constraint_values = [
":foo3",
":bar2",
],
)
sh_test(
name = "pass_on_foo1",
srcs = ["pass.sh"],
Expand Down Expand Up @@ -879,6 +888,44 @@ EOF
expect_log '^//target_skipping:pass_on_everything_but_foo1_and_foo2 * PASSED in'
}
function test_composition() {
cat >> target_skipping/BUILD <<EOF
sh_test(
name = "pass_on_foo3_and_bar2",
srcs = [":pass.sh"],
target_compatible_with = select({
":foo3": [],
"//conditions:default": [":not_compatible"],
}) + select({
":bar2": [],
"//conditions:default": [":not_compatible"],
}),
)
EOF
cd target_skipping || fail "couldn't cd into workspace"
bazel test \
--show_result=10 \
--host_platform=@//target_skipping:foo3_bar2_platform \
--platforms=@//target_skipping:foo3_bar2_platform \
--nocache_test_results \
//target_skipping:pass_on_foo3_and_bar2 &> "${TEST_log}" \
|| fail "Bazel failed unexpectedly."
expect_log '^//target_skipping:pass_on_foo3_and_bar2 * PASSED in'
bazel test \
--show_result=10 \
--host_platform=@//target_skipping:foo1_bar1_platform \
--platforms=@//target_skipping:foo1_bar1_platform \
//target_skipping:pass_on_foo3_and_bar2 &> "${TEST_log}" \
&& fail "Bazel passed unexpectedly."
expect_log 'ERROR: Target //target_skipping:pass_on_foo3_and_bar2 is incompatible and cannot be built, but was explicitly requested'
expect_log "^ //target_skipping:pass_on_foo3_and_bar2 (.*) <-- target platform (//target_skipping:foo1_bar1_platform) didn't satisfy constraint //target_skipping:not_compatible$"
expect_log 'FAILED: Build did NOT complete successfully'
}
function test_incompatible_with_aliased_constraint() {
cat >> target_skipping/BUILD <<EOF
alias(
Expand Down

0 comments on commit 8a10c8e

Please sign in to comment.