Auto merge of #9685 - ehuss:named-profile-updates, r=alexcrichton

bors · bors · commit 4e143fd131e0 · 2021-07-20T21:55:45.000Z
Named profile updates

A few updates for named profiles to push them closer to stabilization:

- Disable the `dir-name` profile setting. `dir-name` primarily exists for translating the built-in profiles or sharing artifacts between profiles. In order to simplify the UI, we would like to not expose it to the user for the initial stabilization. The code to support it is kept in case we want to add it in the future.
- Reserve some profile names. Just to give a little flexibility in the future in case we want to use these, or that they could cause confusion. Also updated the error text a little.
- Add support for custom profiles to legacy commands. Their old behavior is still retained for backwards compatibility. That is:
    * `cargo check`
        * `--profile=test`: This forces the test mode. For example, `cargo check --lib --profile=test` will check the library as a unit test (with `--test`).
    * `cargo fix`
        * `--profile=test`: Forces test mode, same as above.
    * `cargo rustc`
        * `--profile=test`: Forces test mode, same as above.
        * `--profile=bench`: Forces bench mode.
        * `--profile=check`: Forces check mode.
    - These commands also allow mixing the above options with `--release`, which is normally not allowed.
- Fix `cargo bench` to support the `--profile` option. I think it was just forgotten.
diff --git a/src/bin/cargo/commands/bench.rs b/src/bin/cargo/commands/bench.rs
@@ -35,6 +35,7 @@ pub fn cli() -> App {
             "Exclude packages from the benchmark",
         )
         .arg_jobs()
+        .arg_profile("Build artifacts with the specified profile")
         .arg_features()
         .arg_target_triple("Build for the target triple")
         .arg_target_dir()
@@ -55,11 +56,11 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
         config,
         CompileMode::Bench,
         Some(&ws),
-        ProfileChecking::Checked,
+        ProfileChecking::Custom,
     )?;
 
     compile_opts.build_config.requested_profile =
-        args.get_profile_name(config, "bench", ProfileChecking::Checked)?;
+        args.get_profile_name(config, "bench", ProfileChecking::Custom)?;
 
     let ops = TestOptions {
         no_run: args.is_present("no-run"),
diff --git a/src/bin/cargo/commands/build.rs b/src/bin/cargo/commands/build.rs
@@ -53,7 +53,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
         config,
         CompileMode::Build,
         Some(&ws),
-        ProfileChecking::Checked,
+        ProfileChecking::Custom,
     )?;
 
     if let Some(out_dir) = args.value_of_path("out-dir", config) {
diff --git a/src/bin/cargo/commands/check.rs b/src/bin/cargo/commands/check.rs
@@ -41,20 +41,11 @@ pub fn cli() -> App {
 
 pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
     let ws = args.workspace(config)?;
-    let test = match args.value_of("profile") {
-        Some("test") => true,
-        None => false,
-        Some(profile) => {
-            let err = anyhow::format_err!(
-                "unknown profile: `{}`, only `test` is \
-                 currently supported",
-                profile
-            );
-            return Err(CliError::new(err, 101));
-        }
-    };
+    // This is a legacy behavior that causes `cargo check` to pass `--test`.
+    let test = matches!(args.value_of("profile"), Some("test"));
     let mode = CompileMode::Check { test };
-    let compile_opts = args.compile_options(config, mode, Some(&ws), ProfileChecking::Unchecked)?;
+    let compile_opts =
+        args.compile_options(config, mode, Some(&ws), ProfileChecking::LegacyTestOnly)?;
 
     ops::compile(&ws, &compile_opts)?;
     Ok(())
diff --git a/src/bin/cargo/commands/clean.rs b/src/bin/cargo/commands/clean.rs
@@ -28,7 +28,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
         config,
         spec: values(args, "package"),
         targets: args.targets(),
-        requested_profile: args.get_profile_name(config, "dev", ProfileChecking::Checked)?,
+        requested_profile: args.get_profile_name(config, "dev", ProfileChecking::Custom)?,
         profile_specified: args.is_present("profile") || args.is_present("release"),
         doc: args.is_present("doc"),
     };
diff --git a/src/bin/cargo/commands/doc.rs b/src/bin/cargo/commands/doc.rs
@@ -43,7 +43,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
         deps: !args.is_present("no-deps"),
     };
     let mut compile_opts =
-        args.compile_options(config, mode, Some(&ws), ProfileChecking::Checked)?;
+        args.compile_options(config, mode, Some(&ws), ProfileChecking::Custom)?;
     compile_opts.rustdoc_document_private_items = args.is_present("document-private-items");
 
     let doc_opts = DocOptions {
diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs
@@ -67,23 +67,14 @@ pub fn cli() -> App {
 
 pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
     let ws = args.workspace(config)?;
-    let test = match args.value_of("profile") {
-        Some("test") => true,
-        None => false,
-        Some(profile) => {
-            let err = anyhow::format_err!(
-                "unknown profile: `{}`, only `test` is \
-                 currently supported",
-                profile
-            );
-            return Err(CliError::new(err, 101));
-        }
-    };
+    // This is a legacy behavior that causes `cargo fix` to pass `--test`.
+    let test = matches!(args.value_of("profile"), Some("test"));
     let mode = CompileMode::Check { test };
 
     // Unlike other commands default `cargo fix` to all targets to fix as much
     // code as we can.
-    let mut opts = args.compile_options(config, mode, Some(&ws), ProfileChecking::Unchecked)?;
+    let mut opts =
+        args.compile_options(config, mode, Some(&ws), ProfileChecking::LegacyTestOnly)?;
 
     if let CompileFilter::Default { .. } = opts.filter {
         opts.filter = CompileFilter::Only {
diff --git a/src/bin/cargo/commands/install.rs b/src/bin/cargo/commands/install.rs
@@ -131,11 +131,11 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
         config,
         CompileMode::Build,
         workspace.as_ref(),
-        ProfileChecking::Checked,
+        ProfileChecking::Custom,
     )?;
 
     compile_opts.build_config.requested_profile =
-        args.get_profile_name(config, "release", ProfileChecking::Checked)?;
+        args.get_profile_name(config, "release", ProfileChecking::Custom)?;
 
     if args.is_present("list") {
         ops::install_list(root, config)?;
diff --git a/src/bin/cargo/commands/run.rs b/src/bin/cargo/commands/run.rs
@@ -37,7 +37,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
         config,
         CompileMode::Build,
         Some(&ws),
-        ProfileChecking::Checked,
+        ProfileChecking::Custom,
     )?;
 
     // Disallow `spec` to be an glob pattern
diff --git a/src/bin/cargo/commands/rustc.rs b/src/bin/cargo/commands/rustc.rs
@@ -1,6 +1,6 @@
 use crate::command_prelude::*;
-
 use cargo::ops;
+use cargo::util::interning::InternedString;
 
 const PRINT_ARG_NAME: &str = "print";
 
@@ -46,26 +46,24 @@ pub fn cli() -> App {
 
 pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
     let ws = args.workspace(config)?;
+    // This is a legacy behavior that changes the behavior based on the profile.
+    // If we want to support this more formally, I think adding a --mode flag
+    // would be warranted.
     let mode = match args.value_of("profile") {
-        Some("dev") | None => CompileMode::Build,
         Some("test") => CompileMode::Test,
         Some("bench") => CompileMode::Bench,
         Some("check") => CompileMode::Check { test: false },
-        Some(mode) => {
-            let err = anyhow::format_err!(
-                "unknown profile: `{}`, use dev,
-                                   test, or bench",
-                mode
-            );
-            return Err(CliError::new(err, 101));
-        }
+        _ => CompileMode::Build,
     };
     let mut compile_opts = args.compile_options_for_single_package(
         config,
         mode,
         Some(&ws),
-        ProfileChecking::Unchecked,
+        ProfileChecking::LegacyRustc,
     )?;
+    if compile_opts.build_config.requested_profile == "check" {
+        compile_opts.build_config.requested_profile = InternedString::new("dev");
+    }
     let target_args = values(args, "args");
     compile_opts.target_rustc_args = if target_args.is_empty() {
         None
diff --git a/src/bin/cargo/commands/rustdoc.rs b/src/bin/cargo/commands/rustdoc.rs
@@ -44,7 +44,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
         config,
         CompileMode::Doc { deps: false },
         Some(&ws),
-        ProfileChecking::Checked,
+        ProfileChecking::Custom,
     )?;
     let target_args = values(args, "args");
     compile_opts.target_rustdoc_args = if target_args.is_empty() {
diff --git a/src/bin/cargo/commands/test.rs b/src/bin/cargo/commands/test.rs
@@ -66,11 +66,11 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
         config,
         CompileMode::Test,
         Some(&ws),
-        ProfileChecking::Checked,
+        ProfileChecking::Custom,
     )?;
 
     compile_opts.build_config.requested_profile =
-        args.get_profile_name(config, "test", ProfileChecking::Checked)?;
+        args.get_profile_name(config, "test", ProfileChecking::Custom)?;
 
     // `TESTNAME` is actually an argument of the test binary, but it's
     // important, so we explicitly mention it and reconfigure.
diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs
@@ -133,7 +133,6 @@ impl Profiles {
     fn predefined_dir_names() -> HashMap<InternedString, InternedString> {
         let mut dir_names = HashMap::new();
         dir_names.insert(InternedString::new("dev"), InternedString::new("debug"));
-        dir_names.insert(InternedString::new("check"), InternedString::new("debug"));
         dir_names.insert(InternedString::new("test"), InternedString::new("debug"));
         dir_names.insert(InternedString::new("bench"), InternedString::new("release"));
         dir_names
@@ -174,13 +173,6 @@ impl Profiles {
                     ..TomlProfile::default()
                 },
             ),
-            (
-                "check",
-                TomlProfile {
-                    inherits: Some(InternedString::new("dev")),
-                    ..TomlProfile::default()
-                },
-            ),
             (
                 "doc",
                 TomlProfile {
diff --git a/src/cargo/util/command_prelude.rs b/src/cargo/util/command_prelude.rs
@@ -281,8 +281,14 @@ pub fn subcommand(name: &'static str) -> App {
 
 // Determines whether or not to gate `--profile` as unstable when resolving it.
 pub enum ProfileChecking {
-    Checked,
-    Unchecked,
+    // `cargo rustc` historically has allowed "test", "bench", and "check". This
+    // variant explicitly allows those.
+    LegacyRustc,
+    // `cargo check` and `cargo fix` historically has allowed "test". This variant
+    // explicitly allows that on stable.
+    LegacyTestOnly,
+    // All other commands, which allow any valid custom named profile.
+    Custom,
 }
 
 pub trait ArgMatchesExt {
@@ -343,48 +349,58 @@ pub trait ArgMatchesExt {
         default: &str,
         profile_checking: ProfileChecking,
     ) -> CargoResult<InternedString> {
-        let specified_profile = match self._value_of("profile") {
-            None => None,
-            Some(name) => {
-                TomlProfile::validate_name(name, "profile name")?;
-                Some(InternedString::new(name))
-            }
-        };
+        let specified_profile = self._value_of("profile");
+
+        // Check for allowed legacy names.
+        // 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 @ ("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)),
+            _ => {}
+        }
 
-        match profile_checking {
-            ProfileChecking::Unchecked => {}
-            ProfileChecking::Checked => {
-                if specified_profile.is_some() && !config.cli_unstable().unstable_options {
-                    anyhow::bail!("Usage of `--profile` requires `-Z unstable-options`")
-                }
-            }
+        if specified_profile.is_some() && !config.cli_unstable().unstable_options {
+            bail!("usage of `--profile` requires `-Z unstable-options`");
         }
 
-        if self._is_present("release") {
-            if !config.cli_unstable().unstable_options {
-                Ok(InternedString::new("release"))
-            } else {
-                match specified_profile {
-                    Some(name) if name != "release" => {
-                        anyhow::bail!("Conflicting usage of --profile and --release")
-                    }
-                    _ => Ok(InternedString::new("release")),
-                }
+        let conflict = |flag: &str, equiv: &str, specified: &str| -> anyhow::Error {
+            anyhow::format_err!(
+                "conflicting usage of --profile={} and --{flag}\n\
+                 The `--{flag}` flag is the same as `--profile={equiv}`.\n\
+                 Remove one flag or the other to continue.",
+                specified,
+                flag = flag,
+                equiv = equiv
+            )
+        };
+
+        let name = match (
+            self._is_present("release"),
+            self._is_present("debug"),
+            specified_profile,
+        ) {
+            (false, false, None) => default,
+            (true, _, None | Some("release")) => "release",
+            (true, _, Some(name)) => return Err(conflict("release", "release", name)),
+            (_, true, None | Some("dev")) => "dev",
+            (_, true, Some(name)) => return Err(conflict("debug", "dev", name)),
+            // `doc` is separate from all the other reservations because
+            // [profile.doc] was historically allowed, but is deprecated and
+            // has no effect. To avoid potentially breaking projects, it is a
+            // warning in Cargo.toml, but since `--profile` is new, we can
+            // reject it completely here.
+            (_, _, Some("doc")) => {
+                bail!("profile `doc` is reserved and not allowed to be explicitly specified")
             }
-        } else if self._is_present("debug") {
-            if !config.cli_unstable().unstable_options {
-                Ok(InternedString::new("dev"))
-            } else {
-                match specified_profile {
-                    Some(name) if name != "dev" => {
-                        anyhow::bail!("Conflicting usage of --profile and --debug")
-                    }
-                    _ => Ok(InternedString::new("dev")),
-                }
+            (_, _, Some(name)) => {
+                TomlProfile::validate_name(name)?;
+                name
             }
-        } else {
-            Ok(specified_profile.unwrap_or_else(|| InternedString::new(default)))
-        }
+        };
+
+        Ok(InternedString::new(name))
     }
 
     fn packages_from_flags(&self) -> CargoResult<Packages> {
diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs
diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs
diff --git a/tests/testsuite/profile_custom.rs b/tests/testsuite/profile_custom.rs
