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.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"

src/skeleton/mod.rs

@@ -33,9 +33,11 @@ impl Skeleton {
         base_path: P,
         member: Option<String>,
     ) -> Result<Self, anyhow::Error> {
+        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<ParsedManifest>) -> Result<Vec<Manifest>,
     Ok(serialised_manifests)
 }
 
+fn extract_cargo_metadata(path: &Path) -> Result<cargo_metadata::Metadata, anyhow::Error> {
+    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<ParsedManifest>, 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)
+    });
 }

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<P: AsRef<Path>>(base_path: &P) -> Result<Option<String>, an
     }
 }
 
-fn vendored_directory(config_contents: Option<&str>) -> Option<String> {
-    let contents = config_contents.and_then(|contents| contents.parse::<toml::Value>().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<P: AsRef<Path>>(
     base_path: &P,
-    config_contents: Option<&str>,
+    metadata: Metadata,
 ) -> Result<Vec<ParsedManifest>, 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::<BTreeSet<_>>();
 
     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<P: AsRef<Path>>(
         }
     }
 }
-
-/// 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)
-}