Auto merge of #9943 - ehuss:stabilize-named-profiles, r=alexcrichton

Stabilize named profiles

This stabilizes the named profiles feature. As an overview of what this does, it allows specifying custom named profiles, such as:

```toml
[profile.release-lto]
inherits = "release"
lto = true
```

And enables the use of the `--profile` CLI option to choose a profile by name.

Another key change here is that cargo now only uses a single profile per command. Previously, some commands such as `cargo test` would use a mix of profiles based on which package and target were being built.

### Summary of new behavior

* Profiles can now have arbitrary names. New profiles require the `inherits` key.
* The `--profile` flag is now available on all build commands.
* The `CompileMode` is no longer considered for choosing the profile, only one profile will be used. Previously, building a test, benchmark, or doctest would use the test or bench profile, and all dependencies would use the dev/release profiles. This change is done to arguably make it easier to understand, and to possibly give more desired and intuitive behavior.
* The `test` profile now inherits settings from the `dev` profile (and `bench` from `release`).

### Deviations from the original RFC and implementation

* The original RFC indicated that `--all-targets` without `--profile` would retain the old behavior where it would use different profiles for different targets. However, the implementation uses a single profile, to avoid confusion and to keep things simple.
* The `dir-name` key is not exposed to the user. The implementation is retained to handle mapping of built-in profile names (test/dev→debug, bench→release). This can be exposed in the future if necessary.

### Notes about this PR

* Fixed an issue where the duplicate package override check would randomly return matches for inherited profiles like `test`.
* I left some of the old, vestigial code behind to possibly make it easier to revert this PR if necessary. If this does land, I think it can be eventually removed (code using `Feature::named_profiles` and various things using `named_profiles_enabled`).
* Added `target` to reserved list, just because.
* Adds a warning if `--release` is combined with `--profile` in `cargo rustc`, `check`, or `fix`. The `--release` flag was being ignored.

### Hazards and concerns

* This has had very little real-world testing.
* Custom profile directories may conflict with other things in the `target` directory. We have reserved profile names that currently conflict (such as `doc` or `package`). However, they can still collide with target names. This also presents a hazard if Cargo ever wants to add new things to that top directory. We decided to proceed with this because:
    * We currently have no plans to add new built-in profiles.
    * We have reserved several profile names (including anything starting with "cargo"), and the profile name syntax is deliberately limited (so cargo is still free to add `.` prefixed hidden directories).
    * A user creating a profile that collides with a target name resides in the "don't do that" territory. Also, that shouldn't be catastrophic, as the directories are still somewhat organized differently.
* Artifacts may no longer be shared in some circumstances. This can lead to substantially increased `target` directory sizes (and build times), particularly if the `test` profile is not the same as the `dev` profile. Previously, dependencies would use the `dev` profile for both. If the user wants to retain the old behavior, they can use an override like `[profile.test.package."*"]` and set the same settings as `dev`.
* This may break existing workflows. It is possible, though unlikely, that changes to the profile settings will cause changes to how things build in such a way to break behavior.
    * Another example is using something like `cargo build` to prime a cache that is used for `cargo test`, and there is a custom `test` profile, the cache will no longer be primed.
* The legacy behavior with `cargo rustc`, `cargo check`, and `cargo fix` may be confusing. We may in the future consider something like a `--mode` flag to formalize that behavior.
* The `PROFILE` environment variable in build scripts may cause confusion or cause problems since it only sets `release` or `debug`. Some people may be using that to determine if `--release` should be used for a recursive `cargo` invocation. Currently I noted in the documentation that it shouldn't be used. However, I think it could be reasonable to maybe add a separate environment variable (`PROFILE_NAME`?) that exposes the actual profile used. We felt that changing the existing value could cause too much breakage (and the mapping of debug→dev is a little awkward).

Closes #6988

Author: bors

src/cargo/core/features.rs

@@ -387,7 +387,7 @@ features! {
     (unstable, public_dependency, "", "reference/unstable.html#public-dependency"),
 
     // Allow to specify profiles other than 'dev', 'release', 'test', etc.
-    (unstable, named_profiles, "", "reference/unstable.html#custom-named-profiles"),
+    (stable, named_profiles, "1.57", "reference/profiles.html#custom-profiles"),
 
     // Opt-in new-resolver behavior.
     (stable, resolver, "1.51", "reference/resolver.html#resolver-versions"),
@@ -643,7 +643,6 @@ unstable_cli_options!(
     minimal_versions: bool = ("Resolve minimal dependency versions instead of maximum"),
     mtime_on_use: bool = ("Configure Cargo to update the mtime of used files"),
     multitarget: bool = ("Allow passing multiple `--target` flags to the cargo subcommand selected"),
-    named_profiles: bool = ("Allow defining custom profiles"),
     namespaced_features: bool = ("Allow features with `dep:` prefix"),
     no_index_update: bool = ("Do not update the registry index even if the cache is outdated"),
     panic_abort_tests: bool = ("Enable support to run tests with -Cpanic=abort"),
@@ -699,6 +698,10 @@ const STABILIZED_CONFIGURABLE_ENV: &str = "The [env] section is now always enabl
 
 const STABILIZED_PATCH_IN_CONFIG: &str = "The patch-in-config feature is now always enabled.";
 
+const STABILIZED_NAMED_PROFILES: &str = "The named-profiles feature is now always enabled.\n\
+    See https://doc.rust-lang.org/nightly/cargo/reference/profiles.html#custom-profiles \
+    for more information";
+
 fn deserialize_build_std<'de, D>(deserializer: D) -> Result<Option<Vec<String>>, D::Error>
 where
     D: serde::Deserializer<'de>,
@@ -830,7 +833,7 @@ impl CliUnstable {
             "dual-proc-macros" => self.dual_proc_macros = parse_empty(k, v)?,
             // can also be set in .cargo/config or with and ENV
             "mtime-on-use" => self.mtime_on_use = parse_empty(k, v)?,
-            "named-profiles" => self.named_profiles = parse_empty(k, v)?,
+            "named-profiles" => stabilized_warn(k, "1.57", STABILIZED_NAMED_PROFILES),
             "binary-dep-depinfo" => self.binary_dep_depinfo = parse_empty(k, v)?,
             "build-std" => {
                 self.build_std = Some(crate::core::compiler::standard_lib::parse_unstable_flag(v))

src/cargo/core/profiles.rs

@@ -20,6 +20,11 @@ pub struct Profiles {
     dir_names: HashMap<InternedString, InternedString>,
     /// The profile makers. Key is the profile name.
     by_name: HashMap<InternedString, ProfileMaker>,
+    /// The original profiles written by the user in the manifest and config.
+    ///
+    /// This is here to assist with error reporting, as the `ProfileMaker`
+    /// values have the inherits chains all merged together.
+    original_profiles: BTreeMap<InternedString, TomlProfile>,
     /// Whether or not unstable "named" profiles are enabled.
     named_profiles_enabled: bool,
     /// The profile the user requested to use.
@@ -44,6 +49,7 @@ impl Profiles {
                 named_profiles_enabled: false,
                 dir_names: Self::predefined_dir_names(),
                 by_name: HashMap::new(),
+                original_profiles: profiles.clone(),
                 requested_profile,
                 rustc_host,
             };
@@ -97,6 +103,7 @@ impl Profiles {
             named_profiles_enabled: true,
             dir_names: Self::predefined_dir_names(),
             by_name: HashMap::new(),
+            original_profiles: profiles.clone(),
             requested_profile,
             rustc_host,
         };
@@ -420,6 +427,19 @@ impl Profiles {
         resolve: &Resolve,
     ) -> CargoResult<()> {
         for (name, profile) in &self.by_name {
+            // If the user did not specify an override, skip this. This is here
+            // to avoid generating errors for inherited profiles which don't
+            // specify package overrides. The `by_name` profile has had the inherits
+            // chain merged, so we need to look at the original source to check
+            // if an override was specified.
+            if self
+                .original_profiles
+                .get(name)
+                .and_then(|orig| orig.package.as_ref())
+                .is_none()
+            {
+                continue;
+            }
             let found = validate_packages_unique(resolve, name, &profile.toml)?;
             // We intentionally do not validate unmatched packages for config
             // profiles, in case they are defined in a central location. This
@@ -456,6 +476,10 @@ struct ProfileMaker {
     /// The starting, hard-coded defaults for the profile.
     default: Profile,
     /// The TOML profile defined in `Cargo.toml` or config.
+    ///
+    /// This is None if the user did not specify one, in which case the
+    /// `default` is used. Note that the built-in defaults for test/bench/doc
+    /// always set this since they need to declare the `inherits` value.
     toml: Option<TomlProfile>,
 }
 

src/cargo/util/command_prelude.rs

@@ -363,16 +363,22 @@ pub trait ArgMatchesExt {
         // This is an early exit, since it allows combination with `--release`.
         match (specified_profile, profile_checking) {
             // `cargo rustc` has legacy handling of these names
-            (Some(name @ ("dev" | "test" | "bench" | "check")), ProfileChecking::LegacyRustc) |
+            (Some(name @ ("dev" | "test" | "bench" | "check")), ProfileChecking::LegacyRustc)
             // `cargo fix` and `cargo check` has legacy handling of this profile name
-            (Some(name @ "test"), ProfileChecking::LegacyTestOnly) => return Ok(InternedString::new(name)),
+            | (Some(name @ "test"), ProfileChecking::LegacyTestOnly) => {
+                if self._is_present("release") {
+                    config.shell().warn(
+                        "the `--release` flag should not be specified with the `--profile` flag\n\
+                         The `--release` flag will be ignored.\n\
+                         This was historically accepted, but will become an error \
+                         in a future release."
+                    )?;
+                }
+                return Ok(InternedString::new(name));
+            }
             _ => {}
         }
 
-        if specified_profile.is_some() && !config.cli_unstable().unstable_options {
-            bail!("usage of `--profile` requires `-Z unstable-options`");
-        }
-
         let conflict = |flag: &str, equiv: &str, specified: &str| -> anyhow::Error {
             anyhow::format_err!(
                 "conflicting usage of --profile={} and --{flag}\n\

src/cargo/util/toml/mod.rs

@@ -602,6 +602,7 @@ impl TomlProfile {
                 | "rust"
                 | "rustc"
                 | "rustdoc"
+                | "target"
                 | "tmp"
                 | "uninstall"
         ) || lower_name.starts_with("cargo")

src/doc/man/cargo-bench.md

@@ -46,6 +46,14 @@ function to handle running benchmarks.
 > running benchmarks on the stable channel, such as
 > [Criterion](https://crates.io/crates/criterion).
 
+By default, `cargo bench` uses the [`bench` profile], which enables
+optimizations and disables debugging information. If you need to debug a
+benchmark, you can use the `--profile=dev` command-line option to switch to
+the dev profile. You can then run the debug-enabled benchmark within a
+debugger.
+
+[`bench` profile]: ../reference/profiles.html#bench
+
 ## OPTIONS
 
 ### Benchmark Options
@@ -83,6 +91,8 @@ target.
 
 {{> options-target-triple }}
 
+{{> options-profile }}
+
 {{> options-ignore-rust-version }}
 
 {{/options}}
@@ -129,23 +139,6 @@ Rust test harness runs benchmarks serially in a single thread.
 {{> options-jobs }}
 {{/options}}
 
-## PROFILES
-
-Profiles may be used to configure compiler options such as optimization levels
-and debug settings. See
-[the reference](../reference/profiles.html)
-for more details.
-
-Benchmarks are always built with the `bench` profile. Binary and lib targets
-are built separately as benchmarks with the `bench` profile. Library targets
-are built with the `release` profiles when linked to binaries and benchmarks.
-Dependencies use the `release` profile.
-
-If you need a debug build of a benchmark, try building it with
-{{man "cargo-build" 1}} which will use the `test` profile which is by default
-unoptimized and includes debug information. You can then run the debug-enabled
-benchmark manually.
-
 {{> section-environment }}
 
 {{> section-exit-status }}

src/doc/man/cargo-build.md

@@ -35,6 +35,8 @@ they have `required-features` that are missing.
 
 {{> options-release }}
 
+{{> options-profile }}
+
 {{> options-ignore-rust-version }}
 
 {{/options}}
@@ -89,8 +91,6 @@ See <https://github.com/rust-lang/cargo/issues/5579> for more information.
 {{> options-jobs }}
 {{/options}}
 
-{{> section-profiles }}
-
 {{> section-environment }}
 
 {{> section-exit-status }}

src/doc/man/cargo-check.md

@@ -40,7 +40,7 @@ they have `required-features` that are missing.
 
 {{> options-release }}
 
-{{> options-profile }}
+{{> options-profile-legacy-check }}
 
 {{> options-ignore-rust-version }}
 
@@ -76,8 +76,6 @@ they have `required-features` that are missing.
 {{> options-jobs }}
 {{/options}}
 
-{{> section-profiles }}
-
 {{> section-environment }}
 
 {{> section-exit-status }}

src/doc/man/cargo-clean.md

@@ -40,7 +40,11 @@ the target directory.
 {{/option}}
 
 {{#option "`--release`" }}
-Clean all artifacts that were built with the `release` or `bench` profiles.
+Remove all artifacts in the `release` directory.
+{{/option}}
+
+{{#option "`--profile` _name_" }}
+Remove all artifacts in the directory with the given profile name.
 {{/option}}
 
 {{> options-target-dir }}

src/doc/man/cargo-doc.md

@@ -74,6 +74,8 @@ and supports common Unix glob patterns.
 
 {{> options-release }}
 
+{{> options-profile }}
+
 {{> options-ignore-rust-version }}
 
 {{/options}}
@@ -108,8 +110,6 @@ and supports common Unix glob patterns.
 {{> options-jobs }}
 {{/options}}
 
-{{> section-profiles }}
-
 {{> section-environment }}
 
 {{> section-exit-status }}

src/doc/man/cargo-fix.md

@@ -120,7 +120,7 @@ When no target selection options are given, `cargo fix` will fix all targets
 
 {{> options-release }}
 
-{{> options-profile }}
+{{> options-profile-legacy-check }}
 
 {{> options-ignore-rust-version }}
 
@@ -156,8 +156,6 @@ When no target selection options are given, `cargo fix` will fix all targets
 {{> options-jobs }}
 {{/options}}
 
-{{> section-profiles }}
-
 {{> section-environment }}
 
 {{> section-exit-status }}