Auto merge of #10413 - ehuss:beta-fix-rustflags-gate, r=alexcrichton

bors · bors · commit 1e5cac721024 · 2022-02-22T23:10:37.000Z
[beta] Add common profile validation. Beta backport of: * #10411 — This is intended to limit any potential misuse of rustflags in profiles. * #10396 — Fix CI due to clap deprecation
diff --git a/Cargo.toml b/Cargo.toml
@@ -61,7 +61,7 @@ toml_edit =  { version = "0.13.4", features = ["serde", "easy"] }
 unicode-xid = "0.2.0"
 url = "2.2.2"
 walkdir = "2.2"
-clap = "3.0.13"
+clap = "3.1.0"
 unicode-width = "0.1.5"
 openssl = { version = '0.10.11', optional = true }
 im-rc = "15.0.0"
diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs
@@ -1,7 +1,10 @@
 use anyhow::anyhow;
 use cargo::core::{features, CliUnstable};
 use cargo::{self, drop_print, drop_println, CliResult, Config};
-use clap::{AppSettings, Arg, ArgMatches};
+use clap::{
+    error::{ContextKind, ContextValue},
+    AppSettings, Arg, ArgMatches,
+};
 use itertools::Itertools;
 use std::collections::HashMap;
 use std::fmt::Write;
@@ -33,9 +36,17 @@ pub fn main(config: &mut Config) -> CliResult {
     let args = match cli().try_get_matches() {
         Ok(args) => args,
         Err(e) => {
-            if e.kind == clap::ErrorKind::UnrecognizedSubcommand {
+            if e.kind() == clap::ErrorKind::UnrecognizedSubcommand {
                 // An unrecognized subcommand might be an external subcommand.
-                let cmd = e.info[0].clone();
+                let cmd = e
+                    .context()
+                    .find_map(|c| match c {
+                        (ContextKind::InvalidSubcommand, &ContextValue::String(ref cmd)) => {
+                            Some(cmd)
+                        }
+                        _ => None,
+                    })
+                    .expect("UnrecognizedSubcommand implies the presence of InvalidSubcommand");
                 return super::execute_external_subcommand(config, &cmd, &[&cmd, "--help"])
                     .map_err(|_| e.into());
             } else {
@@ -286,9 +297,7 @@ For more information, see issue #10049 <https://github.com/rust-lang/cargo/issue
                 // Note that an alias to an external command will not receive
                 // these arguments. That may be confusing, but such is life.
                 let global_args = GlobalArgs::new(args);
-                let new_args = cli()
-                    .setting(AppSettings::NoBinaryName)
-                    .try_get_matches_from(alias)?;
+                let new_args = cli().no_binary_name(true).try_get_matches_from(alias)?;
 
                 let new_cmd = new_args.subcommand_name().expect("subcommand is required");
                 already_expanded.push(cmd.to_string());
@@ -406,14 +415,11 @@ fn cli() -> App {
         "cargo [OPTIONS] [SUBCOMMAND]"
     };
     App::new("cargo")
-        .setting(
-            AppSettings::DeriveDisplayOrder
-                | AppSettings::AllowExternalSubcommands
-                | AppSettings::NoAutoVersion,
-        )
+        .allow_external_subcommands(true)
+        .setting(AppSettings::DeriveDisplayOrder | AppSettings::NoAutoVersion)
         // Doesn't mix well with our list of common cargo commands.  See clap-rs/clap#3108 for
         // opening clap up to allow us to style our help template
-        .global_setting(AppSettings::DisableColoredHelp)
+        .disable_colored_help(true)
         .override_usage(usage)
         .help_template(
             "\
diff --git a/src/bin/cargo/commands/bench.rs b/src/bin/cargo/commands/bench.rs
@@ -3,7 +3,7 @@ use cargo::ops::{self, TestOptions};
 
 pub fn cli() -> App {
     subcommand("bench")
-        .setting(AppSettings::TrailingVarArg)
+        .trailing_var_arg(true)
         .about("Execute all benchmarks of a local package")
         .arg_quiet()
         .arg(
diff --git a/src/bin/cargo/commands/config.rs b/src/bin/cargo/commands/config.rs
@@ -5,7 +5,8 @@ pub fn cli() -> App {
     subcommand("config")
         .about("Inspect configuration values")
         .after_help("Run `cargo help config` for more detailed information.\n")
-        .setting(clap::AppSettings::SubcommandRequiredElseHelp)
+        .subcommand_required(true)
+        .arg_required_else_help(true)
         .subcommand(
             subcommand("get")
                 .arg(Arg::new("key").help("The config key to display"))
diff --git a/src/bin/cargo/commands/git_checkout.rs b/src/bin/cargo/commands/git_checkout.rs
@@ -5,7 +5,7 @@ const REMOVED: &str = "The `git-checkout` subcommand has been removed.";
 pub fn cli() -> App {
     subcommand("git-checkout")
         .about("This subcommand has been removed")
-        .setting(AppSettings::Hidden)
+        .hide(true)
         .override_help(REMOVED)
 }
 
diff --git a/src/bin/cargo/commands/report.rs b/src/bin/cargo/commands/report.rs
@@ -6,7 +6,8 @@ pub fn cli() -> App {
     subcommand("report")
         .about("Generate and display various kinds of reports")
         .after_help("Run `cargo help report` for more detailed information.\n")
-        .setting(clap::AppSettings::SubcommandRequiredElseHelp)
+        .subcommand_required(true)
+        .arg_required_else_help(true)
         .subcommand(
             subcommand("future-incompatibilities")
                 .alias("future-incompat")
diff --git a/src/bin/cargo/commands/run.rs b/src/bin/cargo/commands/run.rs
@@ -8,7 +8,7 @@ pub fn cli() -> App {
     subcommand("run")
         // subcommand aliases are handled in aliased_command()
         // .alias("r")
-        .setting(AppSettings::TrailingVarArg)
+        .trailing_var_arg(true)
         .about("Run a binary or example of the local package")
         .arg_quiet()
         .arg(
diff --git a/src/bin/cargo/commands/rustc.rs b/src/bin/cargo/commands/rustc.rs
@@ -7,7 +7,7 @@ const CRATE_TYPE_ARG_NAME: &str = "crate-type";
 
 pub fn cli() -> App {
     subcommand("rustc")
-        .setting(AppSettings::TrailingVarArg)
+        .trailing_var_arg(true)
         .about("Compile a package, and pass extra options to the compiler")
         .arg_quiet()
         .arg(Arg::new("args").multiple_values(true).help("Rustc flags"))
diff --git a/src/bin/cargo/commands/rustdoc.rs b/src/bin/cargo/commands/rustdoc.rs
@@ -4,7 +4,7 @@ use crate::command_prelude::*;
 
 pub fn cli() -> App {
     subcommand("rustdoc")
-        .setting(AppSettings::TrailingVarArg)
+        .trailing_var_arg(true)
         .about("Build a package's documentation, using specified custom flags.")
         .arg_quiet()
         .arg(Arg::new("args").multiple_values(true))
diff --git a/src/bin/cargo/commands/test.rs b/src/bin/cargo/commands/test.rs
@@ -6,7 +6,7 @@ pub fn cli() -> App {
     subcommand("test")
         // Subcommand aliases are handled in `aliased_command()`.
         // .alias("t")
-        .setting(AppSettings::TrailingVarArg)
+        .trailing_var_arg(true)
         .about("Execute all unit and integration tests and build examples of a local package")
         .arg(
             Arg::new("TESTNAME")
diff --git a/src/cargo/util/command_prelude.rs b/src/cargo/util/command_prelude.rs
@@ -22,7 +22,7 @@ pub use crate::core::compiler::CompileMode;
 pub use crate::{CliError, CliResult, Config};
 pub use clap::{AppSettings, Arg, ArgMatches};
 
-pub type App = clap::App<'static>;
+pub type App = clap::Command<'static>;
 
 pub trait AppExt: Sized {
     fn _arg(self, arg: Arg<'static>) -> Self;
@@ -281,7 +281,9 @@ pub fn multi_opt(name: &'static str, value_name: &'static str, help: &'static st
 }
 
 pub fn subcommand(name: &'static str) -> App {
-    App::new(name).setting(AppSettings::DeriveDisplayOrder | AppSettings::DontCollapseArgsInUsage)
+    App::new(name)
+        .dont_collapse_args_in_usage(true)
+        .setting(AppSettings::DeriveDisplayOrder)
 }
 
 /// Determines whether or not to gate `--profile` as unstable when resolving it.
diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs
@@ -448,10 +448,7 @@ impl ser::Serialize for ProfilePackageSpec {
     where
         S: ser::Serializer,
     {
-        match *self {
-            ProfilePackageSpec::Spec(ref spec) => spec.serialize(s),
-            ProfilePackageSpec::All => "*".serialize(s),
-        }
+        self.to_string().serialize(s)
     }
 }
 
@@ -471,21 +468,33 @@ impl<'de> de::Deserialize<'de> for ProfilePackageSpec {
     }
 }
 
+impl fmt::Display for ProfilePackageSpec {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            ProfilePackageSpec::Spec(spec) => spec.fmt(f),
+            ProfilePackageSpec::All => f.write_str("*"),
+        }
+    }
+}
+
 impl TomlProfile {
     pub fn validate(
         &self,
         name: &str,
         features: &Features,
         warnings: &mut Vec<String>,
     ) -> CargoResult<()> {
+        self.validate_profile(name, features)?;
         if let Some(ref profile) = self.build_override {
             features.require(Feature::profile_overrides())?;
-            profile.validate_override("build-override", features)?;
+            profile.validate_override("build-override")?;
+            profile.validate_profile(&format!("{name}.build-override"), features)?;
         }
         if let Some(ref packages) = self.package {
             features.require(Feature::profile_overrides())?;
-            for profile in packages.values() {
-                profile.validate_override("package", features)?;
+            for (override_name, profile) in packages {
+                profile.validate_override("package")?;
+                profile.validate_profile(&format!("{name}.package.{override_name}"), features)?;
             }
         }
 
@@ -548,21 +557,6 @@ impl TomlProfile {
             }
         }
 
-        if self.rustflags.is_some() {
-            features.require(Feature::profile_rustflags())?;
-        }
-
-        if let Some(codegen_backend) = &self.codegen_backend {
-            features.require(Feature::codegen_backend())?;
-            if codegen_backend.contains(|c: char| !c.is_ascii_alphanumeric() && c != '_') {
-                bail!(
-                    "`profile.{}.codegen-backend` setting of `{}` is not a valid backend name.",
-                    name,
-                    codegen_backend,
-                );
-            }
-        }
-
         Ok(())
     }
 
@@ -645,7 +639,28 @@ impl TomlProfile {
         Ok(())
     }
 
-    fn validate_override(&self, which: &str, features: &Features) -> CargoResult<()> {
+    /// Validates a profile.
+    ///
+    /// This is a shallow check, which is reused for the profile itself and any overrides.
+    fn validate_profile(&self, name: &str, features: &Features) -> CargoResult<()> {
+        if let Some(codegen_backend) = &self.codegen_backend {
+            features.require(Feature::codegen_backend())?;
+            if codegen_backend.contains(|c: char| !c.is_ascii_alphanumeric() && c != '_') {
+                bail!(
+                    "`profile.{}.codegen-backend` setting of `{}` is not a valid backend name.",
+                    name,
+                    codegen_backend,
+                );
+            }
+        }
+        if self.rustflags.is_some() {
+            features.require(Feature::profile_rustflags())?;
+        }
+        Ok(())
+    }
+
+    /// Validation that is specific to an override.
+    fn validate_override(&self, which: &str) -> CargoResult<()> {
         if self.package.is_some() {
             bail!("package-specific profiles cannot be nested");
         }
@@ -661,9 +676,6 @@ impl TomlProfile {
         if self.rpath.is_some() {
             bail!("`rpath` may not be specified in a `{}` profile", which)
         }
-        if self.codegen_backend.is_some() {
-            features.require(Feature::codegen_backend())?;
-        }
         Ok(())
     }
 
diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs
@@ -3,6 +3,7 @@
 use std::env;
 
 use cargo_test_support::project;
+use cargo_test_support::registry::Package;
 
 #[cargo_test]
 fn profile_overrides() {
@@ -660,6 +661,41 @@ fn rustflags_requires_cargo_feature() {
             "\
 [ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
 
+Caused by:
+  feature `profile-rustflags` is required
+
+  The package requires the Cargo feature called `profile-rustflags`, but that feature is \
+  not stabilized in this version of Cargo (1.[..]).
+  Consider adding `cargo-features = [\"profile-rustflags\"]` to the top of Cargo.toml \
+  (above the [package] table) to tell Cargo you are opting in to use this unstable feature.
+  See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#profile-rustflags-option \
+  for more information about the status of this feature.
+",
+        )
+        .run();
+
+    Package::new("bar", "1.0.0").publish();
+    p.change_file(
+        "Cargo.toml",
+        r#"
+            [package]
+            name = "foo"
+            version = "0.0.1"
+
+            [dependencies]
+            bar = "1.0"
+
+            [profile.dev.package.bar]
+            rustflags = ["-C", "link-dead-code=yes"]
+        "#,
+    );
+    p.cargo("check")
+        .masquerade_as_nightly_cargo()
+        .with_status(101)
+        .with_stderr(
+            "\
+error: failed to parse manifest at `[ROOT]/foo/Cargo.toml`
+
 Caused by:
   feature `profile-rustflags` is required
 

Original file line number	Diff line number	Diff line change
@@ -5,7 +5,7 @@ const REMOVED: &str = "The `git-checkout` subcommand has been removed.";
`5`	`5`	`pub fn cli() -> App {`
`6`	`6`	`subcommand("git-checkout")`
`7`	`7`	`.about("This subcommand has been removed")`
`8`		`- .setting(AppSettings::Hidden)`
	`8`	`+ .hide(true)`
`9`	`9`	`.override_help(REMOVED)`
`10`	`10`	`}`
`11`	`11`