From 87345a19327659f3e2e0899076e6c201eaf5ff9f Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Mon, 8 Jan 2024 21:55:41 +0800 Subject: [PATCH 1/4] test: correct not_inherit_workspace_package_table_if_not_members case Signed-off-by: hi-rustin --- .../in/Cargo.toml | 7 +++++++ .../mod.rs | 2 +- .../out/Cargo.toml | 7 ++++++- .../out/{foo => bar}/Cargo.toml | 2 +- .../out/foo/src/main.rs | 3 --- .../stderr.log | 11 ++++++++++- 6 files changed, 25 insertions(+), 7 deletions(-) rename tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/{foo => bar}/Cargo.toml (97%) delete mode 100644 tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/foo/src/main.rs diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/in/Cargo.toml b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/in/Cargo.toml index 2d204581cfc..e05d879af7a 100644 --- a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/in/Cargo.toml +++ b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/in/Cargo.toml @@ -1,3 +1,5 @@ +[workspace] +exclude = ["bar"] [workspace.package] authors = ["Rustaceans"] description = "foo" @@ -13,3 +15,8 @@ include = ["foo"] license = "MIT OR Apache-2.0" publish = false repository = "foo" + +[package] +name = "foo" +version = "0.1.0" +edition = "2018" diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/mod.rs b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/mod.rs index cdddf0e64fe..1634ece12fb 100644 --- a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/mod.rs +++ b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/mod.rs @@ -11,7 +11,7 @@ fn case() { snapbox::cmd::Command::cargo_ui() .arg("new") - .args(["foo"]) + .args(["bar"]) .current_dir(cwd) .assert() .success() diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/Cargo.toml b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/Cargo.toml index 8de6ed4bfe5..e05d879af7a 100644 --- a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/Cargo.toml +++ b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/Cargo.toml @@ -1,5 +1,5 @@ [workspace] -members = ["foo"] +exclude = ["bar"] [workspace.package] authors = ["Rustaceans"] description = "foo" @@ -15,3 +15,8 @@ include = ["foo"] license = "MIT OR Apache-2.0" publish = false repository = "foo" + +[package] +name = "foo" +version = "0.1.0" +edition = "2018" diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/foo/Cargo.toml b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/Cargo.toml similarity index 97% rename from tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/foo/Cargo.toml rename to tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/Cargo.toml index 4fcf77121b3..4e4d52ddf7c 100644 --- a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/foo/Cargo.toml +++ b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "foo" +name = "bar" version = "0.1.0" authors.workspace = true description.workspace = true diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/foo/src/main.rs b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/foo/src/main.rs deleted file mode 100644 index e7a11a969c0..00000000000 --- a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/foo/src/main.rs +++ /dev/null @@ -1,3 +0,0 @@ -fn main() { - println!("Hello, world!"); -} diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/stderr.log b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/stderr.log index 9b501592449..dffb055589e 100644 --- a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/stderr.log +++ b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/stderr.log @@ -1 +1,10 @@ - Created binary (application) `foo` package +warning: compiling this new package may not work due to invalid workspace configuration + +failed to parse manifest at `[ROOT]/case/bar/Cargo.toml` + +Caused by: + error inheriting `edition` from workspace root manifest's `workspace.package.edition` + +Caused by: + failed to find a workspace root + Created binary (application) `bar` package From 47f4b75847587efd6b2ce1968989011721beb7ab Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Mon, 8 Jan 2024 22:52:06 +0800 Subject: [PATCH 2/4] fix: only inherit workspace package table if the new package is a member Signed-off-by: hi-rustin --- src/cargo/ops/cargo_new.rs | 149 ++++++++++-------- .../out/bar/Cargo.toml | 15 +- .../out/bar/src/main.rs | 3 + .../stderr.log | 9 -- 4 files changed, 85 insertions(+), 91 deletions(-) create mode 100644 tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/src/main.rs diff --git a/src/cargo/ops/cargo_new.rs b/src/cargo/ops/cargo_new.rs index f126a5f469a..070c147b667 100644 --- a/src/cargo/ops/cargo_new.rs +++ b/src/cargo/ops/cargo_new.rs @@ -816,35 +816,42 @@ fn mk(config: &Config, opts: &MkOptions<'_>) -> CargoResult<()> { // This should not block the creation of the new project. It is only a best effort to // inherit the workspace package keys. if let Ok(mut workspace_document) = root_manifest.parse::() { - if let Some(workspace_package_keys) = workspace_document - .get("workspace") - .and_then(|workspace| workspace.get("package")) - .and_then(|package| package.as_table()) - { - update_manifest_with_inherited_workspace_package_keys( - opts, - &mut manifest, - workspace_package_keys, - ) - } - - // Try to inherit the workspace lints key if it exists. - if workspace_document - .get("workspace") - .and_then(|workspace| workspace.get("lints")) - .is_some() - { - let mut table = toml_edit::Table::new(); - table["workspace"] = toml_edit::value(true); - manifest["lints"] = toml_edit::Item::Table(table); - } - - // Try to add the new package to the workspace members. - update_manifest_with_new_member( + let (display_path, can_be_a_member) = get_display_path_and_check_membership( &root_manifest_path, - &mut workspace_document, - opts.path, + &path, + &workspace_document, )?; + // Only try to inherit the workspace stuff if the new package can be a member of the workspace. + if can_be_a_member { + if let Some(workspace_package_keys) = workspace_document + .get("workspace") + .and_then(|workspace| workspace.get("package")) + .and_then(|package| package.as_table()) + { + update_manifest_with_inherited_workspace_package_keys( + opts, + &mut manifest, + workspace_package_keys, + ) + } + // Try to inherit the workspace lints key if it exists. + if workspace_document + .get("workspace") + .and_then(|workspace| workspace.get("lints")) + .is_some() + { + let mut table = toml_edit::Table::new(); + table["workspace"] = toml_edit::value(true); + manifest["lints"] = toml_edit::Item::Table(table); + } + + // Try to add the new package to the workspace members. + update_manifest_with_new_member( + &root_manifest_path, + &mut workspace_document, + &display_path, + )?; + } } } @@ -955,8 +962,52 @@ fn update_manifest_with_inherited_workspace_package_keys( fn update_manifest_with_new_member( root_manifest_path: &Path, workspace_document: &mut toml_edit::Document, - package_path: &Path, + display_path: &str, ) -> CargoResult<()> { + // If the members element already exist, check if one of the patterns + // in the array already includes the new package's relative path. + // - Add the relative path if the members don't match the new package's path. + // - Create a new members array if there are no members element in the workspace yet. + if let Some(members) = workspace_document + .get_mut("workspace") + .and_then(|workspace| workspace.get_mut("members")) + .and_then(|members| members.as_array_mut()) + { + for member in members.iter() { + let pat = member + .as_str() + .with_context(|| format!("invalid non-string member `{}`", member))?; + let pattern = glob::Pattern::new(pat) + .with_context(|| format!("cannot build glob pattern from `{}`", pat))?; + + if pattern.matches(&display_path) { + return Ok(()); + } + } + + let was_sorted = is_sorted(members.iter().map(Value::as_str)); + members.push(display_path); + if was_sorted { + members.sort_by(|lhs, rhs| lhs.as_str().cmp(&rhs.as_str())); + } + } else { + let mut array = Array::new(); + array.push(display_path); + + workspace_document["workspace"]["members"] = toml_edit::value(array); + } + + write_atomic( + &root_manifest_path, + workspace_document.to_string().to_string().as_bytes(), + ) +} + +fn get_display_path_and_check_membership( + root_manifest_path: &Path, + package_path: &Path, + workspace_document: &toml_edit::Document, +) -> CargoResult<(String, bool)> { // Find the relative path for the package from the workspace root directory. let workspace_root = root_manifest_path.parent().with_context(|| { format!( @@ -981,8 +1032,6 @@ fn update_manifest_with_new_member( } let display_path = components.join("/"); - // Don't add the new package to the workspace's members - // if there is an exclusion match for it. if let Some(exclude) = workspace_document .get("workspace") .and_then(|workspace| workspace.get("exclude")) @@ -993,46 +1042,10 @@ fn update_manifest_with_new_member( .as_str() .with_context(|| format!("invalid non-string exclude path `{}`", member))?; if pat == display_path { - return Ok(()); + return Ok((display_path, false)); } } } - // If the members element already exist, check if one of the patterns - // in the array already includes the new package's relative path. - // - Add the relative path if the members don't match the new package's path. - // - Create a new members array if there are no members element in the workspace yet. - if let Some(members) = workspace_document - .get_mut("workspace") - .and_then(|workspace| workspace.get_mut("members")) - .and_then(|members| members.as_array_mut()) - { - for member in members.iter() { - let pat = member - .as_str() - .with_context(|| format!("invalid non-string member `{}`", member))?; - let pattern = glob::Pattern::new(pat) - .with_context(|| format!("cannot build glob pattern from `{}`", pat))?; - - if pattern.matches(&display_path) { - return Ok(()); - } - } - - let was_sorted = is_sorted(members.iter().map(Value::as_str)); - members.push(&display_path); - if was_sorted { - members.sort_by(|lhs, rhs| lhs.as_str().cmp(&rhs.as_str())); - } - } else { - let mut array = Array::new(); - array.push(&display_path); - - workspace_document["workspace"]["members"] = toml_edit::value(array); - } - - write_atomic( - &root_manifest_path, - workspace_document.to_string().to_string().as_bytes(), - ) + Ok((display_path, true)) } diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/Cargo.toml b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/Cargo.toml index 4e4d52ddf7c..825efac3438 100644 --- a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/Cargo.toml +++ b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/Cargo.toml @@ -1,20 +1,7 @@ [package] name = "bar" version = "0.1.0" -authors.workspace = true -description.workspace = true -edition.workspace = true -homepage.workspace = true -keywords.workspace = true -readme.workspace = true -rust-version.workspace = true -categories.workspace = true -documentation.workspace = true -exclude.workspace = true -include.workspace = true -license.workspace = true -publish.workspace = true -repository.workspace = true +edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/src/main.rs b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/src/main.rs new file mode 100644 index 00000000000..e7a11a969c0 --- /dev/null +++ b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/bar/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/stderr.log b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/stderr.log index dffb055589e..4d67591f449 100644 --- a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/stderr.log +++ b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/stderr.log @@ -1,10 +1 @@ -warning: compiling this new package may not work due to invalid workspace configuration - -failed to parse manifest at `[ROOT]/case/bar/Cargo.toml` - -Caused by: - error inheriting `edition` from workspace root manifest's `workspace.package.edition` - -Caused by: - failed to find a workspace root Created binary (application) `bar` package From 874645e39459fcec07fc9e7888cdb6ac43cf5b0a Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Tue, 9 Jan 2024 21:49:33 +0800 Subject: [PATCH 3/4] test: add lints Signed-off-by: hi-rustin --- .../in/Cargo.toml | 2 ++ .../out/Cargo.toml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/in/Cargo.toml b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/in/Cargo.toml index e05d879af7a..26a4849d72a 100644 --- a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/in/Cargo.toml +++ b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/in/Cargo.toml @@ -15,6 +15,8 @@ include = ["foo"] license = "MIT OR Apache-2.0" publish = false repository = "foo" +[workspace.lints.rust] +unsafe_code = "forbid" [package] name = "foo" diff --git a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/Cargo.toml b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/Cargo.toml index e05d879af7a..26a4849d72a 100644 --- a/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/Cargo.toml +++ b/tests/testsuite/cargo_new/not_inherit_workspace_package_table_if_not_members/out/Cargo.toml @@ -15,6 +15,8 @@ include = ["foo"] license = "MIT OR Apache-2.0" publish = false repository = "foo" +[workspace.lints.rust] +unsafe_code = "forbid" [package] name = "foo" From db4ed038c19eb59d865bb1f5ba2af1e8ba79ffd7 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Tue, 9 Jan 2024 22:00:33 +0800 Subject: [PATCH 4/4] refactor: separate get_display_path_and_check_membership Signed-off-by: hi-rustin --- src/cargo/ops/cargo_new.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/cargo/ops/cargo_new.rs b/src/cargo/ops/cargo_new.rs index 070c147b667..29184d8ed1d 100644 --- a/src/cargo/ops/cargo_new.rs +++ b/src/cargo/ops/cargo_new.rs @@ -816,11 +816,8 @@ fn mk(config: &Config, opts: &MkOptions<'_>) -> CargoResult<()> { // This should not block the creation of the new project. It is only a best effort to // inherit the workspace package keys. if let Ok(mut workspace_document) = root_manifest.parse::() { - let (display_path, can_be_a_member) = get_display_path_and_check_membership( - &root_manifest_path, - &path, - &workspace_document, - )?; + let display_path = get_display_path(&root_manifest_path, &path)?; + let can_be_a_member = can_be_workspace_member(&display_path, &workspace_document)?; // Only try to inherit the workspace stuff if the new package can be a member of the workspace. if can_be_a_member { if let Some(workspace_package_keys) = workspace_document @@ -1003,11 +1000,7 @@ fn update_manifest_with_new_member( ) } -fn get_display_path_and_check_membership( - root_manifest_path: &Path, - package_path: &Path, - workspace_document: &toml_edit::Document, -) -> CargoResult<(String, bool)> { +fn get_display_path(root_manifest_path: &Path, package_path: &Path) -> CargoResult { // Find the relative path for the package from the workspace root directory. let workspace_root = root_manifest_path.parent().with_context(|| { format!( @@ -1031,7 +1024,14 @@ fn get_display_path_and_check_membership( components.push(comp); } let display_path = components.join("/"); + Ok(display_path) +} +// Check if the package can be a member of the workspace. +fn can_be_workspace_member( + display_path: &str, + workspace_document: &toml_edit::Document, +) -> CargoResult { if let Some(exclude) = workspace_document .get("workspace") .and_then(|workspace| workspace.get("exclude")) @@ -1042,10 +1042,9 @@ fn get_display_path_and_check_membership( .as_str() .with_context(|| format!("invalid non-string exclude path `{}`", member))?; if pat == display_path { - return Ok((display_path, false)); + return Ok(false); } } } - - Ok((display_path, true)) + Ok(true) }