Merge #73

73: Do not read manifest if information from manifest is not required r=taiki-e a=taiki-e

Very few options require information from Cargo manifest, so it can improve performance by avoiding reading and parsing them in many cases.

Co-authored-by: Taiki Endo <te316e89@gmail.com>

.github/workflows/ci.yml

@@ -31,6 +31,8 @@ jobs:
           # This is the minimum supported Rust version of this crate.
           # When updating this, the reminder to update the minimum supported Rust version in README.md.
           - 1.36.0
+          - 1.38.0
+          - 1.39.0
           - stable
           - beta
           - nightly

CHANGELOG.md

@@ -21,13 +21,16 @@ This project adheres to [Semantic Versioning](https://semver.org).
 
 * [Fix an issue where using `--features` with `--each-feature` or `--feature-powerset` together would result in the same feature combination being performed multiple times.][64]
 
+* [Improve performance by avoiding reading and parsing Cargo manifest.][73]
+
 [42]: https://github.com/taiki-e/cargo-hack/pull/42
 [61]: https://github.com/taiki-e/cargo-hack/pull/61
 [62]: https://github.com/taiki-e/cargo-hack/pull/62
 [63]: https://github.com/taiki-e/cargo-hack/pull/63
 [64]: https://github.com/taiki-e/cargo-hack/pull/64
 [65]: https://github.com/taiki-e/cargo-hack/pull/65
 [66]: https://github.com/taiki-e/cargo-hack/pull/66
+[73]: https://github.com/taiki-e/cargo-hack/pull/73
 
 ## [0.3.14] - 2020-10-10
 

src/cli.rs

@@ -233,6 +233,13 @@ pub(crate) struct Args<'a> {
     pub(crate) color: Option<Coloring>,
 }
 
+impl Args<'_> {
+    /// Return `true` if options that require information from cargo manifest is specified.
+    pub(crate) fn require_manifest_info(&self, version: u32) -> bool {
+        (version < 39 && self.ignore_private) || self.no_dev_deps || self.remove_dev_deps
+    }
+}
+
 #[derive(Clone, Copy)]
 pub(crate) enum Coloring {
     Auto,

src/context.rs

@@ -4,34 +4,52 @@ use crate::{
     cli::{self, Args, Coloring, RawArgs},
     manifest::Manifest,
     metadata::{Metadata, Package, PackageId},
-    ProcessBuilder, Result,
+    version, ProcessBuilder, Result,
 };
 
 pub(crate) struct Context<'a> {
     args: Args<'a>,
     metadata: Metadata,
     manifests: HashMap<PackageId, Manifest>,
     cargo: OsString,
+    cargo_version: u32,
 }
 
 impl<'a> Context<'a> {
     pub(crate) fn new(args: &'a RawArgs, coloring: &mut Option<Coloring>) -> Result<Self> {
         let cargo = cargo_binary();
+
+        // If failed to determine cargo version, assign 0 to skip all version-dependent decisions.
+        let cargo_version = match version::from_path(&cargo) {
+            Ok(version) => version.minor,
+            Err(e) => {
+                warn!(*coloring, "{}", e);
+                0
+            }
+        };
+
         let args = cli::perse_args(args, coloring, &cargo)?;
         assert!(
             args.subcommand.is_some() || args.remove_dev_deps,
             "no subcommand or valid flag specified"
         );
-        let metadata = Metadata::new(&args, &cargo)?;
+        let metadata = Metadata::new(&args, &cargo, cargo_version)?;
 
-        let mut manifests = HashMap::with_capacity(metadata.workspace_members.len());
-        for id in &metadata.workspace_members {
-            let manifest_path = &metadata.packages[id].manifest_path;
-            let manifest = Manifest::new(manifest_path)?;
-            manifests.insert(id.clone(), manifest);
-        }
+        // Only a few options require information from cargo manifest.
+        // If manifest information is not required, do not read and parse them.
+        let manifests = if args.require_manifest_info(cargo_version) {
+            let mut manifests = HashMap::with_capacity(metadata.workspace_members.len());
+            for id in &metadata.workspace_members {
+                let manifest_path = &metadata.packages[id].manifest_path;
+                let manifest = Manifest::new(manifest_path)?;
+                manifests.insert(id.clone(), manifest);
+            }
+            manifests
+        } else {
+            HashMap::new()
+        };
 
-        Ok(Self { args, metadata, manifests, cargo })
+        Ok(Self { args, metadata, manifests, cargo, cargo_version })
     }
 
     // Accessor methods.
@@ -44,15 +62,23 @@ impl<'a> Context<'a> {
     // pub(crate) fn nodes(&self, id: &PackageId) -> &Node {
     //     &self.metadata.resolve.nodes[id]
     // }
-    pub(crate) fn current_manifest(&self) -> Option<&PackageId> {
+    pub(crate) fn current_package(&self) -> Option<&PackageId> {
         self.metadata.resolve.root.as_ref()
     }
     pub(crate) fn workspace_root(&self) -> &Path {
         &self.metadata.workspace_root
     }
     pub(crate) fn manifests(&self, id: &PackageId) -> &Manifest {
+        debug_assert!(self.require_manifest_info(self.cargo_version));
         &self.manifests[id]
     }
+    pub(crate) fn is_private(&self, id: &PackageId) -> bool {
+        if self.cargo_version >= 39 {
+            !self.packages(id).publish
+        } else {
+            !self.manifests(id).publish
+        }
+    }
 
     pub(crate) fn process(&self) -> ProcessBuilder<'_> {
         let mut command = ProcessBuilder::new(&self.cargo);

src/main.rs

@@ -18,6 +18,7 @@ mod metadata;
 mod process;
 mod remove_dev_deps;
 mod restore;
+mod version;
 
 use anyhow::{bail, Context as _};
 use std::{fmt::Write, fs};
@@ -86,7 +87,7 @@ enum Kind<'a> {
 }
 
 fn determine_kind<'a>(cx: &'a Context<'_>, id: &PackageId, progress: &mut Progress) -> Kind<'a> {
-    if cx.ignore_private && cx.manifests(id).is_private() {
+    if cx.ignore_private && cx.is_private(id) {
         return Kind::SkipAsPrivate;
     }
     if cx.subcommand.is_none() {
@@ -185,10 +186,10 @@ fn determine_package_list<'a>(
             .filter(|id| cx.package.contains(&&*cx.packages(id).name))
             .map(|id| (id, determine_kind(cx, id, progress)))
             .collect()
-    } else if cx.current_manifest().is_none() {
+    } else if cx.current_package().is_none() {
         cx.workspace_members().map(|id| (id, determine_kind(cx, id, progress))).collect()
     } else {
-        let current_package = &cx.manifests(cx.current_manifest().unwrap()).package.name;
+        let current_package = &cx.packages(cx.current_package().unwrap()).name;
         cx.workspace_members()
             .find(|id| cx.packages(id).name == *current_package)
             .map(|id| vec![(id, determine_kind(cx, id, progress))])

src/manifest.rs

@@ -6,10 +6,14 @@ use crate::Result;
 
 type ParseResult<T> = Result<T, &'static str>;
 
+// Refs:
+// * https://github.com/rust-lang/cargo/blob/0.47.0/src/cargo/util/toml/mod.rs
+// * https://gitlab.com/crates.rs/cargo_toml
+
 pub(crate) struct Manifest {
     pub(crate) raw: String,
-    // parsed manifest
-    pub(crate) package: Package,
+    // `metadata.package.publish` requires Rust 1.39
+    pub(crate) publish: bool,
 }
 
 impl Manifest {
@@ -18,78 +22,34 @@ impl Manifest {
             .with_context(|| format!("failed to read manifest from {}", path.display()))?;
         let toml = toml::from_str(&raw)
             .with_context(|| format!("failed to parse manifest as toml: {}", path.display()))?;
-        let package = Package::from_table(toml).map_err(|s| {
+        let package = Package::from_table(&toml).map_err(|s| {
             format_err!("failed to parse `{}` field from manifest: {}", s, path.display())
         })?;
-        Ok(Self { raw, package })
-    }
-
-    // `metadata.package.publish` requires Rust 1.39
-    pub(crate) fn is_private(&self) -> bool {
-        self.package.publish == false
+        Ok(Self { raw, publish: package.publish })
     }
 
     pub(crate) fn remove_dev_deps(&self) -> String {
         super::remove_dev_deps::remove_dev_deps(&self.raw)
     }
 }
 
-// Refs:
-// * https://github.com/rust-lang/cargo/blob/0.47.0/src/cargo/util/toml/mod.rs
-// * https://gitlab.com/crates.rs/cargo_toml
-
-pub(crate) struct Package {
-    pub(crate) name: String,
-    pub(crate) publish: Publish,
+struct Package {
+    publish: bool,
 }
 
 impl Package {
-    fn from_table(mut table: Table) -> ParseResult<Self> {
-        let package = table.get_mut("package").and_then(Value::as_table_mut).ok_or("package")?;
-        let name = package.remove("name").and_then(into_string).ok_or("name")?;
-        let publish = Publish::from_value(package.get("publish")).ok_or("publish")?;
-
-        Ok(Self { name, publish })
-    }
-}
-
-pub(crate) enum Publish {
-    Flag(bool),
-    Registry { is_empty: bool },
-}
-
-impl Publish {
-    fn from_value(value: Option<&Value>) -> Option<Self> {
-        Some(match value {
-            None => Self::default(),
-            Some(Value::Array(a)) => Publish::Registry { is_empty: a.is_empty() },
-            Some(Value::Boolean(b)) => Publish::Flag(*b),
-            Some(_) => return None,
+    fn from_table(table: &Table) -> ParseResult<Self> {
+        let package = table.get("package").and_then(Value::as_table).ok_or("package")?;
+        let _name = package.get("name").and_then(Value::as_str).ok_or("name")?;
+
+        Ok(Self {
+            // Publishing is unrestricted if `true`, and forbidden if `false` or the `Array` is empty.
+            publish: match package.get("publish") {
+                None => true,
+                Some(Value::Boolean(b)) => *b,
+                Some(Value::Array(a)) => !a.is_empty(),
+                Some(_) => return Err("publish"),
+            },
         })
     }
 }
-
-impl Default for Publish {
-    fn default() -> Self {
-        Publish::Flag(true)
-    }
-}
-
-impl PartialEq<Publish> for bool {
-    fn eq(&self, p: &Publish) -> bool {
-        match *p {
-            Publish::Flag(flag) => flag == *self,
-            Publish::Registry { is_empty } => is_empty != *self,
-        }
-    }
-}
-
-impl PartialEq<bool> for Publish {
-    fn eq(&self, b: &bool) -> bool {
-        b.eq(self)
-    }
-}
-
-fn into_string(value: Value) -> Option<String> {
-    if let Value::String(string) = value { Some(string) } else { None }
-}