Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

move up looking at index summary enum #12749

Merged
merged 2 commits into from
Oct 25, 2023
Merged

move up looking at index summary enum #12749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dff1874
move handling precise up
Eh2406 Oct 25, 2023
57b9372
move yank handling up
Eh2406 Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 7 additions & 61 deletions src/cargo/sources/registry/index.rs
View file
Open in desktop
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 {
Copy link
Contributor Author

@Eh2406 Eh2406 Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by moving filtering of yanked versions from beneath this code to above this code there may be situations in which called is now different. I don't think this will break anything, tests still pass, but it needs more thinking than I gave it.

Copy link
Contributor

@epage epage Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we query offline and see if anything came up, if it did, we move on. We then query online.

So I guess in theory if what the offline check found was yanked, then we might move on, filter it out, and then error instead of switching to on. Whether that can actually happen and cause problems needs a wider view of this stack than I have at this time. From how this is all behaving (this should likely only happen with locked packages which will be allow-listed), I'm assuming it will be fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That analysis sounds correct.

(this should likely only happen with locked packages which will be allow-listed)

I think the problematic case is that there is a downloaded version that has been yanked and is not in the allowed list. The old behavior would filter out the version because it yanked and not in the allow list, meaning that the first pass would see no candidates and so fallback to the second pass. Whereas, the new code will see the yanked version witch is available off-line so return it is the only candidate not falling back to the second pass.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it looks like we are allowing yanked in that first pass, which is fine because its a lockfile, bypassing this problem. Do I have that right?

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
Open in desktop
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() {
Eh2406 marked this conversation as resolved.
Show resolved Hide resolved
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
Eh2406 marked this conversation as resolved.
Show resolved Hide resolved
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());
})?
Eh2406 marked this conversation as resolved.
Show resolved Hide resolved
.is_pending();
}
}
Expand Down
46 changes: 41 additions & 5 deletions src/cargo/util/semver_ext.rs
View file
Open in desktop
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