Config Profiles (RFC 2282 Part 2) #5506

ehuss · 2018-05-10T04:21:14Z

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.

rust-highfive · 2018-05-10T04:21:19Z

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

ehuss · 2018-05-10T04:22:15Z

Questions:

Do you care much about item visibility? I've been a little sloppy throwing pub onto things that could be a little tighter.
I implemented a bunch of warnings/validations, but I can imagine some of them could be noisy (and it is a lot of code). We can rip them out or change them if they look like they'll be too problematic.
- The unknown key check could use serde_ignore, but then you lose the ability to know exactly which file the value came from (and warnings would be duplicated for every manifest in a workspace). I can switch it though, and remove ~40 lines of yucky code.

matklad · 2018-05-10T12:48:29Z

Do you care much about item visibility? I've been a little sloppy throwing pub onto things that could be a little tighter.

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 RLS uses cargo as a library, so we need to make sure it has access to all stuff it needs.

I am not sure I'll have a change to review this today, but on the first sight this is great!

alexcrichton

This is looking great, thanks so much @ehuss!

alexcrichton · 2018-05-10T16:03:28Z

src/cargo/core/profiles.rs

+    fn validate_config(config: &Config, warnings: &mut Vec<String>) -> CargoResult<()> {
+        static VALIDATE_ONCE: atomic::AtomicBool = atomic::ATOMIC_BOOL_INIT;
+
+        if VALIDATE_ONCE.swap(true, atomic::Ordering::SeqCst) {


Could this perhaps be a flag stored directly in the Config instance instead of globally?

alexcrichton · 2018-05-10T16:04:08Z

src/cargo/core/profiles.rs

+        }
+
+        // cv: Value<HashMap<String, CV>>
+        if let Some(cv) = config.get_table("profile")? {


Could this use a match with an early return to avoid indentation?

alexcrichton · 2018-05-10T16:05:13Z

src/cargo/core/profiles.rs

+            }
+            // Warn about incorrect key names.
+            for profile_cv in cv.val.values() {
+                if let ConfigValue::Table(ref profile, _) = *profile_cv {


Should this warn if the value isn't a table?

alexcrichton · 2018-05-10T16:08:11Z

src/cargo/util/config.rs

@@ -844,13 +841,17 @@ impl ConfigValue {
                    };
                }
            }
-            (expected, found) => {


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"?

alexcrichton · 2018-05-10T16:10:47Z

src/cargo/util/toml/mod.rs

+            return Ok(None);
+        }
+        if let Some(util::config::Value { val, .. }) =
+            config.get_table(&format!("profile.{}", name))?


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?

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.)

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.

ehuss · 2018-05-31T17:48:43Z

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.

alexcrichton · 2018-06-01T14:10:32Z

r=me, this looks fantastic! @matklad want to take a final look though?

matklad · 2018-06-02T18:24:23Z

"this looks fantastic" is a very accurate description of this PR!

@bors r=alexcrichton

bors · 2018-06-02T18:24:24Z

📌 Commit 84a80d8 has been approved by alexcrichton

bors · 2018-06-02T18:24:31Z

⌛ Testing commit 84a80d8 with merge 9f09778...

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.

bors · 2018-06-02T18:49:50Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 9f09778 to master...

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.

rust-highfive assigned matklad May 10, 2018

alexcrichton reviewed May 10, 2018

View reviewed changes

Config Profiles (RFC 2282 Part 2)

2f7b522

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.

ehuss force-pushed the config-profile branch from c5034fc to 6bf1328 Compare May 31, 2018 17:06

Use new config API.

84a80d8

Also, require "override" feature if used in a config.

ehuss force-pushed the config-profile branch from 6bf1328 to 84a80d8 Compare May 31, 2018 17:20

bors merged commit 84a80d8 into rust-lang:master Jun 2, 2018

Manishearth mentioned this pull request Mar 6, 2019

Tracking issue for RFC 2282 - Cargo profile dependencies rust-lang/rust#48683

Closed

5 tasks

ehuss mentioned this pull request Jan 23, 2020

Stabilize config-profile. #7823

Merged

ehuss added this to the 1.28.0 milestone Feb 6, 2022

chordtoll mentioned this pull request Feb 20, 2022

Disk usage 2-3x somewhere around 1.48 rust-lang/rust#84097

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config Profiles (RFC 2282 Part 2) #5506

Config Profiles (RFC 2282 Part 2) #5506

ehuss commented May 10, 2018

rust-highfive commented May 10, 2018

ehuss commented May 10, 2018

matklad commented May 10, 2018

alexcrichton left a comment

alexcrichton May 10, 2018

alexcrichton May 10, 2018

alexcrichton May 10, 2018

alexcrichton May 10, 2018

alexcrichton May 10, 2018

ehuss May 10, 2018

alexcrichton May 10, 2018

ehuss commented May 31, 2018

alexcrichton commented Jun 1, 2018

matklad commented Jun 2, 2018

bors commented Jun 2, 2018

bors commented Jun 2, 2018

bors commented Jun 2, 2018

Config Profiles (RFC 2282 Part 2) #5506

Config Profiles (RFC 2282 Part 2) #5506

Conversation

ehuss commented May 10, 2018

rust-highfive commented May 10, 2018

ehuss commented May 10, 2018

matklad commented May 10, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton May 10, 2018

Choose a reason for hiding this comment

alexcrichton May 10, 2018

Choose a reason for hiding this comment

alexcrichton May 10, 2018

Choose a reason for hiding this comment

alexcrichton May 10, 2018

Choose a reason for hiding this comment

alexcrichton May 10, 2018

Choose a reason for hiding this comment

ehuss May 10, 2018

Choose a reason for hiding this comment

alexcrichton May 10, 2018

Choose a reason for hiding this comment

ehuss commented May 31, 2018

alexcrichton commented Jun 1, 2018

matklad commented Jun 2, 2018

bors commented Jun 2, 2018

bors commented Jun 2, 2018

bors commented Jun 2, 2018