From 39f175be0410537eb22516685d679dcfff1dc6af Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 4 Dec 2022 02:25:41 +1100 Subject: [PATCH] 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 --- crates/bin/src/entry.rs | 85 ++++++++++++---- .../binstalk-manifests/src/cargo_crates_v1.rs | 99 ++++++++++++++++++- crates/binstalk-manifests/src/lib.rs | 3 + 3 files changed, 164 insertions(+), 23 deletions(-) diff --git a/crates/bin/src/entry.rs b/crates/bin/src/entry.rs index 48a7161d..be51cf0f 100644 --- a/crates/bin/src/entry.rs +++ b/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); + /// Return (install_path, cargo_roots, metadata, temp_dir) fn compute_paths_and_load_manifests( roots: Option, install_path: Option, -) -> Result<(PathBuf, PathBuf, Option, tempfile::TempDir)> { +) -> Result<(PathBuf, PathBuf, Option, 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()); - Some(Records::load_from_path(&manifest_path)?) + debug!( + "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, force: bool, - metadata: Option<&Records>, + metadata: Option<&Metadata>, ) -> impl Iterator)> + '_ { 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)) - if version_req.is_latest_compatible(&metadata.current_version) => + (false, Some(curr_version), Some(version_req)) + if version_req.is_latest_compatible(curr_version) => { debug!("Bailing out early because we can assume wanted is already installed from metafile"); - info!( - "{} v{} is already installed, use --force to override", - crate_name.name, metadata.current_version - ); + info!("{name} v{curr_version} is already installed, use --force to override"); None } - // we have to assume that the version req could be *, - // and therefore a remote upgraded version could exist - (false, Some(metadata), _) => { - Some((crate_name, Some(metadata.current_version.clone()))) + // The version req is "*" thus a remote upgraded version could exist + (false, Some(curr_version), None) => { + Some((crate_name, Some(curr_version.clone()))) } _ => Some((crate_name, None)), diff --git a/crates/binstalk-manifests/src/cargo_crates_v1.rs b/crates/binstalk-manifests/src/cargo_crates_v1.rs index 8d9033fa..6db43bd1 100644 --- a/crates/binstalk-manifests/src/cargo_crates_v1.rs +++ b/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>, + #[serde(with = "tuple_vec_map")] + v1: Vec<(String, Vec)>, } impl CratesToml { @@ -50,12 +53,22 @@ impl CratesToml { } pub fn load_from_path(path: impl AsRef) -> Result { - 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) { - 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, 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(); } } diff --git a/crates/binstalk-manifests/src/lib.rs b/crates/binstalk-manifests/src/lib.rs index a87d5bcc..418764f6 100644 --- a/crates/binstalk-manifests/src/lib.rs +++ b/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;