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

Not possible to override string flag in the config section of bazelrc file from command line #13231

Closed
AndersSundman opened this issue Mar 16, 2021 · 9 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug
Milestone

Comments

@AndersSundman
Copy link
Contributor

AndersSundman commented Mar 16, 2021

Description of the problem / feature request:

Flags added to the build:linux config (from --enable_platform_specific_config=true), can't be overridden from the command line.

The command line flag value should take precedence, as it does when the flag is added to any other config, but right now the command line option is ignored.

See repro below for a more detailed description of the issue.

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

We would like to set a skylib string_flag (a build_config rule) to a default value for Linux but with the option to override that value from the command line.

Here is a small BUILD file that introduces the color flag and uses it to select a define or another in a cc_binary rule.

load("@bazel_skylib//rules:common_settings.bzl", "string_flag")

config_setting(
    name = "is_red",
    flag_values = {
        ":color": "red",
    },
)

config_setting(
    name = "is_black",
    flag_values = {
        ":color": "black",
    },
)

string_flag(
    name = "color",
    build_setting_default = "undefined",
    values = [
        "red",
        "black",
        "undefined",
    ],
)

cc_binary(
    name = "test",
    srcs = ["test.cpp"],
    copts = select({
        ":is_red": ["-DRED"],
        ":is_black": ["-DBLACK"],
    }),
)

Here is a .bazelrc file

build --enable_platform_specific_config=true
build:linux --//:color=red

The intention is for color to be undefined for all platforms except linux. For linux it gets set to red in the .bazelrc file.

The bug is that it's not possible to override this color value from the command line

bazel build //:test --//:color=black

Even when doing this ^ (on linux), the value of color is red.

Without the build:linux in the .bazelrc, overriding works as expected:

This bazel build //:test --//:color=red --//:color=black makes color black.

Even adding another build:red --//:color=red config to the .bazelrc works as expected. I.e. bazel build //:test --config=red --//:color=black makes color black.

The build:linux config must be special in some (faulty) way.

What operating system are you running Bazel on?

Linux (but I expect that this is the same with auto detected windows config, etc)

What's the output of bazel info release?

release 4.0.0

Have you found anything relevant by searching the web?

We have looked, but haven't been able to find any other reports of this bug.

@AndersSundman
Copy link
Contributor Author

Here is a complete repro example: example.tar.gz

@oquenchil oquenchil added team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request untriaged labels Mar 19, 2021
@aiuto
Copy link
Contributor

aiuto commented Apr 7, 2021

@meteorcloudy You added this flag. Can you comment on it? It's not really Configurability's wheelhouse.

@meteorcloudy meteorcloudy added P2 We'll consider working on this in future. (Assignee optional) type: bug and removed untriaged labels Apr 7, 2021
@meteorcloudy
Copy link
Member

This looks like a bug, I'll take a look

@aiuto
Copy link
Contributor

aiuto commented Apr 7, 2021

This feature has some problems as implemented. Most of the time people use config:xx to specify the target platform configuration. The examples that people pointed out in #5055 are mostly about the need to auto-specify different options based on the host platform.

The fact that it just selects on 'linux' makes it too easy to fall into the trap of putting both host specific and target specific configuration under the same heading 'linux'.

Proposal:

  • change behavior so it picks up build:host=linux instead of build:linux
  • enable_platform_specific_config is True by default. Or, just remove it because we have just changed the --config processing to be a little more useful.

@aiuto aiuto removed team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request labels Apr 7, 2021
@meteorcloudy
Copy link
Member

change behavior so it picks up build:host=linux instead of build:linux

The documentation says it clearly it will pick based on the host configurations, anyone who uses this flag should be aware of that. I agree we should have picked a better name, but changing it now will just break our uesrs.

enable_platform_specific_config is True by default. Or, just remove it because we have just changed the --config processing to be a little more useful.

I don't quite understand why should we turn it on by default or just remove it.

@meteorcloudy
Copy link
Member

meteorcloudy commented Apr 16, 2021

@AndersSundman

I looked a bit in to this, it doesn't seem to be caused by the --enable_platform_specific_config flag. Because if you turn off --enable_platform_specific_config and do
bazel build //:test --config=linux --//:color=black. The color would still be red.

I tested some other flags (--build, --compiler), the override works as expected with configs in bazelrc file. It seems the problem is string_flag being used in .bazelrc and command line.

@meteorcloudy
Copy link
Member

Even adding another build:red --//:color=red config to the .bazelrc works as expected. I.e. bazel build //:test --config=red --//:color=black makes color black.

@AndersSundman Are you sure this works?

@meteorcloudy meteorcloudy changed the title Not possible to override config flag from command line with enable_platform_specific_config Not possible to override string flag in the config section of bazelrc file from command line Apr 16, 2021
@meteorcloudy meteorcloudy assigned aiuto and unassigned meteorcloudy Apr 16, 2021
@meteorcloudy meteorcloudy added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Apr 16, 2021
@aiuto aiuto removed their assignment Apr 16, 2021
@AndersSundman
Copy link
Contributor Author

Even adding another build:red --//:color=red config to the .bazelrc works as expected. I.e. bazel build //:test --config=red --//:color=black makes color black.

@AndersSundman Are you sure this works?

I can confirm that it seems to be related to the string flags.

I tested the config experiment again and it doesn't seem to work. I must have confused something before.

bazel build //:test --config=red --//:color=black with a build:red --//:color=red in .bazelrc still makes it red.

@wudisheng
Copy link

I experienced exactly the same bug in 4.2.1 release.

@aiuto aiuto added this to the flags cleanup milestone Dec 10, 2021
fmeum added a commit to fmeum/bazel that referenced this issue Aug 16, 2022
Previously, Starlark flags expanded from a --config stanze would always
be added to the end of the residue containing the explicitly given
Starlark flags. As a result, Starlark flags expanded from --config could
not be overridden.

With this commit, Starlark flags are parsed with the same semantics as
a regular allowMultiple option, thus preserving their order through
expansions.

This is implemented by introducing a synthetic allowMultiple option of
type List<String> in OptionsParserImpl that skipped args are parsed
into.

As a result, Starlark options are now available from the new
getSkippedArgs() method on OptionsParser rather than as part of the
residue, with the latter now only containing build targets.

Fixes bazelbuild#13231
Fixes bazelbuild#15679

Closes bazelbuild#15807.

PiperOrigin-RevId: 467931815
Change-Id: Ic64c6e075c08d898e5e7b8bf4c777827134d89fa
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
Previously, Starlark flags expanded from a --config stanze would always
be added to the end of the residue containing the explicitly given
Starlark flags. As a result, Starlark flags expanded from --config could
not be overridden.

With this commit, Starlark flags are parsed with the same semantics as
a regular allowMultiple option, thus preserving their order through
expansions.

This is implemented by introducing a synthetic allowMultiple option of
type List<String> in OptionsParserImpl that skipped args are parsed
into.

As a result, Starlark options are now available from the new
getSkippedArgs() method on OptionsParser rather than as part of the
residue, with the latter now only containing build targets.

Fixes bazelbuild#13231
Fixes bazelbuild#15679

Closes bazelbuild#15807.

PiperOrigin-RevId: 467931815
Change-Id: Ic64c6e075c08d898e5e7b8bf4c777827134d89fa
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: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants