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

Support multiple --bazelrc on command line #12740

Closed
wants to merge 42 commits into from

Conversation

wisechengyi
Copy link
Contributor

@wisechengyi wisechengyi commented Dec 21, 2020

Address #7489

Motivation

Multiple --bazelrc on CLI would be useful for us for various reasons:

Mostly importantly, we will have very long argument lists to pass to bazel query, so they have to be in a bazelrc file. import/try import in bazelrc would still work but very awkwardly. For example, if there are multiple bazelrc files to import, say A and B,

import A needs to be added into $WORKSPACE/.bazelrc
import B needs to be added into A
meaning the former bazelrc file needs to know what comes next.

Therefore allowing multiple --bazelrc would greatly improve the ergonomics, so the caller can create and append the new bazelrc without modifying the previous rc files.

Note

Options passed on CLI will still overwrite any options specified in any bazelrcs.

Result

bazel --bazelrc=x.rc --bazelrc=y.rc ... now works.

  --bazelrc (a string; default: see description)
    The location of the user .bazelrc file containing default values of Bazel 
    options. This option can also be chained together.
    E.g. `--bazelrc=x.rc --bazelrc=y.rc` so options in both RCs will be read.
    Note: `--bazelrc x.rc y.rc` is illegal, and each bazelrc file needs to be 
    accompanied by --bazelrc flag before it.
    If unspecified, Bazel uses the first .bazelrc file it finds in the 
    following two locations: the workspace directory, then the user's home 
    directory. Use /dev/null to disable the search for a user rc file, e.g. in 
    release builds.

@google-cla google-cla bot added the cla: yes label Dec 21, 2020
@wisechengyi wisechengyi changed the title Multi rc Support multiple --bazelrc on command line Dec 21, 2020
Copy link

@bierbaum bierbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Yi, I left some style nits to get this change in line with what is going on in this codebase.

You also might want to add coverage in src/test/cpp/startup_options_test.cc.

src/main/cpp/option_processor.cc Outdated Show resolved Hide resolved
src/main/cpp/option_processor.cc Outdated Show resolved Hide resolved
src/main/cpp/option_processor.cc Outdated Show resolved Hide resolved
@wisechengyi
Copy link
Contributor Author

You also might want to add coverage in src/test/cpp/startup_options_test.cc.

In terms of bazelrcs chaining together, it was already supported, but just not from command line. Hence the scope of this change merely adds the bazelrcs passed via CLI to the final list of bazelrcs that are going to be parsed. I don't see anything specific about CLI related tests in startup_options_test.cc. Happy to add more if @michajlo thinks it is necessary and can kindly offer some guidance.

src/main/cpp/blaze_util.cc Outdated Show resolved Hide resolved
src/main/cpp/blaze_util.cc Outdated Show resolved Hide resolved
src/main/cpp/blaze_util.cc Outdated Show resolved Hide resolved
@aiuto
Copy link
Contributor

aiuto commented Dec 23, 2020

cc: @juliexxia
This should have been discussed as an issue first so we could discuss the merits. It's debatable if we want to add any new complexity to arg parsing, since a few of us have recently noticed that it is a mess in many ways and is ripe for a complete rebuilding.

@wisechengyi
Copy link
Contributor Author

Hi @michajlo I have made the changes as discussed. Please kindly give it another go when you get a chance. Thanks!

@wisechengyi
Copy link
Contributor Author

Comment addressed. Thanks!

@wisechengyi wisechengyi force-pushed the multi_rc branch 2 times, most recently from 318d34c to 34786aa Compare March 11, 2021 17:25
@michajlo
Copy link
Contributor

I had overlooked that there's some documentation that needs to be updated. Can you update https://github.com/bazelbuild/bazel/blob/master/site/docs/guide.md and https://github.com/bazelbuild/bazel/blob/master/site/docs/user-manual.html, and do a quick skim to see if there's anything else we missed?

@wisechengyi
Copy link
Contributor Author

Hi @michajlo, happy to do that. However logistically are we going to run into issues like #12194 where we cannot directly update doc from github PR? If so, would you like me to send a separate PR, and then you can modify the internal repo accordingly?

@michajlo
Copy link
Contributor

I think this instance is fine to change.

Copy link
Contributor Author

@wisechengyi wisechengyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

site/docs/guide.md Show resolved Hide resolved
@bazel-io bazel-io closed this in 61da1d2 Mar 22, 2021
@wisechengyi
Copy link
Contributor Author

Thank you @michajlo !

@wisechengyi wisechengyi deleted the multi_rc branch March 23, 2021 14:08
philwo pushed a commit that referenced this pull request Apr 19, 2021
Address #7489

### Motivation

Multiple --bazelrc on CLI would be useful for us for various reasons:

Mostly importantly, we will have very long argument lists to pass to bazel query, so they have to be in a bazelrc file. `import/try import` in bazelrc would still work but very awkwardly. For example, if there are multiple bazelrc files to import, say `A` and `B`,

import `A` needs to be added into `$WORKSPACE/.bazelrc`
import `B` needs to be added into `A`
meaning the former bazelrc file needs to know what comes next.

Therefore allowing multiple --bazelrc would greatly improve the ergonomics, so the caller can create and append the new bazelrc without modifying the previous rc files.

### Note

Options passed on CLI will still overwrite any options specified in any bazelrcs.

### Result
`bazel --bazelrc=x.rc --bazelrc=y.rc ...` now works.
```
  --bazelrc (a string; default: see description)
    The location of the user .bazelrc file containing default values of Bazel
    options. This option can also be chained together.
    E.g. `--bazelrc=x.rc --bazelrc=y.rc` so options in both RCs will be read.
    Note: `--bazelrc x.rc y.rc` is illegal, and each bazelrc file needs to be
    accompanied by --bazelrc flag before it.
    If unspecified, Bazel uses the first .bazelrc file it finds in the
    following two locations: the workspace directory, then the user's home
    directory. Use /dev/null to disable the search for a user rc file, e.g. in
    release builds.
```

Closes #12740.

PiperOrigin-RevId: 364407234
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Core Skyframe, bazel query, BEP, options parsing, bazelrc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants