Add lib/compatibility.bzl

[philsc]

This patch adds new helper macros for dealing with incompatible target
skipping. Specifically, the macros help make the target_compatible_with
more readable and easier to understand.

Without these helpers you are left to write verbose select() statements.

For example, you might write the following to express a target that is
incompatible with QNX and bare-bones systems.

cc_library(
    name = "lib",
    srcs = ["lib.cc"],
    target_compatible_with = select({
        "@platforms//os:qnx": ["@platforms//:incompatible"],
        "@platforms//os:none": ["@platforms//:incompatible"],
        "//conditions:default": [],
    }),
)

WIth one of the helpers from this patch you can instead rewrite this as:

cc_library(
    name = "lib",
    srcs = ["lib.cc"],
    target_compatible_with = compatibility.none_of(
        "@platforms//os:qnx",
        "@platforms//os:none",
    ),
)

That is now easier to understand. Especially for folks new to
incompatible target skipping.

This patch is based on @trybka's trybka#1. The
implementation is his. I cleaned up the tests and added documentation.

[fmeum]

Just out of curiosity: Is there a particular reason for why target_compatible_with can't deduplicate it's value before processing it? That won't help simplify this for existing Bazel versions though.

[philsc]

I suspect that the reason is that there's some fairly generic logic somewhere that complains about duplicate labels in label_list attributes. E.g. I remember that adding duplicate deps to py_library also throws a bazel error. In other words, I suspect that this behaviour cannot be changed without affecting the same logic for all other attributes. Unless we hard-code something for target_compatible_with.

[fmeum]

True. It's pretty easy to hit this error when programmatically adding dependencies from a macro.

@gregestren Can you perhaps shed some light on why Bazel errors out rather than stably deduping the label list? AFAIK buildifier already handles non-macro duplications.

[gregestren]

@philsc and I followed up on this on another issue/PR (which was that?). I assume the historic logic is to encourage tidy BUILD curation. I ran this by the devs most concerned about API consistency and didn't hear strong opinion on keeping it this way.

[trybka]

Oh hey -- thanks for picking this up!

[philsc]

If anyone has an idea why bazel is segfaulting on shutdown in one of the Windows tests, I'd appreciate it.

[philsc]

Any of the 5 code owners interested in taking a look at this PR?

[comius]

[comius]

Ups, wrong button.

any thoughts? I am not sure what to make of the complete lack of feedback here.

Too many reviewers - each one of them has time to review, but all of them together don't have it :)

[philsc]

@comius , any thoughts?

[comius]

@comius , any thoughts?

I did a quick pass. Looks nice and looks ok, thanks. I'll do another more thorough pass and let you know.

[comius]

What happens if you replace "@bazel_skylib//lib/compatibility:all_of_{}".format(i) with "@platforms//:incompatible" in this line?

It if works, it's much simpler.

[philsc]

Same issue as #381 (comment)

EDIT: Fixed link.

[comius]

There's one alternative, which I considered worse until now. How about:

result = []
for setting in enumerate(settings):
   result += select({
      setting: result,
       "//conditions:default": ["@bazel_skylib//lib/compatibility:incompatible_in_all_of"]

I think this should evaluate to a single label when a condition is not met.

First line could also be result = settings. That might work better in cquery-es.

[philsc]

I have to admit that I am struggling to follow your code snippet. Using setting: result, inside the dictionary means that we're nesting select() statements. Which is, as far as I can tell, not allowed. bazelbuild/bazel#1623 (comment)

Bazel also gets unhappy because the types of each select() path no longer match:

Error: '+' operator applied to incompatible types (select of list, select of select)

[comius]

So any_of(foo1)+none_of(foo1) is always false, right?
What about all+none, any+all, any+any, all+all, none+none, they all work right?

[philsc]

So any_of(foo1)+none_of(foo1) is always false, right?

I have not added a test to explicitly test this, but I don't see any way that it would ever evaluate to true.

What about all+none, any+all, any+any, all+all, none+none, they all work right?

Theoretically yes. The challenge at the moment is that because of bazel's dislike of duplicated labels in label lists, we can't do, say, any+any, all+all, or none+none. I think the only way to make that work is to either:

1. Figure out how to make bazel not complain about duplicate labels in a label list, or
2. Accept a name-like parameter for these helpers that will create new constraint_value targets for that invocation.

EDIT: I'm very interested to hear what you think on this topic.

[comius]

I've got a feeling there is something fishy going on, and I'm not able to put my finger on it. There is negation without universal set and + seems to mean the same as conjunction. The feeling is you're not using the right set of primitives. But I'm failing to figure out what those should be.

For example using + for conjunction, not for single negation and any for disjunction. This way you should be able to write any boolean expression as something like = any(a,b,not(c)) + any(c,d,not(e)). But then a+b only works for constraints. Should you also have as_constraints, that wraps a config value?

I can't really tell what's more usable. So far I've seen the most places need just conjunction of constraints and some more rare places a disjunction of constraints.

[comius]

Thinking out loud: maybe opposite approach is the solution: a single function match([a,b],c,[d,e], noneof=[f,g]) which says (a or b) and c and (d or e) and (not f) and (not g).

With current situation about composition - I would be more happy to make no guarantees about it, because they seem to be quite "flaky". Maybe even better, breaking composition on purpose, by using that single incompatible constraint value.

[philsc]

The single function approach is a cool idea. At first glance, however, I think it's harder for someone to understand who hasn't read the documentation. The intent of the current functions is to make them easy to understand from a newcomer point of view:

This is compatible with any of foo1 or foo2, but not compatible with bar1

The single function approach also cannot work if we restrict ourselves to using the single incompatible value. I.e. you cannot express the and operations without risking creating duplicate labels in the label list for target_compatible_with.

That being said... it appears that with my last refactor (bazelbuild/bazel#14096) I unintentionally made it so you can duplicate labels in target_compatible_with and bazel will deal with it "as you expect". I.e. duplicated labels are effectively NOPs.

So... we could take that as a feature and I think that feature means we can use @platforms//:incompatible for everything 🤔

None of this will work until bazel 6, however, so whatever API you end up being okay with will only work with bazel 6 onwards.

I still prefer the existing API over your single-function unless we expand it to something like the following. I.e. to make it really obvious what's going on when reading a BUILD file.

target_compatible_with = compatibility.match(all_of = [
    compatibility.or(a, b),
    c,
    compatibility.or(d, e),
], none_of = [
    f,
    g,
]),

[philsc]

@comius , I spent some time implementing your match idea, but I'm not convinced that it'll be ergonomic. Starting with this:

target_compatible_with = compatibility.match([":a", ":b"]),  # This means a _or_ b.
target_compatible_with = compatibility.match(":a", ":b"),  # This means a _and_ b.

makes me think the difference is too subtle. Making it more verbose:

target_compatible_with = compatibility.match_all(  # This means a _or_ b.
    compatibility.or_group(":a", ":b"),
),
target_compatible_with = compatibility.match_all(":a", ":b"),  # This means a _and_ b.

makes it easier to understand But then we're not really saving the user much over doing:

target_compatible_with = compatibility.any_of(":a", ":b"),  # This means a _or_ b.
target_compatible_with = compatibility.all_of(":a", ":b"),  # This means a _and_ b.

With bazel HEAD, it's actually pretty easy now to compose the match([a,b],c,[d,e], noneof=[f,g]) example from earlier.

target_compatible_with = compatibility.any_of(a, b) + \
                         compatibility.all_of(c) + \
                         compatibility.any_of(d, e) + \
                         compatibility.none_of(f, g),

WDYT about the current state of things?

[aiuto]

I also think different behavior for list of conditions vs. a single set is far too subtle. People will get that wrong.
any_of and all_of are excellent names.

[philsc]

@comius , any thoughts?

I can disable the tests for pre-6.0 versions of bazel. But that would mean not running tests at all for now.

[philsc]

@comius , WDYT?

[philsc]

@comius , ping