From 03e442fb3d64adf145e8698fb52a74ee65150560 Mon Sep 17 00:00:00 2001 From: danjl1100 Date: Sun, 1 Oct 2023 15:54:37 -0500 Subject: [PATCH] Replace `toml` dependency with `toml_edit` to preserve comments (#407) --------- Co-authored-by: Ivan Petkov --- CHANGELOG.md | 5 + pkgs/crane-utils/Cargo.lock | 33 +- pkgs/crane-utils/Cargo.toml | 2 +- .../crane-resolve-workspace-inheritance.rs | 327 ++++++++++++------ 4 files changed, 228 insertions(+), 139 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39b5622..4a76c70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## Unreleased +### Fixed +* Fixed handling of Cargo workspace inheritance for git-dependencies where said + crate relies on reading non-TOML metadata (i.e. comments) from its Cargo.toml + at build time. ([#407](https://github.com/ipetkov/crane/pull/407)) + ## [0.14.1] - 2023-09-23 ### Fixed diff --git a/pkgs/crane-utils/Cargo.lock b/pkgs/crane-utils/Cargo.lock index 9e49e74..7542da9 100644 --- a/pkgs/crane-utils/Cargo.lock +++ b/pkgs/crane-utils/Cargo.lock @@ -16,7 +16,7 @@ dependencies = [ "pretty_assertions", "serde", "serde_json", - "toml", + "toml_edit", ] [[package]] @@ -124,15 +124,6 @@ dependencies = [ "serde", ] -[[package]] -name = "serde_spanned" -version = "0.6.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "96426c9936fd7a0124915f9185ea1d20aa9445cc9821142f0a73bc9207a2e186" -dependencies = [ - "serde", -] - [[package]] name = "syn" version = "2.0.28" @@ -144,37 +135,19 @@ dependencies = [ "unicode-ident", ] -[[package]] -name = "toml" -version = "0.7.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c17e963a819c331dcacd7ab957d80bc2b9a9c1e71c804826d2f283dd65306542" -dependencies = [ - "indexmap", - "serde", - "serde_spanned", - "toml_datetime", - "toml_edit", -] - [[package]] name = "toml_datetime" version = "0.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7cda73e2f1397b1262d6dfdcef8aafae14d1de7748d66822d3bfeeb6d03e5e4b" -dependencies = [ - "serde", -] [[package]] name = "toml_edit" -version = "0.19.14" +version = "0.20.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f8123f27e969974a3dfba720fdb560be359f57b44302d280ba72e76a74480e8a" +checksum = "ca676d9ba1a322c1b64eb8045a5ec5c0cfb0c9d08e15e9ff622589ad5221c8fe" dependencies = [ "indexmap", - "serde", - "serde_spanned", "toml_datetime", "winnow", ] diff --git a/pkgs/crane-utils/Cargo.toml b/pkgs/crane-utils/Cargo.toml index 4279e36..59ac890 100644 --- a/pkgs/crane-utils/Cargo.toml +++ b/pkgs/crane-utils/Cargo.toml @@ -8,7 +8,7 @@ publish = false anyhow = "1" serde_json = "1" serde = { version = "1", features = ["derive"] } -toml = { version = "0.7.3", features = ["preserve_order"] } +toml_edit = "0.20.1" [dev-dependencies] pretty_assertions = "1.3.0" diff --git a/pkgs/crane-utils/src/bin/crane-resolve-workspace-inheritance.rs b/pkgs/crane-utils/src/bin/crane-resolve-workspace-inheritance.rs index d5da5ed..147d7e1 100644 --- a/pkgs/crane-utils/src/bin/crane-resolve-workspace-inheritance.rs +++ b/pkgs/crane-utils/src/bin/crane-resolve-workspace-inheritance.rs @@ -7,8 +7,9 @@ use std::{ mem, path::Path, process::{Command, Stdio}, + str::FromStr, }; -use toml::value::Table; +use toml_edit::Item; fn main() { let mut args = env::args(); @@ -64,39 +65,29 @@ fn resolve_and_print_cargo_toml(cargo_toml: &Path) -> anyhow::Result<()> { let mut cargo_toml = parse_toml(cargo_toml)?; merge(&mut cargo_toml, &parse_toml(&root_toml.join("Cargo.toml"))?); - toml::to_string(&cargo_toml) - .context("failed to serialize updated Cargo.toml") - .and_then(|string| { - stdout() - .write_all(string.as_bytes()) - .context("failed to print updated Cargo.toml") - .map(drop) - }) + stdout() + .write_all(cargo_toml.to_string().as_bytes()) + .context("failed to print updated Cargo.toml") } -fn parse_toml(path: &Path) -> anyhow::Result { +fn parse_toml(path: &Path) -> anyhow::Result { let mut buf = String::new(); File::open(path) .and_then(|mut file| file.read_to_string(&mut buf)) .with_context(|| format!("cannot read {}", path.display()))?; - toml::from_str(&buf).with_context(|| format!("cannot parse {}", path.display())) + toml_edit::Document::from_str(&buf).with_context(|| format!("cannot parse {}", path.display())) } -fn merge(cargo_toml: &mut toml::Value, root: &toml::Value) { - let (t, rt) = match (cargo_toml, root) { - (toml::Value::Table(t), toml::Value::Table(rt)) => (t, rt), - - // Bail if cargo_toml or workspace root are malformed - _ => return, - }; - - let w = if let Some(toml::Value::Table(w)) = rt.get("workspace") { - w - } else { - // no "workspace" entry, nothing to merge - return; - }; +/// Merge the workspace `root` toml into the specified crate's `cargo_toml` +fn merge(cargo_toml: &mut toml_edit::Document, root: &toml_edit::Document) { + let w: &dyn toml_edit::TableLike = + if let Some(w) = root.get("workspace").and_then(try_as_table_like) { + w + } else { + // no "workspace" entry, nothing to merge + return; + }; // https://doc.rust-lang.org/cargo/reference/workspaces.html#workspaces for (key, ws_key) in [ @@ -105,90 +96,200 @@ fn merge(cargo_toml: &mut toml::Value, root: &toml::Value) { ("dev-dependencies", "dependencies"), ("build-dependencies", "dependencies"), ] { - if let (Some(toml::Value::Table(p)), Some(toml::Value::Table(wp))) = - (t.get_mut(key), w.get(ws_key)) - { - merge_tables(p, wp); + if let Some((cargo_toml, root)) = cargo_toml.get_mut(key).zip(w.get(ws_key)) { + try_merge_cargo_tables(cargo_toml, root); }; - if let Some(toml::Value::Table(targets)) = t.get_mut("target") { - for (_, tp) in targets { - if let (Some(toml::Value::Table(p)), Some(toml::Value::Table(wp))) = - (tp.get_mut(key), w.get(ws_key)) - { - merge_tables(p, wp); - }; + if let Some(targets) = cargo_toml.get_mut("target").and_then(try_as_table_like_mut) { + for (_, tp) in targets.iter_mut() { + if let Some((cargo_toml, root)) = tp.get_mut(key).zip(w.get(ws_key)) { + try_merge_cargo_tables(cargo_toml, root); + } } } } } -fn merge_tables(cargo_toml: &mut Table, root: &Table) { +/// Return a [`toml_edit::TableLike`] representation of the [`Item`] (if any) +fn try_as_table_like(item: &Item) -> Option<&dyn toml_edit::TableLike> { + match item { + Item::Table(w) => Some(w), + Item::Value(toml_edit::Value::InlineTable(w)) => Some(w), + _ => None, + } +} + +/// Return a mutable [`toml_edit::TableLike`] representation of the [`Item`] (if any) +fn try_as_table_like_mut(item: &mut Item) -> Option<&mut dyn toml_edit::TableLike> { + match item { + Item::Table(w) => Some(w), + Item::Value(toml_edit::Value::InlineTable(w)) => Some(w), + _ => None, + } +} + +/// Merge the specified `cargo_toml` and workspace `root` if both are tables +fn try_merge_cargo_tables(cargo_toml: &mut Item, root: &Item) { + let cargo_toml = try_as_table_like_mut(cargo_toml); + let root = try_as_table_like(root); + + if let Some((cargo_toml, root)) = cargo_toml.zip(root) { + merge_cargo_tables(cargo_toml, root); + } +} +/// Merge the specified `cargo_toml` and workspace `root` tables +fn merge_cargo_tables(cargo_toml: &mut T, root: &U) +where + T: toml_edit::TableLike + ?Sized, + U: toml_edit::TableLike + ?Sized, +{ cargo_toml.iter_mut().for_each(|(k, v)| { // Bail if: // - cargo_toml isn't a table (otherwise `workspace = true` can't show up // - the workspace root doesn't have this key - let (t, root_val) = match (&mut *v, root.get(k)) { - (toml::Value::Table(t), Some(root_val)) => (t, root_val), + let (t, root_val) = match try_as_table_like_mut(&mut *v).zip(root.get(&k)) { + Some((t, root_val)) => (t, root_val), _ => return, }; - if let Some(toml::Value::Boolean(true)) = t.get("workspace") { - t.remove("workspace"); - let orig_val = mem::replace(v, root_val.clone()); - merge_into(v, orig_val); + if let Some(Item::Value(toml_edit::Value::Boolean(bool_value))) = t.get("workspace") { + if *bool_value.value() { + t.remove("workspace"); + let orig_val = mem::replace(v, root_val.clone()); + merge_items(v, orig_val); + } } }); } -fn merge_into(dest: &mut toml::Value, additional: toml::Value) { - match additional { - toml::Value::String(_) - | toml::Value::Integer(_) - | toml::Value::Float(_) - | toml::Value::Boolean(_) - | toml::Value::Datetime(_) => { - // Override dest completely for raw values - *dest = additional; - } +/// Recursively merge the `additional` item into the specified `dest` +fn merge_items(dest: &mut Item, additional: Item) { + use toml_edit::Value; - toml::Value::Array(additional) => { - if let toml::Value::Array(dest) = dest { + match additional { + Item::Value(additional) => match additional { + Value::String(_) + | Value::Integer(_) + | Value::Float(_) + | Value::Boolean(_) + | Value::Datetime(_) => { + // Override dest completely for raw values + *dest = Item::Value(additional); + } + + Value::Array(additional) => { + if let Item::Value(Value::Array(dest)) = dest { + dest.extend(additional); + } else { + // Override dest completely if types don't match + *dest = Item::Value(Value::Array(additional)); + } + } + + Value::InlineTable(additional) => { + merge_tables(dest, additional); + } + }, + Item::Table(additional) => { + merge_tables(dest, additional); + } + Item::None => {} + Item::ArrayOfTables(additional) => { + if let Item::ArrayOfTables(dest) = dest { dest.extend(additional); } else { // Override dest completely if types don't match - *dest = toml::Value::Array(additional); + *dest = Item::ArrayOfTables(additional); } } + } +} - toml::Value::Table(additional) => { - if let toml::Value::Table(dest) = dest { - additional - .into_iter() - .for_each(|(k, v)| match dest.get_mut(&k) { - Some(existing) => merge_into(existing, v), - None => { - dest.insert(k, v); - } - }); - } else { +use table_like_ext::merge_tables; +mod table_like_ext { + //! Helper functions to merge values in any combination of the two [`TableLike`] items + //! found in [`toml_edit`] + + use toml_edit::{Item, TableLike}; + + /// Recursively merge the `additional` table into `dest` (overwriting if `dest` is not a table) + pub(super) fn merge_tables(dest: &mut Item, additional: T) + where + T: TableLikeExt, + { + match dest { + Item::Table(dest) => merge_table_like(dest, additional), + Item::Value(toml_edit::Value::InlineTable(dest)) => merge_table_like(dest, additional), + _ => { // Override dest completely if types don't match, but also // skip empty tables (i.e. if we had `key = { workspace = true }` if !additional.is_empty() { - *dest = toml::Value::Table(additional); + *dest = additional.into_item(); } } } } + + /// Recursively merge two tables + fn merge_table_like(dest: &mut T, additional: U) + where + T: TableLike, + U: TableLikeExt, + { + additional + .into_iter() + .map(U::map_iter_item) + .for_each(|(k, v)| match dest.get_mut(&k) { + Some(existing) => super::merge_items(existing, v), + None => { + dest.insert(&k, v); + } + }); + } + + /// Generalized form of the item yielded by [`IntoIterator`] for the two [`TableLike`] types + /// in [`toml_edit`] + type CommonIterItem = (toml_edit::InternalString, Item); + + /// Extension trait to iterate [`Item`]s from a [`TableLike`] item + pub(super) trait TableLikeExt: TableLike + IntoIterator { + /// Convert the iterator item to a common type + fn map_iter_item(item: Self::Item) -> CommonIterItem; + + /// Convert the table into an [`Item`] + fn into_item(self) -> Item; + } + + impl TableLikeExt for toml_edit::Table { + fn map_iter_item(item: Self::Item) -> CommonIterItem { + item + } + + fn into_item(self) -> Item { + Item::Table(self) + } + } + + impl TableLikeExt for toml_edit::InlineTable { + fn map_iter_item(item: Self::Item) -> CommonIterItem { + let (k, v) = item; + (k, Item::Value(v)) + } + + fn into_item(self) -> Item { + Item::Value(toml_edit::Value::InlineTable(self)) + } + } } #[cfg(test)] mod tests { use pretty_assertions::assert_eq; + use std::str::FromStr; #[test] fn smoke() { - let mut cargo_toml = toml::from_str( + let mut cargo_toml = toml_edit::Document::from_str( r#" [package] authors.workspace = true @@ -209,6 +310,7 @@ mod tests { version.workspace = true [dependencies] + # the `foo` dependency is most imporant, so it goes first foo.workspace = true bar.workspace = true baz.workspace = true @@ -240,11 +342,15 @@ mod tests { grault = { version = "grault-vers" } garply = "garply-vers" waldo = "waldo-vers" + + [features] + # this feature is a demonstration that comments are preserved + my_feature = [] "#, ) .unwrap(); - let root_toml = toml::from_str( + let root_toml = toml_edit::Document::from_str( r#" [workspace.package] authors = ["first author", "second author"] @@ -265,6 +371,7 @@ mod tests { version = "some version" [workspace.dependencies] + # top-level workspace comments are not copied - only the values are merged foo = { version = "foo-vers" } bar = { version = "bar-vers", default-features = false } baz = { version = "baz-vers", features = ["baz-feat", "baz-feat2"] } @@ -278,64 +385,68 @@ mod tests { ) .unwrap(); - let expected_toml = toml::from_str::( - r#" + // NOTE: The nonstandard spacing is due to reusing decorations from original keys/values + // in cargo_toml + let expected_toml_str = r#" [package] - authors = ["first author", "second author"] - categories = ["first category", "second category" ] - description = "some description" - documentation = "some doc url" - edition = "2021" - exclude = ["first exclusion", "second exclusion"] - homepage = "some home page" - include = ["first inclusion", "second inclusion"] - keyword = ["first keyword", "second keyword"] - license = "some license" - license-file = "some license-file" - publish = true - readme = "some readme" - repository = "some repository" - rust-version = "some rust-version" - version = "some version" + authors= ["first author", "second author"] + categories= ["first category", "second category" ] + description= "some description" + documentation= "some doc url" + edition= "2021" + exclude= ["first exclusion", "second exclusion"] + homepage= "some home page" + include= ["first inclusion", "second inclusion"] + keyword= ["first keyword", "second keyword"] + license= "some license" + license-file= "some license-file" + publish= true + readme= "some readme" + repository= "some repository" + rust-version= "some rust-version" + version= "some version" [dependencies] - foo = { version = "foo-vers" } - bar = { version = "bar-vers", default-features = false } - baz = { version = "baz-vers", features = ["baz-feat", "baz-feat2"] } - qux = { version = "qux-vers", features = ["qux-feat", "qux-additional"] } - corge = { version = "corge-vers-override", features = ["qux-feat"] } + # the `foo` dependency is most imporant, so it goes first + foo= { version = "foo-vers" } + bar= { version = "bar-vers", default-features = false } + baz= { version = "baz-vers", features = ["baz-feat", "baz-feat2"] } + qux = { version = "qux-vers", features = ["qux-feat","qux-additional"] } + corge = { version = "corge-vers-override" , features = ["qux-feat"] } grault = { version = "grault-vers" } garply = "garply-vers" waldo = "waldo-vers" [target.'cfg(unix)'.dependencies] - unix = { version = "unix-vers", features = ["some"] } + unix = { version = "unix-vers" , features = ["some"] } [dev-dependencies] - foo = { version = "foo-vers" } - bar = { version = "bar-vers", default-features = false } - baz = { version = "baz-vers", features = ["baz-feat", "baz-feat2"] } - qux = { version = "qux-vers", features = ["qux-feat", "qux-additional"] } - corge = { version = "corge-vers-override", features = ["qux-feat"] } + foo= { version = "foo-vers" } + bar= { version = "bar-vers", default-features = false } + baz= { version = "baz-vers", features = ["baz-feat", "baz-feat2"] } + qux = { version = "qux-vers", features = ["qux-feat","qux-additional"] } + corge = { version = "corge-vers-override" , features = ["qux-feat"] } grault = { version = "grault-vers" } garply = "garply-vers" waldo = "waldo-vers" [build-dependencies] - foo = { version = "foo-vers" } - bar = { version = "bar-vers", default-features = false } - baz = { version = "baz-vers", features = ["baz-feat", "baz-feat2"] } - qux = { version = "qux-vers", features = ["qux-feat", "qux-additional"] } - corge = { version = "corge-vers-override", features = ["qux-feat"] } + foo= { version = "foo-vers" } + bar= { version = "bar-vers", default-features = false } + baz= { version = "baz-vers", features = ["baz-feat", "baz-feat2"] } + qux = { version = "qux-vers", features = ["qux-feat","qux-additional"] } + corge = { version = "corge-vers-override" , features = ["qux-feat"] } grault = { version = "grault-vers" } garply = "garply-vers" waldo = "waldo-vers" - "#, - ) - .unwrap(); + + [features] + # this feature is a demonstration that comments are preserved + my_feature = [] + "#; super::merge(&mut cargo_toml, &root_toml); - assert_eq!(expected_toml, cargo_toml); + assert_eq!(expected_toml_str, cargo_toml.to_string()); } }