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

Source::{fuzzy_}query{_vec} can say "try again" #8985

Closed
wants to merge 9 commits into from
Closed

Source::{fuzzy_}query{_vec} can say "try again" #8985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
466e719
Source::{fuzzy_}query{_vec} can say "try again"
Eh2406 Dec 16, 2020
c354199
RegistryData::load can say "try again"
Eh2406 Dec 18, 2020
ea37a74
pull the loop up out of summary_for_patch
Eh2406 Dec 18, 2020
fdf08eb
reuse the resolvers cache
Eh2406 Dec 18, 2020
240cb5f
don't retry just for an error message
Eh2406 Dec 18, 2020
08f96cd
move Poll one layer up in is_yanked
Eh2406 Dec 21, 2020
33190fa
just assert when downloading
Eh2406 Dec 21, 2020
1d9e675
add a PollExt so we can use expect instead of match
Eh2406 Dec 30, 2020
44012fd
I missed one unneeded loop.
Eh2406 Dec 30, 2020
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
use std::fmt;
use std::fmt::Write;
use std::rc::Rc;
use std::task::Poll;
use std::time::Instant;

use cargo::core::dependency::DepKind;
Expand Down Expand Up @@ -130,14 +131,14 @@ pub fn resolve_with_config_raw(
dep: &Dependency,
f: &mut dyn FnMut(Summary),
fuzzy: bool,
) -> CargoResult<()> {
) -> CargoResult<Poll<()>> {
for summary in self.list.iter() {
if fuzzy || dep.matches(summary) {
self.used.insert(summary.package_id());
f(summary.clone());
}
}
Ok(())
Ok(Poll::Ready(()))
}

fn describe_source(&self, _src: SourceId) -> String {
Expand Down
208 changes: 128 additions & 80 deletions src/cargo/core/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use std::collections::{HashMap, HashSet};
use std::task::Poll;

use crate::core::PackageSet;
use crate::core::{Dependency, PackageId, Source, SourceId, SourceMap, Summary};
use crate::sources::config::SourceConfigMap;
use crate::util::errors::{CargoResult, CargoResultExt};
use crate::util::interning::InternedString;
use crate::util::network::PollExt;
use crate::util::{profile, CanonicalUrl, Config};
use anyhow::bail;
use log::{debug, trace};
Expand All @@ -21,12 +23,11 @@ pub trait Registry {
dep: &Dependency,
f: &mut dyn FnMut(Summary),
fuzzy: bool,
) -> CargoResult<()>;
) -> CargoResult<Poll<()>>;

fn query_vec(&mut self, dep: &Dependency, fuzzy: bool) -> CargoResult<Vec<Summary>> {
fn query_vec(&mut self, dep: &Dependency, fuzzy: bool) -> CargoResult<Poll<Vec<Summary>>> {
let mut ret = Vec::new();
self.query(dep, &mut |s| ret.push(s), fuzzy)?;
Ok(ret)
Ok(self.query(dep, &mut |s| ret.push(s), fuzzy)?.map(|_| ret))
}

fn describe_source(&self, source: SourceId) -> String;
Expand Down Expand Up @@ -260,67 +261,86 @@ impl<'cfg> PackageRegistry<'cfg> {
// Remember that each dependency listed in `[patch]` has to resolve to
// precisely one package, so that's why we're just creating a flat list
// of summaries which should be the same length as `deps` above.
let unlocked_summaries = deps
.iter()
.map(|(orig_patch, locked)| {
// Remove double reference in orig_patch. Is there maybe a
// magic pattern that could avoid this?
let orig_patch = *orig_patch;
// Use the locked patch if it exists, otherwise use the original.
let dep = match locked {
Some((locked_patch, _locked_id)) => locked_patch,
None => orig_patch,
};
debug!(
"registering a patch for `{}` with `{}`",
url,
dep.package_name()
);

// Go straight to the source for resolving `dep`. Load it as we
// normally would and then ask it directly for the list of summaries
// corresponding to this `dep`.
self.ensure_loaded(dep.source_id(), Kind::Normal)
.chain_err(|| {
anyhow::format_err!(
"failed to load source for dependency `{}`",
dep.package_name()
)
})?;

let source = self
.sources
.get_mut(dep.source_id())
.expect("loaded source not present");
let summaries = source.query_vec(dep)?;
let (summary, should_unlock) =
summary_for_patch(orig_patch, locked, summaries, source).chain_err(|| {
format!(
"patch for `{}` in `{}` failed to resolve",
orig_patch.package_name(),
let unlocked_summaries = loop {
let try_summaries =
deps.iter()
.map(|(orig_patch, locked)| {
// Remove double reference in orig_patch. Is there maybe a
// magic pattern that could avoid this?
let orig_patch = *orig_patch;
// Use the locked patch if it exists, otherwise use the original.
let dep = match locked {
Some((locked_patch, _locked_id)) => locked_patch,
None => orig_patch,
};
debug!(
"registering a patch for `{}` with `{}`",
url,
)
})?;
debug!(
"patch summary is {:?} should_unlock={:?}",
summary, should_unlock
);
if let Some(unlock_id) = should_unlock {
unlock_patches.push((orig_patch.clone(), unlock_id));
}
dep.package_name()
);

// Go straight to the source for resolving `dep`. Load it as we
// normally would and then ask it directly for the list of summaries
// corresponding to this `dep`.
self.ensure_loaded(dep.source_id(), Kind::Normal)
.chain_err(|| {
anyhow::format_err!(
"failed to load source for dependency `{}`",
dep.package_name()
)
})?;

let source = self
.sources
.get_mut(dep.source_id())
.expect("loaded source not present");
let summaries = match source.query_vec(dep)? {
Poll::Ready(deps) => deps,
Poll::Pending => return Ok(Poll::Pending),
};
let (summary, should_unlock) =
match summary_for_patch(orig_patch, locked, summaries, source)
.chain_err(|| {
format!(
"patch for `{}` in `{}` failed to resolve",
orig_patch.package_name(),
url,
)
})? {
Poll::Ready(x) => x,
Poll::Pending => {
return Ok(Poll::Pending);
}
};
debug!(
"patch summary is {:?} should_unlock={:?}",
summary, should_unlock
);
if let Some(unlock_id) = should_unlock {
unlock_patches.push((orig_patch.clone(), unlock_id));
}

if *summary.package_id().source_id().canonical_url() == canonical {
anyhow::bail!(
"patch for `{}` in `{}` points to the same source, but \
if *summary.package_id().source_id().canonical_url() == canonical {
anyhow::bail!(
"patch for `{}` in `{}` points to the same source, but \
patches must point to different sources",
dep.package_name(),
url
);
}
Ok(summary)
})
.collect::<CargoResult<Vec<_>>>()
.chain_err(|| anyhow::format_err!("failed to resolve patches for `{}`", url))?;
dep.package_name(),
url
);
}
Ok(Poll::Ready(summary))
})
.collect::<CargoResult<Vec<_>>>()
.chain_err(|| anyhow::format_err!("failed to resolve patches for `{}`", url))?;
if try_summaries.iter().all(|p| p.is_ready()) {
break try_summaries
.into_iter()
.map(|p| p.expect("we just checked for this!"))
.collect::<Vec<_>>();
} else {
// TODO: dont hot loop for it to be Ready
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I imagine this bottoms out in something like self.wait_for_sources_to_be_ready(). Each source would already be registered in some internal map of PackageRegistry if we're blocked on it, and then we'd ask each source, in sequence, "do your blocking thing now".

}
};

let mut name_and_version = HashSet::new();
for summary in unlocked_summaries.iter() {
Expand Down Expand Up @@ -396,9 +416,13 @@ impl<'cfg> PackageRegistry<'cfg> {
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 = src.query_vec(&dep)?;
if !results.is_empty() {
return Ok(Some(results.remove(0)));
match src.query_vec(&dep)? {
Poll::Ready(mut results) => {
if !results.is_empty() {
return Ok(Some(results.remove(0)));
}
}
Poll::Pending => bail!("overrides have to be on path deps, how did we get here?"),
}
}
Ok(None)
Expand Down Expand Up @@ -487,7 +511,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
dep: &Dependency,
f: &mut dyn FnMut(Summary),
fuzzy: bool,
) -> CargoResult<()> {
) -> CargoResult<Poll<()>> {
assert!(self.patches_locked);
let (override_summary, n, to_warn) = {
// Look for an override and get ready to query the real source.
Expand Down Expand Up @@ -521,7 +545,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
Some(summary) => (summary, 1, Some(patch)),
None => {
f(patch);
return Ok(());
return Ok(Poll::Ready(()));
}
}
} else {
Expand Down Expand Up @@ -549,7 +573,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
let source = self.sources.get_mut(dep.source_id());
match (override_summary, source) {
(Some(_), None) => anyhow::bail!("override found but no real ones"),
(None, None) => return Ok(()),
(None, None) => return Ok(Poll::Ready(())),

// If we don't have an override then we just ship
// everything upstairs after locking the summary
Expand Down Expand Up @@ -597,10 +621,13 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
n += 1;
to_warn = Some(summary);
};
if fuzzy {
source.fuzzy_query(dep, callback)?;
let pend = if fuzzy {
source.fuzzy_query(dep, callback)?
} else {
source.query(dep, callback)?;
source.query(dep, callback)?
};
if pend.is_pending() {
Copy link
Member

Choose a reason for hiding this comment

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

This is where I'd imagine that a record of this dependency's source id is recorded in an internal map for us to later iterate over and ask the source to block on things.

return Ok(Poll::Pending);
}
}
(override_summary, n, to_warn)
Expand All @@ -615,7 +642,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
self.warn_bad_override(&override_summary, &summary)?;
}
f(self.lock(override_summary));
Ok(())
Ok(Poll::Ready(()))
}

fn describe_source(&self, id: SourceId) -> String {
Expand Down Expand Up @@ -748,9 +775,9 @@ fn summary_for_patch(
locked: &Option<(Dependency, PackageId)>,
mut summaries: Vec<Summary>,
source: &mut dyn Source,
) -> CargoResult<(Summary, Option<PackageId>)> {
) -> CargoResult<Poll<(Summary, Option<PackageId>)>> {
if summaries.len() == 1 {
return Ok((summaries.pop().unwrap(), None));
return Ok(Poll::Ready((summaries.pop().unwrap(), None)));
}
if summaries.len() > 1 {
// TODO: In the future, it might be nice to add all of these
Expand Down Expand Up @@ -779,22 +806,43 @@ fn summary_for_patch(
// No summaries found, try to help the user figure out what is wrong.
if let Some((_locked_patch, locked_id)) = locked {
// Since the locked patch did not match anything, try the unlocked one.
let orig_matches = source.query_vec(orig_patch).unwrap_or_else(|e| {
let orig_matches = match source.query_vec(orig_patch) {
Ok(Poll::Ready(deps)) => Ok(deps),
Ok(Poll::Pending) => {
return Ok(Poll::Pending);
}
Err(x) => Err(x),
}
.unwrap_or_else(|e| {
log::warn!(
"could not determine unlocked summaries for dep {:?}: {:?}",
orig_patch,
e
);
Vec::new()
});
let (summary, _) = summary_for_patch(orig_patch, &None, orig_matches, source)?;
// The unlocked version found a match. This returns a value to
// indicate that this entry should be unlocked.
return Ok((summary, Some(*locked_id)));
return Ok(
match summary_for_patch(orig_patch, &None, orig_matches, source)? {
Poll::Ready((summary, _)) => {
// The unlocked version found a match. This returns a value to
// indicate that this entry should be unlocked.
Poll::Ready((summary, Some(*locked_id)))
}
Poll::Pending => Poll::Pending,
},
);
}
// Try checking if there are *any* packages that match this by name.
let name_only_dep = Dependency::new_override(orig_patch.package_name(), orig_patch.source_id());
let name_summaries = source.query_vec(&name_only_dep).unwrap_or_else(|e| {

let name_summaries = match source.query_vec(&name_only_dep) {
Ok(Poll::Ready(deps)) => Ok(deps),
Ok(Poll::Pending) => {
return Ok(Poll::Pending);
}
Err(x) => Err(x),
}
.unwrap_or_else(|e| {
log::warn!(
"failed to do name-only summary query for {:?}: {:?}",
name_only_dep,
Expand Down
Loading