Feature: Check for installed crates in cargo_install_v1_manifest (#582)

* Add & Impl new fn `CratesToml::collect_into_crates_versions`
   to iterate over crates listed in cargo_crates_v1, accessing their names
   and versions.
* Re-export `CompactString`, `Version` & `Url` in binstalk-manifests
   for convenience
* Fix `CratesToml::load_from_path`: Wrap `File` in `FileLock::new_shared`
   to avoid concurrent write while reading the file.
* Filter out installed crates in cargo_install_v1_metadata
* Make match in `filter_out_installed_crates` more explicit
* Add new test `cargo_crates_v1::test::test_loading`
* Optimize `CratesToml`: Use `Vec` instead of `BTreeMap`
  since we cannot simply call `BTreeMap::get` to find an entry for a crate
  anyway.
  This also accidentally fixed the CI.
* Impl new API `CratesToml::remove`
* Fix`CratesToml::append_to_path` by removing previous records of
   the crates that are just updated.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

crates/bin/src/entry.rs

@@ -1,4 +1,4 @@
-use std::{fs, path::PathBuf, sync::Arc, time::Duration};
+use std::{collections::BTreeMap, fs, io, path::PathBuf, sync::Arc, time::Duration};
 
 use binstalk::{
     errors::BinstallError,
@@ -12,7 +12,10 @@ use binstalk::{
     },
 };
 use binstalk_manifests::{
-    binstall_crates_v1::Records, cargo_crates_v1::CratesToml, cargo_toml_binstall::PkgOverride,
+    binstall_crates_v1::Records as BinstallCratesV1Records,
+    cargo_crates_v1::{CratesToml, CratesTomlParseError},
+    cargo_toml_binstall::PkgOverride,
+    CompactString, Version,
 };
 use crates_io_api::AsyncClient as CratesIoApiClient;
 use log::LevelFilter;
@@ -167,7 +170,7 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -
     }
 
     block_in_place(|| {
-        if let Some(mut records) = metadata {
+        if let Some((mut cargo_binstall_metadata, _)) = metadata {
             // The cargo manifest path is already created when loading
             // metadata.
 
@@ -176,9 +179,9 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -
 
             debug!("Writing binstall/crates-v1.json");
             for metadata in metadata_vec {
-                records.replace(metadata);
+                cargo_binstall_metadata.replace(metadata);
             }
-            records.overwrite()?;
+            cargo_binstall_metadata.overwrite()?;
         }
 
         if no_cleanup {
@@ -194,11 +197,13 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -
     })
 }
 
+type Metadata = (BinstallCratesV1Records, BTreeMap<CompactString, Version>);
+
 /// Return (install_path, cargo_roots, metadata, temp_dir)
 fn compute_paths_and_load_manifests(
     roots: Option<PathBuf>,
     install_path: Option<PathBuf>,
-) -> Result<(PathBuf, PathBuf, Option<Records>, tempfile::TempDir)> {
+) -> Result<(PathBuf, PathBuf, Option<Metadata>, tempfile::TempDir)> {
     block_in_place(|| {
         // Compute cargo_roots
         let cargo_roots = install_path::get_cargo_roots_path(roots).ok_or_else(|| {
@@ -218,12 +223,40 @@ fn compute_paths_and_load_manifests(
 
         // Load metadata
         let metadata = if !custom_install_path {
+            // Read cargo_binstall_metadata
             let metadata_dir = cargo_roots.join("binstall");
             fs::create_dir_all(&metadata_dir).map_err(BinstallError::Io)?;
             let manifest_path = metadata_dir.join("crates-v1.json");
 
-            debug!("Reading {}", manifest_path.display());
+            debug!(
-            Some(Records::load_from_path(&manifest_path)?)
+                "Reading {} from {}",
+                "cargo_binstall_metadata",
+                manifest_path.display()
+            );
+
+            let cargo_binstall_metadata = BinstallCratesV1Records::load_from_path(&manifest_path)?;
+
+            // Read cargo_install_v1_metadata
+            let manifest_path = cargo_roots.join(".crates.toml");
+
+            debug!(
+                "Reading {} from {}",
+                "cargo_install_v1_metadata",
+                manifest_path.display()
+            );
+
+            let cargo_install_v1_metadata = match CratesToml::load_from_path(&manifest_path) {
+                Ok(metadata) => metadata.collect_into_crates_versions()?,
+                Err(CratesTomlParseError::Io(io_err))
+                    if io_err.kind() == io::ErrorKind::NotFound =>
+                {
+                    // .crates.toml does not exist, create an empty BTreeMap
+                    Default::default()
+                }
+                Err(err) => Err(err)?,
+            };
+
+            Some((cargo_binstall_metadata, cargo_install_v1_metadata))
         } else {
             None
         };
@@ -247,30 +280,42 @@ fn compute_paths_and_load_manifests(
 fn filter_out_installed_crates(
     crate_names: Vec<CrateName>,
     force: bool,
-    metadata: Option<&Records>,
+    metadata: Option<&Metadata>,
 ) -> impl Iterator<Item = (CrateName, Option<semver::Version>)> + '_ {
     CrateName::dedup(crate_names)
     .filter_map(move |crate_name| {
+        let name = &crate_name.name;
+
+        let curr_version = metadata
+            .and_then(|metadata| {
+                metadata
+                    .0
+                    .get(name)
+                    .map(|crate_info| &crate_info.current_version)
+                    .into_iter()
+                    .chain(metadata.1.get(name))
+                    // Since the cargo_install_v1_metadata could be out of sync
+                    // from cargo_binstall_metadata, it is better to obtain
+                    // the version from both of them and takes the larger one.
+                    .max()
+            });
+
         match (
             force,
-            metadata.and_then(|records| records.get(&crate_name.name)),
+            curr_version,
             &crate_name.version_req,
         ) {
-            (false, Some(metadata), Some(version_req))
+            (false, Some(curr_version), Some(version_req))
-                if version_req.is_latest_compatible(&metadata.current_version) =>
+                if version_req.is_latest_compatible(curr_version) =>
             {
                 debug!("Bailing out early because we can assume wanted is already installed from metafile");
-                info!(
+                info!("{name} v{curr_version} is already installed, use --force to override");
-                    "{} v{} is already installed, use --force to override",
-                    crate_name.name, metadata.current_version
-                );
                 None
             }
 
-            // we have to assume that the version req could be *,
+            // The version req is "*" thus a remote upgraded version could exist
-            // and therefore a remote upgraded version could exist
+            (false, Some(curr_version), None) => {
-            (false, Some(metadata), _) => {
+                Some((crate_name, Some(curr_version.clone())))
-                Some((crate_name, Some(metadata.current_version.clone())))
             }
 
             _ => Some((crate_name, None)),

crates/binstalk-manifests/src/cargo_crates_v1.rs

@@ -13,12 +13,14 @@ use std::{
     io::{self, Seek},
     iter::IntoIterator,
     path::{Path, PathBuf},
+    str::FromStr,
 };
 
 use compact_str::CompactString;
 use fs_lock::FileLock;
 use home::cargo_home;
 use miette::Diagnostic;
+use semver::Version;
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
 
@@ -31,7 +33,8 @@ use crate_version_source::*;
 
 #[derive(Clone, Debug, Default, Deserialize, Serialize)]
 pub struct CratesToml {
-    v1: BTreeMap<String, Vec<CompactString>>,
+    #[serde(with = "tuple_vec_map")]
+    v1: Vec<(String, Vec<CompactString>)>,
 }
 
 impl CratesToml {
@@ -50,12 +53,22 @@ impl CratesToml {
     }
 
     pub fn load_from_path(path: impl AsRef<Path>) -> Result<Self, CratesTomlParseError> {
-        let file = File::open(path)?;
+        let file = FileLock::new_shared(File::open(path)?)?;
         Self::load_from_reader(file)
     }
 
+    /// Only use it when you know that the crate is not in the manifest.
+    /// Otherwise, you need to call [`CratesToml::remove`] first.
     pub fn insert(&mut self, cvs: &CrateVersionSource, bins: Vec<CompactString>) {
-        self.v1.insert(cvs.to_string(), bins);
+        self.v1.push((cvs.to_string(), bins));
+    }
+
+    pub fn remove(&mut self, name: &str) {
+        self.v1.retain(|(s, _bin)| {
+            s.split_once(' ')
+                .map(|(crate_name, _rest)| crate_name != name)
+                .unwrap_or_default()
+        });
     }
 
     pub fn write(&self) -> Result<(), CratesTomlParseError> {
@@ -96,6 +109,7 @@ impl CratesToml {
         };
 
         for metadata in iter {
+            c1.remove(&metadata.name);
             c1.insert(&CrateVersionSource::from(metadata), metadata.bins.clone());
         }
 
@@ -111,6 +125,20 @@ impl CratesToml {
     {
         Self::append_to_path(Self::default_path()?, iter)
     }
+
+    /// Return BTreeMap with crate name as key and its corresponding version
+    /// as value.
+    pub fn collect_into_crates_versions(
+        self,
+    ) -> Result<BTreeMap<CompactString, Version>, CratesTomlParseError> {
+        self.v1
+            .into_iter()
+            .map(|(s, _bins)| {
+                let cvs = CrateVersionSource::from_str(&s)?;
+                Ok((cvs.name, cvs.version))
+            })
+            .collect()
+    }
 }
 
 #[derive(Debug, Diagnostic, Error)]
@@ -167,5 +195,70 @@ mod tests {
             }],
         )
         .unwrap();
+
+        let crates = CratesToml::load_from_path(&path)
+            .unwrap()
+            .collect_into_crates_versions()
+            .unwrap();
+
+        assert_eq!(crates.len(), 1);
+
+        assert_eq!(
+            crates.get("cargo-binstall").unwrap(),
+            &Version::new(0, 11, 1)
+        );
+
+        // Update
+        CratesToml::append_to_path(
+            &path,
+            &[CrateInfo {
+                name: "cargo-binstall".into(),
+                version_req: "*".into(),
+                current_version: Version::new(0, 12, 0),
+                source: CrateSource::cratesio_registry(),
+                target: TARGET.into(),
+                bins: vec!["cargo-binstall".into()],
+            }],
+        )
+        .unwrap();
+
+        let crates = CratesToml::load_from_path(&path)
+            .unwrap()
+            .collect_into_crates_versions()
+            .unwrap();
+
+        assert_eq!(crates.len(), 1);
+
+        assert_eq!(
+            crates.get("cargo-binstall").unwrap(),
+            &Version::new(0, 12, 0)
+        );
+    }
+
+    #[test]
+    fn test_loading() {
+        let raw_data = br#"
+[v1]
+"alacritty 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)" = ["alacritty"]
+"cargo-audit 0.17.0 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-audit"]
+"cargo-binstall 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-binstall"]
+"cargo-criterion 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-criterion"]
+"cargo-edit 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-add", "cargo-rm", "cargo-set-version", "cargo-upgrade"]
+"cargo-expand 1.0.27 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-expand"]
+"cargo-geiger 0.11.3 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-geiger"]
+"cargo-hack 0.5.15 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-hack"]
+"cargo-nextest 0.9.26 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-nextest"]
+"cargo-supply-chain 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-supply-chain"]
+"cargo-tarpaulin 0.20.1 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-tarpaulin"]
+"cargo-update 8.1.4 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-install-update", "cargo-install-update-config"]
+"cargo-watch 8.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-watch"]
+"cargo-with 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)" = ["cargo-with"]
+"cross 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)" = ["cross", "cross-util"]
+"irust 1.63.3 (registry+https://github.com/rust-lang/crates.io-index)" = ["irust"]
+"tokei 12.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = ["tokei"]
+"xargo 0.3.26 (registry+https://github.com/rust-lang/crates.io-index)" = ["xargo", "xargo-check"]
+        "#;
+
+        CratesToml::load_from_reader(raw_data.as_slice()).unwrap();
     }
 }

crates/binstalk-manifests/src/lib.rs

@@ -14,3 +14,6 @@ pub mod binstall_crates_v1;
 pub mod cargo_crates_v1;
 
 pub use binstalk_types::{cargo_toml_binstall, crate_info};
+pub use compact_str::CompactString;
+pub use semver::Version;
+pub use url::Url;