Error on odd .bazelrc lines

[layus]

I have tried to spare users a long debugging session searching for a .bazelrc
line that was not applied.

The main issue is in the client parser, which assumes that the first word on a line is always a command. If you are unlucky enough, you forgot the command an passed a single flag. The client assumes it is a command with no flags, and passes nothing to the server, hence silently ignoring the bogus line.

I have changed the client to refuse lines with a single word, which is both simpler to do, and prevents what seems like a code smell anyway.

It is thus a breaking change for users who used to comment only the flag and not the command, or those who have empty commands to keep blocks together.
Overall, the RC files lack a proper specification, which makes it impossible to know precisely what is valid.

# The two following lines will no more work.
common # --host_jvm_args=-Xmx1024m
common

The second commit is optional. It turns a warning about unrecognized command names into an error.
I think it makes sense to address these issues early. It may break users config if commands are removed in the future, but that set is pretty stable now.

Fixes #15245

[brentleyjones]

@iancha1992 can we get a review of this?

[iancha1992]

@iancha1992 can we get a review of this?

cc: @Wyverald @meteorcloudy

[Wyverald]

honestly not sure whether this is team-Core or team-Configurability. So I'll ping both of them! @haxorz @gregestren (who's OOO, so @katre in lieu)

[katre]

RC parsing belongs to Scalability (team-core due to legacy naming).

[haxorz]

Yes, and see my comments on the linked issue. Whoever handles this PR internally needs to do that work.

Thank you @layus for moving this forward!

[haxorz]

Justin can you please take a look? Follow up with me internally about the internal work.

[keith]

This change looks great! I did notice it still doesn't diagnose this invalid case:

build --copt=-v \
 # comment \
 --copt=-invalid

[layus]

This change looks great! I did notice it still doesn't diagnose this invalid case:

build --copt=-v \
 # comment \
 --copt=-invalid

@keith This is not invalid per se.

AFAIU it translates to build --copt=-v # comment --copt=-invalid which is valid, and should be expected by the users. At least if they are the kind of users that like shooting themselves in the foot by using backslashes.

Am I missing something ?

[keith]

AFAIU it translates to build --copt=-v # comment --copt=-invalid which is valid, and should be expected by the users. At least if they are the kind of users that like shooting themselves in the foot by using backslashes.

Ah yea I guess it depends on how this should be interpreted, I expected the \ to work the same as bash, which means when used in a comment line it is not respected and is just interpreted as any other character in the comment

[justinhorvitz]

I've started to look into this and have some bad news. I ran the new parser over google-internal checked-in blazerc files, and on a sample of over 700 files, the breakage rate is nearly 2%. This doesn't even include non-checked-in blazerc files on user machines.

While it looks like most of these broken samples really should be errors, I think we need a better way to roll this out than just accepting this PR and expecting people to fix their blazercs when it's released.

To give an example of a breakage, there is a blazerc file that had the following line:

build --some_flag

then there was an effort to remove uses of --some_flag, which left the blazerc with the line:

build

which is now invalid with this PR.

---

We'll need to discuss how to proceed with this and whether it's worth the investment. Would you be open to an optional "strict mode" for rc file parsing? That might end up being the most feasible way to make progress, sadly.

[justinhorvitz]

Or another option is to display a warning for the single token case that's currently ignored. Would this solve your motivating scenario?

[Wyverald]

it'd probably be best if we have an incompatible flag for this anyhow, as it's a breaking change. We can only make this the default behavior in Bazel 8.0 at the earliest, so having a flag users could toggle on in 7.x would be the best way forward.

[justinhorvitz]

Actually, seems tricky to have a flag that controls the semantics of flag parsing. Might need to be a system property. Makes it seem more appealing to just add a warning.

[keith]

I think warnings are easy to lose in the sea of logs that come from bazel in a large project. It seems like a flag would be ideal so google could disable that until the violations are fixed

[Wyverald]

Actually, seems tricky to have a flag that controls the semantics of flag parsing. Might need to be a system property.

ah, that's true... is a startup flag an option, or is that also too late/tricky to implement?

[justinhorvitz]

In the single token case, we could use an empty string. Then the server would error if the single token is not a valid command name. That would push all verification to the server, making a startup option (or system property) possible.

[layus]

In the single token case, we could use an empty string. Then the server would error if the single token is not a valid command name. That would push all verification to the server, making a startup option (or system property) possible.

I think this is the best way forward. Does the server already accept empty strings as options ?

[justinhorvitz]

When passed an empty string with an invalid command name, it works (currently a warning today). When passed an empty string with a valid command name, it adds the empty string as an extra command line token. For example, a build command gives the error:

ERROR: Skipping '': invalid target name '': empty target name

(I didn't know this, but apparently an rc file line like build //target will build that target on every build command.)

If we do pass an empty string from the client for a single-token line, seems like a good idea to have the server reject it with a helpful error message when the command name is valid (if the strict mode is enabled) or ignore it (if the strict mode is disabled).