Config Profiles (RFC 2282 Part 2)#5506
Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
|
Questions:
|
Not really, but, all other things being equal, reasonable module structure and clear interfaces are a win. The only hard concern around visibility is that I am not sure I'll have a change to review this today, but on the first sight this is great! |
alexcrichton
left a comment
There was a problem hiding this comment.
This is looking great, thanks so much @ehuss!
src/cargo/core/profiles.rs
Outdated
There was a problem hiding this comment.
Could this perhaps be a flag stored directly in the Config instance instead of globally?
src/cargo/core/profiles.rs
Outdated
There was a problem hiding this comment.
Could this use a match with an early return to avoid indentation?
src/cargo/core/profiles.rs
Outdated
There was a problem hiding this comment.
Should this warn if the value isn't a table?
src/cargo/util/config.rs
Outdated
There was a problem hiding this comment.
Could a comment be added here indicating that this block is intended for "you can't switch types between tables/arrays, but you can switch other key value types"?
src/cargo/util/toml/mod.rs
Outdated
There was a problem hiding this comment.
One of the downsides of get_table is that it doesn't work well with environment variables so I think that for any new features we ideally want to avoid get_table as much as possible. I saw above that get_table is used a lot in validation which I think is fine, but would it be possible for these values to read from the environment as well?
There was a problem hiding this comment.
I thought about that a little bit. IIUC, it will need to manually get every key, and it wouldn't be able to use serde to do the automatic conversion. I can give it a shot, though I think that will make the patch a little longer.
(Another crazy idea would be to implement a custom Deserializer for Config that would handle everything, but I'm not sure if that would work, and would be a fair amount of code.)
There was a problem hiding this comment.
Heh yeah I think you're spot on. The manual deserialization here hopefully isn't too bad, but I agree that a Config deserializer is probably what we want long-term.
Notes: - `-Z config-profile` CLI option is required to use. - Config values no longer reject mixed base types (integer, string, boolean) in order to support the mixed types in profiles.
Also, require "override" feature if used in a config.
|
I've updated with the new API. I decided to use LazyCell to store the profiles in Config to ensure they are validated only once instead of using the global atomic. I haven't used LazyCell before, so I wasn't familiar with it, but it seems to work pretty well. |
|
r=me, this looks fantastic! @matklad want to take a final look though? |
|
"this looks fantastic" is a very accurate description of this PR! @bors r=alexcrichton |
|
📌 Commit 84a80d8 has been approved by |
Config Profiles (RFC 2282 Part 2) Notes: - `-Z config-profile` CLI option is required to use. - Config values no longer reject mixed base types (integer, string, boolean) in order to support the mixed types in profiles.
|
☀️ Test successful - status-appveyor, status-travis |
Stabilize config-profile. This is a proposal to stabilize config-profiles. This feature was proposed in [RFC 2282](rust-lang/rfcs#2282) and implemented in #5506. Tracking issue is rust-lang/rust#48683. This is intended to land in 1.43 which will reach the stable channel on April 23rd. This is a fairly straightforward extension of profiles where the exact same syntax from `Cargo.toml` can be specified in a config file. Environment variables are supported for everything except the `package` override table, where we do not support the ability to read arbitrary keys in the environment name.
Notes:
-Z config-profileCLI option is required to use.