Inheritance of Target Incompatibility

[fmeum]

This is the discussion thread for the Inheritance of Target Incompatibility proposal, which aims to make it both more obvious and more configurable how target incompatibility caused by target_compatible_with propagates up the dependency graph.

Feedback is very welcome, here or on the doc.

[gregestren]

Review request acknowledged...

[gregestren]

Thanks for taking this on @fmeum (and @philsc for ongoing attention & support).

I read through carefully and added lots of pedantic comments. Thoughts:

• I'm not trying to be pedantic for its own sake. IMO there's a lot of subtlety in this problem space. What exactly do we mean by "incompatibility"? How is that different than the desired resultant behavior? I noted here there are are multiple relevant variables. I'd love to be as crystal-clear on term definitions and state X = result Y expectations as we can to ensure we're all considering the same ideas

• Relatedly, I'm still very interested in teasing out the core model, API solutions aside. These are the core API-independent themes I see:

  • What's the important difference between directly vs. indirectly incompatible?
  • What's the important difference between binaries / tests / top-level targets vs. libraries?
  • How does project / repo ownership factor in?
  • When does failure vs. skipping make sense?

  I'm interested in zeroing in on these core questions before focusing on particular solution details

• You definitely speak to this in your proposal. Going way back to first motivation, the main story I read is:

  • For most projects, $ bazel test //:all shouldn't silently drop coverage because of a third party dep that adds target_compatible_with = [<not compatible>]
  • But for heavily multi-platform projects, we don't want to assume everything in $ bazel test //:all should always run: silent dropping can be a feature as it avoid tedious maintenance burden.

So I suggest we get consensus on these core themes as abstract principles, then consensus on how the implementation fulfills them. I imagine the latter will be easier.

I hope this doesn't come across as obstructionist. I'm thinking this way because I worry about the increasing number of toggles coming into the model: multiple flags, repo membership, propagation semantics, direct vs. indirect, etc. The more we have the more complicated the intersection between them and the harder it is to support a clear story. I'm hoping further first principle contemplation could uncover more opportunity to minimize these toggles.

https://github.com/pcjanzen @trybka as other notable participants.

[gregestren]

I wonder, for example, if we really need to encode a distinction between directly vs. indirectly incompatible targets.

Over at Jazzer, we recently accidentally and silently disabled our entire test suite on Windows by merging a change that added target_compatible_with somewhere far down in the dependency tree.

I could see guarding indirect incompatible target skipping behind a flag as a potential solution to this. Direct incompatible target skipping works very well and without surprises in all contexts I have used it so far.

from #18707, for example, suggests the distinction from first principles. But I'm not yet sure we really need distinct Bazel behavior for those two variants, vs. something the user could intelligently code into their project curation (without too much maintenance burden).

[pcjanzen]

Ignoring all practical considerations, from a purely conceptual standpoint, I feel that targets really should inherit incompatibility from their deps. If I depend on X, and X is not compatible with the current platform, then I am also not compatible with the current platform.

It seems to me that the practical problem then becomes the "spooky action at a distance" that this behavior enables, as described in #18707. And furthermore I think this problem specifically arises in the context of expansion of wildcard patterns //....

Perhaps it would be fruitful to also consider ways to allow BUILD file authors to more specifically control which targets get included in wildcard patterns: would that allow us to leave the inheriting nature of platform-incompatibility as it currently stands?

[tpudlik]

We've been thinking about this in Pigweed, too (https://pwbug.dev/347752345). The most important tension is over tests: it's very convenient for them to be silently skipped when the library-under-test is incompatible with the platform. The danger is a change to the transitive deps, or to the platform configuration, accidentally and silently disabling many libraries and their tests (and sneaking through a CI that does bazel test //...).

This would be much less of a problem if our repo's final product was a single binary (or a handful of them): we could just explicitly request that these be built, and that should catch any compatibility issues. But we provide many libraries, rather than a handful of binaries.

But I'm not sure if what's needed here is an API change in Bazel; maybe it's more about determining good patterns for CI testing. (E.g., what about checking in a file with a "budget" for the number of skipped targets?)

[gregestren]

Separate thread:

But for heavily multi-platform projects, we don't want to assume everything in $ bazel test //:all should always run: silent dropping can be a feature as it avoid tedious maintenance burden.

I'd also love to hear more clarification of how these types of projects work: what kinds of binaries / tests, mixed together how, what real-life changes alter their compatibility properties? How do they integrate the current query-and-tag approach?

[pcjanzen]

Our project has dozens of platforms, some of them embedded. For us the primary problems arise from the interactions between cc_{binary/library/test} and different platforms.
Compatibility changes all the time, primarily because somebody has to do work to support existing libraries/API's on new platforms. Some examples:

• A library may be available "by default" via one platform's native packaging, but won't be compatible on other platforms until somebody does the work of porting it.
• Or a vendor provides an API as a part of some product; we want to support testing clients that use this API on desktop platforms, so we need to write an emulator for that API using POSIX-y primitives.
• One embedded platform has hardware that trivially implements some function; for other platforms we must write software that emulates that function.

Particularly this last case makes it very difficult for our humans to re-express (in BUILD files) the compatibility of each target, based on the compatibility of its deps. Our embedded platforms have dozens of fine-grained constraints reflecting hardware details, and our lowest-level implementation targets are expressed in terms of those. Our top-level targets only know about their target platforms, not the low-level details. The transitive behavior of incompatibility is a huge benefit here; we can have a great big sea of middle targets between the lowest-level and the top-level, and Bazel just figures it out.

[fmeum]

Since the original approach just isn't flexible enough as it doesn't allow direct and incompatibility to be mixed and also permanently relies on a flag, I'm now favoring a different approach based on what @pcjanzen brought up in #18707 (comment).

The new idea is very simple: If a target has target_compatible_with set explicitly (possibly even to []) and it matches the target platform, but a dependency is found to be incompatible, the build should fail instead of silently skipping the target. This ensures that any explicitly set target_compatible_with attributes are accurate and offers users a simple way to ensure that important targets build on all expected platforms, while still allowing any other target to benefit from indirect incompatibility. This would be gated behind an --incompatible_explicit_target_compatible_with flag.

I prototyped the approach in https://github.com/bazelbuild/bazel/pull/22651/files#diff-a982fd7588855b07d791b1b297ffc662b316c80313943ec6998af93edb331c5e (I recommend taking a look at the integration tests).

@gregestren @pcjanzen Please let me know what you think about this alternative.

[fmeum]

When a dependency starts supporting a platform, should there be a way to indicate that a dependent target should become available?

This sounds like something that an aspect or external build tool would need to do as it seems necessary to tap into VCS knowledge for this.

[fmeum]

I've seen examples where tests were suddenly disabled, but setting target_compatible_with=[] on every test feels wrong.

This could be mitigated somewhat by adding a corresponding attribute to package and repo. I just don't see a way to have implicit skipping while not requiring some form of manual tagging like this.

[fmeum]

An alternative could be to just add a no-indirect-incompatibility tag with the same effect. More flexible, but also more likely to be forgotten or subject to typos. package also doesn't have default_tags.

[philsc]

Possibly ugly idea: What if aspects could run on incompatible targets and retrieve a target's IncompatiblePlatformProvider provider?
This was (I think) possible at one point before this patch: 6d71850.
It might need IncompatiblePlatformProvider to be exposed to Starlark though, not sure if that's done yet.

The aspect creation would need a new attribute.

incompatibility_checker_aspect = aspect(
    implementation = _incompatibility_checker_aspect_impl,
    attr_aspects = ['*'],
    apply_to_incompatible_targets = True,
)

The implementation could then decide how deep into the tree it wants to go and what restrictions on transitive compatibility it wants to place.

Admittedly, I very rarely use aspects so I'm not very familiar with how it would fit in here. But it seems doable; and we could provide a useful implementation of this aspect in skylib.

[tpudlik]

It would only result in an error (instead of skipping) if :some_config_setting doesn't match but some transitive dependency is incompatible.

Right, but what's the pattern to use if I don't want an error in this case (but still want to declare the target incompatible with :some_config_setting)?

[] doesn't have a special meaning, it's just an optional way to express that a target should always be compatible (and thus fail if a dep isn't).

Today [] has the meaning "the target is directly (but maybe not indirectly) compatible with any platform". The proposal is to redefine it to mean "the target is directly and indirectly compatible with any platform". But we still need some way to express the old meaning within a target_compatible_with = select(...)!