From 6e3e14e223f9566773b5d903357cbf35ae528ac6 Mon Sep 17 00:00:00 2001 From: David Calavera Date: Fri, 4 Dec 2020 16:29:31 -0800 Subject: [PATCH 1/4] Allow `true` and `false` as options for `strip` option This follows the convention of `lto` and `debug` that allow `true` for the highest level, and `false` for disabled. Signed-off-by: David Calavera --- src/cargo/core/profiles.rs | 18 +++++-- src/cargo/util/toml/mod.rs | 28 +++++++++-- src/doc/src/reference/unstable.md | 7 ++- tests/testsuite/config.rs | 9 ++-- tests/testsuite/profiles.rs | 84 ++++++++++++++++++++++++++++++- 5 files changed, 131 insertions(+), 15 deletions(-) diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index dcc4011e81d..5711de30735 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -590,9 +590,21 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) { if let Some(incremental) = toml.incremental { profile.incremental = incremental; } - if let Some(strip) = toml.strip { - profile.strip = strip; - } + profile.strip = match toml.strip { + Some(StringOrBool::Bool(enabled)) if enabled => Strip::Symbols, + Some(StringOrBool::Bool(enabled)) if !enabled => Strip::None, + Some(StringOrBool::String(ref n)) if matches!(n.as_str(), "off" | "n" | "no" | "none") => { + Strip::None + } + Some(StringOrBool::String(ref n)) if matches!(n.as_str(), "debuginfo") => Strip::DebugInfo, + Some(StringOrBool::String(ref n)) if matches!(n.as_str(), "symbols") => Strip::Symbols, + None => Strip::None, + Some(ref strip) => panic!( + "unknown variant `{}`, expected one of `debuginfo`, `none`, `symbols` for key `strip` + ", + strip + ), + }; } /// The root profile (dev/release). diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index d208340e320..72e51c62460 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -16,7 +16,6 @@ use url::Url; use crate::core::dependency::DepKind; use crate::core::manifest::{ManifestMetadata, TargetSourcePath, Warnings}; use crate::core::nightly_features_allowed; -use crate::core::profiles::Strip; use crate::core::resolver::ResolveBehavior; use crate::core::{Dependency, Manifest, PackageId, Summary, Target}; use crate::core::{Edition, EitherManifest, Feature, Features, VirtualManifest, Workspace}; @@ -442,7 +441,7 @@ pub struct TomlProfile { pub build_override: Option>, pub dir_name: Option, pub inherits: Option, - pub strip: Option, + pub strip: Option, } #[derive(Clone, Debug, PartialEq, Eq, Ord, PartialOrd, Hash)] @@ -561,6 +560,18 @@ impl TomlProfile { if self.strip.is_some() { features.require(Feature::strip())?; + match self.strip { + Some(StringOrBool::Bool(_)) => {} + Some(StringOrBool::String(ref n)) => match n.as_str() { + "off" | "n" | "none" | "no" | "debuginfo" | "symbols" => {} + _ => bail!( + "`strip` setting of `{}` is not a valid setting,\ + must be `symbols`, `debuginfo`, `none`, `true`, or `false`", + n + ), + }, + None => {} + } } Ok(()) } @@ -686,8 +697,8 @@ impl TomlProfile { self.dir_name = Some(*v); } - if let Some(v) = profile.strip { - self.strip = Some(v); + if let Some(v) = &profile.strip { + self.strip = Some(v.clone()); } } } @@ -769,6 +780,15 @@ impl<'de> de::Deserialize<'de> for StringOrBool { } } +impl fmt::Display for StringOrBool { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::String(s) => write!(f, "{}", &s), + Self::Bool(b) => write!(f, "{}", b), + } + } +} + #[derive(PartialEq, Clone, Debug, Serialize)] #[serde(untagged)] pub enum VecStringOrBool { diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 52e3459a389..49ec4187350 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -765,8 +765,11 @@ cargo-features = ["strip"] strip = "debuginfo" ``` -Other possible values of `strip` are `none` and `symbols`. The default is -`none`. +Other possible string values of `strip` are `none`, `symbols`, and `off`. The default is `none`. + +You can also configure this option with the two absolute boolean values +`true` and `false`. The former enables `strip` at its higher level, `symbols`, +whilst the later disables `strip` completely. ### rustdoc-map * Tracking Issue: [#8296](https://github.com/rust-lang/cargo/issues/8296) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 0723d3cad97..4a646bfca7d 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1,6 +1,5 @@ //! Tests for config settings. -use cargo::core::profiles::Strip; use cargo::core::{enable_nightly_features, Shell}; use cargo::util::config::{self, Config, SslVersionConfig, StringList}; use cargo::util::interning::InternedString; @@ -1446,7 +1445,7 @@ fn string_list_advanced_env() { } #[cargo_test] -fn parse_enum() { +fn parse_strip_with_string() { write_config( "\ [profile.release] @@ -1458,11 +1457,11 @@ strip = 'debuginfo' let p: toml::TomlProfile = config.get("profile.release").unwrap(); let strip = p.strip.unwrap(); - assert_eq!(strip, Strip::DebugInfo); + assert_eq!(strip, toml::StringOrBool::String("debuginfo".to_string())); } #[cargo_test] -fn parse_enum_fail() { +fn parse_strip_with_unknown_string_fail() { write_config( "\ [profile.release] @@ -1480,6 +1479,6 @@ strip = 'invalid' error in [..]/.cargo/config: could not load config key `profile.release.strip` Caused by: - unknown variant `invalid`, expected one of `debuginfo`, `none`, `symbols`", + `strip` setting of `wrong` is not a valid setting,must be `symbols`, `debuginfo`, `none`, `true`, or `false`", ); } diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs index d238d9a6392..c923ae1a621 100644 --- a/tests/testsuite/profiles.rs +++ b/tests/testsuite/profiles.rs @@ -509,6 +509,11 @@ fn strip_works() { #[cargo_test] fn strip_requires_cargo_feature() { + if !is_nightly() { + // -Zstrip is unstable + return; + } + let p = project() .file( "Cargo.toml", @@ -542,6 +547,11 @@ Caused by: #[cargo_test] fn strip_rejects_invalid_option() { + if !is_nightly() { + // -Zstrip is unstable + return; + } + let p = project() .file( "Cargo.toml", @@ -567,7 +577,79 @@ fn strip_rejects_invalid_option() { [ERROR] failed to parse manifest at `[CWD]/Cargo.toml` Caused by: - unknown variant `wrong`, expected one of `debuginfo`, `none`, `symbols` for key [..] + unknown variant `wrong`, expected one of `debuginfo`, `none`, `symbols` for key `strip` +", + ) + .run(); +} + +#[cargo_test] +fn strip_accepts_true_to_strip_symbols() { + if !is_nightly() { + // -Zstrip is unstable + return; + } + + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["strip"] + + [package] + name = "foo" + version = "0.1.0" + + [profile.release] + strip = true + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build --release -v") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[COMPILING] foo [..] +[RUNNING] `rustc [..] -Z strip=symbols [..]` +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test] +fn strip_accepts_false_to_disable_strip() { + if !is_nightly() { + // -Zstrip is unstable + return; + } + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["strip"] + + [package] + name = "foo" + version = "0.1.0" + + [profile.release] + strip = false + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build --release -v") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[COMPILING] foo [..] +[RUNNING] `rustc [..] -Z strip=none + [..]` +[FINISHED] [..] ", ) .run(); From 933cbc439e1758a9431d013a77698e9db79019b8 Mon Sep 17 00:00:00 2001 From: David Calavera Date: Mon, 8 Feb 2021 14:20:11 -0800 Subject: [PATCH 2/4] Remove invalid test Verifying options doesn't happen when the file is parsed. This scenario is already covered in another test. See [the profiles test suite](https://github.com/rust-lang/cargo/blob/6e3e14e223f9566773b5d903357cbf35ae528ac6/tests/testsuite/profiles.rs#L549) Signed-off-by: David Calavera --- tests/testsuite/config.rs | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 4a646bfca7d..acf8c45fa31 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1459,26 +1459,3 @@ strip = 'debuginfo' let strip = p.strip.unwrap(); assert_eq!(strip, toml::StringOrBool::String("debuginfo".to_string())); } - -#[cargo_test] -fn parse_strip_with_unknown_string_fail() { - write_config( - "\ -[profile.release] -strip = 'invalid' -", - ); - - let config = new_config(); - - assert_error( - config - .get::("profile.release") - .unwrap_err(), - "\ -error in [..]/.cargo/config: could not load config key `profile.release.strip` - -Caused by: - `strip` setting of `wrong` is not a valid setting,must be `symbols`, `debuginfo`, `none`, `true`, or `false`", - ); -} From eb346171aa6a3b597f6c60257d6edb4d52ab3b54 Mon Sep 17 00:00:00 2001 From: David Calavera Date: Tue, 9 Feb 2021 13:46:47 -0800 Subject: [PATCH 3/4] Simplify Strip option - Accept a string option that's passed to rustc to decide how to handle it. - Remove early validation. Signed-off-by: David Calavera --- src/cargo/core/profiles.rs | 36 ++++++++++++++---------------------- src/cargo/util/toml/mod.rs | 21 --------------------- tests/testsuite/profiles.rs | 20 ++++++-------------- 3 files changed, 20 insertions(+), 57 deletions(-) diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 5711de30735..09b960ea48b 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -552,9 +552,7 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) { } match toml.lto { Some(StringOrBool::Bool(b)) => profile.lto = Lto::Bool(b), - Some(StringOrBool::String(ref n)) if matches!(n.as_str(), "off" | "n" | "no") => { - profile.lto = Lto::Off - } + Some(StringOrBool::String(ref n)) if is_off(n.as_str()) => profile.lto = Lto::Off, Some(StringOrBool::String(ref n)) => profile.lto = Lto::Named(InternedString::new(n)), None => {} } @@ -591,19 +589,10 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) { profile.incremental = incremental; } profile.strip = match toml.strip { - Some(StringOrBool::Bool(enabled)) if enabled => Strip::Symbols, - Some(StringOrBool::Bool(enabled)) if !enabled => Strip::None, - Some(StringOrBool::String(ref n)) if matches!(n.as_str(), "off" | "n" | "no" | "none") => { - Strip::None - } - Some(StringOrBool::String(ref n)) if matches!(n.as_str(), "debuginfo") => Strip::DebugInfo, - Some(StringOrBool::String(ref n)) if matches!(n.as_str(), "symbols") => Strip::Symbols, - None => Strip::None, - Some(ref strip) => panic!( - "unknown variant `{}`, expected one of `debuginfo`, `none`, `symbols` for key `strip` - ", - strip - ), + Some(StringOrBool::Bool(true)) => Strip::Named(InternedString::new("symbols")), + None | Some(StringOrBool::Bool(false)) => Strip::None, + Some(StringOrBool::String(ref n)) if is_off(n.as_str()) => Strip::None, + Some(StringOrBool::String(ref n)) => Strip::Named(InternedString::new(n)), }; } @@ -821,24 +810,22 @@ impl fmt::Display for PanicStrategy { )] #[serde(rename_all = "lowercase")] pub enum Strip { - /// Only strip debugging symbols - DebugInfo, /// Don't remove any symbols None, - /// Strip all non-exported symbols from the final binary - Symbols, + /// Named Strip settings + Named(InternedString), } impl fmt::Display for Strip { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match *self { - Strip::DebugInfo => "debuginfo", Strip::None => "none", - Strip::Symbols => "symbols", + Strip::Named(s) => s.as_str(), } .fmt(f) } } + /// Flags used in creating `Unit`s to indicate the purpose for the target, and /// to ensure the target's dependencies have the correct settings. #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] @@ -1261,3 +1248,8 @@ fn validate_packages_unmatched( } Ok(()) } + +/// Returns `true` if a string is a toggle that turns an option off. +fn is_off(s: &str) -> bool { + matches!(s, "off" | "n" | "no" | "none") +} diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 72e51c62460..6d6b9f1e76a 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -560,18 +560,6 @@ impl TomlProfile { if self.strip.is_some() { features.require(Feature::strip())?; - match self.strip { - Some(StringOrBool::Bool(_)) => {} - Some(StringOrBool::String(ref n)) => match n.as_str() { - "off" | "n" | "none" | "no" | "debuginfo" | "symbols" => {} - _ => bail!( - "`strip` setting of `{}` is not a valid setting,\ - must be `symbols`, `debuginfo`, `none`, `true`, or `false`", - n - ), - }, - None => {} - } } Ok(()) } @@ -780,15 +768,6 @@ impl<'de> de::Deserialize<'de> for StringOrBool { } } -impl fmt::Display for StringOrBool { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::String(s) => write!(f, "{}", &s), - Self::Bool(b) => write!(f, "{}", b), - } - } -} - #[derive(PartialEq, Clone, Debug, Serialize)] #[serde(untagged)] pub enum VecStringOrBool { diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs index c923ae1a621..878ff681e0c 100644 --- a/tests/testsuite/profiles.rs +++ b/tests/testsuite/profiles.rs @@ -546,7 +546,7 @@ Caused by: } #[cargo_test] -fn strip_rejects_invalid_option() { +fn strip_passes_unknown_option_to_rustc() { if !is_nightly() { // -Zstrip is unstable return; @@ -563,7 +563,7 @@ fn strip_rejects_invalid_option() { version = "0.1.0" [profile.release] - strip = 'wrong' + strip = 'unknown' "#, ) .file("src/main.rs", "fn main() {}") @@ -574,10 +574,9 @@ fn strip_rejects_invalid_option() { .with_status(101) .with_stderr( "\ -[ERROR] failed to parse manifest at `[CWD]/Cargo.toml` - -Caused by: - unknown variant `wrong`, expected one of `debuginfo`, `none`, `symbols` for key `strip` +[COMPILING] foo [..] +[RUNNING] `rustc [..] -Z strip=unknown [..]` +[FINISHED] [..] ", ) .run(); @@ -644,13 +643,6 @@ fn strip_accepts_false_to_disable_strip() { p.cargo("build --release -v") .masquerade_as_nightly_cargo() - .with_stderr( - "\ -[COMPILING] foo [..] -[RUNNING] `rustc [..] -Z strip=none - [..]` -[FINISHED] [..] -", - ) + .with_stderr_does_not_contain("-Z strip") .run(); } From 666380f2e362984d7adb4a7aa9ca2c0ff28a6679 Mon Sep 17 00:00:00 2001 From: David Calavera Date: Tue, 9 Feb 2021 14:24:56 -0800 Subject: [PATCH 4/4] Fix assertion for when an unknown option is passed to rustc The compiler emits an error when it receives an unknown option. Signed-off-by: David Calavera --- tests/testsuite/profiles.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs index 878ff681e0c..6ed10bc4a89 100644 --- a/tests/testsuite/profiles.rs +++ b/tests/testsuite/profiles.rs @@ -572,11 +572,11 @@ fn strip_passes_unknown_option_to_rustc() { p.cargo("build --release -v") .masquerade_as_nightly_cargo() .with_status(101) - .with_stderr( + .with_stderr_contains( "\ [COMPILING] foo [..] [RUNNING] `rustc [..] -Z strip=unknown [..]` -[FINISHED] [..] +error: incorrect value `unknown` for debugging option `strip` - either `none`, `debuginfo`, or `symbols` was expected ", ) .run();