From dde75c72c4eb5cae946133c7484de1c4a69ec31a Mon Sep 17 00:00:00 2001 From: maciektr Date: Tue, 11 Jun 2024 16:42:41 +0200 Subject: [PATCH 1/2] Merge tools definition recursively --- scarb/src/core/manifest/toml_manifest.rs | 4 +- scarb/src/internal/serdex.rs | 157 ++++++++++++++++++++++- scarb/tests/profiles.rs | 68 +++++++++- 3 files changed, 222 insertions(+), 7 deletions(-) diff --git a/scarb/src/core/manifest/toml_manifest.rs b/scarb/src/core/manifest/toml_manifest.rs index 48f4e383b..e29f120d8 100644 --- a/scarb/src/core/manifest/toml_manifest.rs +++ b/scarb/src/core/manifest/toml_manifest.rs @@ -29,7 +29,7 @@ use crate::core::{ }; use crate::internal::fsx; use crate::internal::fsx::PathBufUtf8Ext; -use crate::internal::serdex::{toml_merge, RelativeUtf8PathBuf}; +use crate::internal::serdex::{toml_merge, toml_merge_apply_strategy, RelativeUtf8PathBuf}; use crate::internal::to_version::ToVersion; use crate::{ DEFAULT_MODULE_MAIN_FILE, DEFAULT_SOURCE_PATH, DEFAULT_TESTS_PATH, MANIFEST_FILE_NAME, @@ -876,7 +876,7 @@ impl TomlManifest { .transpose()? .map(|tool| { if let Some(profile_tool) = &profile_definition.tool { - toml_merge(&tool, profile_tool) + toml_merge_apply_strategy(&tool, profile_tool) } else { Ok(tool) } diff --git a/scarb/src/internal/serdex.rs b/scarb/src/internal/serdex.rs index 95b127bc1..75a8b89d3 100644 --- a/scarb/src/internal/serdex.rs +++ b/scarb/src/internal/serdex.rs @@ -1,11 +1,92 @@ -use anyhow::Result; +use anyhow::{anyhow, Result}; use camino::{Utf8Path, Utf8PathBuf}; use serde::{Deserialize, Serialize}; use crate::internal::fsx; -/// Merge two `toml::Value` serializable structs. -pub fn toml_merge<'de, T, S>(target: &T, source: &S) -> anyhow::Result +const MERGE_STRATEGY_KEY: &str = "merge-strategy"; + +#[derive(Debug, Default)] +pub enum MergeStrategy { + #[default] + Override, + Merge, +} + +impl TryFrom<&str> for MergeStrategy { + type Error = anyhow::Error; + fn try_from(value: &str) -> Result { + match value { + "override" => Ok(Self::Override), + "merge" => Ok(Self::Merge), + _ => Err(anyhow!( + "invalid merge strategy: {}, must be one of `merge`, `override`", + value + )), + } + } +} + +/// Merge `source` into `target` where `source` and `target` are two `toml::Value` serializable +/// structs. +/// If `source` and `target` are tables and a specific key exists in both, the conflict will be +/// resolved as follows: +/// 1) If value under the conflicting key in either `source` or `target` is not a table, the value +/// in `target` will be overridden with the value from `source`. +/// 2) If values under the conflicting key in both `source` and `target` are tables, +/// the conflict will be resolved with one of two strategies defined by `source`: +/// a) If `source` have a key `merge-strategy` with value `override`, the value in `target` will +/// be overridden with the value from `source`. +/// b) If `source` have a key `merge-strategy` with value `merge`, the value in `target` will be +/// merged with the value from `source` recursively with this function. +/// c) If `source` does not have a key `merge-strategy`, the value in `target` will be overridden. +pub fn toml_merge_apply_strategy<'de, T, S>(target: &T, source: &S) -> Result +where + T: Serialize + Deserialize<'de>, + S: Serialize + Deserialize<'de>, +{ + let mut target_value = toml::Value::try_from(target)?; + let source_value = toml::Value::try_from(source)?; + + if let (Some(target_table), Some(source_table)) = + (target_value.as_table_mut(), source_value.as_table()) + { + for (key, value) in source_table { + match target_table.get_mut(key) { + Some(target_subvalue) if target_subvalue.is_table() && value.is_table() => { + let target_subtable = target_subvalue.as_table_mut().unwrap(); + let value_subtable = value.as_table().unwrap(); + let strategy = value_subtable + .get(MERGE_STRATEGY_KEY) + .and_then(|v| v.as_str()) + .map_or(Ok(MergeStrategy::default()), MergeStrategy::try_from)?; + match &strategy { + MergeStrategy::Override => { + *target_subvalue = toml::Value::try_from(value_subtable.clone())?; + } + MergeStrategy::Merge => { + *target_subvalue = toml::Value::try_from(toml_merge_apply_strategy( + target_subtable, + value_subtable, + )?)?; + } + } + } + _ => { + target_table.insert(key.clone(), value.clone()); + } + } + } + } + + Ok(toml::Value::try_into(target_value)?) +} + +/// Merge `source` into `target` where `source` and `target` are two `toml::Value` serializable +/// structs. +/// If `source` and `target` are tables and a specific key exists in both, the value in `target` +/// will be overridden with the value from `source`. +pub fn toml_merge<'de, T, S>(target: &T, source: &S) -> Result where T: Serialize + Deserialize<'de>, S: Serialize + Deserialize<'de>, @@ -39,3 +120,73 @@ impl RelativeUtf8PathBuf { self.relative_to_directory(root) } } + +#[cfg(test)] +mod tests { + use super::{toml_merge, toml_merge_apply_strategy}; + use test_case::test_case; + + #[test_case(r#"{}"#, r#"{}"#)] + #[test_case(r#"{"a": "a"}"#, r#"{"b":"b"}"#)] + #[test_case(r#"{"a": "a"}"#, r#"{"a":"b"}"#)] + #[test_case(r#"{"a": {"a": "a"}}"#, r#"{"a": {"a": "b"}}"#)] + #[test_case( + r#"{"a": {"a": "a", "merge-strategy": "merge"}}"#, + r#"{"a": {"a": "b"}}"# + )] + #[test_case( + r#"{"a": {"a": "a", "merge-strategy": "override"}}"#, + r#"{"a": {"a": "b", "b": "b"}}"# + )] + fn merge_with_override(source: &'static str, target: &'static str) { + let source: toml::Value = serde_json::from_str(source).unwrap(); + let target: toml::Value = serde_json::from_str(target).unwrap(); + assert_eq!( + toml_merge(&target, &source).unwrap(), + toml_merge_apply_strategy(&target, &source).unwrap() + ); + } + + #[test] + fn merge_with_merge_strategy() { + let target = r#"{"a": {"a": "b", "b": "b"}}"#; + let source = r#"{"a": {"a": "a", "merge-strategy": "merge"}}"#; + let target: toml::Value = serde_json::from_str(target).unwrap(); + let source: toml::Value = serde_json::from_str(source).unwrap(); + let with_override = toml_merge(&target, &source).unwrap(); + let with_merge = toml_merge_apply_strategy(&target, &source).unwrap(); + let with_override = serde_json::to_string(&with_override).unwrap(); + let with_merge = serde_json::to_string(&with_merge).unwrap(); + assert_eq!(with_override, r#"{"a":{"a":"a","merge-strategy":"merge"}}"#); + assert_eq!( + with_merge, + r#"{"a":{"a":"a","b":"b","merge-strategy":"merge"}}"# + ); + } + + #[test_case( + r#"{"a": {"a": "b", "b": "b"}}"#, + r#"{"a": {"a": "a"}, "merge-strategy": "merge"}"# + )] + #[test_case( + r#"{"a": {"merge-strategy": "merge", "a": "b", "b": "b"}}"#, + r#"{"a": {"a": "a"}}"# + )] + fn merge_strategy_must_be_on_source(target: &'static str, source: &'static str) { + let source: toml::Value = serde_json::from_str(source).unwrap(); + let target: toml::Value = serde_json::from_str(target).unwrap(); + assert_eq!( + toml_merge(&target, &source).unwrap(), + toml_merge_apply_strategy(&target, &source).unwrap() + ); + } + + #[test] + fn invalid_merge_strategy() { + let target = r#"{"a": {"a": "b", "b": "b"}}"#; + let source = r#"{"a": {"a": "a", "merge-strategy": "other"}}"#; + let target: toml::Value = serde_json::from_str(target).unwrap(); + let source: toml::Value = serde_json::from_str(source).unwrap(); + assert!(toml_merge_apply_strategy(&target, &source).is_err()); + } +} diff --git a/scarb/tests/profiles.rs b/scarb/tests/profiles.rs index 4e8aed390..200f9b525 100644 --- a/scarb/tests/profiles.rs +++ b/scarb/tests/profiles.rs @@ -610,6 +610,9 @@ fn profile_overrides_tool() { [profile.release.tool.snforge] some-key = "some-other-value" + + [profile.newprof] + inherits = "release" "#}) .dep_cairo_test() .build(&t); @@ -641,11 +644,17 @@ fn profile_overrides_tool() { ); let metadata = Scarb::quick_snapbox() - .args(["--json", "--release", "metadata", "--format-version", "1"]) + .args([ + "--json", + "--profile=newprof", + "metadata", + "--format-version", + "1", + ]) .current_dir(&t) .stdout_json::(); - assert_eq!(metadata.current_profile, "release".to_string()); + assert_eq!(metadata.current_profile, "newprof".to_string()); assert_eq!(metadata.packages.len(), 3); let package = metadata .packages @@ -667,3 +676,58 @@ fn profile_overrides_tool() { "some-other-value" ); } + +#[test] +fn tools_can_be_merged_recursively() { + let t = TempDir::new().unwrap(); + ProjectBuilder::start() + .name("hello") + .manifest_extra(indoc! {r#" + [tool.something.a] + key = "value" + + [profile.release] + inherits = "dev" + + [profile.release.tool.something] + merge-strategy = "merge" + b = "some-value" + "#}) + .build(&t); + + let metadata = Scarb::quick_snapbox() + .args([ + "--json", + "--profile=release", + "metadata", + "--format-version", + "1", + ]) + .current_dir(&t) + .stdout_json::(); + let package = metadata + .packages + .iter() + .find(|pkg| pkg.name.starts_with("hello")) + .cloned() + .unwrap(); + assert_eq!( + package + .manifest_metadata + .tool + .unwrap() + .get("something") + .unwrap() + .as_object() + .unwrap() + .get("a") + .unwrap() + .as_object() + .unwrap() + .get("key") + .unwrap() + .as_str() + .unwrap(), + "value" + ); +} From 5d4c271a445c7bf8fd6ea3b9a6e2c3df9dc26c32 Mon Sep 17 00:00:00 2001 From: maciektr Date: Mon, 17 Jun 2024 10:36:37 +0200 Subject: [PATCH 2/2] Update docs --- scarb/src/internal/serdex.rs | 3 +- website/docs/reference/profiles.md | 115 +++++++++++++++++++++++++++-- 2 files changed, 109 insertions(+), 9 deletions(-) diff --git a/scarb/src/internal/serdex.rs b/scarb/src/internal/serdex.rs index 75a8b89d3..3b491dd0c 100644 --- a/scarb/src/internal/serdex.rs +++ b/scarb/src/internal/serdex.rs @@ -32,7 +32,8 @@ impl TryFrom<&str> for MergeStrategy { /// If `source` and `target` are tables and a specific key exists in both, the conflict will be /// resolved as follows: /// 1) If value under the conflicting key in either `source` or `target` is not a table, the value -/// in `target` will be overridden with the value from `source`. +/// in `target` will be overridden with the value from `source`. This means that no keys from +/// subtables under the conflicting key will be preserved. /// 2) If values under the conflicting key in both `source` and `target` are tables, /// the conflict will be resolved with one of two strategies defined by `source`: /// a) If `source` have a key `merge-strategy` with value `override`, the value in `target` will diff --git a/website/docs/reference/profiles.md b/website/docs/reference/profiles.md index e2564f2ff..0a105fff1 100644 --- a/website/docs/reference/profiles.md +++ b/website/docs/reference/profiles.md @@ -3,15 +3,15 @@ Profiles provide a way to alter the compiler settings. Scarb has 2 built-in profiles: `dev` and `release`. -The profile defaults to `dev` if a profile is not specified on the command-line. -In addition to the built-in profiles, custom user-defined profiles can also be specified. +The profile defaults to `dev` if not specified otherwise. +In addition to the built-in profiles, user can define custom profiles in the package manifest. +Profile can be specified through the command line arguments, or with an environment variable. -Profile settings can be changed in Scarb.toml with the `[profile]` table. -Profile settings defined in dependencies will be ignored. +Profile can be changed in `Scarb.toml` manifest within the `[profile]` section. +Profiles defined in dependencies will be ignored. -Currently, profiles define properties that affect the compiler settings, in a `[cairo]` table -(analogue to the [cairo](./manifest#cairo) section of the manifest definition) and custom tool metadata -(analogue to the [tool](./manifest#tool) section of the manifest definition). +Profile can alter the compiler settings (analog to manifest [`[cairo]`](./manifest#cairo) section) and custom tool +metadata (from [`[tool]`](./manifest#tool) section). ## Overriding built-in profile properties @@ -19,7 +19,8 @@ Each of the built-in profiles come with predefined default properties. The properties of a built-in profile can be overridden by specifying a new property value in a custom profile. -For example, the `dev` profile has the `sierra-replace-ids` property set to `true` by default. +For example, the `dev` profile has the `sierra-replace-ids` (see [`[cairo]`](./manifest#cairo)) property set to `true` +by default. This can be overridden by specifying the same property in a custom profile: ```toml @@ -28,6 +29,104 @@ This can be overridden by specifying the same property in a custom profile: sierra-replace-ids = true ``` +### Overriding tool metadata + +Tool metadata defined in the manifest (see [`[tool]`](./manifest#tool) can be overridden by a profile. + +For example: + +```toml +[tool.some-tool] +local = false +debug = false + +[profile.dev.tool.some-tool] +debug = true +``` + +Note, that by default the subsection defined in the profile replaces one from `tool` section completely. +The config from above would be translated to: + +```toml +[tool.some-tool] +debug = true +``` + +You can change the merge strategy with `merge-strategy` property. +For instance: + +```toml +[tool.some-tool] +local = false +debug = false + +[profile.dev.tool.some-tool] +merge-strategy = "merge" +debug = true +``` + +This would be translated to: + +```toml +[tool.some-tool] +merge-strategy = "merge" +local = false +debug = true +``` + +Your tool config may be more complex than simple key-value pairs, for instance it can contain sub-tables. + +```toml +[tool.some-tool] +top-level-key = "top-level-value" + +[tool.some-tool.environment] +local = false +debug = false + +[profile.dev.tool.some-tool.environment] +debug = true +``` + +In such case, same principles apply for merging sub-tables. +The config from above would be translated to: + +```toml +[tool.some-tool.environment] +debug = true +``` + +If you want to merge sub-tables, you can specify `merge-strategy` property like following snippet. + +```toml +[tool.some-tool] +top-level-key = "top-level-value" + +[tool.some-tool.environment] +local = false +debug = false + +[profile.dev.tool.some-tool] +merge-strategy = "merge" + +[profile.dev.tool.some-tool.environment] +merge-strategy = "merge" +debug = true +``` + +This would be translated to: + +```toml +[tool.some-tool] +merge-strategy = "merge" +top-level-key = "top-level-value" + +[tool.some-tool.environment] +merge-strategy = "merge" +local = false +debug = true +``` + ## Defining custom profiles Custom profiles can be defined in Scarb.toml with the `[profile]` table.