From e8b03443d61b73d32e215d27ae4754da269f3581 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Fri, 20 Mar 2020 13:40:59 +0000 Subject: [PATCH 01/14] Update to semver forked from newer master --- Cargo.toml | 2 +- crates/resolver-tests/src/lib.rs | 4 ++-- tests/testsuite/build.rs | 4 ++-- tests/testsuite/registry.rs | 6 +++--- tests/testsuite/replace.rs | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d5a15a223ae..fec07d145e5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,7 +53,7 @@ percent-encoding = "2.0" remove_dir_all = "0.5.2" rustfix = "0.5.0" same-file = "1" -semver = { version = "0.9.0", features = ["serde"] } +semver = { git = "https://github.com/illicitonion/semver.git", rev = "f1f912703f67ed63b751c525839731a90239bdcf", features = ["serde"] } serde = { version = "1.0.82", features = ["derive"] } serde_ignored = "0.1.0" serde_json = { version = "1.0.30", features = ["raw_value"] } diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index 1f37b18da77..ecd5e172fb5 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -738,8 +738,8 @@ fn meta_test_deep_pretty_print_registry() { "vec![pkg!((\"foo\", \"1.0.1\") => [dep_req(\"bar\", \"^1\"),]),\ pkg!((\"foo\", \"1.0.0\") => [dep_req(\"bar\", \"^2\"),]),\ pkg!((\"foo\", \"2.0.0\") => [dep(\"bar\"),]),\ - pkg!((\"bar\", \"1.0.0\") => [dep_req(\"baz\", \"= 1.0.2\"),dep_req(\"other\", \"^1\"),]),\ - pkg!((\"bar\", \"2.0.0\") => [dep_req(\"baz\", \"= 1.0.1\"),]),\ + pkg!((\"bar\", \"1.0.0\") => [dep_req(\"baz\", \"=1.0.2\"),dep_req(\"other\", \"^1\"),]),\ + pkg!((\"bar\", \"2.0.0\") => [dep_req(\"baz\", \"=1.0.1\"),]),\ pkg!((\"baz\", \"1.0.2\") => [dep_req(\"other\", \"^2\"),]),\ pkg!((\"baz\", \"1.0.1\")),\ pkg!((\"cat\", \"1.0.2\") => [dep_req_kind(\"other\", \"^2\", DepKind::Build, false),]),\ diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index aca629ebdf0..4aa4d759109 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -993,7 +993,7 @@ fn incompatible_dependencies() { error: failed to select a version for `bad`. ... required by package `qux v0.1.0` ... which is depended on by `foo v0.0.1 ([..])` -versions that meet the requirements `>= 1.0.1` are: 1.0.2, 1.0.1 +versions that meet the requirements `>=1.0.1` are: 1.0.2, 1.0.1 all possible versions conflict with previously selected packages. @@ -1038,7 +1038,7 @@ fn incompatible_dependencies_with_multi_semver() { "\ error: failed to select a version for `bad`. ... required by package `foo v0.0.1 ([..])` -versions that meet the requirements `>= 1.0.1, <= 2.0.0` are: 2.0.0, 1.0.1 +versions that meet the requirements `>=1.0.1, <=2.0.0` are: 2.0.0, 1.0.1 all possible versions conflict with previously selected packages. diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 297884544d2..d26db0f0812 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -225,7 +225,7 @@ fn wrong_version() { .with_status(101) .with_stderr_contains( "\ -error: failed to select a version for the requirement `foo = \">= 1.0.0\"` +error: failed to select a version for the requirement `foo = \">=1.0.0\"` candidate versions found which didn't match: 0.0.2, 0.0.1 location searched: `[..]` index (which is replacing registry `[..]`) required by package `foo v0.0.1 ([..])` @@ -240,7 +240,7 @@ required by package `foo v0.0.1 ([..])` .with_status(101) .with_stderr_contains( "\ -error: failed to select a version for the requirement `foo = \">= 1.0.0\"` +error: failed to select a version for the requirement `foo = \">=1.0.0\"` candidate versions found which didn't match: 0.0.4, 0.0.3, 0.0.2, ... location searched: `[..]` index (which is replacing registry `[..]`) required by package `foo v0.0.1 ([..])` @@ -540,7 +540,7 @@ fn relying_on_a_yank_is_bad() { .with_status(101) .with_stderr_contains( "\ -error: failed to select a version for the requirement `baz = \"= 0.0.2\"` +error: failed to select a version for the requirement `baz = \"=0.0.2\"` candidate versions found which didn't match: 0.0.1 location searched: `[..]` index (which is replacing registry `[..]`) required by package `bar v0.0.1` diff --git a/tests/testsuite/replace.rs b/tests/testsuite/replace.rs index 17149fa6d93..9672c462974 100644 --- a/tests/testsuite/replace.rs +++ b/tests/testsuite/replace.rs @@ -549,7 +549,7 @@ fn override_wrong_name() { Caused by: no matching package for override `[..]baz:0.1.0` found location searched: file://[..] -version required: = 0.1.0 +version required: =0.1.0 ", ) .run(); From a721e6278182f79ba97eb114061be05cf766c9c3 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 19 Mar 2020 23:29:31 +0000 Subject: [PATCH 02/14] Try installing exact versions before updating When an exact version is being installed, if we already have that version from the index, we don't need to update the index before installing it. Don't do this if it's not an exact version, because the update may find us a newer version. This is particularly useful for scripts which unconditionally run `cargo install some-crate --version=1.2.3`. Before install-update, I wrote a crate to do this (https://crates.io/crates/cargo-ensure-installed) which I'm trying to replace with just `cargo install`, but the extra latency of updating the index for a no-op is noticeable. This introduces an interesting edge-case around yanked crates; the yanked-ness of crates will no longer change on install for exact version matches which were already downloaded. This feels niche enough to hopefully not matter... --- src/cargo/ops/cargo_install.rs | 10 +++++-- src/cargo/ops/cargo_uninstall.rs | 2 +- .../ops/common_for_install_and_uninstall.rs | 28 ++++++++++++++++--- tests/testsuite/install_upgrade.rs | 3 +- 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 0ff2edbc6b2..b2759366efe 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -151,7 +151,7 @@ fn install_one( krate, vers, config, - true, + NeedsUpdate::True, &mut |git| git.read_packages(), )? } else if source_id.is_path() { @@ -180,7 +180,7 @@ fn install_one( } } src.update()?; - select_pkg(src, krate, vers, config, false, &mut |path| { + select_pkg(src, krate, vers, config, NeedsUpdate::False, &mut |path| { path.read_packages() })? } else { @@ -189,7 +189,11 @@ fn install_one( krate, vers, config, - is_first_install, + if is_first_install { + NeedsUpdate::TryWithoutFirst + } else { + NeedsUpdate::False + }, &mut |_| { bail!( "must specify a crate to install from \ diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index 2d69eabd023..612db0129a2 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -85,7 +85,7 @@ fn uninstall_cwd(root: &Filesystem, bins: &[String], config: &Config) -> CargoRe let tracker = InstallTracker::load(config, root)?; let source_id = SourceId::for_path(config.cwd())?; let src = path_source(source_id, config)?; - let pkg = select_pkg(src, None, None, config, true, &mut |path| { + let pkg = select_pkg(src, None, None, config, NeedsUpdate::True, &mut |path| { path.read_packages() })?; let pkgid = pkg.package_id(); diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index aa5c4ee18a3..b61ebc5c8da 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -525,13 +525,20 @@ pub fn path_source(source_id: SourceId, config: &Config) -> CargoResult( mut source: T, name: Option<&str>, vers: Option<&str>, config: &Config, - needs_update: bool, + needs_update: NeedsUpdate, list_all: &mut dyn FnMut(&mut T) -> CargoResult>, ) -> CargoResult where @@ -542,7 +549,7 @@ where // with other global Cargos let _lock = config.acquire_package_cache_lock()?; - if needs_update { + if let NeedsUpdate::True = needs_update { source.update()?; } @@ -603,8 +610,21 @@ where vers }; let dep = Dependency::parse_no_deprecated(name, vers_spec, source.source_id())?; - let deps = source.query_vec(&dep)?; - match deps.iter().map(|p| p.package_id()).max() { + let deps = (|| { + if let Some(vers_spec) = vers_spec { + if semver::VersionReq::parse(vers_spec).unwrap().is_exact() { + let deps = source.query_vec(&dep)?; + if deps.len() == 1 { + return Ok(deps); + } + } + } + if needs_update != NeedsUpdate::False { + source.update()?; + } + source.query_vec(&dep) + })(); + match deps?.iter().map(|p| p.package_id()).max() { Some(pkgid) => { let pkg = Box::new(&mut source).download_now(pkgid, config)?; Ok(pkg) diff --git a/tests/testsuite/install_upgrade.rs b/tests/testsuite/install_upgrade.rs index ee69bd85b71..3d51ad62503 100644 --- a/tests/testsuite/install_upgrade.rs +++ b/tests/testsuite/install_upgrade.rs @@ -225,8 +225,7 @@ fn ambiguous_version_no_longer_allowed() { cargo_process("install foo --version=1.0") .with_stderr( "\ -[UPDATING] `[..]` index -[ERROR] the `--vers` provided, `1.0`, is not a valid semver version: cannot parse '1.0' as a semver +error: the `--vers` provided, `1.0`, is not a valid semver version: cannot parse '1.0' as a semver if you want to specify semver range, add an explicit qualifier, like ^1.0 ", From 137e518d98c8d1efae9c0251d93e7714de9da13b Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Fri, 20 Mar 2020 23:02:52 +0000 Subject: [PATCH 03/14] Explicitly check whether installed first instead --- src/cargo/ops/cargo_install.rs | 171 ++++++++++++++---- src/cargo/ops/cargo_uninstall.rs | 4 +- .../ops/common_for_install_and_uninstall.rs | 96 ++-------- tests/testsuite/install.rs | 4 +- tests/testsuite/install_upgrade.rs | 87 ++++++++- 5 files changed, 231 insertions(+), 131 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index b2759366efe..ab989533150 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -14,7 +14,8 @@ use crate::ops; use crate::ops::common_for_install_and_uninstall::*; use crate::sources::{GitSource, SourceConfigMap}; use crate::util::errors::{CargoResult, CargoResultExt}; -use crate::util::{paths, Config, Filesystem}; +use crate::util::{paths, Config, Filesystem, Rustc, ToSemver}; +use semver::VersionReq; struct Transaction { bins: Vec, @@ -145,13 +146,22 @@ fn install_one( ) -> CargoResult<()> { let config = opts.config; + let dst = root.join("bin").into_path_unlocked(); + + let is_installed = |pkg: &Package, rustc: &Rustc, target: &str| -> CargoResult { + let tracker = InstallTracker::load(config, root)?; + let (freshness, _duplicates) = + tracker.check_upgrade(&dst, pkg, force, opts, target, &rustc.verbose_version)?; + Ok(freshness == Freshness::Fresh) + }; + let pkg = if source_id.is_git() { select_pkg( - GitSource::new(source_id, config)?, + &mut GitSource::new(source_id, config)?, krate, - vers, + None, config, - NeedsUpdate::True, + true, &mut |git| git.read_packages(), )? } else if source_id.is_path() { @@ -180,20 +190,48 @@ fn install_one( } } src.update()?; - select_pkg(src, krate, vers, config, NeedsUpdate::False, &mut |path| { + select_pkg(&mut src, krate, None, config, false, &mut |path| { path.read_packages() })? } else { + let mut source = map.load(source_id, &HashSet::new())?; + let vers = if let Some(vers) = vers { + Some(parse_semver_req(vers)?) + } else if source.source_id().is_registry() { + Some(VersionReq::any()) + } else { + None + }; + if krate.is_some() && !no_track { + if let Some(vers) = vers.clone() { + if vers.is_exact() { + if let Ok(pkg) = + select_pkg(&mut source, krate, Some(vers), config, false, &mut |_| { + bail!("(Ignored)") + }) + { + let (_ws, rustc, target) = + make_ws_rustc_target(&config, opts, &source_id, pkg.clone())?; + if let Ok(installed) = is_installed(&pkg, &rustc, &target) { + if installed { + let msg = format!( + "package `{}` is already installed, use --force to override", + pkg + ); + config.shell().status("Ignored", &msg)?; + return Ok(()); + } + } + } + } + } + } select_pkg( - map.load(source_id, &HashSet::new())?, + &mut source, krate, vers, config, - if is_first_install { - NeedsUpdate::TryWithoutFirst - } else { - NeedsUpdate::False - }, + is_first_install, &mut |_| { bail!( "must specify a crate to install from \ @@ -204,17 +242,12 @@ fn install_one( )? }; - let (mut ws, git_package) = if source_id.is_git() { - // Don't use ws.current() in order to keep the package source as a git source so that - // install tracking uses the correct source. - (Workspace::new(pkg.manifest_path(), config)?, Some(&pkg)) - } else if source_id.is_path() { - (Workspace::new(pkg.manifest_path(), config)?, None) + let git_package = if source_id.is_git() { + Some(pkg.clone()) } else { - (Workspace::ephemeral(pkg, config, None, false)?, None) + None }; - ws.set_ignore_lock(config.lock_update_allowed()); - ws.set_require_optional_deps(false); + let (mut ws, rustc, target) = make_ws_rustc_target(config, opts, &source_id, pkg)?; let mut td_opt = None; let mut needs_cleanup = false; @@ -232,7 +265,9 @@ fn install_one( ws.set_target_dir(target_dir); } - let pkg = git_package.map_or_else(|| ws.current(), |pkg| Ok(pkg))?; + let pkg = git_package + .as_ref() + .map_or_else(|| ws.current(), |pkg| Ok(pkg))?; if from_cwd { if pkg.manifest().edition() == Edition::Edition2015 { @@ -259,16 +294,6 @@ fn install_one( bail!("specified package `{}` has no binaries", pkg); } - // Preflight checks to check up front whether we'll overwrite something. - // We have to check this again afterwards, but may as well avoid building - // anything if we're gonna throw it away anyway. - let dst = root.join("bin").into_path_unlocked(); - let rustc = config.load_global_rustc(Some(&ws))?; - let target = match &opts.build_config.requested_kind { - CompileKind::Host => rustc.host.as_str(), - CompileKind::Target(target) => target.short_name(), - }; - // Helper for --no-track flag to make sure it doesn't overwrite anything. let no_track_duplicates = || -> CargoResult>> { let duplicates: BTreeMap> = exe_names(pkg, &opts.filter) @@ -293,10 +318,7 @@ fn install_one( // Check for conflicts. no_track_duplicates()?; } else { - let tracker = InstallTracker::load(config, root)?; - let (freshness, _duplicates) = - tracker.check_upgrade(&dst, pkg, force, opts, target, &rustc.verbose_version)?; - if freshness == Freshness::Fresh { + if is_installed(&pkg, &rustc, &target)? { let msg = format!( "package `{}` is already installed, use --force to override", pkg @@ -304,8 +326,6 @@ fn install_one( config.shell().status("Ignored", &msg)?; return Ok(()); } - // Unlock while building. - drop(tracker); } config.shell().status("Installing", pkg)?; @@ -349,7 +369,7 @@ fn install_one( } else { let tracker = InstallTracker::load(config, root)?; let (_freshness, duplicates) = - tracker.check_upgrade(&dst, pkg, force, opts, target, &rustc.verbose_version)?; + tracker.check_upgrade(&dst, pkg, force, opts, &target, &rustc.verbose_version)?; (Some(tracker), duplicates) }; @@ -416,7 +436,7 @@ fn install_one( &successful_bins, vers.map(|s| s.to_string()), opts, - target, + &target, &rustc.verbose_version, ); @@ -491,6 +511,79 @@ fn install_one( } } +fn make_ws_rustc_target<'cfg>( + config: &'cfg Config, + opts: &ops::CompileOptions<'_>, + source_id: &SourceId, + pkg: Package, +) -> CargoResult<(Workspace<'cfg>, Rustc, String)> { + let mut ws = if source_id.is_git() { + // Don't use ws.current() in order to keep the package source as a git source so that + // install tracking uses the correct source. + Workspace::new(pkg.manifest_path(), config)? + } else if source_id.is_path() { + Workspace::new(pkg.manifest_path(), config)? + } else { + Workspace::ephemeral(pkg, config, None, false)? + }; + ws.set_ignore_lock(config.lock_update_allowed()); + ws.set_require_optional_deps(false); + + let rustc = config.load_global_rustc(Some(&ws))?; + let target = match &opts.build_config.requested_kind { + CompileKind::Host => rustc.host.as_str().to_owned(), + CompileKind::Target(target) => target.short_name().to_owned(), + }; + + Ok((ws, rustc, target)) +} + +fn parse_semver_req(v: &str) -> CargoResult { + // If the version begins with character <, >, =, ^, ~ parse it as a + // version range, otherwise parse it as a specific version + let first = v + .chars() + .next() + .ok_or_else(|| format_err!("no version provided for the `--vers` flag"))?; + + let is_req = "<>=^~".contains(first) || v.contains('*'); + if is_req { + match v.parse::() { + Ok(v) => Ok(v), + Err(_) => bail!( + "the `--vers` provided, `{}`, is \ + not a valid semver version requirement\n\n\ + Please have a look at \ + https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html \ + for the correct format", + v + ), + } + } else { + match v.to_semver() { + Ok(v) => Ok(VersionReq::exact(&v)), + Err(e) => { + let mut msg = format!( + "the `--vers` provided, `{}`, is \ + not a valid semver version: {}\n", + v, e + ); + + // If it is not a valid version but it is a valid version + // requirement, add a note to the warning + if v.parse::().is_ok() { + msg.push_str(&format!( + "\nif you want to specify semver range, \ + add an explicit qualifier, like ^{}", + v + )); + } + bail!(msg); + } + } + } +} + fn check_yanked_install(ws: &Workspace<'_>) -> CargoResult<()> { if ws.ignore_lock() || !ws.root().join("Cargo.lock").exists() { return Ok(()); diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index 612db0129a2..37a4f227774 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -84,8 +84,8 @@ pub fn uninstall_one( fn uninstall_cwd(root: &Filesystem, bins: &[String], config: &Config) -> CargoResult<()> { let tracker = InstallTracker::load(config, root)?; let source_id = SourceId::for_path(config.cwd())?; - let src = path_source(source_id, config)?; - let pkg = select_pkg(src, None, None, config, NeedsUpdate::True, &mut |path| { + let mut src = path_source(source_id, config)?; + let pkg = select_pkg(&mut src, None, None, config, true, &mut |path| { path.read_packages() })?; let pkgid = pkg.package_id(); diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index b61ebc5c8da..f271acc2c99 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -13,7 +13,7 @@ use crate::core::{Dependency, Package, PackageId, Source, SourceId}; use crate::ops::{self, CompileFilter, CompileOptions}; use crate::sources::PathSource; use crate::util::errors::{CargoResult, CargoResultExt}; -use crate::util::{Config, ToSemver}; +use crate::util::Config; use crate::util::{FileLock, Filesystem}; /// On-disk tracking for which package installed which binary. @@ -525,20 +525,13 @@ pub fn path_source(source_id: SourceId, config: &Config) -> CargoResult( - mut source: T, + source: &mut T, name: Option<&str>, - vers: Option<&str>, + vers: Option, config: &Config, - needs_update: NeedsUpdate, + needs_update: bool, list_all: &mut dyn FnMut(&mut T) -> CargoResult>, ) -> CargoResult where @@ -549,84 +542,17 @@ where // with other global Cargos let _lock = config.acquire_package_cache_lock()?; - if let NeedsUpdate::True = needs_update { + if needs_update { source.update()?; } if let Some(name) = name { - let vers = if let Some(v) = vers { - // If the version begins with character <, >, =, ^, ~ parse it as a - // version range, otherwise parse it as a specific version - let first = v - .chars() - .next() - .ok_or_else(|| format_err!("no version provided for the `--vers` flag"))?; - - let is_req = "<>=^~".contains(first) || v.contains('*'); - if is_req { - match v.parse::() { - Ok(v) => Some(v.to_string()), - Err(_) => bail!( - "the `--vers` provided, `{}`, is \ - not a valid semver version requirement\n\n\ - Please have a look at \ - https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html \ - for the correct format", - v - ), - } - } else { - match v.to_semver() { - Ok(v) => Some(format!("={}", v)), - Err(e) => { - let mut msg = format!( - "the `--vers` provided, `{}`, is \ - not a valid semver version: {}\n", - v, e - ); - - // If it is not a valid version but it is a valid version - // requirement, add a note to the warning - if v.parse::().is_ok() { - msg.push_str(&format!( - "\nif you want to specify semver range, \ - add an explicit qualifier, like ^{}", - v - )); - } - bail!(msg); - } - } - } - } else { - None - }; - let vers = vers.as_deref(); - let vers_spec = if vers.is_none() && source.source_id().is_registry() { - // Avoid pre-release versions from crate.io - // unless explicitly asked for - Some("*") - } else { - vers - }; - let dep = Dependency::parse_no_deprecated(name, vers_spec, source.source_id())?; - let deps = (|| { - if let Some(vers_spec) = vers_spec { - if semver::VersionReq::parse(vers_spec).unwrap().is_exact() { - let deps = source.query_vec(&dep)?; - if deps.len() == 1 { - return Ok(deps); - } - } - } - if needs_update != NeedsUpdate::False { - source.update()?; - } - source.query_vec(&dep) - })(); - match deps?.iter().map(|p| p.package_id()).max() { + let vers = vers.map(|v| v.to_string()); + let dep = Dependency::parse_no_deprecated(name, vers.as_deref(), source.source_id())?; + let deps = source.query_vec(&dep)?; + match deps.iter().map(|p| p.package_id()).max() { Some(pkgid) => { - let pkg = Box::new(&mut source).download_now(pkgid, config)?; + let pkg = Box::new(source).download_now(pkgid, config)?; Ok(pkg) } None => { @@ -642,7 +568,7 @@ where } } } else { - let candidates = list_all(&mut source)?; + let candidates = list_all(source)?; let binaries = candidates .iter() .filter(|cand| cand.targets().iter().filter(|t| t.is_bin()).count() > 0); diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 6ec9e8c9a94..a137b7b92ba 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -73,7 +73,7 @@ fn multiple_pkgs() { [FINISHED] release [optimized] target(s) in [..] [INSTALLING] [CWD]/home/.cargo/bin/bar[EXE] [INSTALLED] package `bar v0.0.2` (executable `bar[EXE]`) -[ERROR] could not find `baz` in registry `[..]` +[ERROR] could not find `baz` in registry `[..]` with version `*` [SUMMARY] Successfully installed foo, bar! Failed to install baz (see error(s) above). [WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries [ERROR] some crates failed to install @@ -145,7 +145,7 @@ fn missing() { .with_stderr( "\ [UPDATING] [..] index -[ERROR] could not find `bar` in registry `[..]` +[ERROR] could not find `bar` in registry `[..]` with version `*` ", ) .run(); diff --git a/tests/testsuite/install_upgrade.rs b/tests/testsuite/install_upgrade.rs index 3d51ad62503..67bd43d1f56 100644 --- a/tests/testsuite/install_upgrade.rs +++ b/tests/testsuite/install_upgrade.rs @@ -14,9 +14,9 @@ use cargo_test_support::{ basic_manifest, cargo_process, cross_compile, execs, git, process, project, Execs, }; -// Helper for publishing a package. -fn pkg(name: &str, vers: &str) { +fn pkg_maybe_yanked(name: &str, vers: &str, yanked: bool) { Package::new(name, vers) + .yanked(yanked) .file( "src/main.rs", r#"fn main() { println!("{}", env!("CARGO_PKG_VERSION")) }"#, @@ -24,6 +24,11 @@ fn pkg(name: &str, vers: &str) { .publish(); } +// Helper for publishing a package. +fn pkg(name: &str, vers: &str) { + pkg_maybe_yanked(name, vers, false) +} + fn v1_path() -> PathBuf { cargo_home().join(".crates.toml") } @@ -225,7 +230,7 @@ fn ambiguous_version_no_longer_allowed() { cargo_process("install foo --version=1.0") .with_stderr( "\ -error: the `--vers` provided, `1.0`, is not a valid semver version: cannot parse '1.0' as a semver +[ERROR] the `--vers` provided, `1.0`, is not a valid semver version: cannot parse '1.0' as a semver if you want to specify semver range, add an explicit qualifier, like ^1.0 ", @@ -745,3 +750,79 @@ fn deletes_orphaned() { // 0.1.0 should not have any entries. validate_trackers("foo", "0.1.0", &[]); } + +#[cargo_test] +fn already_installed_exact_does_not_update() { + pkg("foo", "1.0.0"); + cargo_process("install foo --version=1.0.0").run(); + cargo_process("install foo --version=1.0.0") + .with_stderr( + "\ +[IGNORED] package `foo v1.0.0` is already installed[..] +[WARNING] be sure to add [..] +", + ) + .run(); + + cargo_process("install foo --version=>=1.0.0") + .with_stderr( + "\ +[UPDATING] `[..]` index +[IGNORED] package `foo v1.0.0` is already installed[..] +[WARNING] be sure to add [..] +", + ) + .run(); + pkg("foo", "1.0.1"); + cargo_process("install foo --version=>=1.0.0") + .with_stderr( + "\ +[UPDATING] `[..]` index +[DOWNLOADING] crates ... +[DOWNLOADED] foo v1.0.1 (registry [..]) +[INSTALLING] foo v1.0.1 +[COMPILING] foo v1.0.1 +[FINISHED] release [optimized] target(s) in [..] +[REPLACING] [CWD]/home/.cargo/bin/foo[EXE] +[REPLACED] package `foo v1.0.0` with `foo v1.0.1` (executable `foo[EXE]`) +[WARNING] be sure to add [..] +", + ) + .run(); +} + +#[cargo_test] +fn already_installed_updates_yank_status_on_upgrade() { + pkg("foo", "1.0.0"); + pkg_maybe_yanked("foo", "1.0.1", true); + cargo_process("install foo --version=1.0.0").run(); + + cargo_process("install foo --version=1.0.1") + .with_status(101) + .with_stderr( + "\ +[UPDATING] `[..]` index +[ERROR] could not find `foo` in registry `[..]` with version `=1.0.1` +", + ) + .run(); + + pkg_maybe_yanked("foo", "1.0.1", false); + + pkg("foo", "1.0.1"); + cargo_process("install foo --version=1.0.1") + .with_stderr( + "\ +[UPDATING] `[..]` index +[DOWNLOADING] crates ... +[DOWNLOADED] foo v1.0.1 (registry [..]) +[INSTALLING] foo v1.0.1 +[COMPILING] foo v1.0.1 +[FINISHED] release [optimized] target(s) in [..] +[REPLACING] [CWD]/home/.cargo/bin/foo[EXE] +[REPLACED] package `foo v1.0.0` with `foo v1.0.1` (executable `foo[EXE]`) +[WARNING] be sure to add [..] +", + ) + .run(); +} From 1e2c2331165f8fc6b21d04edb45af84ff5b30537 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sat, 21 Mar 2020 00:47:08 +0000 Subject: [PATCH 04/14] Fix up updating logic for multiple installs --- src/cargo/ops/cargo_install.rs | 29 ++++++++++++++++----------- tests/testsuite/install_upgrade.rs | 32 ++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 22b9ccd46ce..6b025740580 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -67,7 +67,7 @@ pub fn install( } else { let mut succeeded = vec![]; let mut failed = vec![]; - let mut first = true; + let mut did_update = false; for krate in krates { let root = root.clone(); let map = map.clone(); @@ -82,15 +82,19 @@ pub fn install( opts, force, no_track, - first, + !did_update, ) { - Ok(()) => succeeded.push(krate), + Ok(still_needs_update) => { + succeeded.push(krate); + did_update |= !still_needs_update; + } Err(e) => { crate::display_error(&e, &mut config.shell()); - failed.push(krate) + failed.push(krate); + // We assume an update was performed if we got an error. + did_update = true; } } - first = false; } let mut summary = vec![]; @@ -135,6 +139,7 @@ pub fn install( Ok(()) } +// Returns whether a subsequent call should attempt to update again. fn install_one( config: &Config, root: &Filesystem, @@ -146,8 +151,8 @@ fn install_one( opts: &ops::CompileOptions, force: bool, no_track: bool, - is_first_install: bool, -) -> CargoResult<()> { + needs_update_if_source_is_index: bool, +) -> CargoResult { let dst = root.join("bin").into_path_unlocked(); let is_installed = |pkg: &Package, rustc: &Rustc, target: &str| -> CargoResult { @@ -221,7 +226,7 @@ fn install_one( pkg ); config.shell().status("Ignored", &msg)?; - return Ok(()); + return Ok(true); } } } @@ -233,7 +238,7 @@ fn install_one( krate, vers, config, - is_first_install, + needs_update_if_source_is_index, &mut |_| { bail!( "must specify a crate to install from \ @@ -326,7 +331,7 @@ fn install_one( pkg ); config.shell().status("Ignored", &msg)?; - return Ok(()); + return Ok(false); } } @@ -484,7 +489,7 @@ fn install_one( "Installed", format!("package `{}` {}", pkg, executables(successful_bins.iter())), )?; - Ok(()) + Ok(false) } else { if !to_install.is_empty() { config.shell().status( @@ -509,7 +514,7 @@ fn install_one( ), )?; } - Ok(()) + Ok(false) } } diff --git a/tests/testsuite/install_upgrade.rs b/tests/testsuite/install_upgrade.rs index 67bd43d1f56..41f00087126 100644 --- a/tests/testsuite/install_upgrade.rs +++ b/tests/testsuite/install_upgrade.rs @@ -826,3 +826,35 @@ fn already_installed_updates_yank_status_on_upgrade() { ) .run(); } + +#[cargo_test] +fn partially_already_installed_does_one_update() { + pkg("foo", "1.0.0"); + cargo_process("install foo --version=1.0.0").run(); + pkg("bar", "1.0.0"); + pkg("baz", "1.0.0"); + cargo_process("install foo bar baz --version=1.0.0") + .with_stderr( + "\ +[IGNORED] package `foo v1.0.0` is already installed[..] +[UPDATING] `[..]` index +[DOWNLOADING] crates ... +[DOWNLOADED] bar v1.0.0 (registry [..]) +[INSTALLING] bar v1.0.0 +[COMPILING] bar v1.0.0 +[FINISHED] release [optimized] target(s) in [..] +[INSTALLING] [CWD]/home/.cargo/bin/bar[EXE] +[INSTALLED] package `bar v1.0.0` (executable `bar[EXE]`) +[DOWNLOADING] crates ... +[DOWNLOADED] baz v1.0.0 (registry [..]) +[INSTALLING] baz v1.0.0 +[COMPILING] baz v1.0.0 +[FINISHED] release [optimized] target(s) in [..] +[INSTALLING] [CWD]/home/.cargo/bin/baz[EXE] +[INSTALLED] package `baz v1.0.0` (executable `baz[EXE]`) +[SUMMARY] Successfully installed foo, bar, baz! +[WARNING] be sure to add [..] +", + ) + .run(); +} From 14e3e41ef39e765a4df4dd54442336bb2a8249ae Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sat, 28 Mar 2020 02:05:26 +0000 Subject: [PATCH 05/14] Review comments --- src/cargo/ops/cargo_install.rs | 107 ++++++++++++------ .../ops/common_for_install_and_uninstall.rs | 2 +- 2 files changed, 74 insertions(+), 35 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 6b025740580..9ca60ab221b 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -155,13 +155,6 @@ fn install_one( ) -> CargoResult { let dst = root.join("bin").into_path_unlocked(); - let is_installed = |pkg: &Package, rustc: &Rustc, target: &str| -> CargoResult { - let tracker = InstallTracker::load(config, root)?; - let (freshness, _duplicates) = - tracker.check_upgrade(&dst, pkg, force, opts, target, &rustc.verbose_version)?; - Ok(freshness == Freshness::Fresh) - }; - let pkg = if source_id.is_git() { select_pkg( &mut GitSource::new(source_id, config)?, @@ -203,40 +196,27 @@ fn install_one( } else { let mut source = map.load(source_id, &HashSet::new())?; let vers = if let Some(vers) = vers { - Some(parse_semver_req(vers)?) + let vers = parse_semver_req(vers)?; + if let Ok(Some(pkg)) = + installed_exact_package(&krate, &vers, &mut source, config, opts, root, &dst, force) + { + let msg = format!( + "package `{}` is already installed, use --force to override", + pkg + ); + config.shell().status("Ignored", &msg)?; + return Ok(true); + } + Some(vers) } else if source.source_id().is_registry() { Some(VersionReq::any()) } else { None }; - if krate.is_some() && !no_track { - if let Some(vers) = vers.clone() { - if vers.is_exact() { - if let Ok(pkg) = - select_pkg(&mut source, krate, Some(vers), config, false, &mut |_| { - bail!("(Ignored)") - }) - { - let (_ws, rustc, target) = - make_ws_rustc_target(&config, opts, &source_id, pkg.clone())?; - if let Ok(installed) = is_installed(&pkg, &rustc, &target) { - if installed { - let msg = format!( - "package `{}` is already installed, use --force to override", - pkg - ); - config.shell().status("Ignored", &msg)?; - return Ok(true); - } - } - } - } - } - } select_pkg( &mut source, krate, - vers, + vers.as_ref(), config, needs_update_if_source_is_index, &mut |_| { @@ -325,7 +305,7 @@ fn install_one( // Check for conflicts. no_track_duplicates()?; } else { - if is_installed(&pkg, &rustc, &target)? { + if is_installed(&pkg, config, opts, &rustc, &target, root, &dst, force)? { let msg = format!( "package `{}` is already installed, use --force to override", pkg @@ -518,6 +498,65 @@ fn install_one( } } +fn is_installed( + pkg: &Package, + config: &Config, + opts: &ops::CompileOptions, + rustc: &Rustc, + target: &str, + root: &Filesystem, + dst: &Path, + force: bool, +) -> CargoResult { + let tracker = InstallTracker::load(config, root)?; + let (freshness, _duplicates) = + tracker.check_upgrade(dst, pkg, force, opts, target, &rustc.verbose_version)?; + Ok(freshness == Freshness::Fresh) +} + +/// Checks if vers can only be satisfied by exactly one version of a package in a registry, and it's +/// already installed. If this is the case, we can skip interacting with a registry to check if +/// newer versions may be installable, as no newer version can exist. +fn installed_exact_package<'a, T>( + krate: &Option<&str>, + vers: &VersionReq, + source: &mut T, + config: &Config, + opts: &ops::CompileOptions, + root: &Filesystem, + dst: &Path, + force: bool, +) -> CargoResult> +where + T: Source + 'a, +{ + if krate.is_none() { + // We can't check for an installed crate if we don't know the crate name. + return Ok(None); + } + if !vers.is_exact() { + // If the version isn't exact, we may need to update the registry and look for a newer + // version - we can't know if the package is installed without doing so. + return Ok(None); + } + // Try getting the package from the registry without updating it, to avoid a potentially + // expensive network call in the case that the package is already installed. + // If this fails, the caller will possibly do an index update and try again, this is just a + // best-effort check to see if we can avoid hitting the network. + if let Ok(pkg) = select_pkg(source, *krate, Some(vers), config, false, &mut |_| { + // Don't try to do anything clever here - if this function returns false, the caller will do + // a more in-depth check possibly including an update from the index. + bail!("(Ignored)") + }) { + let (_ws, rustc, target) = + make_ws_rustc_target(&config, opts, &source.source_id(), pkg.clone())?; + if let Ok(true) = is_installed(&pkg, config, opts, &rustc, &target, root, &dst, force) { + return Ok(Some(pkg)); + } + } + Ok(None) +} + fn make_ws_rustc_target<'cfg>( config: &'cfg Config, opts: &ops::CompileOptions, diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index 4911d1f3d7f..91a6aaff71c 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -524,7 +524,7 @@ pub fn path_source(source_id: SourceId, config: &Config) -> CargoResult( source: &mut T, name: Option<&str>, - vers: Option, + vers: Option<&VersionReq>, config: &Config, needs_update: bool, list_all: &mut dyn FnMut(&mut T) -> CargoResult>, From 3c96c054eeb6363f419b739d849bedad0532ad22 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sat, 28 Mar 2020 02:11:20 +0000 Subject: [PATCH 06/14] Remove unnecessary lifetime parameter --- src/cargo/ops/cargo_install.rs | 4 ++-- src/cargo/ops/common_for_install_and_uninstall.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 9ca60ab221b..71ed025d752 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -517,7 +517,7 @@ fn is_installed( /// Checks if vers can only be satisfied by exactly one version of a package in a registry, and it's /// already installed. If this is the case, we can skip interacting with a registry to check if /// newer versions may be installable, as no newer version can exist. -fn installed_exact_package<'a, T>( +fn installed_exact_package( krate: &Option<&str>, vers: &VersionReq, source: &mut T, @@ -528,7 +528,7 @@ fn installed_exact_package<'a, T>( force: bool, ) -> CargoResult> where - T: Source + 'a, + T: Source, { if krate.is_none() { // We can't check for an installed crate if we don't know the crate name. diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index 91a6aaff71c..84b7791d4d1 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -521,7 +521,7 @@ pub fn path_source(source_id: SourceId, config: &Config) -> CargoResult( +pub fn select_pkg( source: &mut T, name: Option<&str>, vers: Option<&VersionReq>, @@ -530,7 +530,7 @@ pub fn select_pkg<'a, T>( list_all: &mut dyn FnMut(&mut T) -> CargoResult>, ) -> CargoResult where - T: Source + 'a, + T: Source, { // This operation may involve updating some sources or making a few queries // which may involve frobbing caches, as a result make sure we synchronize From 7a83d615ed7712decdf5d5a7fa67d428c1ef77d2 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sun, 12 Apr 2020 22:47:19 +0100 Subject: [PATCH 07/14] Commenty review comments --- src/cargo/ops/cargo_install.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 71ed025d752..8a2d1b25043 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -67,6 +67,8 @@ pub fn install( } else { let mut succeeded = vec![]; let mut failed = vec![]; + // "Tracks whether or not the source (such as a registry or git repo) has been updated. + // This is used to avoid updating it multiple times when installing multiple crates. let mut did_update = false; for krate in krates { let root = root.clone(); @@ -140,6 +142,10 @@ pub fn install( } // Returns whether a subsequent call should attempt to update again. +// The `needs_update_if_source_is_index` parameter indicates whether or not the source index should +// be updated. This is used ensure it is only updated once when installing multiple crates. +// The return value here is used so that the caller knows what to pass to the +// `needs_update_if_source_is_index` parameter when `install_one` is called again. fn install_one( config: &Config, root: &Filesystem, @@ -546,7 +552,7 @@ where if let Ok(pkg) = select_pkg(source, *krate, Some(vers), config, false, &mut |_| { // Don't try to do anything clever here - if this function returns false, the caller will do // a more in-depth check possibly including an update from the index. - bail!("(Ignored)") + Ok(vec![]) }) { let (_ws, rustc, target) = make_ws_rustc_target(&config, opts, &source.source_id(), pkg.clone())?; From 6eef9a89a7dd7ad4f6ee30ef8253699f38a24828 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 13 Apr 2020 01:28:34 +0100 Subject: [PATCH 08/14] select_pkg takes a Dependency not name + vers Handling of these is coupled, so do the handling in one place, close to where we parse the command line flags, so we can just pass in a single derived object. --- src/cargo/ops/cargo_install.rs | 81 ++++++++++++------- src/cargo/ops/cargo_uninstall.rs | 2 +- .../ops/common_for_install_and_uninstall.rs | 25 ++---- 3 files changed, 59 insertions(+), 49 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 8a2d1b25043..07d8e9dea2a 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -9,7 +9,9 @@ use tempfile::Builder as TempFileBuilder; use crate::core::compiler::Freshness; use crate::core::compiler::{CompileKind, DefaultExecutor, Executor, RustcTargetData}; use crate::core::resolver::{HasDevUnits, ResolveOpts}; -use crate::core::{Edition, Package, PackageId, PackageIdSpec, Source, SourceId, Workspace}; +use crate::core::{ + Dependency, Edition, Package, PackageId, PackageIdSpec, Source, SourceId, Workspace, +}; use crate::ops; use crate::ops::common_for_install_and_uninstall::*; use crate::sources::{GitSource, SourceConfigMap}; @@ -161,11 +163,36 @@ fn install_one( ) -> CargoResult { let dst = root.join("bin").into_path_unlocked(); + let dependency = { + if let Some(krate) = krate { + let vers = if source_id.is_git() || source_id.is_path() { + // We explicitly ignore the version flag for these source types. + None + } else if let Some(vers_flag) = vers { + Some(parse_semver_flag(vers_flag)?.to_string()) + } else { + if source_id.is_registry() { + // Avoid pre-release versions from crate.io + // unless explicitly asked for + Some(String::from("*")) + } else { + None + } + }; + Some(Dependency::parse_no_deprecated( + krate, + vers.as_deref(), + source_id, + )?) + } else { + None + } + }; + let pkg = if source_id.is_git() { select_pkg( &mut GitSource::new(source_id, config)?, - krate, - None, + &dependency.as_ref(), config, true, &mut |git| git.read_packages(), @@ -196,15 +223,22 @@ fn install_one( } } src.update()?; - select_pkg(&mut src, krate, None, config, false, &mut |path| { + select_pkg(&mut src, &dependency.as_ref(), config, false, &mut |path| { path.read_packages() })? } else { + if !krate.is_some() { + bail!( + "must specify a crate to install from \ + crates.io, or use --path or --git to \ + specify alternate source" + ) + } let mut source = map.load(source_id, &HashSet::new())?; - let vers = if let Some(vers) = vers { - let vers = parse_semver_req(vers)?; + // This should always be true - else we would have bailed on the krate.is_some() check. + if let Some(dep) = &dependency { if let Ok(Some(pkg)) = - installed_exact_package(&krate, &vers, &mut source, config, opts, root, &dst, force) + installed_exact_package(dep, &mut source, config, opts, root, &dst, force) { let msg = format!( "package `{}` is already installed, use --force to override", @@ -213,25 +247,13 @@ fn install_one( config.shell().status("Ignored", &msg)?; return Ok(true); } - Some(vers) - } else if source.source_id().is_registry() { - Some(VersionReq::any()) - } else { - None - }; + } select_pkg( &mut source, - krate, - vers.as_ref(), + &dependency.as_ref(), config, needs_update_if_source_is_index, - &mut |_| { - bail!( - "must specify a crate to install from \ - crates.io, or use --path or --git to \ - specify alternate source" - ) - }, + &mut |_| Ok(vec![]), )? }; @@ -524,8 +546,7 @@ fn is_installed( /// already installed. If this is the case, we can skip interacting with a registry to check if /// newer versions may be installable, as no newer version can exist. fn installed_exact_package( - krate: &Option<&str>, - vers: &VersionReq, + dep: &Dependency, source: &mut T, config: &Config, opts: &ops::CompileOptions, @@ -536,11 +557,7 @@ fn installed_exact_package( where T: Source, { - if krate.is_none() { - // We can't check for an installed crate if we don't know the crate name. - return Ok(None); - } - if !vers.is_exact() { + if !dep.version_req().is_exact() { // If the version isn't exact, we may need to update the registry and look for a newer // version - we can't know if the package is installed without doing so. return Ok(None); @@ -549,7 +566,7 @@ where // expensive network call in the case that the package is already installed. // If this fails, the caller will possibly do an index update and try again, this is just a // best-effort check to see if we can avoid hitting the network. - if let Ok(pkg) = select_pkg(source, *krate, Some(vers), config, false, &mut |_| { + if let Ok(pkg) = select_pkg(source, &Some(dep), config, false, &mut |_| { // Don't try to do anything clever here - if this function returns false, the caller will do // a more in-depth check possibly including an update from the index. Ok(vec![]) @@ -590,7 +607,9 @@ fn make_ws_rustc_target<'cfg>( Ok((ws, rustc, target)) } -fn parse_semver_req(v: &str) -> CargoResult { +/// Parses x.y.z as if it were =x.y.z, and gives CLI-specific error messages in the case of invalid +/// values. +fn parse_semver_flag(v: &str) -> CargoResult { // If the version begins with character <, >, =, ^, ~ parse it as a // version range, otherwise parse it as a specific version let first = v diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index 37a4f227774..4608d8a8d87 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -85,7 +85,7 @@ fn uninstall_cwd(root: &Filesystem, bins: &[String], config: &Config) -> CargoRe let tracker = InstallTracker::load(config, root)?; let source_id = SourceId::for_path(config.cwd())?; let mut src = path_source(source_id, config)?; - let pkg = select_pkg(&mut src, None, None, config, true, &mut |path| { + let pkg = select_pkg(&mut src, &None, config, true, &mut |path| { path.read_packages() })?; let pkgid = pkg.package_id(); diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index 84b7791d4d1..ac39c80d7c8 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -5,7 +5,6 @@ use std::io::SeekFrom; use std::path::{Path, PathBuf}; use anyhow::{bail, format_err}; -use semver::VersionReq; use serde::{Deserialize, Serialize}; use crate::core::compiler::Freshness; @@ -523,8 +522,7 @@ pub fn path_source(source_id: SourceId, config: &Config) -> CargoResult( source: &mut T, - name: Option<&str>, - vers: Option<&VersionReq>, + dep: &Option<&Dependency>, config: &Config, needs_update: bool, list_all: &mut dyn FnMut(&mut T) -> CargoResult>, @@ -541,26 +539,19 @@ where source.update()?; } - if let Some(name) = name { - let vers = vers.map(|v| v.to_string()); - let dep = Dependency::parse_no_deprecated(name, vers.as_deref(), source.source_id())?; + if let &Some(dep) = dep { let deps = source.query_vec(&dep)?; match deps.iter().map(|p| p.package_id()).max() { Some(pkgid) => { let pkg = Box::new(source).download_now(pkgid, config)?; Ok(pkg) } - None => { - let vers_info = vers - .map(|v| format!(" with version `{}`", v)) - .unwrap_or_default(); - bail!( - "could not find `{}` in {}{}", - name, - source.source_id(), - vers_info - ) - } + None => bail!( + "could not find `{}` in {} with version `{}`", + dep.package_name(), + source.source_id(), + dep.version_req(), + ), } } else { let candidates = list_all(source)?; From bf6a6278d7f8157f9ca9bdf4260314ea710d327c Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 13 Apr 2020 02:50:46 +0100 Subject: [PATCH 09/14] select_pkg takes Dependency _or_ list_all not both It only ever actually uses one, so let's reflect that in the types. --- src/cargo/ops/cargo_install.rs | 60 ++++++------ src/cargo/ops/cargo_uninstall.rs | 10 +- .../ops/common_for_install_and_uninstall.rs | 94 ++++++++++--------- 3 files changed, 91 insertions(+), 73 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 07d8e9dea2a..58fbff12cd8 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -14,7 +14,7 @@ use crate::core::{ }; use crate::ops; use crate::ops::common_for_install_and_uninstall::*; -use crate::sources::{GitSource, SourceConfigMap}; +use crate::sources::{GitSource, PathSource, SourceConfigMap}; use crate::util::errors::{CargoResult, CargoResultExt}; use crate::util::{paths, Config, Filesystem, Rustc, ToSemver}; use semver::VersionReq; @@ -190,12 +190,16 @@ fn install_one( }; let pkg = if source_id.is_git() { + let dep_or_list_all_fn = if let Some(dep) = dependency { + DependencyOrListAllFn::Dependency(dep) + } else { + DependencyOrListAllFn::ListAllFn(|git: &mut GitSource<'_>| git.read_packages()) + }; select_pkg( &mut GitSource::new(source_id, config)?, - &dependency.as_ref(), + dep_or_list_all_fn, config, true, - &mut |git| git.read_packages(), )? } else if source_id.is_path() { let mut src = path_source(source_id, config)?; @@ -223,37 +227,38 @@ fn install_one( } } src.update()?; - select_pkg(&mut src, &dependency.as_ref(), config, false, &mut |path| { - path.read_packages() - })? + + let dep_or_list_all_fn = if let Some(dep) = dependency { + DependencyOrListAllFn::Dependency(dep) + } else { + DependencyOrListAllFn::ListAllFn(|path: &mut PathSource<'_>| path.read_packages()) + }; + select_pkg(&mut src, dep_or_list_all_fn, config, false)? } else { - if !krate.is_some() { + if !dependency.is_some() { bail!( "must specify a crate to install from \ crates.io, or use --path or --git to \ specify alternate source" ) } + let dep = dependency.unwrap(); // Verified with is_some/bail above. let mut source = map.load(source_id, &HashSet::new())?; - // This should always be true - else we would have bailed on the krate.is_some() check. - if let Some(dep) = &dependency { - if let Ok(Some(pkg)) = - installed_exact_package(dep, &mut source, config, opts, root, &dst, force) - { - let msg = format!( - "package `{}` is already installed, use --force to override", - pkg - ); - config.shell().status("Ignored", &msg)?; - return Ok(true); - } + if let Ok(Some(pkg)) = + installed_exact_package(dep.clone(), &mut source, config, opts, root, &dst, force) + { + let msg = format!( + "package `{}` is already installed, use --force to override", + pkg + ); + config.shell().status("Ignored", &msg)?; + return Ok(true); } select_pkg( &mut source, - &dependency.as_ref(), + DependencyOrListAllFn::) -> CargoResult>>::Dependency(dep), config, needs_update_if_source_is_index, - &mut |_| Ok(vec![]), )? }; @@ -546,7 +551,7 @@ fn is_installed( /// already installed. If this is the case, we can skip interacting with a registry to check if /// newer versions may be installable, as no newer version can exist. fn installed_exact_package( - dep: &Dependency, + dep: Dependency, source: &mut T, config: &Config, opts: &ops::CompileOptions, @@ -566,11 +571,12 @@ where // expensive network call in the case that the package is already installed. // If this fails, the caller will possibly do an index update and try again, this is just a // best-effort check to see if we can avoid hitting the network. - if let Ok(pkg) = select_pkg(source, &Some(dep), config, false, &mut |_| { - // Don't try to do anything clever here - if this function returns false, the caller will do - // a more in-depth check possibly including an update from the index. - Ok(vec![]) - }) { + if let Ok(pkg) = select_pkg( + source, + DependencyOrListAllFn:: CargoResult>>::Dependency(dep), + config, + false, + ) { let (_ws, rustc, target) = make_ws_rustc_target(&config, opts, &source.source_id(), pkg.clone())?; if let Ok(true) = is_installed(&pkg, config, opts, &rustc, &target, root, &dst, force) { diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index 4608d8a8d87..52b9a345294 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -5,6 +5,7 @@ use std::env; use crate::core::PackageId; use crate::core::{PackageIdSpec, SourceId}; use crate::ops::common_for_install_and_uninstall::*; +use crate::sources::PathSource; use crate::util::errors::CargoResult; use crate::util::paths; use crate::util::Config; @@ -85,9 +86,12 @@ fn uninstall_cwd(root: &Filesystem, bins: &[String], config: &Config) -> CargoRe let tracker = InstallTracker::load(config, root)?; let source_id = SourceId::for_path(config.cwd())?; let mut src = path_source(source_id, config)?; - let pkg = select_pkg(&mut src, &None, config, true, &mut |path| { - path.read_packages() - })?; + let pkg = select_pkg( + &mut src, + DependencyOrListAllFn::ListAllFn(|path: &mut PathSource<'_>| path.read_packages()), + config, + true, + )?; let pkgid = pkg.package_id(); uninstall_pkgid(root, tracker, pkgid, bins, config) } diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index ac39c80d7c8..5fe4ac55b01 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -519,16 +519,21 @@ pub fn path_source(source_id: SourceId, config: &Config) -> CargoResult { + Dependency(Dependency), + ListAllFn(F), +} + /// Gets a Package based on command-line requirements. -pub fn select_pkg( +pub fn select_pkg( source: &mut T, - dep: &Option<&Dependency>, + dep_or_list_all_fn: DependencyOrListAllFn, config: &Config, needs_update: bool, - list_all: &mut dyn FnMut(&mut T) -> CargoResult>, ) -> CargoResult where T: Source, + F: FnMut(&mut T) -> CargoResult>, { // This operation may involve updating some sources or making a few queries // which may involve frobbing caches, as a result make sure we synchronize @@ -539,50 +544,53 @@ where source.update()?; } - if let &Some(dep) = dep { - let deps = source.query_vec(&dep)?; - match deps.iter().map(|p| p.package_id()).max() { - Some(pkgid) => { - let pkg = Box::new(source).download_now(pkgid, config)?; - Ok(pkg) + match dep_or_list_all_fn { + DependencyOrListAllFn::Dependency(dep) => { + let deps = source.query_vec(&dep)?; + match deps.iter().map(|p| p.package_id()).max() { + Some(pkgid) => { + let pkg = Box::new(source).download_now(pkgid, config)?; + Ok(pkg) + } + None => bail!( + "could not find `{}` in {} with version `{}`", + dep.package_name(), + source.source_id(), + dep.version_req(), + ), } - None => bail!( - "could not find `{}` in {} with version `{}`", - dep.package_name(), - source.source_id(), - dep.version_req(), - ), } - } else { - let candidates = list_all(source)?; - let binaries = candidates - .iter() - .filter(|cand| cand.targets().iter().filter(|t| t.is_bin()).count() > 0); - let examples = candidates - .iter() - .filter(|cand| cand.targets().iter().filter(|t| t.is_example()).count() > 0); - let pkg = match one(binaries, |v| multi_err("binaries", v))? { - Some(p) => p, - None => match one(examples, |v| multi_err("examples", v))? { + DependencyOrListAllFn::ListAllFn(mut list_all) => { + let candidates = list_all(source)?; + let binaries = candidates + .iter() + .filter(|cand| cand.targets().iter().filter(|t| t.is_bin()).count() > 0); + let examples = candidates + .iter() + .filter(|cand| cand.targets().iter().filter(|t| t.is_example()).count() > 0); + let pkg = match one(binaries, |v| multi_err("binaries", v))? { Some(p) => p, - None => bail!( - "no packages found with binaries or \ + None => match one(examples, |v| multi_err("examples", v))? { + Some(p) => p, + None => bail!( + "no packages found with binaries or \ examples" - ), - }, - }; - return Ok(pkg.clone()); - - fn multi_err(kind: &str, mut pkgs: Vec<&Package>) -> String { - pkgs.sort_unstable_by_key(|a| a.name()); - format!( - "multiple packages with {} found: {}", - kind, - pkgs.iter() - .map(|p| p.name().as_str()) - .collect::>() - .join(", ") - ) + ), + }, + }; + return Ok(pkg.clone()); + + fn multi_err(kind: &str, mut pkgs: Vec<&Package>) -> String { + pkgs.sort_unstable_by_key(|a| a.name()); + format!( + "multiple packages with {} found: {}", + kind, + pkgs.iter() + .map(|p| p.name().as_str()) + .collect::>() + .join(", ") + ) + } } } } From 4432ac3d10b73394a66cf78f1011ffe644828947 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Tue, 14 Apr 2020 00:25:03 +0100 Subject: [PATCH 10/14] Add a little docstring --- src/cargo/ops/common_for_install_and_uninstall.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index 5fe4ac55b01..f85198abd64 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -519,6 +519,8 @@ pub fn path_source(source_id: SourceId, config: &Config) -> CargoResult { Dependency(Dependency), ListAllFn(F), From d2b277521e7c06f447b87fbfb7717011f11be274 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sat, 2 May 2020 13:30:29 +0100 Subject: [PATCH 11/14] Simplify Remove enum, pull out function and dependent function --- src/cargo/ops/cargo_install.rs | 174 +++++++++--------- src/cargo/ops/cargo_uninstall.rs | 4 +- .../ops/common_for_install_and_uninstall.rs | 121 ++++++------ 3 files changed, 149 insertions(+), 150 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index e7c817bb52f..377e76cf0ea 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -170,103 +170,97 @@ fn install_one( let dst = root.join("bin").into_path_unlocked(); - let dependency = { - if let Some(krate) = krate { - let vers = if source_id.is_git() || source_id.is_path() { - // We explicitly ignore the version flag for these source types. - None - } else if let Some(vers_flag) = vers { - Some(parse_semver_flag(vers_flag)?.to_string()) - } else { - if source_id.is_registry() { - // Avoid pre-release versions from crate.io - // unless explicitly asked for - Some(String::from("*")) + let pkg = { + let dep = { + if let Some(krate) = krate { + let vers = if let Some(vers_flag) = vers { + Some(parse_semver_flag(vers_flag)?.to_string()) } else { - None - } - }; - Some(Dependency::parse_no_deprecated( - krate, - vers.as_deref(), - source_id, - )?) - } else { - None - } - }; - - let pkg = if source_id.is_git() { - let dep_or_list_all_fn = if let Some(dep) = dependency { - DependencyOrListAllFn::Dependency(dep) - } else { - DependencyOrListAllFn::ListAllFn(|git: &mut GitSource<'_>| git.read_packages()) + if source_id.is_registry() { + // Avoid pre-release versions from crate.io + // unless explicitly asked for + Some(String::from("*")) + } else { + None + } + }; + Some(Dependency::parse_no_deprecated( + krate, + vers.as_deref(), + source_id, + )?) + } else { + None + } }; - select_pkg( - &mut GitSource::new(source_id, config)?, - dep_or_list_all_fn, - config, - true, - )? - } else if source_id.is_path() { - let mut src = path_source(source_id, config)?; - if !src.path().is_dir() { - bail!( - "`{}` is not a directory. \ - --path must point to a directory containing a Cargo.toml file.", - src.path().display() - ) - } - if !src.path().join("Cargo.toml").exists() { - if from_cwd { + + if source_id.is_git() { + let mut source = GitSource::new(source_id, config)?; + select_pkg( + &mut source, + dep, + |git: &mut GitSource<'_>| git.read_packages(), + config, + )? + } else if source_id.is_path() { + let mut src = path_source(source_id, config)?; + if !src.path().is_dir() { bail!( - "`{}` is not a crate root; specify a crate to \ + "`{}` is not a directory. \ + --path must point to a directory containing a Cargo.toml file.", + src.path().display() + ) + } + if !src.path().join("Cargo.toml").exists() { + if from_cwd { + bail!( + "`{}` is not a crate root; specify a crate to \ install from crates.io, or use --path or --git to \ specify an alternate source", - src.path().display() - ); - } else { - bail!( - "`{}` does not contain a Cargo.toml file. \ + src.path().display() + ); + } else { + bail!( + "`{}` does not contain a Cargo.toml file. \ --path must point to a directory containing a Cargo.toml file.", - src.path().display() - ) + src.path().display() + ) + } } - } - src.update()?; - - let dep_or_list_all_fn = if let Some(dep) = dependency { - DependencyOrListAllFn::Dependency(dep) + select_pkg( + &mut src, + dep, + |path: &mut PathSource<'_>| path.read_packages(), + config, + )? } else { - DependencyOrListAllFn::ListAllFn(|path: &mut PathSource<'_>| path.read_packages()) - }; - select_pkg(&mut src, dep_or_list_all_fn, config, false)? - } else { - if !dependency.is_some() { - bail!( - "must specify a crate to install from \ + if let Some(dep) = dep { + let mut source = map.load(source_id, &HashSet::new())?; + if let Ok(Some(pkg)) = installed_exact_package( + dep.clone(), + &mut source, + config, + opts, + root, + &dst, + force, + ) { + let msg = format!( + "package `{}` is already installed, use --force to override", + pkg + ); + config.shell().status("Ignored", &msg)?; + return Ok(true); + } + select_dep_pkg(&mut source, dep, config, needs_update_if_source_is_index)? + } else { + bail!( + "must specify a crate to install from \ crates.io, or use --path or --git to \ specify alternate source" - ) - } - let dep = dependency.unwrap(); // Verified with is_some/bail above. - let mut source = map.load(source_id, &HashSet::new())?; - if let Ok(Some(pkg)) = - installed_exact_package(dep.clone(), &mut source, config, opts, root, &dst, force) - { - let msg = format!( - "package `{}` is already installed, use --force to override", - pkg - ); - config.shell().status("Ignored", &msg)?; - return Ok(true); + ) + } } - select_pkg( - &mut source, - DependencyOrListAllFn::) -> CargoResult>>::Dependency(dep), - config, - needs_update_if_source_is_index, - )? }; let git_package = if source_id.is_git() { @@ -578,12 +572,7 @@ where // expensive network call in the case that the package is already installed. // If this fails, the caller will possibly do an index update and try again, this is just a // best-effort check to see if we can avoid hitting the network. - if let Ok(pkg) = select_pkg( - source, - DependencyOrListAllFn:: CargoResult>>::Dependency(dep), - config, - false, - ) { + if let Ok(pkg) = select_dep_pkg(source, dep, config, false) { let (_ws, rustc, target) = make_ws_rustc_target(&config, opts, &source.source_id(), pkg.clone())?; if let Ok(true) = is_installed(&pkg, config, opts, &rustc, &target, root, &dst, force) { @@ -644,6 +633,7 @@ fn parse_semver_flag(v: &str) -> CargoResult { ), } } else { + println!("DWH: Converting..."); match v.to_semver() { Ok(v) => Ok(VersionReq::exact(&v)), Err(e) => { diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index 52b9a345294..6f07940c215 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -88,9 +88,9 @@ fn uninstall_cwd(root: &Filesystem, bins: &[String], config: &Config) -> CargoRe let mut src = path_source(source_id, config)?; let pkg = select_pkg( &mut src, - DependencyOrListAllFn::ListAllFn(|path: &mut PathSource<'_>| path.read_packages()), + None, + |path: &mut PathSource<'_>| path.read_packages(), config, - true, )?; let pkgid = pkg.package_id(); uninstall_pkgid(root, tracker, pkgid, bins, config) diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index f85198abd64..78e53af5c4f 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -519,23 +519,15 @@ pub fn path_source(source_id: SourceId, config: &Config) -> CargoResult { - Dependency(Dependency), - ListAllFn(F), -} - /// Gets a Package based on command-line requirements. -pub fn select_pkg( +pub fn select_dep_pkg( source: &mut T, - dep_or_list_all_fn: DependencyOrListAllFn, + dep: Dependency, config: &Config, needs_update: bool, ) -> CargoResult where T: Source, - F: FnMut(&mut T) -> CargoResult>, { // This operation may involve updating some sources or making a few queries // which may involve frobbing caches, as a result make sure we synchronize @@ -546,54 +538,71 @@ where source.update()?; } - match dep_or_list_all_fn { - DependencyOrListAllFn::Dependency(dep) => { - let deps = source.query_vec(&dep)?; - match deps.iter().map(|p| p.package_id()).max() { - Some(pkgid) => { - let pkg = Box::new(source).download_now(pkgid, config)?; - Ok(pkg) - } - None => bail!( - "could not find `{}` in {} with version `{}`", - dep.package_name(), - source.source_id(), - dep.version_req(), - ), - } + let deps = source.query_vec(&dep)?; + match deps.iter().map(|p| p.package_id()).max() { + Some(pkgid) => { + let pkg = Box::new(source).download_now(pkgid, config)?; + Ok(pkg) } - DependencyOrListAllFn::ListAllFn(mut list_all) => { - let candidates = list_all(source)?; - let binaries = candidates - .iter() - .filter(|cand| cand.targets().iter().filter(|t| t.is_bin()).count() > 0); - let examples = candidates - .iter() - .filter(|cand| cand.targets().iter().filter(|t| t.is_example()).count() > 0); - let pkg = match one(binaries, |v| multi_err("binaries", v))? { + None => bail!( + "could not find `{}` in {} with version `{}`", + dep.package_name(), + source.source_id(), + dep.version_req(), + ), + } +} + +pub fn select_pkg( + source: &mut T, + dep: Option, + mut list_all: F, + config: &Config, +) -> CargoResult +where + T: Source, + F: FnMut(&mut T) -> CargoResult>, +{ + // This operation may involve updating some sources or making a few queries + // which may involve frobbing caches, as a result make sure we synchronize + // with other global Cargos + let _lock = config.acquire_package_cache_lock()?; + + source.update()?; + + return if let Some(dep) = dep { + select_dep_pkg(source, dep, config, false) + } else { + let candidates = list_all(source)?; + let binaries = candidates + .iter() + .filter(|cand| cand.targets().iter().filter(|t| t.is_bin()).count() > 0); + let examples = candidates + .iter() + .filter(|cand| cand.targets().iter().filter(|t| t.is_example()).count() > 0); + let pkg = match one(binaries, |v| multi_err("binaries", v))? { + Some(p) => p, + None => match one(examples, |v| multi_err("examples", v))? { Some(p) => p, - None => match one(examples, |v| multi_err("examples", v))? { - Some(p) => p, - None => bail!( - "no packages found with binaries or \ - examples" - ), - }, - }; - return Ok(pkg.clone()); - - fn multi_err(kind: &str, mut pkgs: Vec<&Package>) -> String { - pkgs.sort_unstable_by_key(|a| a.name()); - format!( - "multiple packages with {} found: {}", - kind, - pkgs.iter() - .map(|p| p.name().as_str()) - .collect::>() - .join(", ") - ) - } - } + None => bail!( + "no packages found with binaries or \ + examples" + ), + }, + }; + Ok(pkg.clone()) + }; + + fn multi_err(kind: &str, mut pkgs: Vec<&Package>) -> String { + pkgs.sort_unstable_by_key(|a| a.name()); + format!( + "multiple packages with {} found: {}", + kind, + pkgs.iter() + .map(|p| p.name().as_str()) + .collect::>() + .join(", ") + ) } } From 9d1c6a7e3c1035bd8c243342852d873e5919727b Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sat, 2 May 2020 14:16:05 +0100 Subject: [PATCH 12/14] Simplify git_package/pkg handling --- src/cargo/ops/cargo_install.rs | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 377e76cf0ea..188fbf23b6d 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -263,12 +263,14 @@ fn install_one( } }; - let git_package = if source_id.is_git() { - Some(pkg.clone()) + let (mut ws, rustc, target) = make_ws_rustc_target(config, opts, &source_id, pkg.clone())?; + let pkg = if source_id.is_git() { + // Don't use ws.current() in order to keep the package source as a git source so that + // install tracking uses the correct source. + pkg } else { - None + ws.current()?.clone() }; - let (mut ws, rustc, target) = make_ws_rustc_target(config, opts, &source_id, pkg)?; let mut td_opt = None; let mut needs_cleanup = false; @@ -286,10 +288,6 @@ fn install_one( ws.set_target_dir(target_dir); } - let pkg = git_package - .as_ref() - .map_or_else(|| ws.current(), |pkg| Ok(pkg))?; - if from_cwd { if pkg.manifest().edition() == Edition::Edition2015 { config.shell().warn( @@ -317,7 +315,7 @@ fn install_one( // Helper for --no-track flag to make sure it doesn't overwrite anything. let no_track_duplicates = || -> CargoResult>> { - let duplicates: BTreeMap> = exe_names(pkg, &opts.filter) + let duplicates: BTreeMap> = exe_names(&pkg, &opts.filter) .into_iter() .filter(|name| dst.join(name).exists()) .map(|name| (name, None)) @@ -349,7 +347,7 @@ fn install_one( } } - config.shell().status("Installing", pkg)?; + config.shell().status("Installing", &pkg)?; check_yanked_install(&ws)?; @@ -390,7 +388,7 @@ fn install_one( } else { let tracker = InstallTracker::load(config, root)?; let (_freshness, duplicates) = - tracker.check_upgrade(&dst, pkg, force, opts, &target, &rustc.verbose_version)?; + tracker.check_upgrade(&dst, &pkg, force, opts, &target, &rustc.verbose_version)?; (Some(tracker), duplicates) }; @@ -453,7 +451,7 @@ fn install_one( if let Some(mut tracker) = tracker { tracker.mark_installed( - pkg, + &pkg, &successful_bins, vers.map(|s| s.to_string()), opts, @@ -461,7 +459,7 @@ fn install_one( &rustc.verbose_version, ); - if let Err(e) = remove_orphaned_bins(&ws, &mut tracker, &duplicates, pkg, &dst) { + if let Err(e) = remove_orphaned_bins(&ws, &mut tracker, &duplicates, &pkg, &dst) { // Don't hard error on remove. config .shell() @@ -588,11 +586,7 @@ fn make_ws_rustc_target<'cfg>( source_id: &SourceId, pkg: Package, ) -> CargoResult<(Workspace<'cfg>, Rustc, String)> { - let mut ws = if source_id.is_git() { - // Don't use ws.current() in order to keep the package source as a git source so that - // install tracking uses the correct source. - Workspace::new(pkg.manifest_path(), config)? - } else if source_id.is_path() { + let mut ws = if source_id.is_git() || source_id.is_path() { Workspace::new(pkg.manifest_path(), config)? } else { Workspace::ephemeral(pkg, config, None, false)? From 18ceb9fead590650fb4ec1757c04da6ed67efb3c Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Fri, 8 May 2020 21:13:24 +0100 Subject: [PATCH 13/14] Remove stray debugging line --- src/cargo/ops/cargo_install.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 188fbf23b6d..ac2fff45f04 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -627,7 +627,6 @@ fn parse_semver_flag(v: &str) -> CargoResult { ), } } else { - println!("DWH: Converting..."); match v.to_semver() { Ok(v) => Ok(VersionReq::exact(&v)), Err(e) => { From b71927224fd9306b2b5bd2b4f8c22268eadfeb6a Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Wed, 20 May 2020 20:38:23 +0100 Subject: [PATCH 14/14] Switch to using existing is_locked implementation semver hasn't merged the upstream PR (yet) --- Cargo.toml | 2 +- crates/resolver-tests/src/lib.rs | 4 ++-- src/cargo/ops/cargo_install.rs | 2 +- tests/testsuite/build.rs | 4 ++-- tests/testsuite/install.rs | 2 +- tests/testsuite/install_upgrade.rs | 2 +- tests/testsuite/registry.rs | 6 +++--- tests/testsuite/replace.rs | 2 +- 8 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e1a6e06b9d3..ef5857b845b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,7 +52,7 @@ percent-encoding = "2.0" remove_dir_all = "0.5.2" rustfix = "0.5.0" same-file = "1" -semver = { git = "https://github.com/illicitonion/semver.git", rev = "f1f912703f67ed63b751c525839731a90239bdcf", features = ["serde"] } +semver = { version = "0.9.0", features = ["serde"] } serde = { version = "1.0.82", features = ["derive"] } serde_ignored = "0.1.0" serde_json = { version = "1.0.30", features = ["raw_value"] } diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index a1cdb9d11a2..ef47f11ffc5 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -734,8 +734,8 @@ fn meta_test_deep_pretty_print_registry() { "vec![pkg!((\"foo\", \"1.0.1\") => [dep_req(\"bar\", \"^1\"),]),\ pkg!((\"foo\", \"1.0.0\") => [dep_req(\"bar\", \"^2\"),]),\ pkg!((\"foo\", \"2.0.0\") => [dep(\"bar\"),]),\ - pkg!((\"bar\", \"1.0.0\") => [dep_req(\"baz\", \"=1.0.2\"),dep_req(\"other\", \"^1\"),]),\ - pkg!((\"bar\", \"2.0.0\") => [dep_req(\"baz\", \"=1.0.1\"),]),\ + pkg!((\"bar\", \"1.0.0\") => [dep_req(\"baz\", \"= 1.0.2\"),dep_req(\"other\", \"^1\"),]),\ + pkg!((\"bar\", \"2.0.0\") => [dep_req(\"baz\", \"= 1.0.1\"),]),\ pkg!((\"baz\", \"1.0.2\") => [dep_req(\"other\", \"^2\"),]),\ pkg!((\"baz\", \"1.0.1\")),\ pkg!((\"cat\", \"1.0.2\") => [dep_req_kind(\"other\", \"^2\", DepKind::Build, false),]),\ diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 9434b1b3def..9b1b08a7254 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -559,7 +559,7 @@ fn installed_exact_package( where T: Source, { - if !dep.version_req().is_exact() { + if !dep.is_locked() { // If the version isn't exact, we may need to update the registry and look for a newer // version - we can't know if the package is installed without doing so. return Ok(None); diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 93b652fb241..dc987c6e602 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -1023,7 +1023,7 @@ fn incompatible_dependencies() { error: failed to select a version for `bad`. ... required by package `qux v0.1.0` ... which is depended on by `foo v0.0.1 ([..])` -versions that meet the requirements `>=1.0.1` are: 1.0.2, 1.0.1 +versions that meet the requirements `>= 1.0.1` are: 1.0.2, 1.0.1 all possible versions conflict with previously selected packages. @@ -1068,7 +1068,7 @@ fn incompatible_dependencies_with_multi_semver() { "\ error: failed to select a version for `bad`. ... required by package `foo v0.0.1 ([..])` -versions that meet the requirements `>=1.0.1, <=2.0.0` are: 2.0.0, 1.0.1 +versions that meet the requirements `>= 1.0.1, <= 2.0.0` are: 2.0.0, 1.0.1 all possible versions conflict with previously selected packages. diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 01a81fc3e29..68bcf22e8b0 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -175,7 +175,7 @@ fn bad_version() { .with_stderr( "\ [UPDATING] [..] index -[ERROR] could not find `foo` in registry `[..]` with version `=0.2.0` +[ERROR] could not find `foo` in registry `[..]` with version `= 0.2.0` ", ) .run(); diff --git a/tests/testsuite/install_upgrade.rs b/tests/testsuite/install_upgrade.rs index 41f00087126..59a904d5854 100644 --- a/tests/testsuite/install_upgrade.rs +++ b/tests/testsuite/install_upgrade.rs @@ -802,7 +802,7 @@ fn already_installed_updates_yank_status_on_upgrade() { .with_stderr( "\ [UPDATING] `[..]` index -[ERROR] could not find `foo` in registry `[..]` with version `=1.0.1` +[ERROR] could not find `foo` in registry `[..]` with version `= 1.0.1` ", ) .run(); diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 0197facaef7..e298e23e625 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -223,7 +223,7 @@ fn wrong_version() { .with_status(101) .with_stderr_contains( "\ -error: failed to select a version for the requirement `foo = \">=1.0.0\"` +error: failed to select a version for the requirement `foo = \">= 1.0.0\"` candidate versions found which didn't match: 0.0.2, 0.0.1 location searched: `[..]` index (which is replacing registry `[..]`) required by package `foo v0.0.1 ([..])` @@ -238,7 +238,7 @@ required by package `foo v0.0.1 ([..])` .with_status(101) .with_stderr_contains( "\ -error: failed to select a version for the requirement `foo = \">=1.0.0\"` +error: failed to select a version for the requirement `foo = \">= 1.0.0\"` candidate versions found which didn't match: 0.0.4, 0.0.3, 0.0.2, ... location searched: `[..]` index (which is replacing registry `[..]`) required by package `foo v0.0.1 ([..])` @@ -543,7 +543,7 @@ fn relying_on_a_yank_is_bad() { .with_status(101) .with_stderr_contains( "\ -error: failed to select a version for the requirement `baz = \"=0.0.2\"` +error: failed to select a version for the requirement `baz = \"= 0.0.2\"` candidate versions found which didn't match: 0.0.1 location searched: `[..]` index (which is replacing registry `[..]`) required by package `bar v0.0.1` diff --git a/tests/testsuite/replace.rs b/tests/testsuite/replace.rs index 9672c462974..17149fa6d93 100644 --- a/tests/testsuite/replace.rs +++ b/tests/testsuite/replace.rs @@ -549,7 +549,7 @@ fn override_wrong_name() { Caused by: no matching package for override `[..]baz:0.1.0` found location searched: file://[..] -version required: =0.1.0 +version required: = 0.1.0 ", ) .run();