Fixes around multiple `[patch]` per crate #7303

alexcrichton · 2019-08-26T19:58:15Z

This commit fixes a few bugs in handling of [patch] where multiple
version of the same crate name have been patched in. Two sub-issues were
discovered when investigating #7264:

The first issue is that the source of the assertion, the logic in
lock, revealed a fundamental flaw in the logic. The lock function
serves the purpose of applying a lock file to a dependency candidate
and ensure that it stays within locked versions of dependencies, if
they're previously found. The logic here to handle [patch], however,
happened a bit too late and was a bit too zealous to simply lock a
dependency by name instead of accounting for the version as well.

The updated logic is to move the locking of dependencies here to
during the normal iteration over the list of dependencies. Adjacent to
matches_id we check matches_ignoring_source. If the latter returns
true then we double-check that the previous dependency is still in
[patch], and then we let it through. This means that patches with
multiple versions should be correctly handled where edges drawn with
[patch] are preserved.
The second issue, after fixing this, was found where if the exact same
version was listed in [patch] multiple times then we would
continuously update the original source since one of the replacements
gets lost along the way. This commit adds a first-class warning
disallowing patches pointing to the exact same crate version, since we
don't have a way to prioritize amongst them anyway.

Closes #7264

rust-highfive · 2019-08-26T19:58:18Z

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

bors · 2019-08-27T16:13:13Z

☔ The latest upstream changes (presumably #7263) made this pull request unmergeable. Please resolve the merge conflicts.

tests/testsuite/patch.rs

ehuss · 2019-08-27T17:06:14Z

One more thing, can you add some documentation on LockedMap? Like what the different values are, and what its purpose is. It is a bit difficult to understand how all the locking works, and I think that would be a good place to start clarifying. For example, you have to read a lot of code just to understand what the tuple values are, or that the String key is a package name.

alexcrichton · 2019-08-27T21:04:23Z

Ok I think that should cover everything, thanks for taking a look!

Definitely agreed that we should have internal documentation of how lock files work, as it's pretty subtle in how it actually ends up working!

ehuss · 2019-08-27T21:19:10Z

src/cargo/core/registry.rs

@@ -79,7 +80,28 @@ pub struct PackageRegistry<'cfg> {
    patches_available: HashMap<Url, Vec<PackageId>>,
 }

-type LockedMap = HashMap<SourceId, HashMap<String, Vec<(PackageId, Vec<PackageId>)>>>;
+/// A map of all "locked packages" which is filled in when parsing a lock file
+/// and is used to guide dependency resolution by altering summaires as they're


Suggested change

/// and is used to guide dependency resolution by altering summaires as they're

/// and is used to guide dependency resolution by altering summaries as they're

Thanks, r=me with typo fix.

This commit fixes a few bugs in handling of `[patch]` where multiple version of the same crate name have been patched in. Two sub-issues were discovered when investigating rust-lang#7264: * The first issue is that the source of the assertion, the logic in `lock`, revealed a fundamental flaw in the logic. The `lock` function serves the purpose of applying a lock file to a dependency candidate and ensure that it stays within locked versions of dependencies, if they're previously found. The logic here to handle `[patch]`, however, happened a bit too late and was a bit too zealous to simply lock a dependency by name instead of accounting for the version as well. The updated logic is to move the locking of dependencies here to during the normal iteration over the list of dependencies. Adjacent to `matches_id` we check `matches_ignoring_source`. If the latter returns `true` then we double-check that the previous dependency is still in `[patch]`, and then we let it through. This means that patches with multiple versions should be correctly handled where edges drawn with `[patch]` are preserved. * The second issue, after fixing this, was found where if the exact same version was listed in `[patch]` multiple times then we would continuously update the original source since one of the replacements gets lost along the way. This commit adds a first-class warning disallowing patches pointing to the exact same crate version, since we don't have a way to prioritize amongst them anyway. Closes rust-lang#7264

alexcrichton · 2019-08-27T22:12:39Z

@bors: r=ehuss

bors · 2019-08-27T22:12:40Z

📌 Commit 803bf32 has been approved by ehuss

bors · 2019-08-27T22:12:48Z

⌛ Testing commit 803bf32 with merge ba37dac...

Fixes around multiple `[patch]` per crate This commit fixes a few bugs in handling of `[patch]` where multiple version of the same crate name have been patched in. Two sub-issues were discovered when investigating #7264: * The first issue is that the source of the assertion, the logic in `lock`, revealed a fundamental flaw in the logic. The `lock` function serves the purpose of applying a lock file to a dependency candidate and ensure that it stays within locked versions of dependencies, if they're previously found. The logic here to handle `[patch]`, however, happened a bit too late and was a bit too zealous to simply lock a dependency by name instead of accounting for the version as well. The updated logic is to move the locking of dependencies here to during the normal iteration over the list of dependencies. Adjacent to `matches_id` we check `matches_ignoring_source`. If the latter returns `true` then we double-check that the previous dependency is still in `[patch]`, and then we let it through. This means that patches with multiple versions should be correctly handled where edges drawn with `[patch]` are preserved. * The second issue, after fixing this, was found where if the exact same version was listed in `[patch]` multiple times then we would continuously update the original source since one of the replacements gets lost along the way. This commit adds a first-class warning disallowing patches pointing to the exact same crate version, since we don't have a way to prioritize amongst them anyway. Closes #7264

bors · 2019-08-27T22:34:01Z

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing ba37dac to master...

Update cargo, books ## cargo 8 commits in 22f7dd0495cd72ce2082d318d5a9b4dccb9c5b8c..fe0e5a48b75da2b405c8ce1ba2674e174ae11d5d 2019-08-27 16:10:51 +0000 to 2019-09-04 00:51:27 +0000 - Rename `--all` to `--workspace` (rust-lang/cargo#7241) - Basic standard library support. (rust-lang/cargo#7216) - Allow using 'config.toml' instead of just 'config' files. (rust-lang/cargo#7295) - Retry on SSL Connect Error. (rust-lang/cargo#7318) - minimal-copy `deserialize` for `InternedString` (rust-lang/cargo#7310) - Fix typo in cargo vendor examples (rust-lang/cargo#7320) - Fixes around multiple `[patch]` per crate (rust-lang/cargo#7303) - Improve error messages on mkdir failure (rust-lang/cargo#7306) ## reference 7 commits in d191a0c..090c015 2019-08-15 08:42:23 +0200 to 2019-09-03 13:59:28 -0700 - Fix rust-lang/reference#664: Review Oxford comma usage. (rust-lang/reference#668) - Fix some links. (rust-lang/reference#667) - Remove trait object warning. (rust-lang/reference#666) - Specify pattern types in `let` statements and `for` expressions (rust-lang/reference#663) - Fix loop expression link. (rust-lang/reference#662) - async-await initial reference material (rust-lang/reference#635) - Correct errors in the reference of extern functions definitions and declarations (rust-lang/reference#652) ## rust-by-example 1 commits in 580839d90aacd537f0293697096fa8355bc4e673..e76be6b2dc84c6a992e186157efe29d625e29b94 2019-08-17 23:17:50 -0300 to 2019-09-03 07:42:26 -0300 - Change link to russian translation repository (rust-lang/rust-by-example#1245) ## embedded-book 1 commits in 432ca26686c11d396eed6a59499f93ce1bf2433c..5ca585c4a7552efb546e7681c3de0712f4ae4fdc 2019-08-09 23:20:22 +0000 to 2019-08-27 13:39:14 +0000 - Fixup book CI (rust-embedded/book#205)

rust-highfive assigned ehuss Aug 26, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 26, 2019

ehuss reviewed Aug 27, 2019

View reviewed changes

tests/testsuite/patch.rs Show resolved Hide resolved

tests/testsuite/patch.rs Outdated Show resolved Hide resolved

tests/testsuite/patch.rs Show resolved Hide resolved

tests/testsuite/patch.rs Outdated Show resolved Hide resolved

alexcrichton force-pushed the duplicate-patch branch 2 times, most recently from a2f53db to 87e8024 Compare August 27, 2019 21:03

ehuss reviewed Aug 27, 2019

View reviewed changes

alexcrichton force-pushed the duplicate-patch branch from 87e8024 to 803bf32 Compare August 27, 2019 22:12

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2019

bors merged commit 803bf32 into rust-lang:master Aug 27, 2019

ehuss mentioned this pull request Sep 4, 2019

Update cargo, books rust-lang/rust#64150

Merged

alexcrichton deleted the duplicate-patch branch September 16, 2019 18:24

ehuss added this to the 1.39.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes around multiple `[patch]` per crate #7303

Fixes around multiple `[patch]` per crate #7303

alexcrichton commented Aug 26, 2019

rust-highfive commented Aug 26, 2019

bors commented Aug 27, 2019

ehuss commented Aug 27, 2019

alexcrichton commented Aug 27, 2019

ehuss Aug 27, 2019

alexcrichton commented Aug 27, 2019

bors commented Aug 27, 2019

bors commented Aug 27, 2019

bors commented Aug 27, 2019

	/// and is used to guide dependency resolution by altering summaires as they're
	/// and is used to guide dependency resolution by altering summaries as they're

Fixes around multiple [patch] per crate #7303

Fixes around multiple [patch] per crate #7303

Conversation

alexcrichton commented Aug 26, 2019

rust-highfive commented Aug 26, 2019

bors commented Aug 27, 2019

ehuss commented Aug 27, 2019

alexcrichton commented Aug 27, 2019

ehuss Aug 27, 2019

Choose a reason for hiding this comment

alexcrichton commented Aug 27, 2019

bors commented Aug 27, 2019

bors commented Aug 27, 2019

bors commented Aug 27, 2019

Fixes around multiple `[patch]` per crate #7303

Fixes around multiple `[patch]` per crate #7303