cquery ignores test config in .bazelrc files

[illicitonion]

I expect cquery when querying tests to reflect flags like --test_env and --test_arg. It appears to reflect these if they're specified on the command line, but not if they're in a .bazelrc file:

Reproduction (making a clean workspace for each run, so we're definitely not seeing weird leftover state from previous runs):

$ dir=$(mktemp -d); (cd "${dir}" && touch WORKSPACE && touch test.sh && chmod 0755 test.sh && echo >BUILD 'sh_test(name = "test", srcs = ["test.sh"])' && bazel cquery //:test 2>/dev/null)
//:test (86e5752)

$ dir=$(mktemp -d); (cd "${dir}" && touch WORKSPACE && touch test.sh && chmod 0755 test.sh && echo >BUILD 'sh_test(name = "test", srcs = ["test.sh"])' && bazel cquery //:test --test_arg=beep 2>/dev/null)
//:test (6a51fd2)

$ dir=$(mktemp -d); (cd "${dir}" && touch WORKSPACE && touch test.sh && chmod 0755 test.sh && echo >BUILD 'sh_test(name = "test", srcs = ["test.sh"])' && echo 'test --test_arg=beep' > .bazelrc && bazel cquery //:test 2>/dev/null)
//:test (86e5752)

cquery does appear to correctly pay attention to .bazelrc files for things that would affect the build actions that produce the tests, but appears to be ignored for things that would affect the test actions.

What operating system are you running Bazel on?

macOS

What's the output of bazel info release?

release 4.0.0

/cc @gregestren

[gregestren]

@sdtwigg has input on this.

[illicitonion]

@sdtwigg I'd love to hear your thoughts, if you wouldn't mind sharing!

[sdtwigg]

This is roughly working as expected. The cquery command inherits from the build command so it will only see bazelrc entries with build or cquery (although I am not familiar with anyone using the latter as a matter of course).

In practice, we generally advise being careful with triggering off test in bazelrc and instead prefer build because, depending on affected options, it could result in a redoing lot of analysis phase work if users swap between commands.

[illicitonion]

In practice, we generally advise being careful with triggering off test in bazelrc and instead prefer build because, depending on affected options, it could result in a redoing lot of analysis phase work if users swap between commands.

The specific problem I'm trying to solve is: "Given this patch, what tests will have different results (and thus need running on CI)", and using cquery I have an accurate set of commands to perform to determine this except for .bazelrc files which affect the test configuration. We want to trigger any tests which would be affected by a change to .bazelrc, but ideally we don't want to trigger all tests on any change to a .bazelrc (and we don't want to have to parse the .bazelrc to follow any import or try-import statements to know that we need to trigger on those changes too).

Is there a nice way to do this? If not, could we perhaps add a bazel tquery command or a flag to cquery to be sensitive to config which affects the test-runner action?

This is roughly working as expected. The cquery command inherits from the build command so it will only see bazelrc entries with build or cquery (although I am not familiar with anyone using the latter as a matter of course).

Given that, shouldn't bazel cquery --test_arg=beep ... error, rather than heed the flag?

[gregestren]

Can you define further what it means for a test to be affected by a change? Like...different output? Different config hash? Is your interest specifically in --test_* flags or any flag set by the test: command?

[illicitonion]

Can you define further what it means for a test to be affected by a change? Like...different output? Different config hash? Is your interest specifically in --test_* flags or any flag set by the test: command?

I'd give the definition:

A test is affected by a change if, running the test after the change, it may have a different result than running it before the change.

With the assumption that tests are deterministic, reliable, and self-contained (dealing with things like flakiness or talking on the network is another matter).

We'd rather over-estimate than under-estimate (under-estimating leads to broken changes getting merged, which is really bad. Over-estimating leads to wasted resources and potentially extra developer latency, which is bad, but nowhere near as bad).

So - different output, definitely. Different config hash - probably a reasonable proxy for different output (but by ignoring config which affects the test runner action, we veer towards under-estimating, which is problematic). Any flag set by the test: command - ideally not - things like --test_summary or --test_output ideally wouldn't matter. But again, we'd rather over-trigger than under-trigger (hence we care about --test_arg), but we'd rather minimise over-triggering where we can :)

[gregestren]

Thanks. Even more naively, wouldn't all test targets show a different hash from a changed flag? If the approximation isn't actually running the test to compare the output, what's distinguishing affected vs. non-affected tests? I think there's something more fundamental I'm missing. I know there's --trim_test_configuration, but I don't think that applies yet?

[illicitonion]

Thanks. Even more naively, wouldn't all test targets show a different hash from a changed flag?

Yes, if someone adds a --test_arg flag in a .bazelrc file, we want to trigger all tests.

If the approximation isn't actually running the test to compare the output, what's distinguishing affected vs. non-affected tests?

The next step in the pipeline is to run the affected tests. The goal here is to determine what targets need to run for a particular CI run. So the steps are:

Determine affected targets -> Run affected tests -> Publish affected releasable artifacts

We're looking to avoid running unaffected tests (partially to avoid spending resources where we may get spurious cache misses, partially to avoid running flaky tests, and partially to avoid failing a PR because of known-failing tests elsewhere in the repo).

[gregestren]

Getting back to technical details, I acknowledge there's a weird interaction between .bazelzrc's test ... and actual build and test (and other) subcommands.

For example, this works just fine:

$ bazel build --nobuild //foo --test_arg=beep

cquery is basically the same command. So erroring cquery involves erroring the build command. If I recall correctly, the test: directive simply doesn't add those flags to the build command. So it's not that the flags don't work with the command, the command doesn't know it's supposed to apply them.

I was hoping canonicalize-flags had some way to include .bazelrc input, including following imports. But it looks like it doesn't, and it wasn't intended to. :(

[illicitonion]

Getting back to technical details, I acknowledge there's a weird interaction between .bazelzrc's test ... and actual build and test (and other) subcommands.

For example, this works just fine:

$ bazel build --nobuild //foo --test_arg=beep

cquery is basically the same command. So erroring cquery involves erroring the build command. If I recall correctly, the test: directive simply doesn't add those flags to the build command. So it's not that the flags don't work with the command, the command doesn't know it's supposed to apply them.

Yeah, it's a really weird corner - there's presumably special handling somewhere to accept test flags on a build command-line, which surprised me - I had previously assumed that the above command line would error out in the same way that bazel query //foo --test_arg=beep does.

[gregestren]

I don't think build distinguishes --test* flags differently from any other flag. bazelrc's test ... directive does, but it's totally unrelated.

cquery inherits the build command here:

bazel/src/main/java/com/google/devtools/build/lib/runtime/commands/CqueryCommand.java

Line 51 in 09c621e

inherits = {BuildCommand.class},

which gives it access to all build and test flags.

query doesn't:

bazel/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java

Lines 55 to 62 in 09c621e

	@Command(
	name = "query",
	options = {
	PackageOptions.class,
	QueryOptions.class,
	KeepGoingOption.class,
	LoadingPhaseThreadsOption.class
	},

and I think only gets the flags explicitly itemized in its definition (*Option.class). query definitely doesn't understand build flags, e.g.:

$  bazel query //foo --cpu=blah
ERROR: --cpu=blah :: Unrecognized option: --cpu=blah

Happy to chat about this as much as you want. It's good to explore reality, but I'm not sure where this takes us toward envisioning a happy resolution.

[illicitonion]

Yeah, it would definitely be good to get back towards a solution!

How would you feel about something like a new cquery flag like --include_test_config (which I guess would be somewhat tricky to implement because command inheritance is static), or a new command bazel tquery (as in, "test query" - easy to implement, but a much more intrusive CLI change...)? Or any better ideas for how to expose this information!

Slightly less helpful but still handy could be a bazel print-bazelrc-paths or similar, to follow Bazel's internal bazelrc import and try-import logic to at least give a list of files which should trigger all tests if a more accurate solution isn't viable.

[gregestren]

For better or worse I'm increasingly inspired by the Lukacs Berki and Alan Donovan school of thought of fearing Bazel's complexity and being ridiculously cautious about expanding it (and of course ideally reducing it).

So my major first reactions are to fully explore what's possible today. Expanding on your second slightly less helpful point, could this help?

$ bazel test --nobuild <sample test target> --announce_rc

I experimented a bit and I think it provides enough raw information to reproduce whatever you need: both loaded bazelrcs and specific flags loaded. Not ideally presented, but...still useful?

[gregestren]

Expanding on your second slightly less helpful point,

Tone clarification: "slightly less helpful" in that you noted it as such. Not that your feedback is less helpful to me. 😄

[illicitonion]

could this help?

$ bazel test --nobuild <sample test target> --announce_rc

I experimented a bit and I think it provides enough raw information to reproduce whatever you need: both loaded bazelrcs and specific flags loaded. Not ideally presented, but...still useful?

I like the creativity! I think it has the basis for something helpful, but... A handful of issues:

1. Quoting/escaping: If I add:

test --test_arg="foo bar"
test --test_output=all

to a .bazelrc file, the output I see is:

  'test' options: --test_arg=foo bar --test_output=all

I guess it's probably relatively easy to fix this by passing the args through a shlex-like library before outputting them?
2. Structure - it would be nice to be able to have these lines output to a file, rather than stderr, so I don't need to grep through stderr for them. And if we're doing that, it might as well be structured output (which would also solve the output problem)... So maybe we could make --announce_rc take either a boolean or a path, and if a path, output a JSON/proto/textproto file there? Or I guess a new flag --announce_rc_json=[path]?
3. There's a risk of double-parsing accumulating flags if we specify these flags manually, and also bazel is looking at bazelrc files... So I guess we'd need to do this query, build the command line from what's parsed out, and then run future cqueries with --bazelrc=/dev/null --nohome_rc --nosystem_rc --noworkspace_rc?
4. As I read the implementation, the ordering of these is that later lines take precedence over earlier lines, but this isn't specified/documented on the flag. Documenting this would be nice :)

WDYT?

[gregestren]

You mentioned earlier that a --test_arg change would trigger all tests. What flag would exclude some tests? Do you have custom logic for mapping a specific flag to an affected set?

All in the last comment makes sense, but I'd still like to explore more what's possible now and exactly how the functionality is used. Expanding yet further the space of possibilities, I also thought about:

1. printing out all flags, post-expansion, which I understand is retrievable in machine-readable form as BEP output
2. Modifying CqueryCommand to inherit TestCommand. That's a simple one-liner and not an explicit API expansion. I'm not sure if there's a philosophical reason to inherit BuildCommand specifically. There must be consequences I'm not thinking of. But I'm not sure this is an unworkable option?

[illicitonion]

You mentioned earlier that a --test_arg change would trigger all tests. What flag would exclude some tests? Do you have custom logic for mapping a specific flag to an affected set?

In general most test flags I think either affect all or nothing, but having to write down somewhere that --test_arg affects tests and --test_output doesn't, for every test flag, isn't scalable, and would require ongoing maintenance as Bazel changes. Even though bazel has some metadata about this in its source code (and documentation), it's not reliable or exhaustive.

All in the last comment makes sense, but I'd still like to explore more what's possible now and exactly how the functionality is used. Expanding yet further the space of possibilities, I also thought about:

1. printing out all flags, post-expansion, which I understand is retrievable in machine-readable form as BEP output

I tried this out, and I couldn't come up with a reliable algorithm for parsing this information out. Things I saw:

1. In started there's an optionsDescription field, but overriding flags aren't deduplicated, so you can have multiple --test_output flags present, and you need to know which flags are accumulating and which are overriding.
2. In structuredCommandLine, some flags appear as --default_override flags, and it's not at all clear how to handle these. Here's how it renders a --test_arg flag (which is accumulating) in a .bazelrc:

            {
              "combinedForm": "--default_override=1:test=--test_arg=beep",
              "optionName": "default_override",
              "optionValue": "1:test=--test_arg=beep",
              "effectTags": [
                "CHANGES_INPUTS"
              ],
              "metadataTags": [
                "HIDDEN"
              ]
            },

and one specified on the command line:

            {
              "combinedForm": "--test_arg=boop",
              "optionName": "test_arg",
              "optionValue": "boop",
              "effectTags": [
                "UNKNOWN"
              ]
            }

And this is how two --test_output flags appeared specified in different precedences:

            {
              "combinedForm": "--default_override=1:test=--test_output=all",
              "optionName": "default_override",
              "optionValue": "1:test=--test_output=all",
              "effectTags": [
                "CHANGES_INPUTS"
              ],
              "metadataTags": [
                "HIDDEN"
              ]
            },

            {
              "combinedForm": "--default_override=3:test=--test_output=errors",
              "optionName": "default_override",
              "optionValue": "3:test=--test_output=errors",
              "effectTags": [
                "CHANGES_INPUTS"
              ],
              "metadataTags": [
                "HIDDEN"
              ]
            },

I can't see an obvious pattern in these outputs which would let me say "the resolved flags here are --test_arg=beep --test_arg=boop --test_output=errors (and that the --test_output=all can be ignored because it's been overridden by the later --test_output=errors, but both --test_arg flags matter), let alone passing that through a filter to say "But you can ignore the --test_output flag anyway". And I would definitely struggle to have confidence that any code I wrote to do this would reliably work, and would continue to as Bazel evolves.

3. unstructuredCommandLine effectively has the same information as structuredCommandLine, but without as much metadata. It shows:

      "--default_override=1:test=--test_arg=beep",
      "--default_override=1:test=--test_output=all",
[...]
      "--default_override=3:test=--test_output=errors",
[...]
      "--test_output=errors",
      "--test_arg=boop"

but I don't see a clear, reliable path to transform that into what's actually relevant.

2. Modifying CqueryCommand to inherit TestCommand. That's a simple one-liner and not an explicit API expansion. I'm not sure if there's a philosophical reason to inherit BuildCommand specifically. There must be consequences I'm not thinking of. But I'm not sure this is an unworkable option?

Honestly this would be perfect for me :)

[illicitonion]

I filed #13491 - let's see what breaks! :)

[gregestren]

In general most test flags I think either affect all or nothing, but having to write down somewhere that --test_arg affects tests and --test_output doesn't, for every test flag, isn't scalable, and would require ongoing maintenance as Bazel changes. Even though bazel has some metadata about this in its source code (and documentation), it's not reliable or exhaustive.

This makes sense! (but I had to do the ad hoc code looking up you describe to verify, which provides your point). Thank you.

1. Modifying CqueryCommand to inherit TestCommand. That's a simple one-liner and not an explicit API expansion. I'm not sure if there's a philosophical reason to inherit BuildCommand specifically. There must be consequences I'm not thinking of. But I'm not sure this is an unworkable option?

Honestly this would be perfect for me :)

Looks like the PR presubmit is clean?

Even better would be to teach bazelrc processing to understand cquery as a separate command. Let me check that... I happened to look at that code recently (329b22a).

In spirit cquery is neither build nor test, and it would be great to reflect that as much as possible.

This does shake up the analysis caching concerns @sdtwigg mentioned above - switching between build or test or cquery may re-do analysis depending on what flags bazelrc fulfills where. But I do hope those distinctions don't happen often in practice.

[illicitonion]

Even better would be to teach bazelrc processing to understand cquery as a separate command. Let me check that... I happened to look at that code recently (329b22a).

In spirit cquery is neither build nor test, and it would be great to reflect that as much as possible.

This does shake up the analysis caching concerns @sdtwigg mentioned above - switching between build or test or cquery may re-do analysis depending on what flags bazelrc fulfills where. But I do hope those distinctions don't happen often in practice.

I'm not sure I completely follow what this would look like - is the idea that you could specify what command cquery should act like?

[gregestren]

The relevant implementation logic is

bazel/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java

Lines 162 to 179 in 329b22a

	/**
	* Parses the unconditional options from .rc files for the current command.
	*
	* <p>This is not as trivial as simply taking the list of options for the specified command
	* because commands can inherit arguments from each other, and we have to respect that (e.g. if an
	* option is specified for 'build', it needs to take effect for the 'test' command, too). More
	* specific commands should have priority over the broader commands (say a "build" option that
	* conflicts with a "common" option should override the common one regardless of order.)
	*
	* <p>For each command, the options are parsed in rc order. This uses the master rc file first,
	* and follows import statements. This is the order in which they were passed by the client.
	*/
	void parseRcOptions(
	EventHandler eventHandler, ListMultimap<String, RcChunkOfArgs> commandToRcArgs)
	throws OptionsParsingException {
	for (String commandToParse : getCommandNamesToParse(commandAnnotation)) {
	// Get all args defined for this command (or "common"), grouped by rc chunk.
	for (RcChunkOfArgs rcArgs : commandToRcArgs.get(commandToParse)) {

commandAnnotation has the name of the current command.

There's interesting enough commentary in the Javadoc. The criteria for qualifying which bazelrc flags are included are

bazel/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java

Lines 379 to 384 in 329b22a

	private static void getCommandNamesToParseHelper(
	Command commandAnnotation, List<String> accumulator) {
	for (Class<? extends BlazeCommand> base : commandAnnotation.inherits()) {
	getCommandNamesToParseHelper(base.getAnnotation(Command.class), accumulator);
	}
	accumulator.add(commandAnnotation.name());

which basically means "the current command and its inheritance hierarchy". Which is why test also gets the settings for build (but not vice versa), everything gets the settings for common, etc.

I was wondering if we could be even more explicit: tell cquery to also take test lines without implying cquery inherits from test. The answer above looks like no, unless we want to complicate the criteria, which I don't think we do.

The only other impact of changing the inheritance is whatever overrides in https://github.com/bazelbuild/bazel/blob/f6a822f5f922ff33a3ad97491c26a6a28e4a7692/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java will also kick in. Since it's cquery, which is spiritually equivalent to bazel build --nobuild, the post-analysis overrides shouldn't matter. If we're convinced there's nothing weird there, that'd give me confidence the short-and-sweet change you did in your PR is a good solution (maybe with a comment clarifying why it does that).

One further question: test and build aren't the only commands. Are there any other commands this solution wouldn't cover? Would that matter?

[illicitonion]

One further question: test and build aren't the only commands. Are there any other commands this solution wouldn't cover? Would that matter?

Of the existing ones, the only one I imagine believe people wanting to act like is coverage... But already that means n > 0! And I can imagine new Commands in the future, though I don't know how likely that is...

The relevant implementation logic is
[...]
which basically means "the current command and its inheritance hierarchy". Which is why test also gets the settings for build (but not vice versa), everything gets the settings for common, etc.

Thanks for the pointers!

I was wondering if we could be even more explicit: tell cquery to also take test lines without implying cquery inherits from test. The answer above looks like no, unless we want to complicate the criteria, which I don't think we do.

If we can find a nice way to signal it (it can't Just be a flag, because it's needed to drive the flag parsing - maybe it could be a flag which can only be specified on the command line...), being able to drive "This cquery should act as if it inherits from X" would I think be useful. And apart from working out how to signal it, I think that's a reasonably limited added complexity to the criteria - effectively we'd remove the inherits from CqueryCommand, and make getCommandNamesToParseHelper look something like:

  private void getCommandNamesToParseHelper(
      Command commandAnnotation, List<String> accumulator) {
    Command commandForInheritance = commandAnnotation;
    if (commandAnnotation.name().equals("cquery")) {
      String commandToActAs = "build"; // TODO: Work out how to pass this signal in from a flag or similar
      commandForInheritance = runtime.getCommandMap().get(commandToActAs).getClass().getAnnotation(Command.class);
    }
    for (Class<? extends BlazeCommand> base : commandForInheritance.inherits()) {
      getCommandNamesToParseHelper(base.getAnnotation(Command.class), accumulator);
    }
    accumulator.add(commandAnnotation.name());
  }

It's a special-case, but it's quite nicely localised, and I think makes a lot of sense from a "cquery acts like other commands but isn't itself them" perspective.

The only other impact of changing the inheritance is whatever overrides in https://github.com/bazelbuild/bazel/blob/f6a822f5f922ff33a3ad97491c26a6a28e4a7692/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java will also kick in. Since it's cquery, which is spiritually equivalent to bazel build --nobuild, the post-analysis overrides shouldn't matter. If we're convinced there's nothing weird there, that'd give me confidence the short-and-sweet change you did in your PR is a good solution (maybe with a comment clarifying why it does that).

I personally don't care about this particular edge-case, but technically the fact that setting --test_strategy=exclusive sets --test_sharding_strategy=disabled could be an impactful change for someone...

[gregestren]

I'm disinclined to follow the cquery inherits TestCommand approach if it's still not a complete solution due to the possibility of other commands. In principle people could set directives for any command in the bazelrc, and the total number is high.

Having some variation of --for_command for cquery with the code you describe could indeed be complete. But you still have to know what you'd want to set that flag to, and I don't see any way to do that comprehensively unless you still have external scripting trying to read / grep the transitive bazelrc structure. You could hard-set that to "the flags you care about". But it's always possible for someone to write a patch that sidesteps that.

I guess this ultimately depends on what you, as a user, expect from your CI model. I'm fuzzier on that than you are, naturally, which I think makes it harder for me to see something decisive.

Can your CI model confidently state which commands it cares about where? i.e. if someone adds any of these to a bazelrc:

query --some_flag
canonicalize-flags -- --some_flag   # Yes, this is possible!
run  --some_flag

are you comfortable with a statically declared set of which commands matter? Even though technically it could miss relevant changes when that set is miscoded?

If so, I suppose --for_command=<cmd> would work. It also still seems possible to me to get a reasonably accurate solution with pure external scripting: for each changed bazelrc, do a simple grep for the changed lines. If the line modifies an import, load the import too (yes, it's annoying). Run bazel config <any config> (or bazel config <any config> --output=json) to get a definitive list of all flags that affect analysis. If any modified bazelrc line changes any flag that appears in the bazel config results, that signifies the need to re-run tests.

I realize the --for_command=<cmd> has some more going for it. It's more direct and should be more precise (w.r.t. the same flag overwritten by different lines). But I think the "status quo" approach still retains correctness without any restrictions on the logic used to determine qualification.

[illicitonion]

I'm disinclined to follow the cquery inherits TestCommand approach if it's still not a complete solution due to the possibility of other commands. In principle people could set directives for any command in the bazelrc, and the total number is high.

I think that's probably fair enough (though I also lean slightly towards not making the perfect the enemy of the good - maybe the first feature request for a conflicting inherit could be the trigger for a more complex solution like --for_command :))

Having some variation of --for_command for cquery with the code you describe could indeed be complete. But you still have to know what you'd want to set that flag to, and I don't see any way to do that comprehensively unless you still have external scripting trying to read / grep the transitive bazelrc structure. You could hard-set that to "the flags you care about". But it's always possible for someone to write a patch that sidesteps that.

I guess this ultimately depends on what you, as a user, expect from your CI model. I'm fuzzier on that than you are, naturally, which I think makes it harder for me to see something decisive.

Can your CI model confidently state which commands it cares about where? i.e. if someone adds any of these to a bazelrc:

query --some_flag
canonicalize-flags -- --some_flag   # Yes, this is possible!
run  --some_flag

are you comfortable with a statically declared set of which commands matter? Even though technically it could miss relevant changes when that set is miscoded?

If so, I suppose --for_command=<cmd> would work.

I think it's reasonable to view cquery as a building block. I can certainly imagine CI systems (such as the one I'm working on) where this constraint is satisfied. In particular, if the config to the CI is somewhat like that offered in bazel-ci where you can configure declaratively which targets are tested, which are run, etc, that may tell you that you need to run both cquery --command_for=test and cquery --command_for=run (or just some subset), based on your input configuration.

If your CI system is to run arbitrary user-supplied shell-scripts, for sure all bets are off, but that doesn't mean we shouldn't provide building blocks for more constrained and controllable systems.

It also still seems possible to me to get a reasonably accurate solution with pure external scripting: for each changed bazelrc, do a simple grep for the changed lines. If the line modifies an import, load the import too (yes, it's annoying).

This is a recipe for pain, I think. Bazel's configuration language has no stability guarantees (i.e. that new constructs won't be introduced), and is already rather complex. Needing to properly parse and follow things like --config flags (including the platform-specific ones), the parsing order and precedence of the assorted bazelrc files, in inherence hierarchy of commands (which may change over time), etc. And then still needing to accept that you're treating this as a superset, and are going to over-trigger builds as a result... It doesn't feel like a good solution.

Bazel could perhaps offer some other building blocks here, such as a better structured canonicalize-flags that includes .bazelrc files, or similar, but at that point we're building a lot of infrastructure that needs supporting, which feels worse than something like cquery --for_command

Run bazel config <any config> (or bazel config <any config> --output=json) to get a definitive list of all flags that affect analysis.

Ooh, I did not know bazel config existed! I will have to experiment with this... (Though it notes itself as experimental and unsupported :)) - I'll let you know if I get anywhere exciting with it!

I realize the --for_command=<cmd> has some more going for it. It's more direct and should be more precise (w.r.t. the same flag overwritten by different lines). But I think the "status quo" approach still retains correctness without any restrictions on the logic used to determine qualification.

I think it retains exactly the same restrictions on logic? bazel config operates on whatever configurations happen to have been loaded by previous commands, so you'd need to run bazel [command] --nobuild ... for whichever commands you're going to run in CI in order for the configuration to be loaded. If you have a .bazelrc file with:

test --test_arg=beep
run --override_repository=com_google_protobuf=experimental_protobuf_fork

you would need to know whether your CI is going to run test, run, or both, so you know whether you need to bazel test --nobuild, bazel run --nobuild, or both, in order to pre-populate the configurations so that you can query them.

[gregestren]

I'll respond to your other comments further, but re:

Ooh, I did not know bazel config existed! I will have to experiment with this... (Though it notes itself as experimental and unsupported :))

It started out experimental but we quickly realized it's useful enough to deserve proper support. Where is it still tagged experimental?

The current documentation for it is obscure, in recognition of that history. I believe it's only referenced in the docs at https://docs.bazel.build/versions/master/configurable-attributes.html#why-doesnt-my-select-choose-what-i-expect and https://docs.bazel.build/versions/master/cquery.html#configurations

[illicitonion]

I'll respond to your other comments further, but re:

Ooh, I did not know bazel config existed! I will have to experiment with this... (Though it notes itself as experimental and unsupported :))

It started out experimental but we quickly realized it's useful enough to deserve proper support. Where is it still tagged experimental?

Mostly the last line of this:

% bazel help config | head -n 12
                                                           [bazel release 4.1.0]
Usage: bazel config <options> [<config_id> | <config_id> <config_id>]

Displays the given configuration objects, if they can be found in the Skyframe cache. The best way
to use this command is to first run a build (or cquery), and then immediately run the config
command to view the configuration. Note that any flags passed to build (or cquery) must also be
passed to config, or else the Skyframe cache will be evicted and no configurations will be found.

If two configuration ids are passed, the difference between them will be computed and displayed,
instead of the entire configurations.

This command is experimental and unsupported.

From https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/runtime/commands/config.txt

It's also not listed in https://docs.bazel.build/versions/master/command-line-reference.html#commands

[gregestren]

I'm sorry I'm behind on responsiveness at the moment. This is still in my inbox radar.

[gregestren]

I'm disinclined to follow the cquery inherits TestCommand approach if it's still not a complete solution due to the possibility of other commands. In principle people could set directives for any command in the bazelrc, and the total number is high.

I think that's probably fair enough (though I also lean slightly towards not making the perfect the enemy of the good - maybe the first feature request for a conflicting inherit could be the trigger for a more complex solution like --for_command :))

(trying to converge vs. diverge the conversation): does this mean you find theTestCommand approach tentatively sufficient?

If so, that sounds great in the name of minimal new complexity. If not, carry on below...

I realize the --for_command=<cmd> has some more going for it. It's more direct and should be more precise (w.r.t. the same flag overwritten by different lines). But I think the "status quo" approach still retains correctness without any restrictions on the logic used to determine qualification.

I think it retains exactly the same restrictions on logic? bazel config operates on whatever configurations happen to have been loaded by previous commands, so you'd need to run bazel [command] --nobuild ... for whichever commands you're going to run in CI in order for the configuration to be loaded. If you have a .bazelrc file with:

test --test_arg=beep
run --override_repository=com_google_protobuf=experimental_protobuf_fork

you would need to know whether your CI is going to run test, run, or both, so you know whether you need to bazel test --nobuild, bazel run --nobuild, or both, in order to pre-populate the configurations so that you can query them.

I'm just saying that bazel config provides a complete list of flags that affect configuration (for test, run, build, or any command that configures targets). Yes, you have to seed it with a build. But it doesn't matter which: bazel --bazelrc=/dev/null build --nobuild //:trivial_target would be sufficient. Then given that full list of flags you can define whatever criteria you want around them. Yes, parsing bazelrc is a task left to the user with this approach.

The cquery approach is similar, but more opaque? In that it gives you just a blob that doesn't emit what the flags actually are? (and which you can, unironically, feed directly into bazel config).

Anyway... my most important point is to consider when intrinsic new Bazel complexity is worth the cost, both in terms of adding it and considering how it might have to evolve over time. Inheriting from TestCommand seems lowest cost there (I think). --for_command (or any new flag) slightly higher. If, after exploring all these possibilities, you see the latter paying itself off, we can certainly fit it in. We'd want to be as clear as possible on its purpose since I don't think the semantics alone will be obvious to casual browsers.

[illicitonion]

I'm disinclined to follow the cquery inherits TestCommand approach if it's still not a complete solution due to the possibility of other commands. In principle people could set directives for any command in the bazelrc, and the total number is high.

I think that's probably fair enough (though I also lean slightly towards not making the perfect the enemy of the good - maybe the first feature request for a conflicting inherit could be the trigger for a more complex solution like --for_command :))

(trying to converge vs. diverge the conversation): does this mean you find theTestCommand approach tentatively sufficient?

If so, that sounds great in the name of minimal new complexity. If not, carry on below...

Absolutely. Let's ship it! :)

[illicitonion]

This was fixed by 20aff82