Skip to content

Commit 9f87523

Browse files
borsbors
committed
Auto merge of #12749 - Eh2406:move_up, r=epage
move up looking at index summary enum This builds on the work from #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 #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.
2 parents f5418fc + 9ddaa61 commit 9f87523

File tree

3 files changed

+83
-96
lines changed

3 files changed

+83
-96
lines changed

‎src/cargo/sources/registry/index.rs‎

Lines changed: 7 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -98,9 +98,8 @@ use cargo_util::{paths, registry::make_dep_path};
9898
use semver::Version;
9999
use serde::Deserialize;
100100
use std::borrow::Cow;
101-
use std::cmp::Ordering;
102101
use std::collections::BTreeMap;
103-
use std::collections::{HashMap, HashSet};
102+
use std::collections::HashMap;
104103
use std::fs;
105104
use std::io::ErrorKind;
106105
use std::path::Path;
@@ -574,8 +573,7 @@ impl<'cfg> RegistryIndex<'cfg> {
574573
name: InternedString,
575574
req: &OptVersionReq,
576575
load: &mut dyn RegistryData,
577-
yanked_whitelist: &HashSet<PackageId>,
578-
f: &mut dyn FnMut(Summary),
576+
f: &mut dyn FnMut(IndexSummary),
579577
) -> Poll<CargoResult<()>> {
580578
if self.config.offline() {
581579
// This should only return `Poll::Ready(Ok(()))` if there is at least 1 match.
@@ -592,31 +590,15 @@ impl<'cfg> RegistryIndex<'cfg> {
592590
let callback = &mut |s: IndexSummary| {
593591
if !s.is_offline() {
594592
called = true;
595-
f(s.into_summary());
593+
f(s);
596594
}
597595
};
598-
ready!(self.query_inner_with_online(
599-
name,
600-
req,
601-
load,
602-
yanked_whitelist,
603-
callback,
604-
false
605-
)?);
596+
ready!(self.query_inner_with_online(name, req, load, callback, false)?);
606597
if called {
607598
return Poll::Ready(Ok(()));
608599
}
609600
}
610-
self.query_inner_with_online(
611-
name,
612-
req,
613-
load,
614-
yanked_whitelist,
615-
&mut |s| {
616-
f(s.into_summary());
617-
},
618-
true,
619-
)
601+
self.query_inner_with_online(name, req, load, f, true)
620602
}
621603

622604
/// Inner implementation of [`Self::query_inner`]. Returns the number of
@@ -628,15 +610,10 @@ impl<'cfg> RegistryIndex<'cfg> {
628610
name: InternedString,
629611
req: &OptVersionReq,
630612
load: &mut dyn RegistryData,
631-
yanked_whitelist: &HashSet<PackageId>,
632613
f: &mut dyn FnMut(IndexSummary),
633614
online: bool,
634615
) -> Poll<CargoResult<()>> {
635-
let source_id = self.source_id;
636-
637-
let summaries = ready!(self.summaries(name, req, load))?;
638-
639-
let summaries = summaries
616+
ready!(self.summaries(name, &req, load))?
640617
// First filter summaries for `--offline`. If we're online then
641618
// everything is a candidate, otherwise if we're offline we're only
642619
// going to consider candidates which are actually present on disk.
@@ -654,38 +631,7 @@ impl<'cfg> RegistryIndex<'cfg> {
654631
IndexSummary::Offline(s.as_summary().clone())
655632
}
656633
})
657-
// Next filter out all yanked packages. Some yanked packages may
658-
// leak through if they're in a whitelist (aka if they were
659-
// previously in `Cargo.lock`
660-
.filter(|s| !s.is_yanked() || yanked_whitelist.contains(&s.package_id()));
661-
662-
// Handle `cargo update --precise` here.
663-
let precise = source_id.precise_registry_version(name.as_str());
664-
let summaries = summaries.filter(|s| match precise {
665-
Some((current, requested)) => {
666-
if req.matches(current) {
667-
// Unfortunately crates.io allows versions to differ only
668-
// by build metadata. This shouldn't be allowed, but since
669-
// it is, this will honor it if requested. However, if not
670-
// specified, then ignore it.
671-
let s_vers = s.package_id().version();
672-
match (s_vers.build.is_empty(), requested.build.is_empty()) {
673-
(true, true) => s_vers == requested,
674-
(true, false) => false,
675-
(false, true) => {
676-
// Compare disregarding the metadata.
677-
s_vers.cmp_precedence(requested) == Ordering::Equal
678-
}
679-
(false, false) => s_vers == requested,
680-
}
681-
} else {
682-
true
683-
}
684-
}
685-
None => true,
686-
});
687-
688-
summaries.for_each(f);
634+
.for_each(f);
689635
Poll::Ready(Ok(()))
690636
}
691637

‎src/cargo/sources/registry/mod.rs‎

Lines changed: 35 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -712,25 +712,33 @@ impl<'cfg> Source for RegistrySource<'cfg> {
712712
kind: QueryKind,
713713
f: &mut dyn FnMut(Summary),
714714
) -> Poll<CargoResult<()>> {
715-
// If this is a precise dependency, then it came from a lock file and in
715+
let mut req = dep.version_req().clone();
716+
717+
// Handle `cargo update --precise` here.
718+
if let Some((_, requested)) = self
719+
.source_id
720+
.precise_registry_version(dep.package_name().as_str())
721+
.filter(|(c, _)| req.matches(c))
722+
{
723+
req.update_precise(&requested);
724+
}
725+
726+
// If this is a locked dependency, then it came from a lock file and in
716727
// theory the registry is known to contain this version. If, however, we
717728
// come back with no summaries, then our registry may need to be
718729
// updated, so we fall back to performing a lazy update.
719-
if kind == QueryKind::Exact && dep.source_id().has_precise() && !self.ops.is_updated() {
730+
if kind == QueryKind::Exact && req.is_locked() && !self.ops.is_updated() {
720731
debug!("attempting query without update");
721732
let mut called = false;
722-
ready!(self.index.query_inner(
723-
dep.package_name(),
724-
dep.version_req(),
725-
&mut *self.ops,
726-
&self.yanked_whitelist,
727-
&mut |s| {
728-
if dep.matches(&s) {
733+
ready!(self
734+
.index
735+
.query_inner(dep.package_name(), &req, &mut *self.ops, &mut |s| {
736+
if dep.matches(s.as_summary()) {
737+
// We are looking for a package from a lock file so we do not care about yank
729738
called = true;
730-
f(s);
739+
f(s.into_summary());
731740
}
732-
},
733-
))?;
741+
},))?;
734742
if called {
735743
Poll::Ready(Ok(()))
736744
} else {
@@ -740,22 +748,23 @@ impl<'cfg> Source for RegistrySource<'cfg> {
740748
}
741749
} else {
742750
let mut called = false;
743-
ready!(self.index.query_inner(
744-
dep.package_name(),
745-
dep.version_req(),
746-
&mut *self.ops,
747-
&self.yanked_whitelist,
748-
&mut |s| {
751+
ready!(self
752+
.index
753+
.query_inner(dep.package_name(), &req, &mut *self.ops, &mut |s| {
749754
let matched = match kind {
750-
QueryKind::Exact => dep.matches(&s),
755+
QueryKind::Exact => dep.matches(s.as_summary()),
751756
QueryKind::Fuzzy => true,
752757
};
753-
if matched {
754-
f(s);
758+
// Next filter out all yanked packages. Some yanked packages may
759+
// leak through if they're in a whitelist (aka if they were
760+
// previously in `Cargo.lock`
761+
if matched
762+
&& (!s.is_yanked() || self.yanked_whitelist.contains(&s.package_id()))
763+
{
764+
f(s.into_summary());
755765
called = true;
756766
}
757-
}
758-
))?;
767+
}))?;
759768
if called {
760769
return Poll::Ready(Ok(()));
761770
}
@@ -777,13 +786,9 @@ impl<'cfg> Source for RegistrySource<'cfg> {
777786
}
778787
any_pending |= self
779788
.index
780-
.query_inner(
781-
name_permutation,
782-
dep.version_req(),
783-
&mut *self.ops,
784-
&self.yanked_whitelist,
785-
f,
786-
)?
789+
.query_inner(name_permutation, &req, &mut *self.ops, &mut |s| {
790+
f(s.into_summary());
791+
})?
787792
.is_pending();
788793
}
789794
}

‎src/cargo/util/semver_ext.rs‎

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ pub enum OptVersionReq {
88
Req(VersionReq),
99
/// The exact locked version and the original version requirement.
1010
Locked(Version, VersionReq),
11+
/// The exact requested version and the original version requirement.
12+
UpdatePrecise(Version, VersionReq),
1113
}
1214

1315
pub trait VersionExt {
@@ -53,7 +55,7 @@ impl OptVersionReq {
5355
pub fn is_exact(&self) -> bool {
5456
match self {
5557
OptVersionReq::Any => false,
56-
OptVersionReq::Req(req) => {
58+
OptVersionReq::Req(req) | OptVersionReq::UpdatePrecise(_, req) => {
5759
req.comparators.len() == 1 && {
5860
let cmp = &req.comparators[0];
5961
cmp.op == Op::Exact && cmp.minor.is_some() && cmp.patch.is_some()
@@ -69,8 +71,24 @@ impl OptVersionReq {
6971
let version = version.clone();
7072
*self = match self {
7173
Any => Locked(version, VersionReq::STAR),
72-
Req(req) => Locked(version, req.clone()),
73-
Locked(_, req) => Locked(version, req.clone()),
74+
Req(req) | Locked(_, req) | UpdatePrecise(_, req) => Locked(version, req.clone()),
75+
};
76+
}
77+
78+
pub fn update_precise(&mut self, version: &Version) {
79+
assert!(
80+
self.matches(version),
81+
"cannot update_precise {} to {}",
82+
self,
83+
version
84+
);
85+
use OptVersionReq::*;
86+
let version = version.clone();
87+
*self = match self {
88+
Any => UpdatePrecise(version, VersionReq::STAR),
89+
Req(req) | Locked(_, req) | UpdatePrecise(_, req) => {
90+
UpdatePrecise(version, req.clone())
91+
}
7492
};
7593
}
7694

@@ -100,6 +118,23 @@ impl OptVersionReq {
100118
// we should not silently use `1.0.0+foo` even though they have the same version.
101119
v == version
102120
}
121+
OptVersionReq::UpdatePrecise(v, _) => {
122+
// This is used for the `--precise` field of cargo update.
123+
//
124+
// Unfortunately crates.io allowed versions to differ only
125+
// by build metadata. This shouldn't be allowed, but since
126+
// it is, this will honor it if requested.
127+
//
128+
// In that context we treat a requirement that does not have
129+
// build metadata as allowing any metadata. But, if a requirement
130+
// has build metadata, then we only allow it to match the exact
131+
// metadata.
132+
v.major == version.major
133+
&& v.minor == version.minor
134+
&& v.patch == version.patch
135+
&& v.pre == version.pre
136+
&& (v.build == version.build || v.build.is_empty())
137+
}
103138
}
104139
}
105140
}
@@ -108,8 +143,9 @@ impl Display for OptVersionReq {
108143
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
109144
match self {
110145
OptVersionReq::Any => f.write_str("*"),
111-
OptVersionReq::Req(req) => Display::fmt(req, f),
112-
OptVersionReq::Locked(_, req) => Display::fmt(req, f),
146+
OptVersionReq::Req(req)
147+
| OptVersionReq::Locked(_, req)
148+
| OptVersionReq::UpdatePrecise(_, req) => Display::fmt(req, f),
113149
}
114150
}
115151
}

0 commit comments

Comments
 (0)