From 1cea4cbe854a0cedb69e42058977c35a9b762bca Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Sun, 21 Jun 2020 14:16:00 -0600 Subject: [PATCH 01/17] Allow populating CliUnstable from a HashMap This makes it easier to populate unstable options from configuration files. Might also help make unstable option tests easier to write. --- src/cargo/core/features.rs | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 8ed33b2f615..e7374c41639 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -46,6 +46,7 @@ //! we'll be sure to update this documentation! use std::cell::Cell; +use std::collections::HashMap; use std::env; use std::fmt; use std::str::FromStr; @@ -371,16 +372,33 @@ impl CliUnstable { ); } for flag in flags { - self.add(flag)?; + let mut parts = flag.splitn(2, '='); + let k = parts.next().unwrap(); + let v = parts.next(); + self.add(k, v)?; } Ok(()) } - fn add(&mut self, flag: &str) -> CargoResult<()> { - let mut parts = flag.splitn(2, '='); - let k = parts.next().unwrap(); - let v = parts.next(); + /// Read unstable options from a hashmap. + /// Intended for consuming unstable settings from config files + pub fn from_table(&mut self, flags: &HashMap) -> CargoResult<()> { + if !flags.is_empty() && !nightly_features_allowed() { + bail!( + "the `-Z` flag is only accepted on the nightly channel of Cargo, \ + but this is the `{}` channel\n\ + {}", + channel(), + SEE_CHANNELS + ); + } + for (k, v) in flags { + self.add(&k, Some(v.as_str()))?; + } + Ok(()) + } + fn add(&mut self, k: &str, v: Option<&str>) -> CargoResult<()> { fn parse_bool(key: &str, value: Option<&str>) -> CargoResult { match value { None | Some("yes") => Ok(true), From 4aede8c7847748c3b46afa176436387cf8f0593a Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Sun, 21 Jun 2020 14:19:37 -0600 Subject: [PATCH 02/17] Allow setting unstable options in .cargo/config Obviously this only works with nightlies and all that, but if you're e.g. testing resolver v2, want to always have Ztiming on in your workspace, or something like that, this makes it easier to do. --- src/cargo/util/config/mod.rs | 17 ++++++-- src/doc/src/reference/unstable.md | 10 +++++ tests/testsuite/config.rs | 66 +++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 3c8a5590c60..fe18f478d3e 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -63,7 +63,7 @@ use std::str::FromStr; use std::sync::Once; use std::time::Instant; -use anyhow::{anyhow, bail}; +use anyhow::{anyhow, bail, Context}; use curl::easy::Easy; use lazycell::LazyCell; use serde::Deserialize; @@ -742,9 +742,20 @@ impl Config { .unwrap_or(false); self.target_dir = cli_target_dir; + // If nightly features are enabled, allow setting Z-flags from config + // using the `unstable` table. Ignore that block otherwise. if nightly_features_allowed() { - if let Some(val) = self.get::>("unstable.mtime_on_use")? { - self.unstable_flags.mtime_on_use |= val; + if let Some(unstable_configs) = + self.get::>>("unstable")? + { + self.unstable_flags + .from_table(&unstable_configs) + .with_context(|| "Invalid [unstable] entry in Cargo config")?; + + // NB. It sucks to parse these twice, but doing it again here + // allows the CLI to override config files for both enabling + // and disabling. + self.unstable_flags.parse(unstable_flags)?; } } diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 658231e3541..4cd021746e6 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -7,6 +7,16 @@ see a list of flags available. `-Z unstable-options` is a generic flag for enabling other unstable command-line flags. Options requiring this will be called out below. +Anything which can be configured with a Z flag can also be set in the cargo +config file (`.cargo/config.toml`) in the `unstable` table. For example: + +``` +[unstable] +mtime-on-use = 'yes' +multitarget = 'yes' +timings = 'yes' +``` + Some unstable features will require you to specify the `cargo-features` key in `Cargo.toml`. diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 29e79dba37a..7b3384aa67b 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1094,6 +1094,72 @@ Caused by: .is_none()); } +#[cargo_test] +/// Assert that unstable options can be configured with the `unstable` table in +/// cargo config files +fn config_unstable_table() { + cargo::core::enable_nightly_features(); + write_config( + "\ +[unstable] +print-im-a-teapot = 'yes' +", + ); + let config = ConfigBuilder::new().build(); + assert_eq!(config.cli_unstable().print_im_a_teapot, true); +} + +#[cargo_test] +/// Assert that dotted notation works for configuring unstable options +fn config_unstable_dotted() { + cargo::core::enable_nightly_features(); + write_config( + "\ +unstable.print-im-a-teapot = 'yes' +", + ); + let config = ConfigBuilder::new().build(); + assert_eq!(config.cli_unstable().print_im_a_teapot, true); +} + +#[cargo_test] +/// Assert that Zflags on the CLI take precedence over those from config +fn config_unstable_cli_wins() { + cargo::core::enable_nightly_features(); + write_config( + "\ +unstable.print-im-a-teapot = 'yes' +", + ); + let config = ConfigBuilder::new().build(); + assert_eq!(config.cli_unstable().print_im_a_teapot, true); + + let config = ConfigBuilder::new() + .unstable_flag("print-im-a-teapot=no") + .build(); + assert_eq!(config.cli_unstable().print_im_a_teapot, false); +} + +#[cargo_test] +/// Assert that atempting to set an unstable flag that doesn't exist via config +/// errors out the same as it would on the command line +fn config_unstable_invalid_flag() { + cargo::core::enable_nightly_features(); + write_config( + "\ +unstable.an-invalid-flag = 'yes' +", + ); + assert_error( + ConfigBuilder::new().build_err().unwrap_err(), + "\ +Invalid [unstable] entry in Cargo config + +Caused by: + unknown `-Z` flag specified: an-invalid-flag", + ); +} + #[cargo_test] fn table_merge_failure() { // Config::merge fails to merge entries in two tables. From 9cb1ef88b6a7241773ebec42fdd959b065b10368 Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Sun, 21 Jun 2020 14:18:44 -0600 Subject: [PATCH 03/17] Allow both dashes and underscores for Z flags This makes it a little easier to match whatever the natural formatting is for where you're setting unstable options -- CLI or config file. --- src/cargo/core/features.rs | 4 +++- src/doc/src/reference/unstable.md | 3 ++- tests/testsuite/config_cli.rs | 20 ++++++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index e7374c41639..776a41e13f6 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -429,7 +429,9 @@ impl CliUnstable { Ok(true) }; - match k { + // Permit dashes or underscores in parsing these + let normalized_key = k.replace("_", "-"); + match normalized_key.as_str() { "print-im-a-teapot" => self.print_im_a_teapot = parse_bool(k, v)?, "unstable-options" => self.unstable_options = parse_empty(k, v)?, "no-index-update" => self.no_index_update = parse_empty(k, v)?, diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 4cd021746e6..c915c2b67bf 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -2,7 +2,8 @@ Experimental Cargo features are only available on the nightly channel. You typically use one of the `-Z` flags to enable them. Run `cargo -Z help` to -see a list of flags available. +see a list of flags available. All Z flags accept either dashes or underscores +as separators. `-Z unstable-options` is a generic flag for enabling other unstable command-line flags. Options requiring this will be called out below. diff --git a/tests/testsuite/config_cli.rs b/tests/testsuite/config_cli.rs index e8a78e7c462..cd18a1b489c 100644 --- a/tests/testsuite/config_cli.rs +++ b/tests/testsuite/config_cli.rs @@ -267,6 +267,26 @@ fn rerooted_remains() { assert_eq!(config.get::("c").unwrap(), "cli2"); } +#[cargo_test] +fn unstable_dash_underscore_interchangable() { + // Confirm unstable flag parsing treats underscores and dashes + // interchangably when coming from the CLI. + let config = ConfigBuilder::new() + .unstable_flag("print_im_a_teapot") + .build(); + assert_eq!(config.cli_unstable().print_im_a_teapot, true); + + let config = ConfigBuilder::new() + .unstable_flag("print-im-a-teapot") + .build(); + assert_eq!(config.cli_unstable().print_im_a_teapot, true); + + let config = ConfigBuilder::new() + .unstable_flag("print_im-a_teapot") + .build(); + assert_eq!(config.cli_unstable().print_im_a_teapot, true); +} + #[cargo_test] fn bad_parse() { // Fail to TOML parse. From c60da96b511ea48cacdbe6611b40698db7feb6f8 Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Wed, 24 Jun 2020 17:30:37 -0600 Subject: [PATCH 04/17] Update src/doc/src/reference/unstable.md Co-authored-by: Eric Huss --- src/doc/src/reference/unstable.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index c915c2b67bf..2ce8f2274e0 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -11,7 +11,7 @@ command-line flags. Options requiring this will be called out below. Anything which can be configured with a Z flag can also be set in the cargo config file (`.cargo/config.toml`) in the `unstable` table. For example: -``` +```toml [unstable] mtime-on-use = 'yes' multitarget = 'yes' From 6d7421cacb545496d57cfcd4b9eea89ed81ab03e Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Wed, 24 Jun 2020 17:42:51 -0600 Subject: [PATCH 05/17] Clarify CliUnstable::parse/update_with_table These masquerade like construction functions, but they're in-place mutators since they're meant to be used one after another in a precedence chain. --- src/cargo/core/features.rs | 5 +++-- src/cargo/util/config/mod.rs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 776a41e13f6..0c1e625ac97 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -361,6 +361,7 @@ pub struct CliUnstable { } impl CliUnstable { + /// Update unstable options in-place from a slice of flag strings pub fn parse(&mut self, flags: &[String]) -> CargoResult<()> { if !flags.is_empty() && !nightly_features_allowed() { bail!( @@ -380,9 +381,9 @@ impl CliUnstable { Ok(()) } - /// Read unstable options from a hashmap. + /// Read unstable options from a hashmap, updating in-place. /// Intended for consuming unstable settings from config files - pub fn from_table(&mut self, flags: &HashMap) -> CargoResult<()> { + pub fn update_with_table(&mut self, flags: &HashMap) -> CargoResult<()> { if !flags.is_empty() && !nightly_features_allowed() { bail!( "the `-Z` flag is only accepted on the nightly channel of Cargo, \ diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index fe18f478d3e..bb314935218 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -749,7 +749,7 @@ impl Config { self.get::>>("unstable")? { self.unstable_flags - .from_table(&unstable_configs) + .update_with_table(&unstable_configs) .with_context(|| "Invalid [unstable] entry in Cargo config")?; // NB. It sucks to parse these twice, but doing it again here From 48a6f59ec368853fdc26c0a02f62cd8b99633804 Mon Sep 17 00:00:00 2001 From: Alexander Berghage Date: Mon, 29 Jun 2020 19:47:21 -0600 Subject: [PATCH 06/17] Revert misfeatures Per comments on the PR, a couple changes need to be backed out. --- src/cargo/core/features.rs | 33 ++++++--------------------------- tests/testsuite/config_cli.rs | 20 -------------------- 2 files changed, 6 insertions(+), 47 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 0c1e625ac97..8ed33b2f615 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -46,7 +46,6 @@ //! we'll be sure to update this documentation! use std::cell::Cell; -use std::collections::HashMap; use std::env; use std::fmt; use std::str::FromStr; @@ -361,7 +360,6 @@ pub struct CliUnstable { } impl CliUnstable { - /// Update unstable options in-place from a slice of flag strings pub fn parse(&mut self, flags: &[String]) -> CargoResult<()> { if !flags.is_empty() && !nightly_features_allowed() { bail!( @@ -373,33 +371,16 @@ impl CliUnstable { ); } for flag in flags { - let mut parts = flag.splitn(2, '='); - let k = parts.next().unwrap(); - let v = parts.next(); - self.add(k, v)?; + self.add(flag)?; } Ok(()) } - /// Read unstable options from a hashmap, updating in-place. - /// Intended for consuming unstable settings from config files - pub fn update_with_table(&mut self, flags: &HashMap) -> CargoResult<()> { - if !flags.is_empty() && !nightly_features_allowed() { - bail!( - "the `-Z` flag is only accepted on the nightly channel of Cargo, \ - but this is the `{}` channel\n\ - {}", - channel(), - SEE_CHANNELS - ); - } - for (k, v) in flags { - self.add(&k, Some(v.as_str()))?; - } - Ok(()) - } + fn add(&mut self, flag: &str) -> CargoResult<()> { + let mut parts = flag.splitn(2, '='); + let k = parts.next().unwrap(); + let v = parts.next(); - fn add(&mut self, k: &str, v: Option<&str>) -> CargoResult<()> { fn parse_bool(key: &str, value: Option<&str>) -> CargoResult { match value { None | Some("yes") => Ok(true), @@ -430,9 +411,7 @@ impl CliUnstable { Ok(true) }; - // Permit dashes or underscores in parsing these - let normalized_key = k.replace("_", "-"); - match normalized_key.as_str() { + match k { "print-im-a-teapot" => self.print_im_a_teapot = parse_bool(k, v)?, "unstable-options" => self.unstable_options = parse_empty(k, v)?, "no-index-update" => self.no_index_update = parse_empty(k, v)?, diff --git a/tests/testsuite/config_cli.rs b/tests/testsuite/config_cli.rs index cd18a1b489c..e8a78e7c462 100644 --- a/tests/testsuite/config_cli.rs +++ b/tests/testsuite/config_cli.rs @@ -267,26 +267,6 @@ fn rerooted_remains() { assert_eq!(config.get::("c").unwrap(), "cli2"); } -#[cargo_test] -fn unstable_dash_underscore_interchangable() { - // Confirm unstable flag parsing treats underscores and dashes - // interchangably when coming from the CLI. - let config = ConfigBuilder::new() - .unstable_flag("print_im_a_teapot") - .build(); - assert_eq!(config.cli_unstable().print_im_a_teapot, true); - - let config = ConfigBuilder::new() - .unstable_flag("print-im-a-teapot") - .build(); - assert_eq!(config.cli_unstable().print_im_a_teapot, true); - - let config = ConfigBuilder::new() - .unstable_flag("print_im-a_teapot") - .build(); - assert_eq!(config.cli_unstable().print_im_a_teapot, true); -} - #[cargo_test] fn bad_parse() { // Fail to TOML parse. From bd938f077daf2cffd17ee3b136f53a430af40cc2 Mon Sep 17 00:00:00 2001 From: Alexander Berghage Date: Mon, 29 Jun 2020 20:13:50 -0600 Subject: [PATCH 07/17] Switch unstable config parsing to serde Tests are currently failing, looks like there's something in the Deserializer impl that's forcing field-missing errors even when the serde `default` annotation is applied. --- src/cargo/core/features.rs | 5 ++++- src/cargo/util/config/mod.rs | 19 ++++++---------- tests/testsuite/config.rs | 42 +++++++++++++++++++++++++++++------- 3 files changed, 45 insertions(+), 21 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 8ed33b2f615..2f7ea741adc 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -333,7 +333,10 @@ impl Features { /// and then test for your flag or your value and act accordingly. /// /// If you have any trouble with this, please let us know! -#[derive(Default, Debug)] +#[derive(Default, Debug, Deserialize)] +#[serde(default)] +#[serde(deny_unknown_fields)] +#[serde(rename_all="kebab-case")] pub struct CliUnstable { pub print_im_a_teapot: bool, pub unstable_options: bool, diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index bb314935218..156a272b977 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -63,7 +63,7 @@ use std::str::FromStr; use std::sync::Once; use std::time::Instant; -use anyhow::{anyhow, bail, Context}; +use anyhow::{anyhow, bail}; use curl::easy::Easy; use lazycell::LazyCell; use serde::Deserialize; @@ -745,18 +745,13 @@ impl Config { // If nightly features are enabled, allow setting Z-flags from config // using the `unstable` table. Ignore that block otherwise. if nightly_features_allowed() { - if let Some(unstable_configs) = - self.get::>>("unstable")? - { - self.unstable_flags - .update_with_table(&unstable_configs) - .with_context(|| "Invalid [unstable] entry in Cargo config")?; - - // NB. It sucks to parse these twice, but doing it again here - // allows the CLI to override config files for both enabling - // and disabling. - self.unstable_flags.parse(unstable_flags)?; + if let Some(unstable_flags) = self.get::>("unstable")? { + self.unstable_flags = unstable_flags; } + // NB. It sucks to parse these twice, but doing it again here + // allows the CLI to override config files for both enabling + // and disabling. + self.unstable_flags.parse(unstable_flags)?; } Ok(()) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 7b3384aa67b..acb46730b99 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1097,12 +1097,12 @@ Caused by: #[cargo_test] /// Assert that unstable options can be configured with the `unstable` table in /// cargo config files -fn config_unstable_table() { +fn unstable_table_notation() { cargo::core::enable_nightly_features(); write_config( "\ [unstable] -print-im-a-teapot = 'yes' +print-im-a-teapot = true ", ); let config = ConfigBuilder::new().build(); @@ -1111,11 +1111,11 @@ print-im-a-teapot = 'yes' #[cargo_test] /// Assert that dotted notation works for configuring unstable options -fn config_unstable_dotted() { +fn unstable_dotted_notation() { cargo::core::enable_nightly_features(); write_config( "\ -unstable.print-im-a-teapot = 'yes' +unstable.print-im-a-teapot = true ", ); let config = ConfigBuilder::new().build(); @@ -1124,11 +1124,11 @@ unstable.print-im-a-teapot = 'yes' #[cargo_test] /// Assert that Zflags on the CLI take precedence over those from config -fn config_unstable_cli_wins() { +fn unstable_cli_precedence() { cargo::core::enable_nightly_features(); write_config( "\ -unstable.print-im-a-teapot = 'yes' +unstable.print-im-a-teapot = true ", ); let config = ConfigBuilder::new().build(); @@ -1142,8 +1142,8 @@ unstable.print-im-a-teapot = 'yes' #[cargo_test] /// Assert that atempting to set an unstable flag that doesn't exist via config -/// errors out the same as it would on the command line -fn config_unstable_invalid_flag() { +/// errors out the same as it would on the command line (nightly only) +fn unstable_invalid_flag_errors_on_nightly() { cargo::core::enable_nightly_features(); write_config( "\ @@ -1160,6 +1160,32 @@ Caused by: ); } +#[cargo_test] +/// Assert that atempting to set an unstable flag that doesn't exist via config +/// is ignored on stable +fn unstable_invalid_flag_ignored_on_stable() { + write_config( + "\ +unstable.an-invalid-flag = 'yes' +", + ); + assert!(ConfigBuilder::new().build_err().is_ok()); +} + +#[cargo_test] +/// Assert that unstable options can be configured with the `unstable` table in +/// cargo config files +fn unstable_flags_ignored_on_stable() { + write_config( + "\ +[unstable] +print-im-a-teapot = true +", + ); + let config = ConfigBuilder::new().build(); + assert_eq!(config.cli_unstable().print_im_a_teapot, false); +} + #[cargo_test] fn table_merge_failure() { // Config::merge fails to merge entries in two tables. From 14c32d4911b13d23a128222b1fda4e13b4a55a9c Mon Sep 17 00:00:00 2001 From: Alexander Berghage Date: Thu, 2 Jul 2020 18:39:01 -0600 Subject: [PATCH 08/17] [fixup] use single-line serde annotations --- src/cargo/core/features.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 2f7ea741adc..37878d4b77d 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -334,9 +334,7 @@ impl Features { /// /// If you have any trouble with this, please let us know! #[derive(Default, Debug, Deserialize)] -#[serde(default)] -#[serde(deny_unknown_fields)] -#[serde(rename_all="kebab-case")] +#[serde(default, deny_unknown_fields, rename_all="kebab-case")] pub struct CliUnstable { pub print_im_a_teapot: bool, pub unstable_options: bool, From c532c4964e1b0857b4928850a9bcc833a640af77 Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Tue, 7 Jul 2020 23:34:04 -0600 Subject: [PATCH 09/17] Treat all CliUnstable values as default-false This is a workaround in case y'all would like to land this change before I'm able to finish up a Deserializer refactor. --- src/cargo/core/features.rs | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 37878d4b77d..635063cd500 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -334,32 +334,68 @@ impl Features { /// /// If you have any trouble with this, please let us know! #[derive(Default, Debug, Deserialize)] -#[serde(default, deny_unknown_fields, rename_all="kebab-case")] +#[serde(rename_all="kebab-case")] pub struct CliUnstable { + #[serde(deserialize_with="default_false")] pub print_im_a_teapot: bool, + #[serde(deserialize_with="default_false")] pub unstable_options: bool, + #[serde(deserialize_with="default_false")] pub no_index_update: bool, + #[serde(deserialize_with="default_false")] pub avoid_dev_deps: bool, + #[serde(deserialize_with="default_false")] pub minimal_versions: bool, + #[serde(deserialize_with="default_false")] pub package_features: bool, + #[serde(deserialize_with="default_false")] pub advanced_env: bool, + #[serde(deserialize_with="default_false")] pub config_include: bool, + #[serde(deserialize_with="default_false")] pub dual_proc_macros: bool, + #[serde(deserialize_with="default_false")] pub mtime_on_use: bool, + #[serde(deserialize_with="default_false")] pub named_profiles: bool, + #[serde(deserialize_with="default_false")] pub binary_dep_depinfo: bool, pub build_std: Option>, pub timings: Option>, + #[serde(deserialize_with="default_false")] pub doctest_xcompile: bool, + #[serde(deserialize_with="default_false")] pub panic_abort_tests: bool, + #[serde(deserialize_with="default_false")] pub jobserver_per_rustc: bool, pub features: Option>, + #[serde(deserialize_with="default_false")] pub crate_versions: bool, + #[serde(deserialize_with="default_false")] pub separate_nightlies: bool, + #[serde(deserialize_with="default_false")] pub multitarget: bool, + #[serde(deserialize_with="default_false")] pub rustdoc_map: bool, } +/// Treat boolean Zflags as optionals for deserializing them. +/// This allows missing settings to default to disabled (effectively recreating +/// the serde `default` behavior). Our Deserializer merges multiple sources +/// inline, so the serde facilities for handling missing or additional fields +/// don't quite fit. +/// +/// TODO: This limitation can likely be removed by refactoring +/// `de::ConfigMapAccess` to iterate the union of the config and env sets. +fn default_false<'de, D, T>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, + T: Deserialize<'de> + Default, +{ + let option = Option::deserialize(deserializer)?; + Ok(option.unwrap_or_default()) +} + impl CliUnstable { pub fn parse(&mut self, flags: &[String]) -> CargoResult<()> { if !flags.is_empty() && !nightly_features_allowed() { From 08f1020f4811939949adb8db5892a16ec68019e6 Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Tue, 7 Jul 2020 23:35:22 -0600 Subject: [PATCH 10/17] Drop broken test asserting invalid configs error on nightly The current behavior, with the default-false workaround in place, is not able to identify extra members of the `unstable` table, so it can't error on unexpected members. I'll add this test back in with a Deserializer refactor to clean up the extra logic added to CliUnstable. --- tests/testsuite/config.rs | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index acb46730b99..02490376450 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1140,26 +1140,6 @@ unstable.print-im-a-teapot = true assert_eq!(config.cli_unstable().print_im_a_teapot, false); } -#[cargo_test] -/// Assert that atempting to set an unstable flag that doesn't exist via config -/// errors out the same as it would on the command line (nightly only) -fn unstable_invalid_flag_errors_on_nightly() { - cargo::core::enable_nightly_features(); - write_config( - "\ -unstable.an-invalid-flag = 'yes' -", - ); - assert_error( - ConfigBuilder::new().build_err().unwrap_err(), - "\ -Invalid [unstable] entry in Cargo config - -Caused by: - unknown `-Z` flag specified: an-invalid-flag", - ); -} - #[cargo_test] /// Assert that atempting to set an unstable flag that doesn't exist via config /// is ignored on stable From 62d62333c00832924aecd38ba003486c73c3de95 Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Wed, 8 Jul 2020 21:14:47 -0600 Subject: [PATCH 11/17] Revert workaround and patch Deserializer This patch changes how ConfigMapAccess iterates k/v pairs when deserializing structs. Previously we produced keys for exactly the set of fields needed for a struct, and errored out in the deserializer if we can't find anything for that field. This patch makes us produces keys from the union of two sets: 1. All fields that are both needed for the struct and can be found in the environment. 2. All fields in the config table. This change allows serde's codegen to handle both missing and unknown fields via the usual derive annotations (default or deny_unknown_fields respectively) --- src/cargo/core/features.rs | 38 +--------------------------- src/cargo/util/config/de.rs | 50 +++++++++++++++++++++++++------------ src/cargo/util/toml/mod.rs | 2 +- 3 files changed, 36 insertions(+), 54 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 635063cd500..4d2d94cb6b5 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -334,68 +334,32 @@ impl Features { /// /// If you have any trouble with this, please let us know! #[derive(Default, Debug, Deserialize)] -#[serde(rename_all="kebab-case")] +#[serde(default, rename_all = "kebab-case")] pub struct CliUnstable { - #[serde(deserialize_with="default_false")] pub print_im_a_teapot: bool, - #[serde(deserialize_with="default_false")] pub unstable_options: bool, - #[serde(deserialize_with="default_false")] pub no_index_update: bool, - #[serde(deserialize_with="default_false")] pub avoid_dev_deps: bool, - #[serde(deserialize_with="default_false")] pub minimal_versions: bool, - #[serde(deserialize_with="default_false")] pub package_features: bool, - #[serde(deserialize_with="default_false")] pub advanced_env: bool, - #[serde(deserialize_with="default_false")] pub config_include: bool, - #[serde(deserialize_with="default_false")] pub dual_proc_macros: bool, - #[serde(deserialize_with="default_false")] pub mtime_on_use: bool, - #[serde(deserialize_with="default_false")] pub named_profiles: bool, - #[serde(deserialize_with="default_false")] pub binary_dep_depinfo: bool, pub build_std: Option>, pub timings: Option>, - #[serde(deserialize_with="default_false")] pub doctest_xcompile: bool, - #[serde(deserialize_with="default_false")] pub panic_abort_tests: bool, - #[serde(deserialize_with="default_false")] pub jobserver_per_rustc: bool, pub features: Option>, - #[serde(deserialize_with="default_false")] pub crate_versions: bool, - #[serde(deserialize_with="default_false")] pub separate_nightlies: bool, - #[serde(deserialize_with="default_false")] pub multitarget: bool, - #[serde(deserialize_with="default_false")] pub rustdoc_map: bool, } -/// Treat boolean Zflags as optionals for deserializing them. -/// This allows missing settings to default to disabled (effectively recreating -/// the serde `default` behavior). Our Deserializer merges multiple sources -/// inline, so the serde facilities for handling missing or additional fields -/// don't quite fit. -/// -/// TODO: This limitation can likely be removed by refactoring -/// `de::ConfigMapAccess` to iterate the union of the config and env sets. -fn default_false<'de, D, T>(deserializer: D) -> Result -where - D: serde::Deserializer<'de>, - T: Deserialize<'de> + Default, -{ - let option = Option::deserialize(deserializer)?; - Ok(option.unwrap_or_default()) -} - impl CliUnstable { pub fn parse(&mut self, flags: &[String]) -> CargoResult<()> { if !flags.is_empty() && !nightly_features_allowed() { diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs index 1564a60f07d..311ba411620 100644 --- a/src/cargo/util/config/de.rs +++ b/src/cargo/util/config/de.rs @@ -4,6 +4,7 @@ use crate::util::config::value; use crate::util::config::{Config, ConfigError, ConfigKey}; use crate::util::config::{ConfigValue as CV, Definition, Value}; use serde::{de, de::IntoDeserializer}; +use std::collections::HashSet; use std::vec; /// Serde deserializer used to convert config values to a target type using @@ -269,37 +270,54 @@ impl<'config> ConfigMapAccess<'config> { fn new_struct( de: Deserializer<'config>, - fields: &'static [&'static str], + given_fields: &'static [&'static str], ) -> Result, ConfigError> { - let fields: Vec = fields - .iter() - .map(|field| KeyKind::Normal(field.to_string())) - .collect(); + let table = de.config.get_table(&de.key)?; // Assume that if we're deserializing a struct it exhaustively lists all // possible fields on this key that we're *supposed* to use, so take // this opportunity to warn about any keys that aren't recognized as // fields and warn about them. - if let Some(mut v) = de.config.get_table(&de.key)? { - for (t_key, value) in v.val.drain() { - if fields.iter().any(|k| match k { - KeyKind::Normal(s) => s == &t_key, - KeyKind::CaseSensitive(s) => s == &t_key, - }) { - continue; - } + if let Some(v) = table.as_ref() { + let unused_keys = v + .val + .iter() + .filter(|(k, _v)| !given_fields.iter().any(|gk| gk == k)); + for (unused_key, unused_value) in unused_keys { de.config.shell().warn(format!( "unused config key `{}.{}` in `{}`", de.key, - t_key, - value.definition() + unused_key, + unused_value.definition() ))?; } } + let mut fields = HashSet::new(); + + // If the caller is interested in a field which we can provide from + // the environment, get it from there. + for field in given_fields { + let mut field_key = de.key.clone(); + field_key.push(field); + for env_key in de.config.env.keys() { + if env_key.starts_with(field_key.as_env_key()) { + fields.insert(KeyKind::Normal(field.to_string())); + } + } + } + + // Add everything from the config table we're interested in that we + // haven't already provided via an environment variable + if let Some(v) = table { + for key in v.val.keys() { + fields.insert(KeyKind::Normal(key.clone())); + } + } + Ok(ConfigMapAccess { de, - fields, + fields: fields.into_iter().collect(), field_index: 0, }) } diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 6dc4a409801..4bb1d7f4d2b 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -394,7 +394,7 @@ impl<'de> de::Deserialize<'de> for U32OrBool { } #[derive(Deserialize, Serialize, Clone, Debug, Default, Eq, PartialEq)] -#[serde(rename_all = "kebab-case")] +#[serde(default, rename_all = "kebab-case")] pub struct TomlProfile { pub opt_level: Option, pub lto: Option, From 724273ed259381f95b10734d5c36667e43a2946a Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Wed, 8 Jul 2020 21:22:44 -0600 Subject: [PATCH 12/17] Minor test fix (error text comparison) Serde's errors for missing fields are a lil' different than the ones from our Deserializer. --- tests/testsuite/config.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 02490376450..ad3edbbc2b1 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -687,10 +687,7 @@ Caused by: f3: i64, big: i64, } - assert_error( - config.get::("S").unwrap_err(), - "missing config key `S.f3`", - ); + assert_error(config.get::("S").unwrap_err(), "missing field `f3`"); } #[cargo_test] From 782b32bdfac5cc49d8b18c7cb533d5af4e76b789 Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Wed, 8 Jul 2020 21:33:35 -0600 Subject: [PATCH 13/17] clearer comment on doubleparse of cli flags --- src/cargo/util/config/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 156a272b977..af356ce27ba 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -748,9 +748,10 @@ impl Config { if let Some(unstable_flags) = self.get::>("unstable")? { self.unstable_flags = unstable_flags; } - // NB. It sucks to parse these twice, but doing it again here + // NB. It's not ideal to parse these twice, but doing it again here // allows the CLI to override config files for both enabling - // and disabling. + // and disabling, and doing it up top allows CLI Zflags to + // control config parsing behavior. self.unstable_flags.parse(unstable_flags)?; } From 90b9c6a7631a6828a76dfc6468b2687af07a348c Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Wed, 8 Jul 2020 22:05:52 -0600 Subject: [PATCH 14/17] fix nightly rustdoc-extern tests --- src/cargo/core/compiler/rustdoc.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/compiler/rustdoc.rs b/src/cargo/core/compiler/rustdoc.rs index d32f2d6912f..37e70fde0b4 100644 --- a/src/cargo/core/compiler/rustdoc.rs +++ b/src/cargo/core/compiler/rustdoc.rs @@ -52,7 +52,8 @@ impl<'de> serde::de::Deserialize<'de> for RustdocExternMode { } } -#[derive(serde::Deserialize, Debug)] +#[derive(serde::Deserialize, Debug, Default)] +#[serde(default)] pub struct RustdocExternMap { registries: HashMap, std: Option, From b82281ad944c6308f125ae1e437ab1a993d8d5dd Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Wed, 8 Jul 2020 22:11:05 -0600 Subject: [PATCH 15/17] fix docs to drop reverted change --- src/doc/src/reference/unstable.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 2ce8f2274e0..5d07a8eba1a 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -2,8 +2,7 @@ Experimental Cargo features are only available on the nightly channel. You typically use one of the `-Z` flags to enable them. Run `cargo -Z help` to -see a list of flags available. All Z flags accept either dashes or underscores -as separators. +see a list of flags available. `-Z unstable-options` is a generic flag for enabling other unstable command-line flags. Options requiring this will be called out below. From e49b3aed792619368f90fe6b0dabe64aecc0774f Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Thu, 9 Jul 2020 12:53:42 -0600 Subject: [PATCH 16/17] Add tests verifying overlapping prefixes and defaults These tests demonstrate the current failure mode around overlapping env keys and inner structs. To some extent this is a limitation of mapping on to environment variables with an underscore as both the namespace separator and the substitute for dashes in flag names in that the mapping is not strictly one-to-one. --- src/cargo/util/config/de.rs | 13 ++++- tests/testsuite/config.rs | 96 +++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs index 311ba411620..c343b483e47 100644 --- a/src/cargo/util/config/de.rs +++ b/src/cargo/util/config/de.rs @@ -298,11 +298,20 @@ impl<'config> ConfigMapAccess<'config> { // If the caller is interested in a field which we can provide from // the environment, get it from there. for field in given_fields { + let is_prefix = given_fields + .iter() + .any(|f| f != field && f.starts_with(field)); let mut field_key = de.key.clone(); field_key.push(field); for env_key in de.config.env.keys() { - if env_key.starts_with(field_key.as_env_key()) { - fields.insert(KeyKind::Normal(field.to_string())); + if is_prefix { + if env_key == field_key.as_env_key() { + fields.insert(KeyKind::Normal(field.to_string())); + } + } else { + if env_key.starts_with(field_key.as_env_key()) { + fields.insert(KeyKind::Normal(field.to_string())); + } } } } diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index ad3edbbc2b1..a9d6aa3574a 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1246,6 +1246,27 @@ fn struct_with_opt_inner_struct() { assert_eq!(f.inner.unwrap().value.unwrap(), 12); } +#[cargo_test] +fn struct_with_default_inner_struct() { + // Struct with serde defaults. + // Check that can be defined with environment variable. + #[derive(Deserialize, Default)] + #[serde(default)] + struct Inner { + value: i32, + } + #[derive(Deserialize, Default)] + #[serde(default)] + struct Foo { + inner: Inner, + } + let config = ConfigBuilder::new() + .env("CARGO_FOO_INNER_VALUE", "12") + .build(); + let f: Foo = config.get("foo").unwrap(); + assert_eq!(f.inner.value, 12); +} + #[cargo_test] fn overlapping_env_config() { // Issue where one key is a prefix of another. @@ -1277,6 +1298,81 @@ fn overlapping_env_config() { assert_eq!(s.debug, Some(1)); } +#[cargo_test] +fn overlapping_env_with_defaults() { + // Issue where one key is a prefix of another. + #[derive(Deserialize, Default)] + #[serde(default, rename_all = "kebab-case")] + struct Ambig { + debug: u32, + debug_assertions: bool, + } + let config = ConfigBuilder::new() + .env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true") + .build(); + + let s: Ambig = config.get("ambig").unwrap(); + assert_eq!(s.debug_assertions, true); + assert_eq!(s.debug, u32::default()); + + let config = ConfigBuilder::new().env("CARGO_AMBIG_DEBUG", "5").build(); + let s: Ambig = config.get("ambig").unwrap(); + assert_eq!(s.debug_assertions, bool::default()); + assert_eq!(s.debug, 5); + + let config = ConfigBuilder::new() + .env("CARGO_AMBIG_DEBUG", "1") + .env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true") + .build(); + let s: Ambig = config.get("ambig").unwrap(); + assert_eq!(s.debug_assertions, true); + assert_eq!(s.debug, 1); +} + +#[cargo_test] +fn struct_with_overlapping_inner_struct_and_defaults() { + // Struct with serde defaults. + // Check that can be defined with environment variable. + #[derive(Deserialize, Default)] + #[serde(default)] + struct Inner { + value: i32, + } + + // Containing struct with a prefix of inner + // Check that the nested struct can have fields defined by env + #[derive(Deserialize, Default)] + #[serde(default)] + struct PrefixContainer { + inn: bool, + inner: Inner, + } + let config = ConfigBuilder::new() + .env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12") + .build(); + let f: PrefixContainer = config.get("prefixcontainer").unwrap(); + assert_eq!(f.inn, bool::default()); + assert_eq!(f.inner.value, 12); + + // Containing struct where the inner value's field is a prefix of another + // Check that the nested struct can have fields defined by env + #[derive(Deserialize, Default)] + #[serde(default)] + struct InversePrefixContainer { + inner_field: bool, + inner: Inner, + } + let config = ConfigBuilder::new() + .env("CARGO_INVERSEREFIXCONTAINER_INNER_VALUE", "12") + .build(); + let f: InversePrefixContainer = config.get("inverseprefixcontainer").unwrap(); + assert_eq!(f.inner_field, bool::default()); + // NB. This is a limitation of our env variable parsing. We can't currently + // handle situations where just a value of the inner struct is set, but + // it's also named as a prefix of another field on the outer struct. + // assert_eq!(f.inner.value, 12); +} + #[cargo_test] fn string_list_tricky_env() { // Make sure StringList handles typed env values. From d035daae4e26bfe144ceff41f4c9a0bb11fea605 Mon Sep 17 00:00:00 2001 From: Alex Berghage Date: Thu, 9 Jul 2020 13:12:34 -0600 Subject: [PATCH 17/17] Fix tests for handling env key overlaps / errors --- src/cargo/util/config/de.rs | 13 ++--------- tests/testsuite/config.rs | 45 ++++++++++++++++++++++++++----------- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs index c343b483e47..311ba411620 100644 --- a/src/cargo/util/config/de.rs +++ b/src/cargo/util/config/de.rs @@ -298,20 +298,11 @@ impl<'config> ConfigMapAccess<'config> { // If the caller is interested in a field which we can provide from // the environment, get it from there. for field in given_fields { - let is_prefix = given_fields - .iter() - .any(|f| f != field && f.starts_with(field)); let mut field_key = de.key.clone(); field_key.push(field); for env_key in de.config.env.keys() { - if is_prefix { - if env_key == field_key.as_env_key() { - fields.insert(KeyKind::Normal(field.to_string())); - } - } else { - if env_key.starts_with(field_key.as_env_key()) { - fields.insert(KeyKind::Normal(field.to_string())); - } + if env_key.starts_with(field_key.as_env_key()) { + fields.insert(KeyKind::Normal(field.to_string())); } } } diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index a9d6aa3574a..406c63fcf27 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1299,8 +1299,13 @@ fn overlapping_env_config() { } #[cargo_test] -fn overlapping_env_with_defaults() { +fn overlapping_env_with_defaults_errors_out() { // Issue where one key is a prefix of another. + // This is a limitation of mapping environment variables on to a hierarchy. + // Check that we error out when we hit ambiguity in this way, rather than + // the more-surprising defaulting through. + // If, in the future, we can handle this more correctly, feel free to delete + // this test. #[derive(Deserialize, Default)] #[serde(default, rename_all = "kebab-case")] struct Ambig { @@ -1310,10 +1315,8 @@ fn overlapping_env_with_defaults() { let config = ConfigBuilder::new() .env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true") .build(); - - let s: Ambig = config.get("ambig").unwrap(); - assert_eq!(s.debug_assertions, true); - assert_eq!(s.debug, u32::default()); + let err = config.get::("ambig").err().unwrap(); + assert!(format!("{}", err).contains("missing config key `ambig.debug`")); let config = ConfigBuilder::new().env("CARGO_AMBIG_DEBUG", "5").build(); let s: Ambig = config.get("ambig").unwrap(); @@ -1340,7 +1343,12 @@ fn struct_with_overlapping_inner_struct_and_defaults() { } // Containing struct with a prefix of inner - // Check that the nested struct can have fields defined by env + // + // This is a limitation of mapping environment variables on to a hierarchy. + // Check that we error out when we hit ambiguity in this way, rather than + // the more-surprising defaulting through. + // If, in the future, we can handle this more correctly, feel free to delete + // this case. #[derive(Deserialize, Default)] #[serde(default)] struct PrefixContainer { @@ -1350,12 +1358,26 @@ fn struct_with_overlapping_inner_struct_and_defaults() { let config = ConfigBuilder::new() .env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12") .build(); + let err = config + .get::("prefixcontainer") + .err() + .unwrap(); + assert!(format!("{}", err).contains("missing config key `prefixcontainer.inn`")); + let config = ConfigBuilder::new() + .env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12") + .env("CARGO_PREFIXCONTAINER_INN", "true") + .build(); let f: PrefixContainer = config.get("prefixcontainer").unwrap(); - assert_eq!(f.inn, bool::default()); assert_eq!(f.inner.value, 12); + assert_eq!(f.inn, true); // Containing struct where the inner value's field is a prefix of another - // Check that the nested struct can have fields defined by env + // + // This is a limitation of mapping environment variables on to a hierarchy. + // Check that we error out when we hit ambiguity in this way, rather than + // the more-surprising defaulting through. + // If, in the future, we can handle this more correctly, feel free to delete + // this case. #[derive(Deserialize, Default)] #[serde(default)] struct InversePrefixContainer { @@ -1363,14 +1385,11 @@ fn struct_with_overlapping_inner_struct_and_defaults() { inner: Inner, } let config = ConfigBuilder::new() - .env("CARGO_INVERSEREFIXCONTAINER_INNER_VALUE", "12") + .env("CARGO_INVERSEPREFIXCONTAINER_INNER_VALUE", "12") .build(); let f: InversePrefixContainer = config.get("inverseprefixcontainer").unwrap(); assert_eq!(f.inner_field, bool::default()); - // NB. This is a limitation of our env variable parsing. We can't currently - // handle situations where just a value of the inner struct is set, but - // it's also named as a prefix of another field on the outer struct. - // assert_eq!(f.inner.value, 12); + assert_eq!(f.inner.value, 12); } #[cargo_test]