Skip to content

Commit

Permalink
Auto merge of #12629 - Eh2406:WhyStringlyTyped, r=epage
Browse files Browse the repository at this point in the history
Read/write the encoded `cargo update --precise` in the same place

### What does this PR try to resolve?

There's a stringly typed interface between https://github.com/rust-lang/cargo/blob/de7537e63296ee13cb78611a988bcc9a6ac26134/src/cargo/ops/cargo_generate_lockfile.rs#L105 and https://github.com/rust-lang/cargo/blob/de7537e63296ee13cb78611a988bcc9a6ac26134/src/cargo/sources/registry/index.rs#L587, the only reason I found it with by finding the original commit #5205

As far as I can tell, anyone could just create this internally meaningful ~structure~ string by passing it on the command line.

This should get cleaned up, for now by moving the encoding and decoding in to the same file.

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

Internal refactor and test still pass.

### Additional information

I am hoping that in the redesign of `cargo update` we can come up with a better design for smuggling this data from the API all the way to querying the registry. It seems like locking the dependency to the selected version would be conceptually simpler, or using the patch system, or something.
  • Loading branch information
bors committed Sep 6, 2023
2 parents 84544d4 + 4d63fbc commit 016fe19
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 34 deletions.
52 changes: 46 additions & 6 deletions src/cargo/core/source/source_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ use crate::core::PackageId;
use crate::sources::registry::CRATES_IO_HTTP_INDEX;
use crate::sources::{DirectorySource, CRATES_IO_DOMAIN, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
use crate::sources::{GitSource, PathSource, RegistrySource};
use crate::util::{config, CanonicalUrl, CargoResult, Config, IntoUrl};
use crate::util::{config, CanonicalUrl, CargoResult, Config, IntoUrl, ToSemver};
use anyhow::Context;
use serde::de;
use serde::ser;
use std::cmp::{self, Ordering};
Expand Down Expand Up @@ -430,11 +431,6 @@ impl SourceId {
}
}

/// Gets the value of the precise field.
pub fn precise(self) -> Option<&'static str> {
self.inner.precise.as_deref()
}

/// Gets the Git reference if this is a git source, otherwise `None`.
pub fn git_reference(self) -> Option<&'static GitReference> {
match self.inner.kind {
Expand All @@ -443,6 +439,33 @@ impl SourceId {
}
}

/// Gets the value of the precise field.
pub fn precise(self) -> Option<&'static str> {
self.inner.precise.as_deref()
}

/// Check if the precise data field stores information for this `name`
/// from a call to [SourceId::with_precise_registry_version].
///
/// If so return the version currently in the lock file and the version to be updated to.
/// If specified, our own source will have a precise version listed of the form
// `<pkg>=<p_req>-><f_req>` where `<pkg>` is the name of a crate on
// this source, `<p_req>` is the version installed and `<f_req>` is the
// version requested (argument to `--precise`).
pub fn precise_registry_version(
self,
name: &str,
) -> Option<(semver::Version, semver::Version)> {
self.inner
.precise
.as_deref()
.and_then(|p| p.strip_prefix(name)?.strip_prefix('='))
.map(|p| {
let (current, requested) = p.split_once("->").unwrap();
(current.to_semver().unwrap(), requested.to_semver().unwrap())
})
}

/// Creates a new `SourceId` from this source with the given `precise`.
pub fn with_precise(self, v: Option<String>) -> SourceId {
SourceId::wrap(SourceIdInner {
Expand All @@ -451,6 +474,23 @@ impl SourceId {
})
}

/// When updating a lock file on a version using `cargo update --precise`
/// the requested version is stored in the precise field.
/// On a registry dependency we also need to keep track of the package that
/// should be updated and even which of the versions should be updated.
/// All of this gets encoded in the precise field using this method.
/// The data can be read with [SourceId::precise_registry_version]
pub fn with_precise_registry_version(
self,
name: impl fmt::Display,
version: &semver::Version,
precise: &str,
) -> CargoResult<SourceId> {
semver::Version::parse(precise)
.with_context(|| format!("invalid version format for precise version `{precise}`"))?;
Ok(self.with_precise(Some(format!("{}={}->{}", name, version, precise))))
}

/// Returns `true` if the remote registry is the standard <https://crates.io>.
pub fn is_crates_io(self) -> bool {
match self.inner.kind {
Expand Down
25 changes: 12 additions & 13 deletions src/cargo/ops/cargo_generate_lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use crate::core::{Resolve, SourceId, Workspace};
use crate::ops;
use crate::util::config::Config;
use crate::util::CargoResult;
use anyhow::Context;
use std::collections::{BTreeMap, HashSet};
use termcolor::Color::{self, Cyan, Green, Red, Yellow};
use tracing::debug;
Expand Down Expand Up @@ -88,27 +87,27 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
} else {
let mut sources = Vec::new();
for name in opts.to_update.iter() {
let dep = previous_resolve.query(name)?;
let pid = previous_resolve.query(name)?;
if opts.recursive {
fill_with_deps(&previous_resolve, dep, &mut to_avoid, &mut HashSet::new());
fill_with_deps(&previous_resolve, pid, &mut to_avoid, &mut HashSet::new());
} else {
to_avoid.insert(dep);
to_avoid.insert(pid);
sources.push(match opts.precise {
Some(precise) => {
// TODO: see comment in `resolve.rs` as well, but this
// seems like a pretty hokey reason to single out
// the registry as well.
let precise = if dep.source_id().is_registry() {
semver::Version::parse(precise).with_context(|| {
format!("invalid version format for precise version `{}`", precise)
})?;
format!("{}={}->{}", dep.name(), dep.version(), precise)
if pid.source_id().is_registry() {
pid.source_id().with_precise_registry_version(
pid.name(),
pid.version(),
precise,
)?
} else {
precise.to_string()
};
dep.source_id().with_precise(Some(precise))
pid.source_id().with_precise(Some(precise.to_string()))
}
}
None => dep.source_id().with_precise(None),
None => pid.source_id().with_precise(None),
});
}
if let Ok(unused_id) =
Expand Down
18 changes: 3 additions & 15 deletions src/cargo/sources/registry/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,7 @@ use crate::core::{PackageId, SourceId, Summary};
use crate::sources::registry::{LoadResponse, RegistryData};
use crate::util::interning::InternedString;
use crate::util::IntoUrl;
use crate::util::{
internal, CargoResult, Config, Filesystem, OptVersionReq, PartialVersion, ToSemver,
};
use crate::util::{internal, CargoResult, Config, Filesystem, OptVersionReq, PartialVersion};
use anyhow::bail;
use cargo_util::{paths, registry::make_dep_path};
use semver::Version;
Expand Down Expand Up @@ -582,18 +580,8 @@ impl<'cfg> RegistryIndex<'cfg> {
.filter(|s| !s.yanked || yanked_whitelist.contains(&s.summary.package_id()))
.map(|s| s.summary.clone());

// Handle `cargo update --precise` here. If specified, our own source
// will have a precise version listed of the form
// `<pkg>=<p_req>o-><f_req>` where `<pkg>` is the name of a crate on
// this source, `<p_req>` is the version installed and `<f_req> is the
// version requested (argument to `--precise`).
let precise = source_id
.precise()
.filter(|p| p.starts_with(name) && p[name.len()..].starts_with('='))
.map(|p| {
let (current, requested) = p[name.len() + 1..].split_once("->").unwrap();
(current.to_semver().unwrap(), requested.to_semver().unwrap())
});
// Handle `cargo update --precise` here.
let precise = source_id.precise_registry_version(name);
let summaries = summaries.filter(|s| match &precise {
Some((current, requested)) => {
if req.matches(current) {
Expand Down

0 comments on commit 016fe19

Please sign in to comment.