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

common does not work with flag_alias #20081

Closed
luispadron opened this issue Nov 7, 2023 · 8 comments
Closed

common does not work with flag_alias #20081

luispadron opened this issue Nov 7, 2023 · 8 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@luispadron
Copy link
Contributor

Description of the bug:

Using the new common in a .bazelrc with flag_alias returns the following error:

ERROR: /Users/<user>/Development/cash-ios/.bazelrc: "common --flag_alias=build_config=//:build_config" disallowed. --flag_alias only supports the "build" command.

Which category does this issue belong to?

CLI, Configurability

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

  • Create a custom config_setting
  • Create a .bazelrc
  • Attempt to add a flag alias with the common command
common --flag_alias=release_variant=//:release_variant

Which operating system are you running Bazel on?

macOS

What is the output of bazel info release?

release 6.3.2

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

No response

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

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

No response

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

Not entirely sure if this is expected but it feels like its not given how common works for other flags.

@katre
Copy link
Member

katre commented Nov 7, 2023

Which commands besides build (and its logical descendents, like test and cquery) do you think need support for --flag_alias?

@luispadron
Copy link
Contributor Author

Which commands besides build (and its logical descendents, like test and cquery) do you think need support for --flag_alias?

For our use case, none and I'm not sure where it might be useful in other commands. I was just basing this off of how common seems to be working for us with other flags. We've been able to replace usage of build in bazelrc with common except for this case.

@katre
Copy link
Member

katre commented Nov 7, 2023

The code that checks this is in BlazeOptionHandler, and the tests are in starlark_configurations_test.sh.

The test makes it look like this is deliberate, and @gregestren added the check that it isn't supported for test, so I'm going to assign to him for further triage.

@katre katre assigned gregestren and unassigned sgowroji, Pavank1992 and iancha1992 Nov 7, 2023
@katre katre added type: feature request P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions and removed type: bug untriaged labels Nov 7, 2023
@katre
Copy link
Member

katre commented Nov 7, 2023

I'm also re-classifying this as a Feature Request instead of a Bug, since the behavior seems to be intentional.

@luispadron
Copy link
Contributor Author

Thats fair, I was considering this a bug based on this: #3054

@gregestren
Copy link
Contributor

329b22a is the original change.

The intention was to not allow flag alias definitions for subsets of build (like test). Motivated specifically by CI systems that start with a $ bazel canonicalize-flags to vet flags. canonicalize-flags itself is a subset of build and any setting has to be reflected there

common goes the other way up the inheritance tree and sounds safe to me.

@gregestren
Copy link
Contributor

This is basically a lowest common ancestor problem.

We can only apply flag alias definitions on nodes in canonicalize-flags's ancestor path.

bazel-io pushed a commit to bazel-io/bazel that referenced this issue Nov 10, 2023
Fixes bazelbuild#20081

Closes bazelbuild#20107.

PiperOrigin-RevId: 581112821
Change-Id: I54941072faa8e9843adcdb7ff8d73ab7414dc86a
katre pushed a commit to bazel-io/bazel that referenced this issue Nov 10, 2023
Fixes bazelbuild#20081

Closes bazelbuild#20107.

PiperOrigin-RevId: 581112821
Change-Id: I54941072faa8e9843adcdb7ff8d73ab7414dc86a
katre pushed a commit to bazel-io/bazel that referenced this issue Nov 10, 2023
Fixes bazelbuild#20081

Closes bazelbuild#20107.

PiperOrigin-RevId: 581112821
Change-Id: I54941072faa8e9843adcdb7ff8d73ab7414dc86a
keertk pushed a commit that referenced this issue Nov 10, 2023
…mmands (#20134)

Fixes #20081

Closes #20107.

Commit
19c73bc

PiperOrigin-RevId: 581112821
Change-Id: I54941072faa8e9843adcdb7ff8d73ab7414dc86a

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
copybara-service bot pushed a commit that referenced this issue Nov 14, 2023
We can lightly refactor test code to no longer need a logical exception in Bazel's option parsing.

Supports #20107 and #20081.

Closes #20111.

Change-Id: I2ea3cc5b67a8a5d047972b2c85ae3a93680be612
PiperOrigin-RevId: 582178022
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.0.0 RC5. Please test out the release candidate and report any issues as soon as possible. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants