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

Don't squash path dependencies with same package name #13863

Closed
wants to merge 2 commits into from
Closed
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
156 changes: 97 additions & 59 deletions src/cargo/core/resolver/encode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ impl EncodableResolve {
/// primary uses is to be used with `resolve_with_previous` to guide the
/// resolver to create a complete Resolve.
pub fn into_resolve(self, original: &str, ws: &Workspace<'_>) -> CargoResult<Resolve> {
let path_deps = build_path_deps(ws)?;
let path_deps = PathDeps::from_workspace(ws)?;
let mut checksums = HashMap::new();

let mut version = match self.version {
Expand Down Expand Up @@ -202,14 +202,18 @@ impl EncodableResolve {
if !all_pkgs.insert(enc_id.clone()) {
anyhow::bail!("package `{}` is specified twice in the lockfile", pkg.name);
}
let id = match pkg.source.as_deref().or_else(|| path_deps.get(&pkg.name)) {
let id = match pkg.source.as_deref().copied().or_else(|| {
path_deps
.get_path_dep(&pkg.name, &pkg.version)
.map(|package_id| package_id.source_id())
}) {
// We failed to find a local package in the workspace.
// It must have been removed and should be ignored.
None => {
debug!("path dependency now missing {} v{}", pkg.name, pkg.version);
continue;
}
Some(&source) => PackageId::try_new(&pkg.name, &pkg.version, source)?,
Some(source) => PackageId::try_new(&pkg.name, &pkg.version, source)?,
};

// If a package has a checksum listed directly on it then record
Expand Down Expand Up @@ -364,8 +368,12 @@ impl EncodableResolve {

let mut unused_patches = Vec::new();
for pkg in self.patch.unused {
let id = match pkg.source.as_deref().or_else(|| path_deps.get(&pkg.name)) {
Some(&src) => PackageId::try_new(&pkg.name, &pkg.version, src)?,
let id = match pkg.source.as_deref().copied().or_else(|| {
path_deps
.get_path_dep(&pkg.name, &pkg.version)
.map(|package_id| package_id.source_id())
}) {
Some(src) => PackageId::try_new(&pkg.name, &pkg.version, src)?,
None => continue,
};
unused_patches.push(id);
Expand Down Expand Up @@ -408,68 +416,98 @@ impl EncodableResolve {
}
}

fn build_path_deps(ws: &Workspace<'_>) -> CargoResult<HashMap<String, SourceId>> {
// If a crate is **not** a path source, then we're probably in a situation
// such as `cargo install` with a lock file from a remote dependency. In
// that case we don't need to fixup any path dependencies (as they're not
// actually path dependencies any more), so we ignore them.
let members = ws
.members()
.filter(|p| p.package_id().source_id().is_path())
.collect::<Vec<_>>();

let mut ret = HashMap::new();
let mut visited = HashSet::new();
for member in members.iter() {
ret.insert(
member.package_id().name().to_string(),
member.package_id().source_id(),
);
visited.insert(member.package_id().source_id());
}
for member in members.iter() {
build_pkg(member, ws, &mut ret, &mut visited);
struct PathDeps {
path_deps: HashMap<String, Vec<PackageId>>,
}

impl PathDeps {
/// Get a best-effort path dependency with given constraints.
///
/// If no exact version match, pickup a random version for patch updating trial
fn get_path_dep(&self, name: &str, version: &str) -> Option<&PackageId> {
let pkg_version = semver::Version::parse(version).ok()?;
self.path_deps.get(name).and_then(|versions| {
versions
.iter()
.find(|package_id| {
package_id.version().cmp_precedence(&pkg_version) == std::cmp::Ordering::Equal
})
.or_else(|| versions.last())
Copy link
Author

Choose a reason for hiding this comment

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

Use last() for behaviour consistency with old cargo. Old cargo use HashMap and insert for path deps construction, therefore the last inserted package was picked.

})
}
for deps in ws.root_patch()?.values() {
for dep in deps {

/// Return all path dependencies recursively in given workspace
fn from_workspace(ws: &Workspace<'_>) -> CargoResult<Self> {
// If a crate is **not** a path source, then we're probably in a situation
// such as `cargo install` with a lock file from a remote dependency. In
// that case we don't need to fixup any path dependencies (as they're not
// actually path dependencies any more), so we ignore them.
let members = ws
.members()
.filter(|p| p.package_id().source_id().is_path())
.collect::<Vec<_>>();

let mut ret = HashSet::new();
let mut visited = HashSet::new();
for member in members.iter() {
ret.insert(member.package_id());
visited.insert(member.package_id().source_id());
}
for member in members.iter() {
build_pkg(member, ws, &mut ret, &mut visited);
}
for deps in ws.root_patch()?.values() {
for dep in deps {
build_dep(dep, ws, &mut ret, &mut visited);
}
}
for (_, dep) in ws.root_replace() {
build_dep(dep, ws, &mut ret, &mut visited);
}
}
for (_, dep) in ws.root_replace() {
build_dep(dep, ws, &mut ret, &mut visited);
}

return Ok(ret);
let path_deps = {
// Mapping from package name to package ids
let mut deps: HashMap<String, Vec<PackageId>> = HashMap::new();
for package_id in ret {
deps.entry(package_id.name().to_string())
.or_default()
.push(package_id);
}
deps
};

return Ok(Self { path_deps });

fn build_pkg(
pkg: &Package,
ws: &Workspace<'_>,
ret: &mut HashMap<String, SourceId>,
visited: &mut HashSet<SourceId>,
) {
for dep in pkg.dependencies() {
build_dep(dep, ws, ret, visited);
fn build_pkg(
pkg: &Package,
ws: &Workspace<'_>,
ret: &mut HashSet<PackageId>,
visited: &mut HashSet<SourceId>,
) {
for dep in pkg.dependencies() {
build_dep(dep, ws, ret, visited);
}
}
}

fn build_dep(
dep: &Dependency,
ws: &Workspace<'_>,
ret: &mut HashMap<String, SourceId>,
visited: &mut HashSet<SourceId>,
) {
let id = dep.source_id();
if visited.contains(&id) || !id.is_path() {
ldm0 marked this conversation as resolved.
Show resolved Hide resolved
return;
fn build_dep(
dep: &Dependency,
ws: &Workspace<'_>,
ret: &mut HashSet<PackageId>,
visited: &mut HashSet<SourceId>,
) {
let id = dep.source_id();
if visited.contains(&id) || !id.is_path() {
return;
}
let path = match id.url().to_file_path() {
Ok(p) => p.join("Cargo.toml"),
Err(_) => return,
};
let Ok(pkg) = ws.load(&path) else { return };
ret.insert(pkg.package_id());
visited.insert(pkg.package_id().source_id());
build_pkg(&pkg, ws, ret, visited);
}
let path = match id.url().to_file_path() {
Ok(p) => p.join("Cargo.toml"),
Err(_) => return,
};
let Ok(pkg) = ws.load(&path) else { return };
ret.insert(pkg.name().to_string(), pkg.package_id().source_id());
visited.insert(pkg.package_id().source_id());
build_pkg(&pkg, ws, ret, visited);
}
}

Expand Down
117 changes: 117 additions & 0 deletions tests/testsuite/patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2914,3 +2914,120 @@ Caused by:
)
.run();
}

#[cargo_test]
fn patch_on_same_package_with_different_version() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "cargo-bug-repo"
version = "0.1.0"
edition = "2015"

[dependencies]
foo = { path = "vendor/foo" }
bar = { path = "vendor/bar" }

[patch.crates-io]
once_cell_0_1 = { package = "once_cell", path = "vendor/once_cell_0_1" }
once_cell_1_19 = { package = "once_cell", path = "vendor/once_cell_1_19" }
"#,
)
.file(
"src/main.rs",
r#"
extern crate foo;
extern crate bar;
fn main() {
foo::print_version();
bar::print_version();
}
"#,
)
.file(
"vendor/bar/src/lib.rs",
r#"
extern crate once_cell;
pub fn print_version() {
dbg!(once_cell::version());
}
"#,
)
.file(
"vendor/bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "0.1.0"
edition = "2015"

[dependencies]
once_cell = "1.19"
"#,
)
.file(
"vendor/foo/src/lib.rs",
r#"
extern crate once_cell;
pub fn print_version() {
dbg!(once_cell::version());
}
"#,
)
.file(
"vendor/foo/Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
edition = "2015"

[dependencies]
once_cell = "0.1"
"#,
)
.file(
"vendor/once_cell_0_1/src/lib.rs",
r#"
pub fn version() -> &'static str {
"0.1.0"
}
"#,
)
.file(
"vendor/once_cell_0_1/Cargo.toml",
&basic_manifest("once_cell", "0.1.0"),
)
.file(
"vendor/once_cell_1_19/src/lib.rs",
r#"
pub fn version() -> &'static str {
"1.19.0"
}
"#,
)
.file(
"vendor/once_cell_1_19/Cargo.toml",
&basic_manifest("once_cell", "1.19.0"),
)
.build();

p.cargo("check")
.with_stderr(
"\
[UPDATING] crates.io index
[LOCKING] 5 packages to latest compatible versions
[CHECKING] once_cell [..] ([CWD]/vendor/once_cell_[..])
[CHECKING] once_cell [..] ([CWD]/vendor/once_cell_[..])
[CHECKING] [..] v0.1.0 ([CWD]/vendor/[..])
[CHECKING] [..] v0.1.0 ([CWD]/vendor/[..])
[CHECKING] cargo-bug-repo v0.1.0 ([CWD])
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
",
)
.run();

p.cargo("check").with_stderr("[FINISHED] [..]").run();
ldm0 marked this conversation as resolved.
Show resolved Hide resolved
}