Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --low_priority_bazelrc #16997

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

illicitonion
Copy link
Contributor

This startup flag behaves exactly as --bazelrc does, but is applied before the workspace bazelrc rather than after.

This is useful for people who write and distribute bazel wrappers (which appears to be most large companies). This allows a bazel wrapper to supply new default values to flags, which a repo may choose to override.

Without this change, it's impossible for a bazel wrapper to decide to do something like set --incompatible_enable_cc_toolchain_resolution because they have no practical way of doing so. if they set this on the command line or in a .bazelrc file passed to the --bazelrc flag, it cannot be overridden other than on the command line. This flag allows a wrapper to point at a bazelrc file with extra defaults, which can be overridden in the repo bazelrc file.

This startup flag behaves exactly as --bazelrc does, but is applied
before the workspace bazelrc rather than after.

This is useful for people who write and distribute bazel wrappers (which
appears to be most large companies). This allows a bazel wrapper to
supply new _default_ values to flags, which a repo may choose to
override.

Without this change, it's impossible for a bazel wrapper to decide to do
something like set `--incompatible_enable_cc_toolchain_resolution`
because they have no practical way of doing so. if they set this on the
command line or in a .bazelrc file passed to the `--bazelrc` flag, it
cannot be overridden other than on the command line. This flag allows a
wrapper to point at a bazelrc file with extra defaults, which can be
overridden in the repo bazelrc file.
@illicitonion
Copy link
Contributor Author

/cc @sdtwigg - we discussed this at BazelCon
/cc also @alexeagle and @brentleyjones as maintainers of significant Bazel wrappers

@brentleyjones
Copy link
Contributor

brentleyjones commented Dec 12, 2022

What is the behavior of multiple --low_priority_bazelrc? Mainly wondering on what the order of everything is when I call bazel like this:

bazel --bazelrc=a.bazelrc --low_priority_bazelrc=low.a.bazelrc --bazelrc=b.bazelrc --low_priority_bazelrc=low.b.bazelrc

@illicitonion
Copy link
Contributor Author

What is the behavior of multiple --low_priority_bazelrc? Mainly wondering on what the order of everything is when I call bazel like this:

bazel --bazelrc=a.bazelrc --low_priority_bazelrc=low.a.bazelrc --bazelrc=b.bazelrc --low_priority_bazelrc=low.b.bazelrc

All --low_priority_bazelrc flags are processed, left to right, before the workspace bazelrc flag, which is processed before the --bazelrc flags, left to right.

So we'd effective process:

low.a.bazelrc
low.b.bazelrc
.bazelrc
a.bazelrc
b.bazelrc

with later entries in the list taking precedence over earlier ones.

@meisterT meisterT added the team-Core Skyframe, bazel query, BEP, options parsing, bazelrc label Dec 12, 2022
@michajlo
Copy link
Contributor

@sgowroji sgowroji added the awaiting-review PR is awaiting review from an assigned reviewer label Dec 13, 2022
@illicitonion
Copy link
Contributor Author

Have you considered invocation policy instead? https://github.com/bazelbuild/bazel/blob/master/src/main/protobuf/invocation_policy.proto

A great question! I had not, but I've experimented with it now, and think there are some pros and cons; here are some thoughts:

Pros:

  1. --invocation_policy is an existing API surface - reducing API surface (particularly for what's already a somewhat complicated corner of the UX) is good.
  2. --invocation_policy supports setting flag values for "all commands which support them", by not setting FlagPolicy.commands. This is a highly desirable behaviour (and there was discussion at BazelCon about wanting to introduce an "applicable" scope to bazelrc files which behaves the same way, like many people expect the common scope to behave already)

Cons:

  1. --invocation_policy can only be set exactly once. This is reasonable, as it's not obvious how to automatically merge multiple values of the flag. But if you imagine a world where there's a common CLI wrapper, like Aspect CLI wants to set some defaults; then some company-internal infrastructure team wants to set some defaults around specific remote execution endpoints/platforms/toolchains (and potentially then some specific team wants to set their own too), this causes problems. I think the core of the problem here is that --invocation_policy is very powerful and expressive (allowing a complex set of defaults, overrides, and restrictions), where just default setting is a much simpler concept where merging in precedence order is much easier to reason about.
  2. Most users are not familiar with --invocation_policy (but are familiar with bazelrc files, because most repos end up having one); so in terms of concepts to understand and reason about, bazelrc files are probably more clear.

There are also a few missing features and rough edges around --invocation_policy that make it less desirable than a more bazelrc-centric approach:

  1. When overrides are forbidden, but the value is specified anyway on the command line, the behaviour appears to be to silently ignore the user-specified value, rather than to error. This would be very surprising behaviour for most of my users. This seems fixable, but would be a breaking change (though an optional setting on InvocationPolicy could make this less scary to change).
  2. Worse, defaults don't show up in places like --announce_rc, so debugging "Why is my value being ignored, and why is some other value being used instead" is tricky.
  3. --invocation_policy doesn't appear to support --config-gated settings. In a bazelrc file I can write build:linux --extra_toolchains=... or build:remote --extra_toolchains=..., but I don't believe invocation_policy supports this. This seems easily fixable, though, replacing repeated string commands with a repeated CommandAndOptionalConfigMessage commands_and_configs.

@michajlo
Copy link
Contributor

In theory per-tool/org config policies is what invocation-policy is for, though I concede this isn't terribly well documented. If there are gaps then I think it's worth figuring out if we can close them before adding another rc knob. Do you think you could see if an invocation-policy approach could be made tenable?

@illicitonion
Copy link
Contributor Author

If there are gaps then I think it's worth figuring out if we can close them before adding another rc knob.

I definitely agree with closing the gaps - I've just sent out #17035 for the --announce_rc change, and will have a look at the not announcing overrides piece next.

In theory per-tool/org config policies is what invocation-policy is for [...] Do you think you could see if an invocation-policy approach could be made tenable?

I think it could be made tenable for the particular use-case I'm looking at right now, but I do worry long-term about the hierarchy issues - I can easily imagine a world where three organisations want to progressively apply their own invocation policies (e.g. as described, aspect-CLI, an infra team, and a product team). Considering how we can merge multiple --invocation_policy flags would be interesting for the future.

@brentleyjones
Copy link
Contributor

I can easily imagine a world where three organisations want to progressively apply their own invocation policies (e.g. as described, aspect-CLI, an infra team, and a product team)

I can do more than imagine this, as rules_xcodeproj and BuildBuddy Workflows already do this, and eventually we will add the BuildBuddy CLI into the mix.

@illicitonion
Copy link
Contributor Author

I can easily imagine a world where three organisations want to progressively apply their own invocation policies (e.g. as described, aspect-CLI, an infra team, and a product team)

I can do more than imagine this, as rules_xcodeproj and BuildBuddy Workflows already do this, and eventually we will add the BuildBuddy CLI into the mix.

It feels like the really fiddly piece of merging invocation policies is around when values are banned. In all other cases, concatenating the policies is a reasonable thing to do (and it's on tools to make sure they pass the flags in the correct order). Maybe we could resolve this by making it allowed to have multiple --invocation_policy flags, but only if they never conflict in terms of banning values?

@michajlo
Copy link
Contributor

I agree the crux will be figuring out how to merge the policies. Without thinking too deep into it, one possibility is allowing individual policies to set their own merge policy, but this could use more thought/discussion (maybe lifted to an issue?).

Note that it's entirely possible that low_priority_bazelrc or something like it wind up being what we want here in the end. The goal of my comments is to ensure we're making the right, balanced decision in the org/tool-specific config case. Conceivably, a follow-up FR would be "I need a way to prevent my org/tool users from overriding flags that break compatibility or would result in contacting untrusted services" - invocation policy already does this, it would be an entirely new category of rc-file features though, at which point we'd have largely duplicated invocation policy in rc-files.

I should also mention, there's some uncertainty about the layering of rc files, divergence in how rc files are handed in bazel/blaze that would need to be reconciled, and tools that introspect rc files that would need to be updated. I'm hoping that by exploring invocation policy we could sidestep dealing with these things :).

@illicitonion
Copy link
Contributor Author

I agree the crux will be figuring out how to merge the policies. Without thinking too deep into it, one possibility is allowing individual policies to set their own merge policy, but this could use more thought/discussion (maybe lifted to an issue?).

Sounds good - @brentleyjones can I interest you in maybe taking the lead on that, as someone with a concrete use-case today?

Note that it's entirely possible that low_priority_bazelrc or something like it wind up being what we want here in the end. The goal of my comments is to ensure we're making the right, balanced decision in the org/tool-specific config case.

Makes sense!

  • When overrides are forbidden, but the value is specified anyway on the command line, the behaviour appears to be to silently ignore the user-specified value, rather than to error. This would be very surprising behaviour for most of my users. This seems fixable, but would be a breaking change (though an optional setting on InvocationPolicy could make this less scary to change).

I've also filed #17042 to address this one, choosing to warn (rather than be silent or error).

@brentleyjones
Copy link
Contributor

I'm willing to test out changes or provide feedback, but I have a lot on my plate currently and can't really take anything else on for a while. We also have work arounds that work for now, so this isn't a blocker for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Core Skyframe, bazel query, BEP, options parsing, bazelrc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants