Skip to content

Commit

Permalink
Auto merge of #12643 - Eh2406:IndexSummaryEnum, r=epage
Browse files Browse the repository at this point in the history 
Index summary enum

### What does this PR try to resolve?

This is a tiny incremental part of allowing registries to return richer information about summaries that are not available for resolution. Eventually this should lead to better error messages, but for now it's just an internal re-factor. 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

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

Internal re-factor and tests still pass.
  • Loading branch information
@bors
bors committed Sep 11, 2023
2 parents c2d26f3 + 5c8cf8d commit bef47cb
Show file tree
Hide file tree
Showing 3 changed files with 205 additions and 83 deletions.
219 changes: 155 additions & 64 deletions src/cargo/sources/registry/index.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,11 +189,64 @@ enum MaybeIndexSummary {
/// from a line from a raw index file, or a JSON blob from on-disk index cache.
///
/// In addition to a full [`Summary`], we have information on whether it is `yanked`.
pub struct IndexSummary {
pub summary: Summary,
pub yanked: bool,
/// Schema version, see [`IndexPackage::v`].
v: u32,
#[derive(Clone, Debug)]
pub enum IndexSummary {
/// Available for consideration
Candidate(Summary),
/// Yanked within its registry
Yanked(Summary),
/// Not available as we are offline and create is not downloaded yet
Offline(Summary),
/// From a newer schema version and is likely incomplete or inaccurate
Unsupported(Summary, u32),
}

impl IndexSummary {
/// Extract the summary from any variant
pub fn as_summary(&self) -> &Summary {
match self {
IndexSummary::Candidate(sum)
| IndexSummary::Yanked(sum)
| IndexSummary::Offline(sum)
| IndexSummary::Unsupported(sum, _) => sum,
}
}

/// Extract the summary from any variant
pub fn into_summary(self) -> Summary {
match self {
IndexSummary::Candidate(sum)
| IndexSummary::Yanked(sum)
| IndexSummary::Offline(sum)
| IndexSummary::Unsupported(sum, _) => sum,
}
}

/// Extract the package id from any variant
pub fn package_id(&self) -> PackageId {
match self {
IndexSummary::Candidate(sum)
| IndexSummary::Yanked(sum)
| IndexSummary::Offline(sum)
| IndexSummary::Unsupported(sum, _) => sum.package_id(),
}
}

/// Returns `true` if the index summary is [`Yanked`].
///
/// [`Yanked`]: IndexSummary::Yanked
#[must_use]
pub fn is_yanked(&self) -> bool {
matches!(self, Self::Yanked(..))
}

/// Returns `true` if the index summary is [`Offline`].
///
/// [`Offline`]: IndexSummary::Offline
#[must_use]
pub fn is_offline(&self) -> bool {
matches!(self, Self::Offline(..))
}
}

/// A representation of the cache on disk that Cargo maintains of summaries.
Expand Down Expand Up @@ -385,13 +438,13 @@ impl<'cfg> RegistryIndex<'cfg> {
/// the index file, aka [`IndexSummary`].
pub fn hash(&mut self, pkg: PackageId, load: &mut dyn RegistryData) -> Poll<CargoResult<&str>> {
let req = OptVersionReq::exact(pkg.version());
let summary = self.summaries(&pkg.name(), &req, load)?;
let summary = self.summaries(pkg.name(), &req, load)?;
let summary = ready!(summary)
.filter(|s| s.summary.version() == pkg.version())
.filter(|s| s.package_id().version() == pkg.version())
.next();
Poll::Ready(Ok(summary
.ok_or_else(|| internal(format!("no hash listed for {}", pkg)))?
.summary
.as_summary()
.checksum()
.ok_or_else(|| internal(format!("no hash listed for {}", pkg)))?))
}
Expand All @@ -407,9 +460,9 @@ impl<'cfg> RegistryIndex<'cfg> {
///
/// Internally there's quite a few layer of caching to amortize this cost
/// though since this method is called quite a lot on null builds in Cargo.
pub fn summaries<'a, 'b>(
fn summaries<'a, 'b>(
&'a mut self,
name: &str,
name: InternedString,
req: &'b OptVersionReq,
load: &mut dyn RegistryData,
) -> Poll<CargoResult<impl Iterator<Item = &'a IndexSummary> + 'b>>
Expand All @@ -421,7 +474,6 @@ impl<'cfg> RegistryIndex<'cfg> {
let source_id = self.source_id;

// First up parse what summaries we have available.
let name = InternedString::new(name);
let summaries = ready!(self.load_summaries(name, load)?);

// Iterate over our summaries, extract all relevant ones which match our
Expand All @@ -435,26 +487,27 @@ impl<'cfg> RegistryIndex<'cfg> {
.versions
.iter_mut()
.filter_map(move |(k, v)| if req.matches(k) { Some(v) } else { None })
.filter_map(move |maybe| match maybe.parse(raw_data, source_id) {
Ok(summary) => Some(summary),
Err(e) => {
info!("failed to parse `{}` registry package: {}", name, e);
None
}
})
.filter(move |is| {
if is.v == 3 && bindeps {
true
} else if is.v > INDEX_V_MAX {
debug!(
"unsupported schema version {} ({} {})",
is.v,
is.summary.name(),
is.summary.version()
);
false
} else {
true
.filter_map(move |maybe| {
match maybe.parse(raw_data, source_id, bindeps) {
Ok(sum @ IndexSummary::Candidate(_) | sum @ IndexSummary::Yanked(_)) => {
Some(sum)
}
Ok(IndexSummary::Unsupported(summary, v)) => {
debug!(
"unsupported schema version {} ({} {})",
v,
summary.name(),
summary.version()
);
None
}
Ok(IndexSummary::Offline(_)) => {
unreachable!("We do not check for off-line until later")
}
Err(e) => {
info!("failed to parse `{}` registry package: {}", name, e);
None
}
}
})))
}
Expand Down Expand Up @@ -518,7 +571,7 @@ impl<'cfg> RegistryIndex<'cfg> {
/// This is primarily used by [`Source::query`](super::Source).
pub fn query_inner(
&mut self,
name: &str,
name: InternedString,
req: &OptVersionReq,
load: &mut dyn RegistryData,
yanked_whitelist: &HashSet<PackageId>,
Expand All @@ -535,14 +588,35 @@ impl<'cfg> RegistryIndex<'cfg> {
// then cargo will fail to download and an error message
// indicating that the required dependency is unavailable while
// offline will be displayed.
if ready!(self.query_inner_with_online(name, req, load, yanked_whitelist, f, false)?)
> 0
{
let mut called = false;
let callback = &mut |s: IndexSummary| {
if !s.is_offline() {
called = true;
f(s.into_summary());
}
};
ready!(self.query_inner_with_online(
name,
req,
load,
yanked_whitelist,
callback,
false
)?);
if called {
return Poll::Ready(Ok(()));
}
}
self.query_inner_with_online(name, req, load, yanked_whitelist, f, true)
.map_ok(|_| ())
self.query_inner_with_online(
name,
req,
load,
yanked_whitelist,
&mut |s| {
f(s.into_summary());
},
true,
)
}

/// Inner implementation of [`Self::query_inner`]. Returns the number of
Expand All @@ -551,13 +625,13 @@ impl<'cfg> RegistryIndex<'cfg> {
/// The `online` controls whether Cargo can access the network when needed.
fn query_inner_with_online(
&mut self,
name: &str,
name: InternedString,
req: &OptVersionReq,
load: &mut dyn RegistryData,
yanked_whitelist: &HashSet<PackageId>,
f: &mut dyn FnMut(Summary),
f: &mut dyn FnMut(IndexSummary),
online: bool,
) -> Poll<CargoResult<usize>> {
) -> Poll<CargoResult<()>> {
let source_id = self.source_id;

let summaries = ready!(self.summaries(name, req, load))?;
Expand All @@ -573,23 +647,28 @@ impl<'cfg> RegistryIndex<'cfg> {
// does not satisfy the requirements, then resolution will
// fail. Unfortunately, whether or not something is optional
// is not known here.
.filter(|s| (online || load.is_crate_downloaded(s.summary.package_id())))
.map(|s| {
if online || load.is_crate_downloaded(s.package_id()) {
s.clone()
} else {
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.yanked || yanked_whitelist.contains(&s.summary.package_id()))
.map(|s| s.summary.clone());
.filter(|s| !s.is_yanked() || yanked_whitelist.contains(&s.package_id()));

// Handle `cargo update --precise` here.
let precise = source_id.precise_registry_version(name);
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.version();
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,
Expand All @@ -609,12 +688,8 @@ impl<'cfg> RegistryIndex<'cfg> {
None => true,
});

let mut count = 0;
for summary in summaries {
f(summary);
count += 1;
}
Poll::Ready(Ok(count))
summaries.for_each(f);
Poll::Ready(Ok(()))
}

/// Looks into the summaries to check if a package has been yanked.
Expand All @@ -624,9 +699,9 @@ impl<'cfg> RegistryIndex<'cfg> {
load: &mut dyn RegistryData,
) -> Poll<CargoResult<bool>> {
let req = OptVersionReq::exact(pkg.version());
let found = ready!(self.summaries(&pkg.name(), &req, load))?
.filter(|s| s.summary.version() == pkg.version())
.any(|summary| summary.yanked);
let found = ready!(self.summaries(pkg.name(), &req, load))?
.filter(|s| s.package_id().version() == pkg.version())
.any(|s| s.is_yanked());
Poll::Ready(Ok(found))
}
}
Expand Down Expand Up @@ -682,6 +757,8 @@ impl Summaries {

let response = ready!(load.load(root, relative, index_version.as_deref())?);

let bindeps = config.cli_unstable().bindeps;

match response {
LoadResponse::CacheValid => {
tracing::debug!("fast path for registry cache of {:?}", relative);
Expand Down Expand Up @@ -712,7 +789,7 @@ impl Summaries {
// allow future cargo implementations to break the
// interpretation of each line here and older cargo will simply
// ignore the new lines.
let summary = match IndexSummary::parse(line, source_id) {
let summary = match IndexSummary::parse(line, source_id, bindeps) {
Ok(summary) => summary,
Err(e) => {
// This should only happen when there is an index
Expand All @@ -731,7 +808,7 @@ impl Summaries {
continue;
}
};
let version = summary.summary.package_id().version().clone();
let version = summary.package_id().version().clone();
cache.versions.push((version.clone(), line));
ret.versions.insert(version, summary.into());
}
Expand Down Expand Up @@ -865,12 +942,17 @@ impl MaybeIndexSummary {
/// Does nothing if this is already `Parsed`, and otherwise the `raw_data`
/// passed in is sliced with the bounds in `Unparsed` and then actually
/// parsed.
fn parse(&mut self, raw_data: &[u8], source_id: SourceId) -> CargoResult<&IndexSummary> {
fn parse(
&mut self,
raw_data: &[u8],
source_id: SourceId,
bindeps: bool,
) -> CargoResult<&IndexSummary> {
let (start, end) = match self {
MaybeIndexSummary::Unparsed { start, end } => (*start, *end),
MaybeIndexSummary::Parsed(summary) => return Ok(summary),
};
let summary = IndexSummary::parse(&raw_data[start..end], source_id)?;
let summary = IndexSummary::parse(&raw_data[start..end], source_id, bindeps)?;
*self = MaybeIndexSummary::Parsed(summary);
match self {
MaybeIndexSummary::Unparsed { .. } => unreachable!(),
Expand All @@ -891,7 +973,7 @@ impl IndexSummary {
///
/// The `line` provided is expected to be valid JSON. It is supposed to be
/// a [`IndexPackage`].
fn parse(line: &[u8], source_id: SourceId) -> CargoResult<IndexSummary> {
fn parse(line: &[u8], source_id: SourceId, bindeps: bool) -> CargoResult<IndexSummary> {
// ****CAUTION**** Please be extremely careful with returning errors
// from this function. Entries that error are not included in the
// index cache, and can cause cargo to get confused when switching
Expand Down Expand Up @@ -924,11 +1006,20 @@ impl IndexSummary {
}
let mut summary = Summary::new(pkgid, deps, &features, links, rust_version)?;
summary.set_checksum(cksum);
Ok(IndexSummary {
summary,
yanked: yanked.unwrap_or(false),
v,
})

let v_max = if bindeps {
INDEX_V_MAX + 1
} else {
INDEX_V_MAX
};

if v_max < v {
Ok(IndexSummary::Unsupported(summary, v))
} else if yanked.unwrap_or(false) {
Ok(IndexSummary::Yanked(summary))
} else {
Ok(IndexSummary::Candidate(summary))
}
}
}

Expand Down
Loading

0 comments on commit bef47cb

Please sign in to comment.