Try installing exact versions before updating #8022
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, Rustc, ToSemver}; | ||
| use semver::VersionReq; | ||
|
|
||
| struct Transaction { | ||
| bins: Vec<PathBuf>, | ||
|
|
@@ -66,7 +67,7 @@ pub fn install( | |
| } else { | ||
| let mut succeeded = vec![]; | ||
| let mut failed = vec![]; | ||
| let mut did_update = false; | ||
illicitonion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for krate in krates { | ||
| let root = root.clone(); | ||
| let map = map.clone(); | ||
|
|
@@ -81,15 +82,19 @@ pub fn install( | |
| opts, | ||
| force, | ||
| no_track, | ||
| !did_update, | ||
| ) { | ||
| Ok(still_needs_update) => { | ||
| succeeded.push(krate); | ||
| did_update |= !still_needs_update; | ||
| } | ||
| Err(e) => { | ||
| crate::display_error(&e, &mut config.shell()); | ||
| failed.push(krate); | ||
| // We assume an update was performed if we got an error. | ||
| did_update = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let mut summary = vec![]; | ||
|
|
@@ -134,6 +139,7 @@ pub fn install( | |
| Ok(()) | ||
| } | ||
|
|
||
| // Returns whether a subsequent call should attempt to update again. | ||
illicitonion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| fn install_one( | ||
| config: &Config, | ||
| root: &Filesystem, | ||
|
|
@@ -145,13 +151,15 @@ fn install_one( | |
| opts: &ops::CompileOptions, | ||
| force: bool, | ||
| no_track: bool, | ||
| needs_update_if_source_is_index: bool, | ||
| ) -> CargoResult<bool> { | ||
| let dst = root.join("bin").into_path_unlocked(); | ||
|
|
||
| let pkg = if source_id.is_git() { | ||
| select_pkg( | ||
| &mut GitSource::new(source_id, config)?, | ||
| krate, | ||
| None, | ||
| config, | ||
| true, | ||
| &mut |git| git.read_packages(), | ||
|
|
@@ -182,16 +190,35 @@ fn install_one( | |
| } | ||
| } | ||
| src.update()?; | ||
| select_pkg(&mut src, krate, None, config, false, &mut |path| { | ||
| path.read_packages() | ||
| })? | ||
| } else { | ||
| let mut source = map.load(source_id, &HashSet::new())?; | ||
|
There was a problem hiding this comment. This function is already pretty long and gnarly. Can this new section of code somehow be moved out to another function? There was a problem hiding this comment. Yeah, that's fair - because of how many shared variables there are between this code and the whole rest of the method, and because we're early-returning based on the code, I was leaving it in-line, and trying to re-use some variables in the closure, but I've pulled it out... We should probably break this up in general, I guess :) |
||
| let vers = if let Some(vers) = 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()) | ||
|
There was a problem hiding this comment. I'm a bit confused by this check. Why is it needed if the code below immediately checks for There was a problem hiding this comment.
There was a problem hiding this comment. There is something really subtle here, so I think it might be worth changing this. From a naive look, I'd like this to be a bit more explicit. I'm thinking, change it back so that |
||
| } else { | ||
| None | ||
| }; | ||
| select_pkg( | ||
| &mut source, | ||
| krate, | ||
| vers.as_ref(), | ||
| config, | ||
| needs_update_if_source_is_index, | ||
| &mut |_| { | ||
| bail!( | ||
| "must specify a crate to install from \ | ||
|
|
@@ -202,17 +229,12 @@ fn install_one( | |
| )? | ||
| }; | ||
|
|
||
| let git_package = if source_id.is_git() { | ||
| Some(pkg.clone()) | ||
| } else { | ||
| None | ||
| }; | ||
| let (mut ws, rustc, target) = make_ws_rustc_target(config, opts, &source_id, pkg)?; | ||
|
|
||
| let mut td_opt = None; | ||
| let mut needs_cleanup = false; | ||
|
|
@@ -230,7 +252,9 @@ 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 { | ||
|
|
@@ -257,16 +281,6 @@ fn install_one( | |
| bail!("specified package `{}` has no binaries", pkg); | ||
| } | ||
|
|
||
| // Helper for --no-track flag to make sure it doesn't overwrite anything. | ||
| let no_track_duplicates = || -> CargoResult<BTreeMap<String, Option<PackageId>>> { | ||
| let duplicates: BTreeMap<String, Option<PackageId>> = exe_names(pkg, &opts.filter) | ||
|
|
@@ -291,19 +305,14 @@ fn install_one( | |
| // Check for conflicts. | ||
| no_track_duplicates()?; | ||
| } else { | ||
| if is_installed(&pkg, config, opts, &rustc, &target, root, &dst, force)? { | ||
| let msg = format!( | ||
| "package `{}` is already installed, use --force to override", | ||
| pkg | ||
| ); | ||
| config.shell().status("Ignored", &msg)?; | ||
| return Ok(false); | ||
| } | ||
| } | ||
|
|
||
| config.shell().status("Installing", pkg)?; | ||
|
|
@@ -347,7 +356,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)?; | ||
| (Some(tracker), duplicates) | ||
| }; | ||
|
|
||
|
|
@@ -414,7 +423,7 @@ fn install_one( | |
| &successful_bins, | ||
| vers.map(|s| s.to_string()), | ||
| opts, | ||
| &target, | ||
| &rustc.verbose_version, | ||
| ); | ||
|
|
||
|
|
@@ -460,7 +469,7 @@ fn install_one( | |
| "Installed", | ||
| format!("package `{}` {}", pkg, executables(successful_bins.iter())), | ||
| )?; | ||
| Ok(false) | ||
| } else { | ||
| if !to_install.is_empty() { | ||
| config.shell().status( | ||
|
|
@@ -485,7 +494,139 @@ fn install_one( | |
| ), | ||
| )?; | ||
| } | ||
| Ok(false) | ||
| } | ||
| } | ||
|
|
||
| fn is_installed( | ||
| pkg: &Package, | ||
| config: &Config, | ||
| opts: &ops::CompileOptions, | ||
| rustc: &Rustc, | ||
| target: &str, | ||
| root: &Filesystem, | ||
| dst: &Path, | ||
| force: bool, | ||
| ) -> CargoResult<bool> { | ||
| 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<T>( | ||
| krate: &Option<&str>, | ||
| vers: &VersionReq, | ||
| source: &mut T, | ||
| config: &Config, | ||
| opts: &ops::CompileOptions, | ||
| root: &Filesystem, | ||
| dst: &Path, | ||
| force: bool, | ||
| ) -> CargoResult<Option<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 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)") | ||
illicitonion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }) { | ||
| 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, | ||
| 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. | ||
illicitonion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Workspace::new(pkg.manifest_path(), config)? | ||
| } else if source_id.is_path() { | ||
| Workspace::new(pkg.manifest_path(), config)? | ||
illicitonion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } 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<VersionReq> { | ||
| // 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::<VersionReq>() { | ||
| 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::<VersionReq>().is_ok() { | ||
| msg.push_str(&format!( | ||
| "\nif you want to specify semver range, \ | ||
| add an explicit qualifier, like ^{}", | ||
| v | ||
| )); | ||
| } | ||
| bail!(msg); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably want an upstream release of this (including dtolnay/semver#205) before merging this PR, but figured I'd see if there was appetite for it first.