Skip to content

Commit

Permalink
Handle activation conflicts for [patch] sources
Browse files Browse the repository at this point in the history
This commit updates the resolver to ensure that it recognizes conflicts
when `[patch]` is used to augment an older version of what's already in
a source, for example. Previously the deduplication based on
semver-compatible versions didn't actually work when `[patch]` was used.
This meant that when you used `[patch]` it might not transitively affect
the entire crate graph, instead just giving you a version of a
dependency and everyone else. This violates the intention of `[patch]`!

The fix here is to catch this use case happening, when a `Dependency`
source specification mismatches an activated package we need to list a
second activation in the resolver to prevent major versions from being
selected from both the original source as well as the source of the id.

Closes #7117
  • Loading branch information
alexcrichton committed Jul 10, 2019
1 parent bee0487 commit 83bb30c
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 3 deletions.
35 changes: 34 additions & 1 deletion src/cargo/core/resolver/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,17 @@ impl Context {

/// Activate this summary by inserting it into our list of known activations.
///
/// The `parent` passed in here is the parent summary/dependency edge which
/// cased `summary` to get activated. This may not be present for the root
/// crate, for example.
///
/// Returns `true` if this summary with the given method is already activated.
pub fn flag_activated(&mut self, summary: &Summary, method: &Method) -> CargoResult<bool> {
pub fn flag_activated(
&mut self,
summary: &Summary,
method: &Method,
parent: Option<(&Summary, &Dependency)>,
) -> CargoResult<bool> {
let id = summary.package_id();
let age: ContextAge = self.age();
match self.activations.entry(id.as_activations_key()) {
Expand All @@ -121,6 +130,30 @@ impl Context {
);
}
v.insert((summary.clone(), age));

// If we've got a parent dependency which activated us, *and*
// the dependency has a different source id listed than the
// `summary` itself, then things get interesting. This basically
// means that a `[patch]` was used to augment `dep.source_id()`
// with `summary`.
//
// In this scenario we want to consider the activation key, as
// viewed from the perspective of `dep.source_id()`, as being
// fulfilled. This means that we need to add a second entry in
// the activations map for the source that was patched, in
// addition to the source of the actual `summary` itself.
//
// Without this it would be possible to have both 1.0.0 and
// 1.1.0 "from crates.io" in a dependency graph if one of those
// versions came from a `[patch]` source.
if let Some((_, dep)) = parent {
if dep.source_id() != id.source_id() {
let key = (id.name(), dep.source_id(), id.version().into());
let prev = self.activations.insert(key, (summary.clone(), age));
assert!(prev.is_none());
}
}

return Ok(false);
}
}
Expand Down
9 changes: 7 additions & 2 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -651,11 +651,16 @@ fn activate(
}
}

let activated = cx.flag_activated(&candidate, &method)?;
let activated = cx.flag_activated(&candidate, &method, parent)?;

let candidate = match registry.replacement_summary(candidate_pid) {
Some(replace) => {
if cx.flag_activated(replace, &method)? && activated {
// Note the `None` for parent here since `[replace]` is a bit wonky
// and doesn't activate the same things that `[patch]` typically
// does. TBH it basically cause panics in the test suite if
// `parent` is passed through here and `[replace]` is otherwise
// on life support so it's not critical to fix bugs anyway per se.
if cx.flag_activated(replace, &method, None)? && activated {
return Ok(None);
}
trace!(
Expand Down
59 changes: 59 additions & 0 deletions tests/testsuite/patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1020,3 +1020,62 @@ fn replace_prerelease() {

p.cargo("build").run();
}

#[cargo_test]
fn patch_older() {
Package::new("baz", "1.0.2").publish();

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = { path = 'bar' }
baz = "=1.0.1"
[patch.crates-io]
baz = { path = "./baz" }
"#,
)
.file("src/lib.rs", "")
.file(
"bar/Cargo.toml",
r#"
[project]
name = "bar"
version = "0.5.0"
authors = []
[dependencies]
baz = "1.0.0"
"#,
)
.file("bar/src/lib.rs", "")
.file(
"baz/Cargo.toml",
r#"
[project]
name = "baz"
version = "1.0.1"
authors = []
"#,
)
.file("baz/src/lib.rs", "")
.build();

p.cargo("build")
.with_stderr(
"\
[UPDATING] [..]
[COMPILING] baz v1.0.1 [..]
[COMPILING] bar v0.5.0 [..]
[COMPILING] foo v0.1.0 [..]
[FINISHED] [..]
",
)
.run();
}

0 comments on commit 83bb30c

Please sign in to comment.