FR: Don't print a warning when a config overrides a non-repeatable option set by another config; also Bug: These warning messages are printed twice

jmmv commented 1 year ago

Description of the feature request:

Whenever flags are repeated on an invocation, Bazel is really noisy. For example:

$ bazel build --config=remote-only //some:target
WARNING: The following configs were expanded more than once: [remote-base]. For repeatable flags, repeats are counted twice and may lead to unexpected behavior.
WARNING: option '--remote_upload_local_results' was expanded to from both option '--config=remote' (source /tmp/bazelrc.5gLB61Wd) and option '--config=remote-only' (source command line options)
WARNING: option '--internal_spawn_scheduler' was expanded to from both option '--config=remote-only' (source command line options) and option '--config=remote-only' (source command line options)
WARNING: option '--remote_local_fallback' was expanded to from both option '--config=remote' (source /tmp/bazelrc.5gLB61Wd) and option '--config=remote-only' (source command line options)
WARNING: option '--spawn_strategy' was expanded to from both option '--config=remote-only' (source command line options) and option '--config=remote-only' (source command line options)
INFO: Invocation ID: 7fc53adc-5952-4562-ba50-4c177dbced0e
WARNING: The following configs were expanded more than once: [remote-base]. For repeatable flags, repeats are counted twice and may lead to unexpected behavior.
WARNING: option '--remote_upload_local_results' was expanded to from both option '--config=remote' (source /tmp/bazelrc.5gLB61Wd) and option '--config=remote-only' (source command line options)
WARNING: option '--internal_spawn_scheduler' was expanded to from both option '--config=remote-only' (source command line options) and option '--config=remote-only' (source command line options)
WARNING: option '--remote_local_fallback' was expanded to from both option '--config=remote' (source /tmp/bazelrc.5gLB61Wd) and option '--config=remote-only' (source command line options)
WARNING: option '--spawn_strategy' was expanded to from both option '--config=remote-only' (source command line options) and option '--config=remote-only' (source command line options)

These warnings all come from our bazelrc, which contains some complex config inheritance to represent various scenarios for remote configuration.

It would be good if we could disable these warnings somehow, or if Bazel didn't print them. Bazel knows which flags support repetition and which ones do not, so it could filter these warnings to only apply to the flags that can be truly problematic.

What underlying problem are you trying to solve with this feature?

Avoid user confusion when they see WARNINGs they have no control of, and also avoid those same users from being desensitized about warning-level messages.

Which operating system are you running Bazel on?

N/A

What is the output of `bazel info release`?

bazel 6.2.0

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

Have you found anything relevant by searching the web?

No response

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

No response

brentleyjones commented 7 months ago

@bazelbuild/triage Can we get this triaged please?

luispadron commented 7 months ago

+1, we see these all the time for common use-cases. Potentially doing these warnings exclusively during use of --announce_rc is one option? It's pretty common to override configs/flags, especially for repeatable ones.

iancha1992 commented 7 months ago

cc: @Wyverald @meteorcloudy

AngelaGuardia commented 2 months ago

+1 on this

meteorcloudy commented 2 months ago

/cc @gregestren

haxorz commented 2 months ago

Hi Julio! Sorry for the lack of response.

Here's my initial impression. Sorry in advance if I missed anything. If so, I'd be happy to pass this along to someone who can look deeper.

Seems like there are three things going on here

(1) You and the others on this GitHub issue think the warning message is not helpful for non-repeatable options. This is a matter of opinion. (2) The warning message is being printed twice. This is clearly spam. (3) Seeming disagreement between @jmmv and @luispadron about how to handle repeatable options.

(1) Imo it's useful in cases where the user made a mistake, because the warning message gives them a hope of reverse engineering what went wrong. But I completely agree it's not-useful and even spammy in cases where everything is WAI from the user's perspective. Without a hint from the user, Bazel doesn't know what situation the user is in.

A compromise could be to err on the side of not being spammy but be less helpful:

Never print a warning message.
But instead ensure there's enough information in the INFO log (bazel info server_log) to figure out what happened, so that way at least power users can self-diagnose and Bazel experts can support other users.
Be willing to revisit this decision if we see the lack of warning message be harmful to users.

We kinda already have enough information, from the --default_override=K:qqq:www=zzz entries in the argument list INFO-logged here. I think it'd be helpful to also just INFO-log the sort of warning message we're currently printing.

(2) Looks like it's happening only for the build family of commands. Demo:

nharmata@nharmata:~/bazel-scratch$ head -n 3 .bazelrc
always:same1 --keep_going=true
always:same2 --keep_going=true
always:different --keep_going=false
nharmata@nharmata:~/bazel-scratch$ bazel query --config=same1 --config=different "set()"
WARNING: option '--keep_going' was expanded to from both option '--config=same1' (source command line options) and option '--config=different' (source command line options)
INFO: Empty results
nharmata@nharmata:~/bazel-scratch$ bazel build --config=same1 --config=different
WARNING: option '--keep_going' was expanded to from both option '--config=same1' (source command line options) and option '--config=different' (source command line options)
WARNING: option '--keep_going' was expanded to from both option '--config=same1' (source command line options) and option '--config=different' (source command line options)
WARNING: Usage: bazel build <options> <targets>.
Invoke `bazel help build` for full description of usage and options.
Your request is correct, but requested an empty set of targets. Nothing will be built.
INFO: Found 0 targets...
INFO: Elapsed time: 0.068s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
nharmata@nharmata:~/bazel-scratch$

I think this is an unintended consequence of 943e8cfb9970ca86f88ba60c944601573b366db5 ("Parse command-line options twice; the second time with the main repo mapping"). Stepping through my above demo in a debugger, this behavior is Working As Implemented (if we're doing parsing twice we're going to be discovering and printing warnings twice) but I imagine it's not Working As Intended. @Wyverald @aranguyen wdyt? Maybe the fix here is to have the second parse call simply not print warning messages or deprecation messages. Are there maybe other unintended consequences of parsing a second time?

(3) Anyone have more thoughts here? I'd prefer we have a single approach for all repeatable options (rather than try to tag/determine which options are "safe" to repeat).

That being said, the compromise in (1) would involve Bazel never printing a warning message. Is everyone okay with that?

luispadron commented 2 months ago

Seeming disagreement between @jmmv and @luispadron about how to handle repeatable options.

Reading the warning again, I think my original comment is confusing and what I meant to say is: I don't know that showing a warning for a flag that gets strictly overridden (not added too) makes sense.

I can however see a case for wanting to warn about a flag being used more than once that would add extra values to that flag, as that may be unexpected?

Good find on the double parsing being the root cause for why the warnings are printed twice!

Wyverald commented 2 months ago

Maybe the fix here is to have the second parse call simply not print warning messages or deprecation messages.

Ooh, thanks for finding this. I wonder if it's possible to not show any duplicate warnings (instead of outright silencing everything from the second pass). That would be slightly more preferable, as technically we could encounter something fishy only during the second pass. But I don't think that really happens in practice, so I'd also be happy to just silence all warnings from the second pass.

Are there maybe other unintended consequences of parsing a second time?

I can't think of any, but then again I didn't think of any before either, and that led to this bug...

haxorz commented 2 months ago

Seeming disagreement between @jmmv and @luispadron about how to handle repeatable options.

Reading the warning again, I think my original comment is confusing and what I meant to say is: I don't know that showing a warning for a flag that gets strictly overridden (not added too) makes sense.

I can however see a case for wanting to warn about a flag being used more than once that would add extra values to that flag, as that may be unexpected?

K, thanks for clarifying!

I've retitled the GitHub issue.

bazelbuild / bazel