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

bazelrc line with missing command should fail fast instead of silently ignoring #15245

Open
alexeagle opened this issue Apr 13, 2022 · 6 comments · May be fixed by #20895
Open

bazelrc line with missing command should fail fast instead of silently ignoring #15245

alexeagle opened this issue Apr 13, 2022 · 6 comments · May be fixed by #20895
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: feature request

Comments

@alexeagle
Copy link
Contributor

Description of the bug:

It's very easy to forget the command in a bazelrc file, for example this mistake in
https://github.com/aspect-build/rules_js/pull/34/files
--host_jvm_args=-DBAZEL_TRACK_SOURCE_DIRECTORIES=1 simply has no effect because it's missing a command before the flags appear on that line, the correct way is

startup --host_jvm_args=-DBAZEL_TRACK_SOURCE_DIRECTORIES=1

This silent failure is confusing to users, and a source of bugs. I've seen many cases at clients where lines in the bazelrc are being silently ignored.

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

No response

Which operating system are you running Bazel on?

No response

What is the output of bazel info release?

No response

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

@alexeagle alexeagle changed the title bazelrc with missing command silently does nothing bazelrc line with missing command silently does nothing Apr 13, 2022
@brandjon brandjon added type: feature request P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed untriaged labels Apr 28, 2022
@brandjon brandjon removed their assignment Apr 28, 2022
@brandjon brandjon changed the title bazelrc line with missing command silently does nothing bazelrc line with missing command should fail fast instead of silently ignoring Apr 28, 2022
@brandjon
Copy link
Member

Not familiar with the code off-hand, but it's possible the rc file has parsers in both Java and C++ (in order for the client to interpret startup flags).

@brandjon brandjon added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc and removed team-CLI Console UI labels Dec 14, 2022
@haxorz
Copy link
Contributor

haxorz commented Feb 6, 2023

This is the same as internal issue 69956078 (Nov 2017).

See my comment 6 there (Dec 2022) for an internal-only thing we'd need to do if we want to do a good job addressing this issue.

@haxorz haxorz added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) untriaged labels Feb 6, 2023
@layus
Copy link
Contributor

layus commented Jan 9, 2024

Hi,

I just got bitten by this one, and it hurts pretty bad given I knew I added the flags, I just missed that damn "build" in front of them.

I see there are some internal blockers. Anything we can do to help move it forward, or work around internal code inside open Bazel ? What is the status of this internal issue ?

For reference: https://bazelbuild.slack.com/archives/CA31HN1T3/p1704752021290139

@haxorz
Copy link
Contributor

haxorz commented Jan 9, 2024

There's some Google-internal code that separately implements the .blazerc parsing logic. It was a port of the C++ code used in the Bazel client codebase (the code in question for this GitHub issue here). We want this code and that code to have the same behavior. So the blocker would be that whoever imports the PR for this GitHub issue here also needs to port the change to that code.

One way to help move this GitHub issue forward would be to mail a PR that fixes the bug. That would spur someone to review it (and then have to do the additional work mentioned above).

@layus layus linked a pull request Jan 15, 2024 that will close this issue
@layus
Copy link
Contributor

layus commented Jan 29, 2024

So, I have #20895 addressing this issue. It is a bit bold, but anything more involved requires the client to know the list of valid commands, which introduces redundancy. I would love to get feedback from people involved here.

@keith
Copy link
Member

keith commented Feb 15, 2024

For reference here are some more edge cases I found:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants