Support custom profiles in legacy commands.

ehuss · ehuss · commit 73dba767d122 · 2021-07-13T15:22:09.000-07:00
This makes the following changes:

- Allows `cargo check`, `cargo fix`, and `cargo rustc` to support custom
  named profiles. This retains the legacy behavior of those commands.
- Fixes `cargo bench` so that it supports custom named profiles.
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
@@ -41,7 +41,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 {
@@ -345,13 +351,18 @@ pub trait ArgMatchesExt {
     ) -> CargoResult<InternedString> {
         let specified_profile = self._value_of("profile");
 
-        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`")
-                }
-            }
+        // 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)),
+            _ => {}
+        }
+
+        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 {
@@ -371,35 +382,15 @@ pub trait ArgMatchesExt {
             specified_profile,
         ) {
             (false, false, None) => default,
-            // `check` is separate from all other reservations because
-            // `--profile=check` has been stable for the `cargo rustc` command
-            // for a long time. The Unchecked commands (check, fix, rustc)
-            // have their own validation.
-            //
-            // This is explicitly ordered before the --release conflict check
-            // since `cargo rustc --profile=check --release` means to check in
-            // release mode.
-            //
-            // When stabilizing, only `cargo rustc` should be allowed to use
-            // `check` with the old semantics.
-            (_, _, Some(name @ ("test" | "bench" | "check"))) => match profile_checking {
-                ProfileChecking::Unchecked => name,
-                ProfileChecking::Checked => {
-                    bail!(
-                        "profile `{}` is reserved and not allowed to be explicitly specified",
-                        name
-                    )
-                }
-            },
             (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, but since `--profile` is new, we can reject it
-            // completely here.
+            // 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")
             }
diff --git a/tests/testsuite/profile_custom.rs b/tests/testsuite/profile_custom.rs
@@ -1,7 +1,25 @@
 //! Tests for named profiles.
 
+use cargo_test_support::paths::CargoPathExt;
 use cargo_test_support::{basic_lib_manifest, project};
 
+#[cargo_test]
+fn gated() {
+    let p = project().file("src/lib.rs", "").build();
+    // `rustc`, `fix`, and `check` have had `--profile` before custom named profiles.
+    // Without unstable, these shouldn't be allowed to access non-legacy names.
+    for command in [
+        "rustc", "fix", "check", "bench", "clean", "install", "test", "build", "doc", "run",
+        "rustdoc",
+    ] {
+        p.cargo(command)
+            .arg("--profile=release")
+            .with_status(101)
+            .with_stderr("error: usage of `--profile` requires `-Z unstable-options`")
+            .run();
+    }
+}
+
 #[cargo_test]
 fn inherits_on_release() {
     let p = project()
@@ -562,18 +580,13 @@ fn reserved_profile_names() {
         .file("src/lib.rs", "")
         .build();
 
-    for name in ["check", "doc"] {
-        p.cargo(&format!("build --profile={} -Zunstable-options", name))
-            .masquerade_as_nightly_cargo()
-            .with_status(101)
-            .with_stderr(&format!(
-                "error: profile `{}` is reserved and not allowed to be explicitly specified",
-                name
-            ))
-            .run();
-    }
+    p.cargo("build --profile=doc -Zunstable-options")
+        .masquerade_as_nightly_cargo()
+        .with_status(101)
+        .with_stderr("error: profile `doc` is reserved and not allowed to be explicitly specified")
+        .run();
     // Not an exhaustive list, just a sample.
-    for name in ["build", "cargo", "rustc", "CaRgO_startswith"] {
+    for name in ["build", "cargo", "check", "rustc", "CaRgO_startswith"] {
         p.cargo(&format!("build --profile={} -Zunstable-options", name))
             .masquerade_as_nightly_cargo()
             .with_status(101)
@@ -653,3 +666,39 @@ Caused by:
         )
         .run();
 }
+
+#[cargo_test]
+fn legacy_commands_support_custom() {
+    // These commands have had `--profile` before custom named profiles.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+               cargo-features = ["named-profiles"]
+
+               [package]
+               name = "foo"
+               version = "0.1.0"
+
+               [profile.super-dev]
+               codegen-units = 3
+               inherits = "dev"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    for command in ["rustc", "fix", "check"] {
+        let mut pb = p.cargo(command);
+        if command == "fix" {
+            pb.arg("--allow-no-vcs");
+        }
+        pb.arg("--profile=super-dev")
+            .arg("-Zunstable-options")
+            .arg("-v")
+            .masquerade_as_nightly_cargo()
+            .with_stderr_contains("[RUNNING] [..]codegen-units=3[..]")
+            .run();
+        p.build_dir().rm_rf();
+    }
+}
