Add cargo config subcommand.
#9302
Conversation
|
r? @Eh2406 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Open questions
Future work
Environment variable handlingI'm a little unhappy with the way environment variables are handled. If a command like If getting a non-table value (like I'm not sure how to go about solving that. The only solution I can think of is defining some kind of schema that describes the structure of the config types. I can't think of a way to do this with serde. Even with that, there are some "lazy" environment variables that I think would be impossible to handle correctly. They are:
|
|
Personally I prefer the |
There was a problem hiding this comment.
Is there any preference in style for the --merged flag. Should it be --merged=yes/no or --no-merged?
Personally over the years I've come to appreciate --flag=val more than --no-flag, so I'd go for --merged. I don't feel strongly on this though.
JSON doesn't show environment variables. Not sure how to handle that.
Since JSON goes to stdout, could warnings about this go to stderr perhaps?
I'm not sure how to go about solving that. The only solution I can think of is defining some kind of schema that describes the structure of the config types. I can't think of a way to do this with serde.
Right now all of Cargo's configuration is entirely untyped and accessed on-the-fly so one of the issues with your profile example is that within Cargo we're always accessing profile configuration in a typed manner, but with this new subcommand it's instead all untyped so we don't know what to do with the map.
One possible alternative is we instead "register" configuration values with Config early on, so Config learns about the structure of all expected configuration. Basically your schema idea but with registration up-front of what the schema is. I have no idea how to do this in an ergonomic fashion though and it's definitely out of the scope of this PR.
src/cargo/ops/cargo_config.rs
Outdated
|
|
||
| fn print_json(config: &Config, key: &ConfigKey, cv: &CV, include_key: bool) { | ||
| let json_value = if key.is_root() || !include_key { | ||
| match cv { |
There was a problem hiding this comment.
Could this be deduplicated with json_add below where there's a function from CV to a json value?
Yea, I felt a little uncomfortable with that, but I can't say why exactly. I went ahead and pushed a commit that prints a note to stderr.
Yea, I will try to think it over for a while. I fear it will be a lot of code and changes, just to accommodate this one use case. I don't think it isn't super important right now. My primary use case is just to help people debug configuration settings, and printing the environment variables is sufficient to me for that purpose. |
|
Ok that all sounds good to me! @bors: r+ |
|
📌 Commit 23d4a68 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 5 commits in 1e8703890f285befb5e32627ad4e0a0454dde1fb..3c44c3c4b7900b8b13c85ead25ccaa8abb7d8989 2021-03-26 16:59:39 +0000 to 2021-03-31 21:21:15 +0000 - Fix semver docs for 1.51. (rust-lang/cargo#9316) - Add `cargo config` subcommand. (rust-lang/cargo#9302) - Give one more example for the --featuers CLI (rust-lang/cargo#9313) - Bump to 0.54.0, update changelog (rust-lang/cargo#9308) - Make the URL to the tracking issue for `--out-dir` into a link (rust-lang/cargo#9309)
This adds an initial version of the
cargo configcommand as discussed in #2362.Closes #2362