From 76421f779a5c86310ca22c68bdde7d6958537c7b Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Fri, 5 Apr 2024 18:06:21 +0800 Subject: [PATCH] feat: Support parse string as git/gitoxide features from ENV and Config - pass 'all' or true to enable all pre-definded git/gitoxide features - support parse git/gitoxide as table in Config, if the field is tagged with #[serde(default)], then it can be skipped --- src/cargo/core/features.rs | 140 +++++++++++++++++- src/cargo/util/context/de.rs | 14 +- tests/testsuite/config.rs | 277 ++++++++++++++++++++++++++++++++--- 3 files changed, 400 insertions(+), 31 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 67caa9a71fe3..6b74f54cc53d 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -759,7 +759,9 @@ unstable_cli_options!( dual_proc_macros: bool = ("Build proc-macros for both the host and the target"), features: Option>, gc: bool = ("Track cache usage and \"garbage collect\" unused files"), + #[serde(deserialize_with = "deserialize_git_features")] git: Option = ("Enable support for shallow git fetch operations"), + #[serde(deserialize_with = "deserialize_gitoxide_features")] gitoxide: Option = ("Use gitoxide for the given git interactions, or all of them if no argument is given"), host_config: bool = ("Enable the `[host]` section in the .cargo/config.toml file"), minimal_versions: bool = ("Resolve minimal dependency versions instead of maximum"), @@ -866,7 +868,8 @@ where )) } -#[derive(Debug, Copy, Clone, Default, Deserialize)] +#[derive(Debug, Copy, Clone, Default, Deserialize, Ord, PartialOrd, Eq, PartialEq)] +#[serde(default)] pub struct GitFeatures { /// When cloning the index, perform a shallow clone. Maintain shallowness upon subsequent fetches. pub shallow_index: bool, @@ -875,12 +878,71 @@ pub struct GitFeatures { } impl GitFeatures { - fn all() -> Self { + pub fn all() -> Self { GitFeatures { shallow_index: true, shallow_deps: true, } } + + fn expecting() -> String { + let fields = vec!["'all'", "'shallow-index'", "'shallow-deps'"]; + format!( + "unstable 'git' only takes {} as valid inputs, your can use 'all' to turn out all git features", + fields.join(" and ") + ) + } +} + +fn deserialize_git_features<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::de::Deserializer<'de>, +{ + struct GitFeaturesVisitor; + + impl<'de> serde::de::Visitor<'de> for GitFeaturesVisitor { + type Value = Option; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(&GitFeatures::expecting()) + } + + fn visit_bool(self, v: bool) -> Result + where + E: serde::de::Error, + { + if v { + Ok(Some(GitFeatures::all())) + } else { + Ok(None) + } + } + + fn visit_str(self, s: &str) -> Result + where + E: serde::de::Error, + { + Ok(parse_git(s.split(",")).map_err(serde::de::Error::custom)?) + } + + fn visit_some(self, deserializer: D) -> Result + where + D: serde::de::Deserializer<'de>, + { + let git = GitFeatures::deserialize(deserializer)?; + Ok(Some(git)) + } + + fn visit_map(self, map: V) -> Result + where + V: serde::de::MapAccess<'de>, + { + let mvd = serde::de::value::MapAccessDeserializer::new(map); + Ok(Some(GitFeatures::deserialize(mvd)?)) + } + } + + deserializer.deserialize_any(GitFeaturesVisitor) } fn parse_git(it: impl Iterator>) -> CargoResult> { @@ -892,19 +954,19 @@ fn parse_git(it: impl Iterator>) -> CargoResult return Ok(Some(GitFeatures::all())), "shallow-index" => *shallow_index = true, "shallow-deps" => *shallow_deps = true, _ => { - bail!( - "unstable 'git' only takes 'shallow-index' and 'shallow-deps' as valid inputs" - ) + bail!(GitFeatures::expecting()) } } } Ok(Some(out)) } -#[derive(Debug, Copy, Clone, Default, Deserialize)] +#[derive(Debug, Copy, Clone, Default, Deserialize, Ord, PartialOrd, Eq, PartialEq)] +#[serde(default)] pub struct GitoxideFeatures { /// All fetches are done with `gitoxide`, which includes git dependencies as well as the crates index. pub fetch: bool, @@ -918,7 +980,7 @@ pub struct GitoxideFeatures { } impl GitoxideFeatures { - fn all() -> Self { + pub fn all() -> Self { GitoxideFeatures { fetch: true, checkout: true, @@ -935,6 +997,67 @@ impl GitoxideFeatures { internal_use_git2: false, } } + + fn expecting() -> String { + let fields = vec!["'all'", "'fetch'", "'checkout'", "'internal-use-git2'"]; + format!( + "unstable 'gitoxide' only takes {} as valid inputs, your can use 'all' to turn out all gitoxide features, for shallow fetches see `shallow-index,shallow-deps`", + fields.join(" and ") + ) + } +} + +fn deserialize_gitoxide_features<'de, D>( + deserializer: D, +) -> Result, D::Error> +where + D: serde::de::Deserializer<'de>, +{ + struct GitoxideFeaturesVisitor; + + impl<'de> serde::de::Visitor<'de> for GitoxideFeaturesVisitor { + type Value = Option; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(&GitoxideFeatures::expecting()) + } + + fn visit_str(self, s: &str) -> Result + where + E: serde::de::Error, + { + Ok(parse_gitoxide(s.split(",")).map_err(serde::de::Error::custom)?) + } + + fn visit_bool(self, v: bool) -> Result + where + E: serde::de::Error, + { + if v { + Ok(Some(GitoxideFeatures::all())) + } else { + Ok(None) + } + } + + fn visit_some(self, deserializer: D) -> Result + where + D: serde::de::Deserializer<'de>, + { + let gitoxide = GitoxideFeatures::deserialize(deserializer)?; + Ok(Some(gitoxide)) + } + + fn visit_map(self, map: V) -> Result + where + V: serde::de::MapAccess<'de>, + { + let mvd = serde::de::value::MapAccessDeserializer::new(map); + Ok(Some(GitoxideFeatures::deserialize(mvd)?)) + } + } + + deserializer.deserialize_any(GitoxideFeaturesVisitor) } fn parse_gitoxide( @@ -949,11 +1072,12 @@ fn parse_gitoxide( for e in it { match e.as_ref() { + "all" => return Ok(Some(GitoxideFeatures::all())), "fetch" => *fetch = true, "checkout" => *checkout = true, "internal-use-git2" => *internal_use_git2 = true, _ => { - bail!("unstable 'gitoxide' only takes `fetch` and 'checkout' as valid input, for shallow fetches see `-Zgit=shallow-index,shallow-deps`") + bail!(GitoxideFeatures::expecting()) } } } diff --git a/src/cargo/util/context/de.rs b/src/cargo/util/context/de.rs index 18a84823bed2..d5e308fcb656 100644 --- a/src/cargo/util/context/de.rs +++ b/src/cargo/util/context/de.rs @@ -62,6 +62,12 @@ impl<'de, 'gctx> de::Deserializer<'de> for Deserializer<'gctx> { let (res, def) = res; return res.map_err(|e| e.with_key_context(&self.key, def)); } + + // The effect here is the same as in [`deserialize_option`]. + if self.gctx.has_key(&self.key, self.env_prefix_ok)? { + return visitor.visit_some(self); + } + Err(ConfigError::missing(&self.key)) } @@ -265,8 +271,14 @@ impl<'gctx> ConfigMapAccess<'gctx> { let mut field_key = de.key.clone(); field_key.push(field); for env_key in de.gctx.env_keys() { - if env_key.starts_with(field_key.as_env_key()) { + if env_key == field_key.as_env_key() { fields.insert(KeyKind::Normal(field.to_string())); + } else { + let mut env_key_prefix = field_key.as_env_key().to_string(); + env_key_prefix.push('_'); + if env_key.starts_with(&env_key_prefix) { + fields.insert(KeyKind::Normal(field.to_string())); + } } } } diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index ee21aba9f9ff..da36bacc9b1f 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1,5 +1,6 @@ //! Tests for config settings. +use cargo::core::features::{GitFeatures, GitoxideFeatures}; use cargo::core::{PackageIdSpec, Shell}; use cargo::util::context::{ self, Definition, GlobalContext, JobsConfig, SslVersionConfig, StringList, @@ -1392,7 +1393,6 @@ fn struct_with_overlapping_inner_struct_and_defaults() { // If, in the future, we can handle this more correctly, feel free to delete // this case. #[derive(Deserialize, Default)] - #[serde(default)] struct PrefixContainer { inn: bool, inner: Inner, @@ -1404,7 +1404,7 @@ fn struct_with_overlapping_inner_struct_and_defaults() { .get::("prefixcontainer") .err() .unwrap(); - assert!(format!("{}", err).contains("missing config key `prefixcontainer.inn`")); + assert!(format!("{}", err).contains("missing field `inn`")); let gctx = GlobalContextBuilder::new() .env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12") .env("CARGO_PREFIXCONTAINER_INN", "true") @@ -1413,6 +1413,21 @@ fn struct_with_overlapping_inner_struct_and_defaults() { assert_eq!(f.inner.value, 12); assert_eq!(f.inn, true); + // Use default attribute of serde, then we can skip setting the inn field + #[derive(Deserialize, Default)] + #[serde(default)] + struct PrefixContainerFieldDefault { + inn: bool, + inner: Inner, + } + let gctx = GlobalContextBuilder::new() + .env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12") + .build(); + let f = gctx + .get::("prefixcontainer") + .unwrap(); + assert_eq!(f.inner.value, 12); + assert_eq!(f.inn, false); // Containing struct where the inner value's field is a prefix of another // // This is a limitation of mapping environment variables on to a hierarchy. @@ -1803,49 +1818,267 @@ fn trim_paths_parsing() { } #[cargo_test] -fn git_features_env() { +fn git_features_as_str() { + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GIT", "shallow-index") + .build(); + verify( + gctx, + Some(GitFeatures { + shallow_index: true, + ..GitFeatures::default() + }), + ); + + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GIT", "all,shallow-index") + .build(); + verify(gctx, Some(GitFeatures::all())); + + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GIT", "shallow-index,shallow-deps") + .build(); + verify(gctx, Some(GitFeatures::all())); + + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GIT", "shallow-index,abc") + .build(); + assert_error( + gctx.get::>("unstable") + .unwrap_err(), + "\ +error in environment variable `CARGO_UNSTABLE_GIT`: could not load config key `unstable.git` + +Caused by: + unstable 'git' only takes 'all' and 'shallow-index' and 'shallow-deps' as valid inputs, your can use 'all' to turn out all git features", + ); + + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GIT", "shallow-deps") + .build(); + verify( + gctx, + Some(GitFeatures { + shallow_index: false, + shallow_deps: true, + }), + ); + + write_config_toml( + "\ + [unstable] + git = 'all' + ", + ); + let gctx = GlobalContextBuilder::new().build(); + verify(gctx, Some(GitFeatures::all())); + + write_config_toml( + "\ + [unstable] + git = 'shallow-index' + ", + ); + let gctx = GlobalContextBuilder::new().build(); + verify( + gctx, + Some(GitFeatures { + shallow_index: true, + shallow_deps: false, + }), + ); + + fn verify(gctx: GlobalContext, expect: Option) { + let unstable_flags = gctx + .get::>("unstable") + .unwrap() + .unwrap(); + assert_eq!(unstable_flags.git, expect); + } +} + +#[cargo_test] +fn git_features_as_table() { let gctx = GlobalContextBuilder::new() .env("CARGO_UNSTABLE_GIT", "true") .build(); - verify(gctx); + verify(gctx, Some(GitFeatures::all())); + + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GIT_SHALLOW_INDEX", "true") + .build(); + verify( + gctx, + Some(GitFeatures { + shallow_index: true, + ..Default::default() + }), + ); + + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GIT_SHALLOW_INDEX", "true") + .env("CARGO_UNSTABLE_GIT_SHALLOW_DEPS", "true") + .build(); + verify( + gctx, + Some(GitFeatures { + shallow_index: true, + shallow_deps: true, + ..Default::default() + }), + ); write_config_toml( "\ [unstable.git] + shallow_deps = false + shallow_index = true ", ); let gctx = GlobalContextBuilder::new().build(); - verify(gctx); - - fn verify(gctx: GlobalContext) { - assert_error( - gctx.get::>("unstable") - .unwrap_err(), - "missing field `shallow_index`", - ); + verify( + gctx, + Some(GitFeatures { + shallow_index: true, + shallow_deps: false, + ..Default::default() + }), + ); + write_config_toml( + "\ + [unstable.git] + ", + ); + let gctx = GlobalContextBuilder::new().build(); + verify(gctx, Some(Default::default())); + + fn verify(gctx: GlobalContext, expect: Option) { + let unstable_flags = gctx + .get::>("unstable") + .unwrap() + .unwrap(); + assert_eq!(unstable_flags.git, expect); } } #[cargo_test] -fn gitoxide_features_env() { +fn gitoxide_features_as_str() { + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GITOXIDE", "fetch") + .build(); + verify( + gctx, + Some(GitoxideFeatures { + fetch: true, + ..GitoxideFeatures::default() + }), + ); + + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GITOXIDE", "all,fetch") + .build(); + verify(gctx, Some(GitoxideFeatures::all())); + + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GITOXIDE", "fetch,checkout") + .build(); + verify(gctx, Some(GitoxideFeatures::all())); + + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GITOXIDE", "fetch,abc") + .build(); + + assert_error( + gctx.get::>("unstable") + .unwrap_err(), + "\ +error in environment variable `CARGO_UNSTABLE_GITOXIDE`: could not load config key `unstable.gitoxide` + +Caused by: + unstable 'gitoxide' only takes 'all' and 'fetch' and 'checkout' and 'internal-use-git2' as valid inputs, your can use 'all' to turn out all gitoxide features, for shallow fetches see `shallow-index,shallow-deps`", + ); + + write_config_toml( + "\ + [unstable] + gitoxide = 'all' + ", + ); + let gctx = GlobalContextBuilder::new().build(); + verify(gctx, Some(GitoxideFeatures::all())); + + write_config_toml( + "\ + [unstable] + gitoxide = \"fetch\" + ", + ); + let gctx = GlobalContextBuilder::new().build(); + verify( + gctx, + Some(GitoxideFeatures { + fetch: true, + ..GitoxideFeatures::default() + }), + ); + + fn verify(gctx: GlobalContext, expect: Option) { + let unstable_flags = gctx + .get::>("unstable") + .unwrap() + .unwrap(); + assert_eq!(unstable_flags.gitoxide, expect); + } +} + +#[cargo_test] +fn gitoxide_features_as_table() { let gctx = GlobalContextBuilder::new() .env("CARGO_UNSTABLE_GITOXIDE", "true") .build(); - verify(gctx); + verify(gctx, Some(GitoxideFeatures::all())); + + let gctx = GlobalContextBuilder::new() + .env("CARGO_UNSTABLE_GITOXIDE_FETCH", "true") + .build(); + verify( + gctx, + Some(GitoxideFeatures { + fetch: true, + ..Default::default() + }), + ); write_config_toml( "\ [unstable.gitoxide] + fetch = true + checkout = false + internal_use_git2 = false ", ); let gctx = GlobalContextBuilder::new().build(); - verify(gctx); - - fn verify(gctx: GlobalContext) { - assert_error( - gctx.get::>("unstable") - .unwrap_err(), - "missing field `fetch`", - ); + verify( + gctx, + Some(GitoxideFeatures { + fetch: true, + checkout: false, + internal_use_git2: false, + }), + ); + write_config_toml( + "\ + [unstable.gitoxide] + ", + ); + let gctx = GlobalContextBuilder::new().build(); + verify(gctx, Some(Default::default())); + + fn verify(gctx: GlobalContext, expect: Option) { + let unstable_flags = gctx + .get::>("unstable") + .unwrap() + .unwrap(); + assert_eq!(unstable_flags.gitoxide, expect); } }