bazelrc: Allow "common" lines to always accept any legal Bazel options

[mmorearty]

Description of the problem / feature request / question:

Currently, if a bazelrc file contains a common line, then all of the specified options are passed to every bazel command that is run. I propose that the common line always allow all legal options that can apply to any command, but silently ignore the ones that don't apply to the current command.

The current behavior can easily lead to unnecessary error messages, and greatly hampers the usefulness of the common feature.

For one thing, there are very few Bazel options that are actually common across every command.

Also, although bazelrc supports inheritance (e.g. test and run will automatically inherit from any build lines), that inheritance isn't always enough. For example, suppose I want to have a custom --package_path for every Bazel command that supports that flag. It's not sufficient to write

build --package_path ...

because that will apply to build, test, and run, but not to fetch, info, and query, which do not inherit from build but do support the --package_path option.

I could write

build --package_path ...
fetch --package_path ...
info --package_path ...
query --package_path ...

But that is (a) messy, (b) requires me to do a lot of research to carefully figure out which commands support --package_path, and (c) could break in the future if I upgrade to a newer version of Bazel that supports --package_path for additional commands.

What I really want is to write

common --package_path ...

but I can't, because that breaks any command that doesn't support that flag, such as bazel help.

I think probably the best way to address this is what I proposed above: The common line always allows all legal options that can apply to any command, but silently ignores the ones that don't apply to the current command.

So common --package_path ... would never cause an error, but common --invalid_flag would cause an error.

Environment info

• Operating System:

OS X

• Bazel version (output of bazel info release):

0.4.5

[laszlocsomor]

SGTM.

@lfpino : What do you think? Any reasons against the suggested behavior? Any difficulties you anticipate?

[lfpino]

Sounds reasonable to me; however If I recall correctly skipping flags is not that easy with our current setup. I'm adding @brandjon and @cvcal who were working on the option processor and may know better.

[cvcal]

Unfortunately, with the current OptionsParser, knowing which flags exist for other commands is not possible and it would be a very invasive change.

Here's an alternative: --invocation_policy is a feature that allows users to control what flags get passed (for some definitions of control) and does allow setting non-existent flags (it just ignores them.) If you have the following policy,

flag_policies { 
  flag_name: 'package_path' set_value { flag_value:'<insert value here>' overridable: true }
}

it will apply to all commands where the flag is defined unless the user specified their own --package_path

[kastiglione]

Was just about to create an issue for this. +1 we want this too.

[purkhusid]

This would be a very nice feature.

[nkoroste]

+1

[pauldraper]

Currently there are 16 commands.

Whenever you add an option to bazelrc, you must figure which of those 16 commands it applies to.

[kylecordes]

Here is an extreme case which I think illustrates why the requested change is worthwhile. I always want to set a disk cache location, regardless of what command is invoked.

common --disk_cache=~/.cache/bazel-disk

With this setting, bazel version breaks. bazel version should never break, I think?

(The workaround was to do as @pauldraper said, which involves a fair amount of scrolling around the command line reference documentation.)

[pauldraper]

My current workaround to save scrolling is this script:

bazel-option-commands

#!/usr/bin/env sh
for command in \
  analyze-profile \
  aquery \
  build \
  canonicalize-flags \
  clean \
  coverage \
  cquery \
  dump \
  fetch \
  help \
  info \
  license \
  mobile-install \
  print_action \
  query \
  run \
  shutdown \
  sync \
  test \
  version; \
  do \
  ! bazel help "$command" 2> /dev/null | grep -q "$1" || echo "$command"
done

Example:

bazel-option-commands workspace_status_command

aquery
build
canonicalize-flags
clean
coverage
cquery
info
mobile-install
print_action
run
test

Maybe someone will create a utility to add these automatically.

[jesseschalken]

This is a huge pain. --repository_cache is valid for 20 different commands? So I have to list it 20 times in .bazelrc?

[jesseschalken]

My Bash isn't great but here is a version of @pauldraper 's script that handles inheritance of options between commands.

bazel-option-commands.bash

#!/usr/bin/env bash

set -e -u -o pipefail

# Commands that don't inherit any options
base_commands=(
  analyze-profile
  build
  dump
  fetch
  help
  license
  query
  shutdown
  sync
  version
)

# Commands that inherit options from build
build_commands=(
  aquery
  canonicalize-flags
  clean
  info
  mobile-install
  print_action
  run
  "test"
  # It is not documented that config inherits from build but it does
  config
)

# Commands that inherit options from test
test_commands=(
  coverage
  cquery
)

option="$1"

all_commands_match=true

function command_has_option {
  local help_content
  if ! help_content="$(bazel help --short "$1")"; then
    exit $?
  fi
  grep -qE "^  --(\\[no\\])?$option\$" <<<"$help_content"
}

function test_command {
  if command_has_option "$1"; then
    echo "$1"
  else
    all_commands_match=false
  fi
}

for command in "${base_commands[@]}"; do
  test_command "$command"
done

if ! command_has_option build; then
  for command in "${build_commands[@]}"; do
    test_command "$command"
  done
fi

if ! command_has_option test; then
  for command in "${test_commands[@]}"; do
    test_command "$command"
  done
fi

if [ "$all_commands_match" = true ]; then
  echo "(all commands match, you can put this in \"common\" in .bazelrc)"
fi

$ ./bazel-options-commands.bash workspace_status_command
build

$ ./bazel-options-commands.bash repository_cache
analyze-profile
build
dump
fetch
help
license
query
shutdown
sync
version
(all commands match, you can put this in "common" in .bazelrc)

[alexeagle]

I brought up this issue with several groups at BazelCon this year, including the first community day unconference session and the rules authors SIG meeting. There was wide consensus that accidental external repository invalidations and analysis cache discards is a high-priority usability issue. Recent posts on this thread highlight the infeasibility of existing workarounds.

@katre agreed that it's reasonable to solve this as @mmorearty proposed in the OP: ignore flags that aren't available on the given command. I believe that is non-breaking for users, since the only behavior change is for bazel invocation that would fail under existing versions of bazel. However he feels that there may be some design discussion required. @gregestren is likely the other expert on this who could help to propose or evaluate designs that can fix it.

[haxorz]

[@mmorearty] but common --invalid_flag would cause an error.

What's the plan for implementing this?

[alexeagle]

My understanding is @mmorearty isn't working in Bazel related stuff now.

Bazel knows the complete list of flags, e.g. from running bazel help flags-as-proto so I imagine it should be possible to check common --invalid_flag against that and report an error if there is no such flag on any command.

[haxorz]

True. I was worried about a slight performance penalty for doing that in a naive manner, due to how internally Bazel uses lots of reflection to express command options.

After running some benchmarks and not seeing that reflection usage show up in a profiler, I realized I had forgotten we already cache the results which is great because that's the approach I would have recommended we take for this FR :)

---

Anyway, I took a skim at the relevant code and I think this FR will be feasible to implement.

[alexeagle]

Just as FYI: I should link to @illicitonion work on using --invocation_policy to approximate this FR as it already has the ability to apply flags only to commands that support them: #16997 (comment)

[haxorz]

tl;dr - The current approach lets users convey their intent, albeit in an annoying manner. The proposed approach is ambiguous here.

[@alexeagle] I believe that is non-breaking for users, since the only behavior change is for bazel invocation that would fail under existing versions of bazel.

This is true, and is a good point.

I'm concerned about the opposite direction though.

Pretend --whatever is a valid option for at least one command, i.e. the proposed error message would never fire. What if a user sets common --whatever, intending for it to apply to all commands? This FR gives users more convenience but also makes it impossible for Bazel to distinguish between intent "please set this option for all commands" and intent "please set this option for all commands that have it".

[Bear with me for a moment] We actually had something similar to my above example internally at Google a few quarters ago. Our company-wide .bazelrc file wisely had build --experimental_oom_more_eagerly_threshold=XX but unwisely didn't have query --experimental_oom_more_eagerly_threshold=XX. I fixed this by using common. --experimental_oom_more_eagerly_threshold is a common command option (and has been since it was introduced in 2017), so this worked. And the previous lack of common conveyed the [unwise] intent of the old .bazelrc code, and so ought to have been caught during code review. Now imagine it weren't a common command option, but applied only to the build family of commands. It'd be very easy for someone to introduce the exact bug I fixed in such a way that would slip past code review and would be hard for someone to notice in the future!

Therefore I'm inclined to introduce a new way to specify options in .bazelrc-land rather than modify the existing common way. Could do something like all-supported-commands --whatever.

[@alexeagle] However he [@katre] feels that there may be some design discussion required.

The above is just my personal inclination. I'll leave it to the assignee of this FR (and their code reviewer) to come to their own opinions.

On that note, we'd be open to a community contribution for this FR. Otherwise team-Core will try to get to it in the next few quarters.

[fmeum]

Over at #18130, I am implementing a new pseudo command that is inherited by all commands, but with unsupported options ignored instead of failing with an error.

This command is currently called all-supported, but that name is up for bikeshedding.

[fmeum]

The debate about the right naming of the pseudo-commands has led me to think more about @haxorz example in which the distinction between common and all-supported semantics would matter.

Our company-wide .bazelrc file wisely had build --experimental_oom_more_eagerly_threshold=XX but unwisely didn't have query --experimental_oom_more_eagerly_threshold=XX.

It would be interesting to learn why build was chosen in the first place. Today, I could see many users reaching for build when in doubt as the current behavior of common easily risks breaking the less frequently used commands. With the "fixed" behavior, most likely the flag would have been added as common right from the start?

Now imagine it weren't a common command option, but applied only to the build family of commands. It'd be very easy for someone to introduce the exact bug I fixed in such a way that would slip past code review and would be hard for someone to notice in the future!

It definitely would, but having such an option on some but not all commands would seem more like an inconsistency (or even bug?) in Bazel than something users should need to be aware of. The situation is of course different for google3 and especially for Bazel developers, but if the average user then finds that common --experimental_oom_more_eagerly_threshold=XX breaks query, I don't think them going ahead and filing an issue is the most likely outcome. They would probably assume that this is for some good reason and go back to enumerating the individual supported commands.

While I agree that it should remain possible to express this intent explicitly, does it really need to be the default way? Having it not be the default way would perhaps even make this expression of intent even more explicit.

All in all, I think I now lean towards the following approach:

1. Let common ignore options that aren't supported by the current command, but are available on some command. This is backwards compatible as it can only make invocations that previously failed not fail anymore. It could start masking missing options on Bazel commands that hadn't ever been run before the switch in behavior but are intended to pick up the option, but that seems very rare and would be noticed as long as the effect isn't mostly invisible.
2. Introduce a new pseudo-command (required? always?) that behaves exactly as common does today. This can be used to ensure that an option really does take effect for every command (or fails).

This allows users to benefit from the new common semantics with no .bazelrc churn, but provides a way to clearly express the intent of having an option apply to all commands in the cases where this matters. 

Edit: We could hide the new behavior behind an experimental flag for Bazel 6.3.0 so that everyone gets a chance to try it out and report issues. Then we could flip it on for Bazel 7.

[haxorz]

@fmeum Thank you very much for carefully thinking through this issue!

It would be interesting to learn why build was chosen in the first place

It's a pretty messy story. (Googlers, start reading from comment # 3 in buganizer entry # 131156929.)

Originally common was actually chosen (in 2019Q2)! It got switched to build as mitigation for the lack of support of changing values of --experimental_oom_more_eagerly_threshold inter-invocation. The issue there was a sequence of invocations like bazel shutdown; bazel --noworkspace_rc info server_pid; bazel build //blah. The bazel build //blah invocation would fail because it would pick up --experimental_oom_more_eagerly_threshold=XX from <workspace>/.bazelrc.

Support for changing values of was added in 5ef93ef but then we never went back to common until 2022Q3 (when I noticed this mistake, by means of noticing extreme query invocations that were hopelessly GC thrashing forever).

I acknowledge this is a messy and silly story. I also acknowledge my point in my Jan 11 comment was hypothetical ("...Now imagine it weren't a common command option..."), so therefore I acknowledge my point is not very compelling.

While I agree that it should remain possible to express this intent explicitly, does it really need to be the default way?

No, it doesn't. It is the way it is for historical purposes. If we were starting from scratch I don't think we'd end up where we are now.

This is backwards compatible as it can only make invocations that previously failed not fail anymore.

Good point.

It could start masking...

I'm fine with this risk as long as the RELNOTES and docs are good.

Introduce a new pseudo-command (required? always?) that behaves exactly as common does today.

I vote for always.

Edit: We could hide the new behavior behind an experimental flag...

Quis custodiet ipsos custodes: "Who configures the command line configuration parser?" :P

Seems sketchy to have flag parsing code's behavior be controlled by a flag. What's your plan there? Have the options parser code first do a pass looking for a special sentinel option? Seems pretty messy to properly support that in .bazelrc files and especially in --config=XYZ stanzas.

Unless I'm missing something obvious, an environment variable might be the way to go here. If you do this, please have BlazeRuntime consume the environment variable and pass a boolean/enum/whatever to ConfigExpander. This is because ConfigExpander is used as a library by RcFileConfiguration which itself is used as a library internally at Google (I had already privately agreed to update that internal code when you make your commit here). On that note, regardless of which approach you take in this GH issue, it might be good to update the unit tests for RcFileConfiguration` just to make sure it works as expected with your chosen approach.

[haxorz]

@justinhorvitz Tagging you since you're not on this issue thread but you are on the PR thread.

[justinhorvitz]

I think it's fine to make common tolerate options that aren't part of the current command. In fact I believe that the current implementation of all-supported still fails if an option doesn't match any command, right? So it seems unlikely that typos would go unnoticed.

I don't see any immediate need for replacing the current semantics of common. Is anyone asking for it? I would be fine with just changing the behavior of common and I don't think it's necessary to guard it with a startup property but wouldn't be against it if people don't mind waiting longer for the feature to land.

[justinhorvitz]

I don't see any immediate need for replacing the current semantics of common. Is anyone asking for it?

As it turns out, importing #18130 is going to require changing some RC lines from common to always, so it was good that no one listened to me.

[fmeum]

As it turns out, importing #18130 is going to require changing some RC lines from common to always, so it was good that no one listened to me.

Could you share an example of such a case? This would be helpful in determining the possible impact of this change to Bazel users.

[justinhorvitz]

Yes - some details still to be ironed out, but here's a summary. We have some tools that use an rc parsing library which uses only a small subset of the options classes used in blaze. To be explicit, it uses only:

• BuildLanguageOptions
• CommonCommandOptions
• MemoryPressureOptions
• CommonQueryOptions

With your change, it is valid to specify an option from some other options class with common. It would be tolerated by blaze but not by this library, since this library doesn't have access to the rest of the options classes. So our current plan is to prohibit common and instead use always for our global rc files. Users will still be able to use common in their personal rc files.

[haxorz]

[justinhorvitz] We have some tools that use an rc parsing library

@fmeum, fwiw this is the thing I alluded to in my comment last week (ctrl+f "This is because").

Sorry for not foreseeing this complication!

[alexeagle]

Thanks @fmeum for tackling this one! Also thank you to the rules authors SIG funders who paid for this work under https://opencollective.com/bazel-rules-authors-sig