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.

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

query{_vec} use IndexSummary #12970

Merged
merged 1 commit into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions crates/resolver-tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use cargo::core::Resolve;
use cargo::core::{Dependency, PackageId, Registry, Summary};
use cargo::core::{GitReference, SourceId};
use cargo::sources::source::QueryKind;
use cargo::sources::IndexSummary;
use cargo::util::{CargoResult, Config, Graph, IntoUrl, RustVersion};

use proptest::collection::{btree_map, vec};
Expand Down Expand Up @@ -131,7 +132,7 @@ pub fn resolve_with_config_raw(
&mut self,
dep: &Dependency,
kind: QueryKind,
f: &mut dyn FnMut(Summary),
f: &mut dyn FnMut(IndexSummary),
) -> Poll<CargoResult<()>> {
for summary in self.list.iter() {
let matched = match kind {
Expand All @@ -140,7 +141,7 @@ pub fn resolve_with_config_raw(
};
if matched {
self.used.insert(summary.package_id());
f(summary.clone());
f(IndexSummary::Candidate(summary.clone()));
}
}
Poll::Ready(Ok(()))
Expand Down
1 change: 1 addition & 0 deletions crates/xtask-bump-check/src/xtask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ fn check_crates_io<'a>(
"`{name}@{current}` needs a bump because its should have a version newer than crates.io: {:?}`",
possibilities
.iter()
.map(|s| s.as_summary())
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking on this more, should we have done these as as_candidate and into_candidate. Currently, we should only be exposing candidates and this allows us to update the registry to expose more things without updating every caller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both yanked and off-line dependencies may still be in this lists. Figuring out how and when to filter... Will require some thought/prs.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, aren't they filtered out? I assumed part of the change with this is you can make more or fewer things candidates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current design, approximately, a index summary always keeps track of anyway it has been tainted. If it is yanked, it will always be a yanked variant. Filtering only removes certain versions because of their taints, it does not transmute away the taint.

It is not clear to me if this design will hold up to all the needed use cases. The point of this PR was to get the types changes out of the way, so that the follow-up PR's could be more clearly about the semantics. (And thus easier to decide what semantics we want.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. My assumption (and original design) was that the taint would be removed when it becomes blessed.

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 may end up being a better plan. I will keep it in mined.

.map(|s| format!("{}@{}", s.name(), s.version()))
.collect::<Vec<_>>(),
);
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/future_incompat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ fn get_updates(ws: &Workspace<'_>, package_ids: &BTreeSet<PackageId>) -> Option<
for (pkg_id, summaries) in summaries {
let mut updated_versions: Vec<_> = summaries
.iter()
.map(|summary| summary.version())
.map(|summary| summary.as_summary().version())
.filter(|version| *version > pkg_id.version())
.collect();
updated_versions.sort();
Expand Down
48 changes: 33 additions & 15 deletions src/cargo/core/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use crate::sources::config::SourceConfigMap;
use crate::sources::source::QueryKind;
use crate::sources::source::Source;
use crate::sources::source::SourceMap;
use crate::sources::IndexSummary;
use crate::util::errors::CargoResult;
use crate::util::interning::InternedString;
use crate::util::{CanonicalUrl, Config};
Expand All @@ -23,10 +24,14 @@ pub trait Registry {
&mut self,
dep: &Dependency,
kind: QueryKind,
f: &mut dyn FnMut(Summary),
f: &mut dyn FnMut(IndexSummary),
) -> Poll<CargoResult<()>>;

fn query_vec(&mut self, dep: &Dependency, kind: QueryKind) -> Poll<CargoResult<Vec<Summary>>> {
fn query_vec(
&mut self,
dep: &Dependency,
kind: QueryKind,
) -> Poll<CargoResult<Vec<IndexSummary>>> {
let mut ret = Vec::new();
self.query(dep, kind, &mut |s| ret.push(s)).map_ok(|()| ret)
}
Expand Down Expand Up @@ -337,6 +342,8 @@ impl<'cfg> PackageRegistry<'cfg> {
}
};

let summaries = summaries.into_iter().map(|s| s.into_summary()).collect();

let (summary, should_unlock) =
match summary_for_patch(orig_patch, &locked, summaries, source) {
Poll::Ready(x) => x,
Expand Down Expand Up @@ -481,13 +488,15 @@ impl<'cfg> PackageRegistry<'cfg> {
Ok(())
}

fn query_overrides(&mut self, dep: &Dependency) -> Poll<CargoResult<Option<Summary>>> {
fn query_overrides(&mut self, dep: &Dependency) -> Poll<CargoResult<Option<IndexSummary>>> {
for &s in self.overrides.iter() {
let src = self.sources.get_mut(s).unwrap();
let dep = Dependency::new_override(dep.package_name(), s);
let mut results = ready!(src.query_vec(&dep, QueryKind::Exact))?;
if !results.is_empty() {
return Poll::Ready(Ok(Some(results.remove(0))));

let mut results = None;
ready!(src.query(&dep, QueryKind::Exact, &mut |s| results = Some(s)))?;
if results.is_some() {
return Poll::Ready(Ok(results));
}
}
Poll::Ready(Ok(None))
Expand Down Expand Up @@ -575,7 +584,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
&mut self,
dep: &Dependency,
kind: QueryKind,
f: &mut dyn FnMut(Summary),
f: &mut dyn FnMut(IndexSummary),
) -> Poll<CargoResult<()>> {
assert!(self.patches_locked);
let (override_summary, n, to_warn) = {
Expand Down Expand Up @@ -607,9 +616,9 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
if patches.len() == 1 && dep.is_locked() {
let patch = patches.remove(0);
match override_summary {
Some(summary) => (summary, 1, Some(patch)),
Some(summary) => (summary, 1, Some(IndexSummary::Candidate(patch))),
None => {
f(patch);
f(IndexSummary::Candidate(patch));
return Poll::Ready(Ok(()));
}
}
Expand Down Expand Up @@ -646,7 +655,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
// everything upstairs after locking the summary
(None, Some(source)) => {
for patch in patches.iter() {
f(patch.clone());
f(IndexSummary::Candidate(patch.clone()));
}

// Our sources shouldn't ever come back to us with two
Expand All @@ -658,14 +667,18 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
// already selected, then we skip this `summary`.
let locked = &self.locked;
let all_patches = &self.patches_available;
let callback = &mut |summary: Summary| {
let callback = &mut |summary: IndexSummary| {
for patch in patches.iter() {
let patch = patch.package_id().version();
if summary.package_id().version() == patch {
return;
}
}
f(lock(locked, all_patches, summary))
f(IndexSummary::Candidate(lock(
locked,
all_patches,
summary.into_summary(),
)))
};
return source.query(dep, kind, callback);
}
Expand Down Expand Up @@ -702,9 +715,12 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
"found an override with a non-locked list"
)));
} else if let Some(summary) = to_warn {
self.warn_bad_override(&override_summary, &summary)?;
self.warn_bad_override(override_summary.as_summary(), summary.as_summary())?;
}
f(self.lock(override_summary));
f(IndexSummary::Candidate(
self.lock(override_summary.into_summary()),
));

Poll::Ready(Ok(()))
}

Expand Down Expand Up @@ -887,6 +903,8 @@ fn summary_for_patch(
Vec::new()
});

let orig_matches = orig_matches.into_iter().map(|s| s.into_summary()).collect();

let summary = ready!(summary_for_patch(orig_patch, &None, orig_matches, source))?;

// The unlocked version found a match. This returns a value to
Expand All @@ -907,7 +925,7 @@ fn summary_for_patch(
});
let mut vers = name_summaries
.iter()
.map(|summary| summary.version())
.map(|summary| summary.as_summary().version())
.collect::<Vec<_>>();
let found = match vers.len() {
0 => format!(""),
Expand Down
21 changes: 12 additions & 9 deletions src/cargo/core/resolver/dep_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ impl<'a> RegistryQueryer<'a> {

let mut ret = Vec::new();
let ready = self.registry.query(dep, QueryKind::Exact, &mut |s| {
ret.push(s);
ret.push(s.into_summary());
})?;
if ready.is_pending() {
self.registry_cache
Expand Down Expand Up @@ -135,16 +135,19 @@ impl<'a> RegistryQueryer<'a> {
return Poll::Pending;
}
};
let s = summaries.next().ok_or_else(|| {
anyhow::format_err!(
"no matching package for override `{}` found\n\
let s = summaries
.next()
.ok_or_else(|| {
anyhow::format_err!(
"no matching package for override `{}` found\n\
location searched: {}\n\
version required: {}",
spec,
dep.source_id(),
dep.version_req()
)
})?;
spec,
dep.source_id(),
dep.version_req()
)
})?
.into_summary();
let summaries = summaries.collect::<Vec<_>>();
if !summaries.is_empty() {
let bullets = summaries
Expand Down
8 changes: 6 additions & 2 deletions src/cargo/core/resolver/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ pub(super) fn activation_error(
let mut new_dep = dep.clone();
new_dep.set_version_req(OptVersionReq::Any);

let mut candidates = loop {
let candidates = loop {
match registry.query_vec(&new_dep, QueryKind::Exact) {
Poll::Ready(Ok(candidates)) => break candidates,
Poll::Ready(Err(e)) => return to_resolve_err(e),
Expand All @@ -239,6 +239,8 @@ pub(super) fn activation_error(
}
};

let mut candidates: Vec<_> = candidates.into_iter().map(|s| s.into_summary()).collect();

candidates.sort_unstable_by(|a, b| b.version().cmp(a.version()));

let mut msg = if !candidates.is_empty() {
Expand Down Expand Up @@ -303,7 +305,7 @@ pub(super) fn activation_error(
} else {
// Maybe the user mistyped the name? Like `dep-thing` when `Dep_Thing`
// was meant. So we try asking the registry for a `fuzzy` search for suggestions.
let mut candidates = loop {
let candidates = loop {
match registry.query_vec(&new_dep, QueryKind::Fuzzy) {
Poll::Ready(Ok(candidates)) => break candidates,
Poll::Ready(Err(e)) => return to_resolve_err(e),
Expand All @@ -314,6 +316,8 @@ pub(super) fn activation_error(
}
};

let mut candidates: Vec<_> = candidates.into_iter().map(|s| s.into_summary()).collect();

candidates.sort_unstable_by_key(|a| a.name());
candidates.dedup_by(|a, b| a.name() == b.name());
let mut candidates: Vec<_> = candidates
Expand Down
14 changes: 13 additions & 1 deletion src/cargo/ops/cargo_add/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,7 @@ fn get_latest_dependency(
unreachable!("registry dependencies required, found a workspace dependency");
}
MaybeWorkspace::Other(query) => {
let mut possibilities = loop {
let possibilities = loop {
match registry.query_vec(&query, QueryKind::Fuzzy) {
std::task::Poll::Ready(res) => {
break res?;
Expand All @@ -554,6 +554,11 @@ fn get_latest_dependency(
}
};

let mut possibilities: Vec<_> = possibilities
.into_iter()
.map(|s| s.into_summary())
.collect();

possibilities.sort_by_key(|s| {
// Fallback to a pre-release if no official release is available by sorting them as
// less.
Expand Down Expand Up @@ -671,6 +676,12 @@ fn select_package(
std::task::Poll::Pending => registry.block_until_ready()?,
}
};

let possibilities: Vec<_> = possibilities
.into_iter()
.map(|s| s.into_summary())
.collect();

match possibilities.len() {
0 => {
let source = dependency
Expand Down Expand Up @@ -889,6 +900,7 @@ fn populate_available_features(
// in the lock file for a given version requirement.
let lowest_common_denominator = possibilities
.iter()
.map(|s| s.as_summary())
.min_by_key(|s| {
// Fallback to a pre-release if no official release is available by sorting them as
// more.
Expand Down
7 changes: 6 additions & 1 deletion src/cargo/ops/common_for_install_and_uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,11 @@ where
Poll::Pending => source.block_until_ready()?,
}
};
match deps.iter().max_by_key(|p| p.package_id()) {
match deps
.iter()
.map(|s| s.as_summary())
.max_by_key(|p| p.package_id())
{
Some(summary) => {
if let (Some(current), Some(msrv)) = (current_rust_version, summary.rust_version()) {
let msrv_req = msrv.to_caret_req();
Expand All @@ -571,6 +575,7 @@ where
};
if let Some(alt) = msrv_deps
.iter()
.map(|s| s.as_summary())
.filter(|summary| {
summary
.rust_version()
Expand Down
7 changes: 4 additions & 3 deletions src/cargo/sources/directory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ use std::fmt::{self, Debug, Formatter};
use std::path::{Path, PathBuf};
use std::task::Poll;

use crate::core::{Dependency, Package, PackageId, SourceId, Summary};
use crate::core::{Dependency, Package, PackageId, SourceId};
use crate::sources::source::MaybePackage;
use crate::sources::source::QueryKind;
use crate::sources::source::Source;
use crate::sources::IndexSummary;
use crate::sources::PathSource;
use crate::util::errors::CargoResult;
use crate::util::Config;
Expand Down Expand Up @@ -99,7 +100,7 @@ impl<'cfg> Source for DirectorySource<'cfg> {
&mut self,
dep: &Dependency,
kind: QueryKind,
f: &mut dyn FnMut(Summary),
f: &mut dyn FnMut(IndexSummary),
) -> Poll<CargoResult<()>> {
if !self.updated {
return Poll::Pending;
Expand All @@ -110,7 +111,7 @@ impl<'cfg> Source for DirectorySource<'cfg> {
QueryKind::Fuzzy => true,
});
for summary in matches.map(|pkg| pkg.summary().clone()) {
f(summary);
f(IndexSummary::Candidate(summary));
}
Poll::Ready(Ok(()))
}
Expand Down
5 changes: 3 additions & 2 deletions src/cargo/sources/git/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@
use crate::core::global_cache_tracker;
use crate::core::GitReference;
use crate::core::SourceId;
use crate::core::{Dependency, Package, PackageId, Summary};
use crate::core::{Dependency, Package, PackageId};
use crate::sources::git::utils::GitRemote;
use crate::sources::source::MaybePackage;
use crate::sources::source::QueryKind;
use crate::sources::source::Source;
use crate::sources::IndexSummary;
use crate::sources::PathSource;
use crate::util::cache_lock::CacheLockMode;
use crate::util::errors::CargoResult;
Expand Down Expand Up @@ -192,7 +193,7 @@ impl<'cfg> Source for GitSource<'cfg> {
&mut self,
dep: &Dependency,
kind: QueryKind,
f: &mut dyn FnMut(Summary),
f: &mut dyn FnMut(IndexSummary),
) -> Poll<CargoResult<()>> {
if let Some(src) = self.path_source.as_mut() {
src.query(dep, kind, f)
Expand Down
4 changes: 3 additions & 1 deletion src/cargo/sources/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ pub use self::config::SourceConfigMap;
pub use self::directory::DirectorySource;
pub use self::git::GitSource;
pub use self::path::PathSource;
pub use self::registry::{RegistrySource, CRATES_IO_DOMAIN, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
pub use self::registry::{
IndexSummary, RegistrySource, CRATES_IO_DOMAIN, CRATES_IO_INDEX, CRATES_IO_REGISTRY,
};
pub use self::replaced::ReplacedSource;

pub mod config;
Expand Down
Loading