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

Handle activation conflicts for [patch] sources #7118

Merged
merged 1 commit into from Jul 12, 2019

Conversation

alexcrichton
Copy link
Member

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

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 rust-lang#7117
@rust-highfive
Copy link

r? @nrc

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2019
@alexcrichton
Copy link
Member Author

r? @Eh2406

@rust-highfive rust-highfive assigned Eh2406 and unassigned nrc Jul 10, 2019
@Eh2406
Copy link
Contributor

Eh2406 commented Jul 10, 2019

This makes sense and seems like a good solution for the currently stable functionality. I suspect that there are going to be related bugs with Public and Private dependencies, but that is not stable so we can deal with it then. Similarly this patch case will be a reliable source of complicated resolver bugs on all interesting changes to the resolver so I added it to the list of thing we should someday fuzz test.

One of the things that makes patch feel more reliable than replace is that the resolver basically has no special code for handling it. I wonder if there is any way to fix this bug without adding special handling? I fear that over time we will find more coroner cases this interacts with, not that I can think of any at the moment.

I also am not sure why things don't work with the replace case. Is it just because flag_activated is already called for both versions, so we don't need parent to double insert? Given that the documentation for the replace section does not say that it is deprecated I feel that it is not yet appropriate to put it on "life support".

@alexcrichton
Copy link
Member Author

Yeah that was one of the things about [patch] I also really liked how it turned out, how little it touched the resolver.

I was having a tough time coming up with solutions to this that didn't touch the resolver much though. One idea I had was to have a sort of "shadow resolution graph" which the resolver operates over but then a "real resolution graph" that's handed to the rest of Cargo. This shadow graph would "lie" and say that [patch] crates come from the source that's being patched (not the actual source of the crate itself) but the real resolution graph would say it comes from the real source. This seemed like a pretty invasive solution though and the one here was more targeted. I'm curious though if you've got other ideas on how to possibly solve this, because I'm out of ideas :(

As for replace, TBH I didn't really investigate it that much. It caused panics in the test suite pretty easily by not passing in None (and passing in parent). Remember though that documentation doesn't always reflect reality! The [replace] feature AFAIK hasn't received any support or bugfixes in years now at this point, which is defacto "on life support" even if not explicitly stated in my mind at least

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 11, 2019

I wish I had better ideas. Your idea for a "shadow resolution graph" sounds similar to what we do for replace currently; I wonder if we could have dep_cache do the check for the same source and treat it like a replace if they differ. But if we don't have a better solution then this is good.

@alexcrichton
Copy link
Member Author

Hm maybe? I'm not really sure what you mean though and how that would work. It would also still involve changing the resolver in one way or another as opposed to staying out of the resolver like [patch] basically does today.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 12, 2019

I'm not really sure what you mean though

I spent some time trying this. The idea does not work with the way replace currently works. Sorry for the noize.

I hope someday we can find a shared abstraction for [patch] and [replace]. But this works so.

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 12, 2019

📌 Commit 83bb30c has been approved by Eh2406

@bors 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 Jul 12, 2019
@bors
Copy link
Collaborator

bors commented Jul 12, 2019

⌛ Testing commit 83bb30c with merge 0dd7c50...

bors added a commit that referenced this pull request Jul 12, 2019
Handle activation conflicts for `[patch]` sources

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
@bors
Copy link
Collaborator

bors commented Jul 12, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: Eh2406
Pushing 0dd7c50 to master...

@bors bors merged commit 83bb30c into rust-lang:master Jul 12, 2019
@alexcrichton alexcrichton deleted the patch-bug branch July 15, 2019 20:54
bors added a commit to rust-lang/rust that referenced this pull request Jul 17, 2019
Update mdbook, cargo, books

This updates the last of the books using mdbook 0.1, finally removing it from the build.  Cargo, the reference, and the rustc book were the last to change.  This should have a noticeable impact on CI times.

I decided to remove the version switcher from the rustbook tool.  It seemed a little awkward keeping it with one version.  If we ever need it in the future, we can always add it back, it is a relatively small amount of code.  I would just like to avoid the temptation to split the versions in the future.

## cargo

12 commits in 677a180f4c8ca1dcef594f68dd0e63e4f08621f5..e3563dbdcd2e370bc4f11d080f739d82d25773fd
2019-07-08 13:43:02 +0000 to 2019-07-16 19:22:44 +0000
- Add Cirrus CI badge to manifest (rust-lang/cargo#7119)
- Update mdbook to 0.3. (rust-lang/cargo#7140)
- Remove unused feature filter. (rust-lang/cargo#7131)
- Remove now-unused `WorkspaceResolve` (rust-lang/cargo#7127)
- Fix some clippy warnings. (rust-lang/cargo#7135)
- Also ignore remap-path-prefix in metadata for `cargo rustc`. (rust-lang/cargo#7134)
- Fix some formatting for some strings. (rust-lang/cargo#7129)
- Handle activation conflicts for `[patch]` sources (rust-lang/cargo#7118)
- [BETA] Fix `cargo new` in root directory. (rust-lang/cargo#7126)
- Remove MyFnOnce (rust-lang/cargo#7125)
- Don't suppress error messages with `-q` (rust-lang/cargo#7116)
- Fix changelog date (rust-lang/cargo#7115)

## book

2 commits in 6c0d83499c8e77e06a71d28c5e1adccec278d4f3..7ddc46460f09a5cd9bd2a620565bdc20b3315ea9
2019-06-23 20:25:30 -0400 to 2019-06-27 09:50:36 -0400
- reexport to re-export
- Fixes made in final layout check

## reference

4 commits in 7a5aab5..8e7d614
2019-06-20 17:38:52 +0200 to 2019-07-16 21:02:33 +0100
- Update to mdbook 0.3. (rust-lang/reference#641)
- Remove extra parenthesis (rust-lang/reference#621)
- Correct sentence about unsized coercions (rust-lang/reference#628)
- unions: call out field offset issues (rust-lang/reference#627)

## edition-guide

4 commits in f8072acde5ce29c7570d7986180bbded2d22e287..f6c8b92d4e63edd28e862be952f33861f35956f8
2019-06-14 23:27:05 +0200 to 2019-07-06 22:10:32 +0200
- Fix syntax highlighting in macro guide (rust-lang/edition-guide#182)
- Fix error example for modern compilers. (rust-lang/edition-guide#160)
- Try cargo install-upgrade for mdbook. (rust-lang/edition-guide#181)
- correct the travis build badge URL (rust-lang/edition-guide#180)

## embedded-book

2 commits in ef27b517dcd0b990c888c0d7caeff52a5a115619..ff334e74fdb9f197e621efa6d7c3105be892e888
2019-06-18 22:59:47 +0000 to 2019-07-16 13:47:34 +0000
- use_core() is required for #![no_std] compatible generated bindings  (rust-embedded/book#197)
- concurrency: update shared resource examples  (rust-embedded/book#196)

## nomicon

7 commits in 341c221116a8b9f1010cf1eece952b80c5ec7f54..b7f0aba2f88a8feade70595efcfa3438e54e96c0
2019-06-19 09:08:47 -0700 to 2019-07-11 15:11:36 -0400
- chore: Remove redundant Eq import
- Fix link to rfc1857
- Move word "reading" out of the link to "The Book"
- update the link to `mdbook`
- re-wrap more text
- re-wrap the text to 80 columns
- Better explain how to use `mdbook`

## rust-by-example

8 commits in 62b3ff2cb44dd8b648c3ef2a9347c3706d148014..e3679e214d8db44586aca9b20aa27517007d1923
2019-06-24 09:17:21 -0300 to 2019-07-15 11:13:44 -0300
- Collection of fixes for the 'Vectors` chapter. (rust-lang/rust-by-example#1216)
- Changed mutable collected_iterator into immutable (rust-lang/rust-by-example#1219)
- Fix minor formatting inconsistencies (rust-lang/rust-by-example#1220)
- Make `use` example runnable (rust-lang/rust-by-example#1215)
- Fix: Automatically Typo (rust-lang/rust-by-example#1214)
- Collection of fixes for the `Strings` chapter. (rust-lang/rust-by-example#1212)
- Added link to russian translation (rust-lang/rust-by-example#1213)
- A couple of fixes for the `HashMap` chapter. (rust-lang/rust-by-example#1211)
@ehuss ehuss added this to the 1.38.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolution with [patch] isn't preserving "one major version per source" restriction
6 participants