Add named config profiles. #7750

ehuss · 2019-12-28T19:15:55Z

This adds support for named config profiles. Previously, only dev and release were allowed in config files, it now supports all profile names. I think it would be strange to have arbitrarily named profiles in Cargo.toml, but not allow them in config. This is a deviation from the RFC, but RFC 2282 was written before named profiles which I think changes the landscape.

This diff is a little large due to some refactoring to make it work well. Overview of the changes:

Removed ProfileKind and only use an InternedString to track the name of the profile. I didn't feel like the enum carried its cognitive weight, and it seems to simplify some things.
Profiles is no longer stored in the manifest. There was no need to do a bunch of processing for each manifest. Manifest now only retains the low-level TomlProfiles. A single Profiles now lives in BuildContext.
The profile name requested by the user is no longer passed around. It is given to Profiles::new and retained inside Profiles.
Profiles::get_profile no longer follows the priority stack and inheritance each time a profile is requested. Instead, the profile is computed once (in Profile::new) and merged into a single profile. This simplifies getting a profile, and makes it easier to deal with getting the config values in one place.
I switched profile names to be InternedString instead of String. There's not a strong reason to do this, other than it seemed a little strange to be creating lots of Strings.
- I also added PartialEq<str> for InternedString. It has come up a few times in the past, and it seems useful. I'm not sure if it was excluded intentionally?
The validation that the profile exists is now done in one place (Profiles::new).
I removed the back-compatibility for the overrides key (which was renamed to package back in October).

Notes:

Some of the error messages aren't as good as before, because they don't tell you where the error is located (Cargo.toml or .cargo/config). This is because the location data is lost by the time validation is done. Hopefully it will be obvious from the profile name and error message. I tried to improve error messages wherever I could.
There are more calls to clone() than I would like, but they are kinda hard to avoid. Should be fewer than before.
I noticed a bug with -Zpanic-abort-tests not supporting named profiles. I'll fix that separately.
I think this fixes some bugs where package overrides in config weren't merging properly with package overrides defined in Cargo.toml.

rust-highfive · 2019-12-28T19:16:00Z

r? @Eh2406

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

ehuss · 2019-12-28T19:23:54Z

cc custom named profile tracking issue: #6988
cc config profile tracking issue: rust-lang/rust#48683
cc @da-x FYI

alexcrichton · 2020-01-06T19:58:50Z

This all looks fantastic to me and all the refactorings sound great to me as well. Thanks for writing this all up @ehuss!

bors · 2020-01-08T18:27:52Z

☔ The latest upstream changes (presumably #7776) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2020-01-13T18:26:19Z

Ping @Eh2406 or @alexcrichton, not sure if either of you can review this, or if there are any questions I can answer or any way I can make the review easier.

alexcrichton · 2020-01-13T21:04:49Z

Oh sorry I think the only reason I didn't r+ at the time was maybe it needed a rebase? Or maybe I forgot?

In an ycase...

@bors: r+

bors · 2020-01-13T21:04:51Z

📌 Commit c92f4986423d41cbc940b76d6ea0fc61c0c351a4 has been approved by alexcrichton

bors · 2020-01-13T21:04:58Z

⌛ Testing commit c92f4986423d41cbc940b76d6ea0fc61c0c351a4 with merge e3311f94d7a2df7e042dd21c0899158f565d2ab2...

bors · 2020-01-13T21:24:01Z

💔 Test failed - checks-azure

ehuss · 2020-01-13T21:37:10Z

Updated due to formatting changes from anyhow.
@bors r=alexcrichton

bors · 2020-01-13T21:37:12Z

📌 Commit dafacbb has been approved by alexcrichton

bors · 2020-01-13T21:37:19Z

⌛ Testing commit dafacbb with merge ad3dbe1...

Add named config profiles. This adds support for named config profiles. Previously, only `dev` and `release` were allowed in config files, it now supports all profile names. I think it would be strange to have arbitrarily named profiles in `Cargo.toml`, but not allow them in config. This is a deviation from the RFC, but RFC 2282 was written before named profiles which I think changes the landscape. This diff is a little large due to some refactoring to make it work well. Overview of the changes: - Removed `ProfileKind` and only use an `InternedString` to track the name of the profile. I didn't feel like the enum carried its cognitive weight, and it seems to simplify some things. - `Profiles` is no longer stored in the manifest. There was no need to do a bunch of processing for each manifest. `Manifest` now only retains the low-level `TomlProfiles`. A single `Profiles` now lives in `BuildContext`. - The profile name requested by the user is no longer passed around. It is given to `Profiles::new` and retained inside `Profiles`. - `Profiles::get_profile` no longer follows the priority stack and inheritance each time a profile is requested. Instead, the profile is computed once (in `Profile::new`) and merged into a single profile. This simplifies getting a profile, and makes it easier to deal with getting the config values in one place. - I switched profile names to be `InternedString` instead of `String`. There's not a strong reason to do this, other than it seemed a little strange to be creating lots of `String`s. - I also added `PartialEq<str>` for `InternedString`. It has come up a few times in the past, and it seems useful. I'm not sure if it was excluded intentionally? - The validation that the profile exists is now done in one place (`Profiles::new`). - I removed the back-compatibility for the `overrides` key (which was renamed to `package` back in October). Notes: - Some of the error messages aren't as good as before, because they don't tell you where the error is located (`Cargo.toml` or `.cargo/config`). This is because the location data is lost by the time validation is done. Hopefully it will be obvious from the profile name and error message. I tried to improve error messages wherever I could. - There are more calls to `clone()` than I would like, but they are kinda hard to avoid. Should be fewer than before. - I noticed a bug with `-Zpanic-abort-tests` not supporting named profiles. I'll fix that separately. - I think this fixes some bugs where package overrides in config weren't merging properly with package overrides defined in `Cargo.toml`.

bors · 2020-01-13T22:01:16Z

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing ad3dbe1 to master...

Update cargo rls ## cargo 12 commits in 6e1ca924a67dd1ac89c33f294ef26b5c43b89168..ad3dbe10e1e654fb1f032a5dd9481d7cbaa00d65 2020-01-06 19:11:37 +0000 to 2020-01-13 21:37:15 +0000 - Add named config profiles. (rust-lang/cargo#7750) - Make cargo-rustc crate-type-aware (rust-lang/cargo#7755) - Rename `Kind` (rust-lang/cargo#7791) - Update bash completion (rust-lang/cargo#7789) - Add another curl spurious network error (rust-lang/cargo#7788) - Some small tweaks around error in configuration (rust-lang/cargo#7783) - Fix tests with `url` crate update (rust-lang/cargo#7787) - Fix .gitignore of Cargo.lock in a subdirectory. (rust-lang/cargo#7779) - Bump crates-io (rust-lang/cargo#7778) - Migrate from the `failure` crate to `anyhow` (rust-lang/cargo#7776) - Fix several needless_borrow clippy lints. (rust-lang/cargo#7771) - Fix some links (rust-lang/cargo#7770) ## rls 2 commits in 7c0489c5ff4f5c594e65a3b22efd9ce373deab9b..b27e1173969639448cd2e486b1c5f0fcb1b3b17c 2020-01-04 20:15:37 +0100 to 2020-01-13 11:40:20 +0100 - Update Cargo (rust-lang/rls#1613) - Rustup to #68024 (rust-lang/rls#1612)

Fix config profiles using "dev" in `cargo test`. Fix a bug where the "dev" profile was not loaded from config when running `cargo test` when "dev" is not listed in `Cargo.toml`. There was a mistake in #7750 where it did not consider implicit profiles. Config profiles need to be loaded explicitly in order to properly handle environment variables. However, it was only looking at the profile requested on the command-line and those listed in `Cargo.toml`. `cargo test` also implicitly uses the "dev" profile for dependencies, so make sure those are loaded from config as well.

…richton Fix config profiles using "dev" in `cargo test`. Fix a bug where the "dev" profile was not loaded from config when running `cargo test` when "dev" is not listed in `Cargo.toml`. There was a mistake in rust-lang#7750 where it did not consider implicit profiles. Config profiles need to be loaded explicitly in order to properly handle environment variables. However, it was only looking at the profile requested on the command-line and those listed in `Cargo.toml`. `cargo test` also implicitly uses the "dev" profile for dependencies, so make sure those are loaded from config as well.

rust-highfive assigned Eh2406 Dec 28, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2019

da-x approved these changes Dec 29, 2019

View reviewed changes

ehuss force-pushed the named-config-profiles branch from 03e3a8d to c92f498 Compare January 8, 2020 19:35

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 13, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 13, 2020

ehuss added 2 commits January 13, 2020 13:27

Add named config profiles.

77ee608

Update tests for formatting changes due to anyhow changes.

dafacbb

ehuss force-pushed the named-config-profiles branch from c92f498 to dafacbb Compare January 13, 2020 21:36

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 13, 2020

bors merged commit dafacbb into rust-lang:master Jan 13, 2020

ehuss mentioned this pull request Jan 14, 2020

Update cargo rls rust-lang/rust#68221

Merged

ehuss mentioned this pull request Mar 17, 2020

Fix config profiles using "dev" in cargo test. #8012

Merged

ehuss added this to the 1.42.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add named config profiles. #7750

Add named config profiles. #7750

ehuss commented Dec 28, 2019

rust-highfive commented Dec 28, 2019

ehuss commented Dec 28, 2019

alexcrichton commented Jan 6, 2020

bors commented Jan 8, 2020

ehuss commented Jan 13, 2020

alexcrichton commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

ehuss commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

Add named config profiles. #7750

Add named config profiles. #7750

Conversation

ehuss commented Dec 28, 2019

rust-highfive commented Dec 28, 2019

ehuss commented Dec 28, 2019

alexcrichton commented Jan 6, 2020

bors commented Jan 8, 2020

ehuss commented Jan 13, 2020

alexcrichton commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

ehuss commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020