From 3a1236c980244ebe9799956c16c732a511bde6d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 17 Apr 2023 08:52:05 +0000 Subject: [PATCH] Use `cargo metadata` to find workspace manifests (#204) * Fix skeleton tests so that they produce valid Cargo workspaces Tested by parsing these workspaces with `cargo metadata` (not present in this commit). * Resolve manifests using Cargo metadata * Add test for workspace glob members * Review changes --- Cargo.lock | 62 +++++++++++++++ Cargo.toml | 1 + src/skeleton/mod.rs | 27 ++++++- src/skeleton/read.rs | 146 ++++++++++++---------------------- tests/skeletons.rs | 183 ++++++++++++++++++++++++++++--------------- 5 files changed, 259 insertions(+), 160 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0e61f19..af6c190 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -122,6 +122,15 @@ dependencies = [ "serde", ] +[[package]] +name = "camino" +version = "1.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c530edf18f37068ac2d977409ed5cd50d53d73bc653c7647b48eb78976ac9ae2" +dependencies = [ + "serde", +] + [[package]] name = "cargo-chef" version = "0.1.55" @@ -131,6 +140,7 @@ dependencies = [ "assert_fs", "atty", "cargo-manifest", + "cargo_metadata", "clap", "env_logger", "expect-test", @@ -154,6 +164,29 @@ dependencies = [ "toml", ] +[[package]] +name = "cargo-platform" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cbdb825da8a5df079a43676dbe042702f1707b1109f713a01420fbb4cc71fa27" +dependencies = [ + "serde", +] + +[[package]] +name = "cargo_metadata" +version = "0.15.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eee4243f1f26fc7a42710e7439c149e2b10b05472f88090acce52632f231a73a" +dependencies = [ + "camino", + "cargo-platform", + "semver", + "serde", + "serde_json", + "thiserror", +] + [[package]] name = "cc" version = "1.0.79" @@ -623,6 +656,15 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "semver" +version = "1.0.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bebd363326d05ec3e2f532ab7660680f3b02130d780c299bca73469d521bc0ed" +dependencies = [ + "serde", +] + [[package]] name = "serde" version = "1.0.159" @@ -708,6 +750,26 @@ version = "0.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3369f5ac52d5eb6ab48c6b4ffdc8efbcad6b89c765749064ba298f2c68a16a76" +[[package]] +name = "thiserror" +version = "1.0.40" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "978c9a314bd8dc99be594bc3c175faaa9794be04a5a5e153caba6915336cebac" +dependencies = [ + "thiserror-impl", +] + +[[package]] +name = "thiserror-impl" +version = "1.0.40" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f9456a42c5b0d803c8cd86e73dd7cc9edd429499f37a3550d286d5e86720569f" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "thread_local" version = "1.1.7" diff --git a/Cargo.toml b/Cargo.toml index a110109..50dbef4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,7 @@ cargo-manifest = "0.8" fs-err = "2.5.0" toml = { version = "0.7", features = ["preserve_order"] } expect-test = "1.1.0" +cargo_metadata = "0.15" [dev-dependencies] assert_cmd = "2" diff --git a/src/skeleton/mod.rs b/src/skeleton/mod.rs index 77f3cbb..035cb2c 100644 --- a/src/skeleton/mod.rs +++ b/src/skeleton/mod.rs @@ -33,9 +33,11 @@ impl Skeleton { base_path: P, member: Option, ) -> Result { + let metadata = extract_cargo_metadata(base_path.as_ref())?; + // Read relevant files from the filesystem let config_file = read::config(&base_path)?; - let mut manifests = read::manifests(&base_path, config_file.as_deref())?; + let mut manifests = read::manifests(&base_path, metadata)?; if let Some(member) = member { ignore_all_members_except(&mut manifests, member); } @@ -335,9 +337,16 @@ fn serialize_manifests(manifests: Vec) -> Result, Ok(serialised_manifests) } +fn extract_cargo_metadata(path: &Path) -> Result { + let mut cmd = cargo_metadata::MetadataCommand::new(); + cmd.current_dir(path); + + cmd.exec().context("Cannot extract Cargo metadata") +} + /// If the top-level `Cargo.toml` has a `members` field, replace it with /// a list consisting of just the specified member. -fn ignore_all_members_except(manifests: &mut [ParsedManifest], member: String) { +fn ignore_all_members_except(manifests: &mut Vec, member: String) { let workspace_toml = manifests .iter_mut() .find(|manifest| manifest.relative_path == std::path::PathBuf::from("Cargo.toml")); @@ -346,6 +355,18 @@ fn ignore_all_members_except(manifests: &mut [ParsedManifest], member: String) { .and_then(|toml| toml.contents.get_mut("workspace")) .and_then(|workspace| workspace.get_mut("members")) { - *members = toml::Value::Array(vec![toml::Value::String(member)]); + *members = toml::Value::Array(vec![toml::Value::String(member.clone())]); } + + // Keep only manifest(s) with package name equal to `member` + manifests.retain(|manifest| { + let package_name = manifest + .contents + .as_table() + .and_then(|t| t.get("package")) + .and_then(|t| t.as_table()) + .and_then(|t| t.get("name")) + .and_then(|t| t.as_str()); + package_name.is_none() || package_name == Some(&member) + }); } diff --git a/src/skeleton/read.rs b/src/skeleton/read.rs index cd2d979..69eba33 100644 --- a/src/skeleton/read.rs +++ b/src/skeleton/read.rs @@ -1,7 +1,7 @@ //! Logic to read all the files required to build a caching layer for a project. use super::ParsedManifest; -use anyhow::Context; -use globwalk::{GlobWalkerBuilder, WalkError}; +use cargo_metadata::Metadata; +use std::collections::BTreeSet; use std::fs; use std::path::Path; use std::str::FromStr; @@ -35,87 +35,66 @@ pub(super) fn config>(base_path: &P) -> Result, an } } -fn vendored_directory(config_contents: Option<&str>) -> Option { - let contents = config_contents.and_then(|contents| contents.parse::().ok())?; - let source = contents.get("source")?; - let crates_io = source.get("crates-io")?; - let vendored_field_suffix = crates_io - .get("replace-with") - .and_then(|value| value.as_str())?; - let vendored_sources = source.get(vendored_field_suffix)?; - Some(vendored_sources.get("directory")?.as_str()?.to_owned()) -} - pub(super) fn manifests>( base_path: &P, - config_contents: Option<&str>, + metadata: Metadata, ) -> Result, anyhow::Error> { - let vendored_path = vendored_directory(config_contents); - let builder = if let Some(path) = vendored_path { - let exclude_vendored_sources = format!("!{}", path); - GlobWalkerBuilder::from_patterns(base_path, &["/**/Cargo.toml", &exclude_vendored_sources]) - } else { - GlobWalkerBuilder::new(base_path, "/**/Cargo.toml") - }; - let walker = builder - .build() - .context("Failed to scan the files in the current directory.")?; + let manifest_paths = metadata + .workspace_packages() + .iter() + .map(|package| package.manifest_path.clone().into_std_path_buf()) + .chain( + metadata + .root_package() + .map(|p| p.clone().manifest_path.into_std_path_buf()) + .or_else(|| Some(base_path.as_ref().join("Cargo.toml"))), + ) + .collect::>(); let mut manifests = vec![]; - for manifest in walker { - match manifest { - Ok(manifest) => { - let absolute_path = manifest.path().to_path_buf(); - let contents = fs::read_to_string(&absolute_path)?; + for absolute_path in manifest_paths { + let contents = fs::read_to_string(&absolute_path)?; - let mut parsed = cargo_manifest::Manifest::from_str(&contents)?; - // Required to detect bin/libs when the related section is omitted from the manifest - parsed.complete_from_path(&absolute_path)?; + let mut parsed = cargo_manifest::Manifest::from_str(&contents)?; + // Required to detect bin/libs when the related section is omitted from the manifest + parsed.complete_from_path(&absolute_path)?; - let mut intermediate = toml::Value::try_from(parsed)?; + let mut intermediate = toml::Value::try_from(parsed)?; - // Specifically, toml gives no guarantees to the ordering of the auto binaries - // in its results. We will manually sort these to ensure that the output - // manifest will match. - let bins = intermediate - .get_mut("bin") - .and_then(|bins| bins.as_array_mut()); - if let Some(bins) = bins { - bins.sort_by(|bin_a, bin_b| { - let bin_a_path = bin_a - .as_table() - .and_then(|table| table.get("path").or_else(|| table.get("name"))) - .and_then(|path| path.as_str()) - .unwrap(); - let bin_b_path = bin_b - .as_table() - .and_then(|table| table.get("path").or_else(|| table.get("name"))) - .and_then(|path| path.as_str()) - .unwrap(); - bin_a_path.cmp(bin_b_path) - }); - } - - let relative_path = - pathdiff::diff_paths(&absolute_path, base_path).ok_or_else(|| { - anyhow::anyhow!( - "Failed to compute relative path of manifest {:?}", - &absolute_path - ) - })?; - manifests.push(ParsedManifest { - relative_path, - contents: intermediate, - }); - } - Err(e) => match handle_walk_error(e) { - ErrorStrategy::Ignore => {} - ErrorStrategy::Crash(e) => { - return Err(e.into()); - } - }, + // Specifically, toml gives no guarantees to the ordering of the auto binaries + // in its results. We will manually sort these to ensure that the output + // manifest will match. + let bins = intermediate + .get_mut("bin") + .and_then(|bins| bins.as_array_mut()); + if let Some(bins) = bins { + bins.sort_by(|bin_a, bin_b| { + let bin_a_path = bin_a + .as_table() + .and_then(|table| table.get("path").or_else(|| table.get("name"))) + .and_then(|path| path.as_str()) + .unwrap(); + let bin_b_path = bin_b + .as_table() + .and_then(|table| table.get("path").or_else(|| table.get("name"))) + .and_then(|path| path.as_str()) + .unwrap(); + bin_a_path.cmp(bin_b_path) + }); } + + let relative_path = pathdiff::diff_paths(&absolute_path, base_path).ok_or_else(|| { + anyhow::anyhow!( + "Failed to compute relative path of manifest {:?}", + &absolute_path + ) + })?; + manifests.push(ParsedManifest { + relative_path, + contents: intermediate, + }); } + Ok(manifests) } @@ -135,24 +114,3 @@ pub(super) fn lockfile>( } } } - -/// What should we should when we encounter an issue while walking the current directory? -/// -/// If `ErrorStrategy::Ignore`, just skip the file/directory and keep going. -/// If `ErrorStrategy::Crash`, stop exploring and return an error to the caller. -enum ErrorStrategy { - Ignore, - Crash(WalkError), -} - -/// Ignore directory/files for which we don't have enough permissions to perform our scan. -#[must_use] -fn handle_walk_error(e: WalkError) -> ErrorStrategy { - if let Some(inner) = e.io_error() { - if std::io::ErrorKind::PermissionDenied == inner.kind() { - log::warn!("Missing permission to read entry: {}\nSkipping.", inner); - return ErrorStrategy::Ignore; - } - } - ErrorStrategy::Crash(e) -} diff --git a/tests/skeletons.rs b/tests/skeletons.rs index d9a2668..6e3f514 100644 --- a/tests/skeletons.rs +++ b/tests/skeletons.rs @@ -188,9 +188,6 @@ name = "test-dummy" version = "0.1.0" edition = "2018" -[lib] -name = "test-dummy" - [[bench]] name = "basics" harness = false @@ -506,19 +503,20 @@ version = "0.0.1" #[test] pub fn workspace_version_lock() { // Arrange + // project-a is named with a dash to test that such unnormalized name can be handled. let project = CargoWorkspace::new() .manifest( ".", r#" [workspace] members = [ - "src/project_a", + "src/project-a", "src/project_b", ] "#, ) .bin_package( - "src/project_a", + "src/project-a", r#" [package] name = "project-a" @@ -530,7 +528,7 @@ name = "test-dummy" path = "src/main.rs" [dependencies] -uuid = { version = "=0.8.0", features = ["v4"] } +either = { version = "=1.8.1" } "#, ) .lib_package( @@ -545,8 +543,8 @@ edition = "2018" crate-type = ["cdylib"] [dependencies] -uuid = { version = "=0.8.0", features = ["v4"] } -project_a = { version = "0.0.1", path = "../project_a" } +either = { version = "=1.8.1" } +project-a = { version = "1.2.3", path = "../project-a" } "#, ) .file( @@ -556,26 +554,26 @@ project_a = { version = "0.0.1", path = "../project_a" } # It is not intended for manual editing. version = 3 +[[package]] +name = "either" +version = "1.8.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7fcaabb2fef8c910e7f4c7ce9f67a1283a1715879a7c230ca9d6d1ae31f16d91" + [[package]] name = "project-a" version = "1.2.3" dependencies = [ - "uuid", + "either", ] [[package]] name = "project_b" version = "4.5.6" dependencies = [ - "uuid", - "project-a" + "either", + "project_a", ] - -[[package]] -name = "uuid" -version = "0.8.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bc5cf98d8186244414c848017f0e2676b3fcb46807f6668a97dfe67359a3c4b7" "#, ) .build(); @@ -620,8 +618,8 @@ version = "0.0.1" assert!(lock_file.contains( r#" [[package]] -name = "uuid" -version = "0.8.0" +name = "either" +version = "1.8.1" "# )); @@ -630,7 +628,7 @@ version = "0.8.0" &first.contents, expect_test::expect![[r#" [workspace] - members = ["src/project_a", "src/project_b"] + members = ["src/project-a", "src/project_b"] "#]], ); let second = skeleton.manifests[1].clone(); @@ -662,9 +660,8 @@ version = "0.8.0" autotests = true autobenches = true - [dependencies.uuid] - version = "=0.8.0" - features = ["v4"] + [dependencies.either] + version = "=1.8.1" "#]], ); let third = skeleton.manifests[2].clone(); @@ -685,13 +682,12 @@ version = "0.8.0" autotests = true autobenches = true - [dependencies.project_a] - version = "0.0.1" - path = "../project_a" + [dependencies.either] + version = "=1.8.1" - [dependencies.uuid] - version = "=0.8.0" - features = ["v4"] + [dependencies.project-a] + version = "0.0.1" + path = "../project-a" [lib] test = true @@ -731,11 +727,12 @@ replace-with = "vendored-sources" [source.vendored-sources] directory = "vendor" - "#, +"#, ) - .manifest( + .lib_package( "vendor/rocket", - r#"[package] + r#" +[package] edition = "2018" name = "rocket" version = "0.5.0-rc.1" @@ -749,19 +746,36 @@ keywords = ["rocket", "web", "framework", "server"] categories = ["web-programming::http-server"] license = "MIT OR Apache-2.0" repository = "https://github.com/SergioBenitez/Rocket" + [package.metadata.docs.rs] all-features = true + [dependencies.rocket_dep] -version = "0.3.2""#, +version = "0.3.2" +"#, ) - .manifest( + .file( + "vendor/rocket/.cargo-checksum.json", + r#" +{"files": {}} +"#, + ) + .lib_package( "vendor/rocket_dep", - r#"[package] + r#" +[package] edition = "2018" name = "rocket_dep" version = "0.3.2" authors = ["Test author"] -description = "sample package representing all of rocket's dependencies""#, +description = "sample package representing all of rocket's dependencies" +"#, + ) + .file( + "vendor/rocket_dep/.cargo-checksum.json", + r#" +{"files": {}} +"#, ) .build(); @@ -792,6 +806,15 @@ members = [ [package] name = "backend" version = "0.1.0" +edition = "2018" + "#, + ) + .bin_package( + "ci", + r#" +[package] +name = "ci" +version = "0.1.0" edition = "2018" "#, ) @@ -831,7 +854,6 @@ pub fn mask_workspace_dependencies() { ".", r#" [workspace] - members = [ "project_a", "project_b", @@ -845,11 +867,10 @@ license = "Apache-2.0" [workspace.dependencies] anyhow = "1.0.66" project_a = { path = "project_a", version = "0.2.0" } -project_b = { path = "project_b", version = "0.2.0" } "#, ) .bin_package( - "src/project_a", + "project_a", r#" [package] name = "project_a" @@ -858,12 +879,11 @@ edition.workspace = true license.workspace = true [dependencies] -project_b = { workspace = true } anyhow = { workspace = true } "#, ) .lib_package( - "src/project_b", + "project_b", r#" [package] name = "project_b" @@ -892,25 +912,21 @@ anyhow = { workspace = true } check( &first.contents, expect_test::expect![[r#" - [workspace] - members = ["project_a", "project_b"] - - [workspace.dependencies] - anyhow = "1.0.66" - - [workspace.dependencies.project_a] - version = "0.0.1" - path = "project_a" - - [workspace.dependencies.project_b] - version = "0.0.1" - path = "project_b" - - [workspace.package] - edition = "2021" - version = "0.0.1" - license = "Apache-2.0" - "#]], + [workspace] + members = ["project_a", "project_b"] + + [workspace.dependencies] + anyhow = "1.0.66" + + [workspace.dependencies.project_a] + version = "0.0.1" + path = "project_a" + + [workspace.package] + edition = "2021" + version = "0.0.1" + license = "Apache-2.0" + "#]], ); let second = skeleton.manifests[1].clone(); @@ -951,9 +967,6 @@ anyhow = { workspace = true } [dependencies.anyhow] workspace = true - - [dependencies.project_b] - workspace = true "#]], ); @@ -1002,6 +1015,50 @@ anyhow = { workspace = true } ); } +#[test] +pub fn workspace_glob_members() { + // Arrange + let project = CargoWorkspace::new() + .manifest( + ".", + r#" +[workspace] +members = ["crates/*"] + "#, + ) + .bin_package( + "crates/project_a", + r#" +[package] +name = "project_a" +version = "0.0.1" + "#, + ) + .lib_package( + "crates/project_b", + r#" +[package] +name = "project_b" +version = "0.0.1" + "#, + ) + .lib_package( + "crates-unused/project_c", + r#" +[package] +name = "project_c" +version = "0.0.1" + "#, + ) + .build(); + + // Act + let skeleton = Skeleton::derive(project.path(), None).unwrap(); + + // Assert + assert_eq!(skeleton.manifests.len(), 3); +} + fn check(actual: &str, expect: Expect) { let actual = actual.to_string(); expect.assert_eq(&actual);