Skipping incompatible targets is too cumbersome

[marczych-zoox]

Description of the feature request:

Skipping incompatible targets with target pattern expansion (e.g. //...) is great: https://bazel.build/extending/platforms#skipping-incompatible-targets. However, it's common in CI to use bazel query to find the minimal set of targets to build/test to validate a proposed change. This typically produces a list of explicit targets which may include incompatible targets. Filtering them with cquery (https://bazel.build/extending/platforms#skipping-incompatible-targets) works but is an extra step which takes time. Additionally, there might be thousands of targets to filter which must be chunked into separate command line invocations because cquery doesn't have any support for target pattern files like build and query do.

A command line argument like --skip_incompatible_explicit_targets that skips incompatible explicit targets would allow us to take the output from bazel query and use it directly in a bazel test command without using cquery to filter out incompatible targets.

What underlying problem are you trying to solve with this feature?

I'd like to speed up our CI builds by not filtering all of the affected targets through cquery.

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

release 4.2.2-2.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

We've internally forked bazel but this feature request is unrelated to our modifications.

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

It outputs a few commits from our internal fork.

Have you found anything relevant by searching the web?

I started a discussion in the Bazel Slack workspace here: https://bazelbuild.slack.com/archives/CA31HN1T3/p1670884944989049

Any other information, logs, or outputs that you want to share?

No response

[gregestren]

I understand what you're expressing. I'm generally hesitant to add more flags to toggle Bazel features - Bazel has too many toggles as it is. That doesn't mean we can't add them. Just that we want to to careful due diligence to justify their cost.

@philsc do you remember if we considered this sort of thing when defining the current semantics? What was the rationale again for making direct calls errors while //... calls skips?

[philsc]

Yeah, this is a fair criticism I think.

do you remember if we considered this sort of thing when defining the current semantics? What was the rationale again for making direct calls errors while //... calls skips?

Looking at the original proposal, it looks like we didn't have a justification for the "why" it should work this way. My suspicion is that it just "felt" like the right thing to do. I.e. if you request bazel to build something that can't be built, that "feels" like an error. Most likely user error.

But that doesn't account for the fact that query is by far the most ergonomic tool to build CI systems on top of (as opposed to cquery). So something should change.

I'm hesitant to change the default behaviour because (at least in my experience) the error that gets printed when trying to build an incompatible target is super helpful. It will tell the user immediately why their target cannot be built. Obviously not helpful for a CI system.

I'm not opposed to adding a flag to toggle the behaviour. I can't immediately think of a better way to handle this.

[marczych-zoox]

Thanks for the comments!

I understand the hesitation of adding yet another command line flag. I'm on the fence about what the default behavior should be. On one hand, if you ask bazel to build something it shouldn't silently skip it and exit 0. However, that's kind of what //... is doing but there's already a precedent for skipping manual targets so it makes some sense.

Maybe bazel test //some/build:target returning 4 and printing out "No test targets were found, yet testing was requested" is a more appropriate analogy here? Would it make sense to have bazel build //incompatible:target return a new exit code that indicates that it skipped an incompatible target so scripts can look for that to determine if the command was successful or not? Although, I'm not sure what it would bazel test //incompatible/build:target would return in that case...

[gregestren]

team-Configurability discussed this at today's triage meeting and the consensus was this use case is clear enough to justify a flag. I agree with both your feedback above.

The main other feedback was to not make such a flag part of build configuration, so people can't write configuration transitions over it. https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java would be a good place for the flag.

Re: communicating the error, we'd prefer updates to the BEP protos for machine-readable interpretation: see #16805 for similar related discussion.

[nikhilkalige]

Just wanted to add a note that passing targets to cquery and aquery using file recently got fixed here (fb23246). Also, would it be better for this to be some sort of filter in the query language?

[marczych-zoox]

The main other feedback was to not make such a flag part of build configuration, so people can't write configuration transitions over it. https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java would be a good place for the flag.

Sounds good! I can push an update to the PR.

Just wanted to add a note that passing targets to cquery and aquery using file recently got fixed here (fb23246).

Nice!

Also, would it be better for this to be some sort of filter in the query language?

This sounds much easier than the --output=starlark --starlark:file=example.cquery approach here: https://bazel.build/extending/platforms#skipping-incompatible-targets. However, --skip_incompatible_explicit_targets is still nice to avoid running a cquery to begin with.