Skip to content

Commit

Permalink
Auto merge of rust-lang#12749 - Eh2406:move_up, r=epage
Browse files Browse the repository at this point in the history 
move up looking at index summary enum

This builds on the work from rust-lang#12643 and the discussion of the overall approach is at https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/better.20error.20messages.20for.20filtered.20versions.2E

### What does this PR try to resolve?

This uses the enum added in rust-lang#12643 to delay making decisions about yanked and `–– precise` versions until higher in the stack.

### How should we test and review this PR?

This is an internal re-factoring and all the tests still pass. BUT, this is also tricky interleaved code where small changes may have important impacts to semantics.

### Additional information

I will try and call out potentially breaking changes as inline comments. If any of these are controversial enough to deserve separate discussion I'm happy split it into separate PR's.
  • Loading branch information
@bors
bors committed Oct 25, 2023
2 parents df35092 + 57b9372 commit 9bf67a1
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 96 deletions.
68 changes: 7 additions & 61 deletions src/cargo/sources/registry/index.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,8 @@ use cargo_util::{paths, registry::make_dep_path};
use semver::Version;
use serde::Deserialize;
use std::borrow::Cow;
use std::cmp::Ordering;
use std::collections::BTreeMap;
use std::collections::{HashMap, HashSet};
use std::collections::HashMap;
use std::fs;
use std::io::ErrorKind;
use std::path::Path;
Expand Down Expand Up @@ -574,8 +573,7 @@ impl<'cfg> RegistryIndex<'cfg> {
name: InternedString,
req: &OptVersionReq,
load: &mut dyn RegistryData,
yanked_whitelist: &HashSet<PackageId>,
f: &mut dyn FnMut(Summary),
f: &mut dyn FnMut(IndexSummary),
) -> Poll<CargoResult<()>> {
if self.config.offline() {
// This should only return `Poll::Ready(Ok(()))` if there is at least 1 match.
Expand All @@ -592,31 +590,15 @@ impl<'cfg> RegistryIndex<'cfg> {
let callback = &mut |s: IndexSummary| {
if !s.is_offline() {
called = true;
f(s.into_summary());
f(s);
}
};
ready!(self.query_inner_with_online(
name,
req,
load,
yanked_whitelist,
callback,
false
)?);
ready!(self.query_inner_with_online(name, req, load, callback, false)?);
if called {
return Poll::Ready(Ok(()));
}
}
self.query_inner_with_online(
name,
req,
load,
yanked_whitelist,
&mut |s| {
f(s.into_summary());
},
true,
)
self.query_inner_with_online(name, req, load, f, true)
}

/// Inner implementation of [`Self::query_inner`]. Returns the number of
Expand All @@ -628,15 +610,10 @@ impl<'cfg> RegistryIndex<'cfg> {
name: InternedString,
req: &OptVersionReq,
load: &mut dyn RegistryData,
yanked_whitelist: &HashSet<PackageId>,
f: &mut dyn FnMut(IndexSummary),
online: bool,
) -> Poll<CargoResult<()>> {
let source_id = self.source_id;

let summaries = ready!(self.summaries(name, req, load))?;

let summaries = summaries
ready!(self.summaries(name, &req, load))?
// First filter summaries for `--offline`. If we're online then
// everything is a candidate, otherwise if we're offline we're only
// going to consider candidates which are actually present on disk.
Expand All @@ -654,38 +631,7 @@ impl<'cfg> RegistryIndex<'cfg> {
IndexSummary::Offline(s.as_summary().clone())
}
})
// Next filter out all yanked packages. Some yanked packages may
// leak through if they're in a whitelist (aka if they were
// previously in `Cargo.lock`
.filter(|s| !s.is_yanked() || yanked_whitelist.contains(&s.package_id()));

// Handle `cargo update --precise` here.
let precise = source_id.precise_registry_version(name.as_str());
let summaries = summaries.filter(|s| match precise {
Some((current, requested)) => {
if req.matches(current) {
// Unfortunately crates.io allows versions to differ only
// by build metadata. This shouldn't be allowed, but since
// it is, this will honor it if requested. However, if not
// specified, then ignore it.
let s_vers = s.package_id().version();
match (s_vers.build.is_empty(), requested.build.is_empty()) {
(true, true) => s_vers == requested,
(true, false) => false,
(false, true) => {
// Compare disregarding the metadata.
s_vers.cmp_precedence(requested) == Ordering::Equal
}
(false, false) => s_vers == requested,
}
} else {
true
}
}
None => true,
});

summaries.for_each(f);
.for_each(f);
Poll::Ready(Ok(()))
}

Expand Down
65 changes: 35 additions & 30 deletions src/cargo/sources/registry/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -712,25 +712,33 @@ impl<'cfg> Source for RegistrySource<'cfg> {
kind: QueryKind,
f: &mut dyn FnMut(Summary),
) -> Poll<CargoResult<()>> {
// If this is a precise dependency, then it came from a lock file and in
let mut req = dep.version_req().clone();

// Handle `cargo update --precise` here.
if let Some((_, requested)) = self
.source_id
.precise_registry_version(dep.package_name().as_str())
.filter(|(c, _)| req.matches(c))
{
req.update_precise(&requested);
}

// If this is a locked dependency, then it came from a lock file and in
// theory the registry is known to contain this version. If, however, we
// come back with no summaries, then our registry may need to be
// updated, so we fall back to performing a lazy update.
if kind == QueryKind::Exact && dep.source_id().has_precise() && !self.ops.is_updated() {
if kind == QueryKind::Exact && req.is_locked() && !self.ops.is_updated() {
debug!("attempting query without update");
let mut called = false;
ready!(self.index.query_inner(
dep.package_name(),
dep.version_req(),
&mut *self.ops,
&self.yanked_whitelist,
&mut |s| {
if dep.matches(&s) {
ready!(self
.index
.query_inner(dep.package_name(), &req, &mut *self.ops, &mut |s| {
if dep.matches(s.as_summary()) {
// We are looking for a package from a lock file so we do not care about yank
called = true;
f(s);
f(s.into_summary());
}
},
))?;
},))?;
if called {
Poll::Ready(Ok(()))
} else {
Expand All @@ -740,22 +748,23 @@ impl<'cfg> Source for RegistrySource<'cfg> {
}
} else {
let mut called = false;
ready!(self.index.query_inner(
dep.package_name(),
dep.version_req(),
&mut *self.ops,
&self.yanked_whitelist,
&mut |s| {
ready!(self
.index
.query_inner(dep.package_name(), &req, &mut *self.ops, &mut |s| {
let matched = match kind {
QueryKind::Exact => dep.matches(&s),
QueryKind::Exact => dep.matches(s.as_summary()),
QueryKind::Fuzzy => true,
};
if matched {
f(s);
// Next filter out all yanked packages. Some yanked packages may
// leak through if they're in a whitelist (aka if they were
// previously in `Cargo.lock`
if matched
&& (!s.is_yanked() || self.yanked_whitelist.contains(&s.package_id()))
{
f(s.into_summary());
called = true;
}
}
))?;
}))?;
if called {
return Poll::Ready(Ok(()));
}
Expand All @@ -777,13 +786,9 @@ impl<'cfg> Source for RegistrySource<'cfg> {
}
any_pending |= self
.index
.query_inner(
name_permutation,
dep.version_req(),
&mut *self.ops,
&self.yanked_whitelist,
f,
)?
.query_inner(name_permutation, &req, &mut *self.ops, &mut |s| {
f(s.into_summary());
})?
.is_pending();
}
}
Expand Down
46 changes: 41 additions & 5 deletions src/cargo/util/semver_ext.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ pub enum OptVersionReq {
Req(VersionReq),
/// The exact locked version and the original version requirement.
Locked(Version, VersionReq),
/// The exact requested version and the original version requirement.
UpdatePrecise(Version, VersionReq),
}

pub trait VersionExt {
Expand Down Expand Up @@ -53,7 +55,7 @@ impl OptVersionReq {
pub fn is_exact(&self) -> bool {
match self {
OptVersionReq::Any => false,
OptVersionReq::Req(req) => {
OptVersionReq::Req(req) | OptVersionReq::UpdatePrecise(_, req) => {
req.comparators.len() == 1 && {
let cmp = &req.comparators[0];
cmp.op == Op::Exact && cmp.minor.is_some() && cmp.patch.is_some()
Expand All @@ -69,8 +71,24 @@ impl OptVersionReq {
let version = version.clone();
*self = match self {
Any => Locked(version, VersionReq::STAR),
Req(req) => Locked(version, req.clone()),
Locked(_, req) => Locked(version, req.clone()),
Req(req) | Locked(_, req) | UpdatePrecise(_, req) => Locked(version, req.clone()),
};
}

pub fn update_precise(&mut self, version: &Version) {
assert!(
self.matches(version),
"cannot update_precise {} to {}",
self,
version
);
use OptVersionReq::*;
let version = version.clone();
*self = match self {
Any => UpdatePrecise(version, VersionReq::STAR),
Req(req) | Locked(_, req) | UpdatePrecise(_, req) => {
UpdatePrecise(version, req.clone())
}
};
}

Expand Down Expand Up @@ -100,6 +118,23 @@ impl OptVersionReq {
// we should not silently use `1.0.0+foo` even though they have the same version.
v == version
}
OptVersionReq::UpdatePrecise(v, _) => {
// This is used for the `--precise` field of cargo update.
//
// Unfortunately crates.io allowed versions to differ only
// by build metadata. This shouldn't be allowed, but since
// it is, this will honor it if requested.
//
// In that context we treat a requirement that does not have
// build metadata as allowing any metadata. But, if a requirement
// has build metadata, then we only allow it to match the exact
// metadata.
v.major == version.major
&& v.minor == version.minor
&& v.patch == version.patch
&& v.pre == version.pre
&& (v.build == version.build || v.build.is_empty())
}
}
}
}
Expand All @@ -108,8 +143,9 @@ impl Display for OptVersionReq {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
OptVersionReq::Any => f.write_str("*"),
OptVersionReq::Req(req) => Display::fmt(req, f),
OptVersionReq::Locked(_, req) => Display::fmt(req, f),
OptVersionReq::Req(req)
| OptVersionReq::Locked(_, req)
| OptVersionReq::UpdatePrecise(_, req) => Display::fmt(req, f),
}
}
}
Expand Down

0 comments on commit 9bf67a1

Please sign in to comment.