Error on odd .bazelrc lines #19
Conversation
|
@iancha1992 can we get a review of this? |
|
|
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) |
|
RC parsing belongs to Scalability (team-core due to legacy naming). |
|
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! |
|
Justin can you please take a look? Follow up with me internally about the internal work. |
|
This change looks great! I did notice it still doesn't diagnose this invalid case: |
WalkthroughThe changes introduce stricter validation and error handling for parsing and processing rc files and command options. In the C++ code, the parser now checks for incomplete command lines and returns an error if a command lacks arguments. In the Java code, encountering an invalid command name during rc options structuring now results in an exception rather than a warning, altering the control flow to halt processing on such errors. Corresponding tests in both C++ and Java have been updated or added to verify these new error conditions and ensure the correct exceptions and messages are produced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RcFileParser (C++)
User->>RcFileParser: Parse rc file
RcFileParser->>RcFileParser: Check for command token
alt Command token present
RcFileParser->>RcFileParser: Check for arguments
alt Arguments missing
RcFileParser->>User: Return INVALID_FORMAT error with message
else Arguments present
RcFileParser->>User: Continue parsing
end
else Command token missing
RcFileParser->>User: Return INVALID_FORMAT error
end
sequenceDiagram
participant User
participant BlazeOptionHandler (Java)
User->>BlazeOptionHandler: structureRcOptionsAndConfigs(...)
BlazeOptionHandler->>BlazeOptionHandler: Validate command name
alt Command name invalid
BlazeOptionHandler->>User: Throw OptionsParsingException
else Command name valid
BlazeOptionHandler->>User: Continue processing options
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code Graph Analysis (2)src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java (1)
src/test/cpp/rc_options_test.cc (1)
🔇 Additional comments (8)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@keith This is not invalid per se. AFAIU it translates to Am I missing something ? |
Ah yea I guess it depends on how this should be interpreted, I expected the |
|
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: then there was an effort to remove uses of 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. |
|
Or another option is to display a warning for the single token case that's currently ignored. Would this solve your motivating scenario? |
|
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. |
|
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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses an issue in the client parser for .bazelrc files, where lines with a single word (e.g., a command without flags or just a flag) were silently ignored. I've modified the client to reject such incomplete lines, preventing potential misconfigurations. Additionally, I've promoted a warning about unrecognized command names to an error. This change might break existing configurations that rely on commenting only the flag or using empty commands for block organization.
Highlights
- Error Handling: The pull request introduces stricter error handling for
.bazelrcfiles, specifically for incomplete lines (lines with only a command or a flag) and invalid command names. - Breaking Change: This change is potentially breaking for users who have
.bazelrcfiles with single-word lines (e.g.,common # --host_jvm_args=-Xmx1024mor justcommon). - Test Coverage: The pull request includes updates to the test suite to reflect the new error handling behavior, ensuring that incomplete lines and invalid command names now result in parsing errors.
Changelog
Click here to see the changelog
- src/main/cpp/rc_file.cc
- Added a check to ensure that
.bazelrclines contain both a command and at least one argument. If a line only contains a single word, aParseError::INVALID_FORMATerror is returned. - The error message includes the filename and the offending line.
- Added a check to ensure that
- src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
- Changed the handling of unrecognized command names from a warning to an
OptionsParsingException.
- Changed the handling of unrecognized command names from a warning to an
- src/test/cpp/rc_options_test.cc
- Added a helper function
FailToParseRcWithErrorAndRegexto simplify testing for specific parse errors and error messages. - Updated existing tests and added new tests to verify that incomplete lines in
.bazelrcfiles now result inParseError::INVALID_FORMATerrors. - Tests were added to verify that invalid import declarations now result in
ParseError::INVALID_FORMATerrors. - Tests were added to verify that import cycles are detected and reported as
ParseError::IMPORT_LOOPerrors. - Tests were added to verify that non-existent files are reported as
ParseError::UNREADABLE_FILEerrors.
- Added a helper function
- src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java
- Updated the test
testStructureRcOptionsAndConfigs_invalidCommandto assert that anOptionsParsingExceptionis thrown when an invalid command is encountered, instead of just checking for a warning event.
- Updated the test
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A config's cryptic call,
Ignored, it tripped up all.
Now errors bloom,
Dispelling gloom,
No silent failures enthrall.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
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 |
ah, that's true... is a startup flag an option, or is that also too late/tricky to implement? |
|
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 ? |
|
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 (I didn't know this, but apparently an rc file line like 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). |
There was a problem hiding this comment.
Code Review
This pull request introduces a breaking change to the .bazelrc parsing logic, which now enforces that every line must start with a command. This change prevents silent failures due to incomplete lines and improves error reporting. The tests have been updated to reflect this change. Overall, the changes seem reasonable and improve the robustness of the .bazelrc parsing.
Summary of Findings
- Breaking Change: The change to require a command on every line in .bazelrc is a breaking change that may affect existing users. Ensure that the impact is well-understood and communicated.
- Error Message Clarity: The error message for incomplete lines could be improved to provide more context to the user, such as suggesting to add a command or comment out the line.
- Test Coverage: The tests cover various scenarios, but it's important to ensure that all edge cases and potential user errors are covered, especially those related to the breaking change.
Merge Readiness
The pull request introduces a breaking change, so it's important to ensure that the impact is well-understood and communicated to users. The code changes improve the robustness of .bazelrc parsing and provide better error reporting. The tests have been updated to reflect the changes. I recommend addressing the review comments before merging. I am unable to approve this pull request, and other reviewers should review and approve this code before merging.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
You need to send this to the actual bazelbuild/bazel project: you are free to merge this to your own coderabbit-test/bazel project as you wish, we don't need to review it. |
|
@visz11, @arvi18 I am not too happy to suddenly get notified of all that happens on this repo. Do you need to use real data and ping real ppl for your tests ? @katre got confused and I was certainly too. Not to mention it is a bit shady to appropriate all the IP of other ppl. The real PR is here: bazelbuild#20895 @visz11, @arvi18 Could you turn this repo private to do your testing ? |
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 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 bazelbuild#15245
Summary by CodeRabbit