Fix optional deps in multiple sections #5480
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
matklad
reviewed
May 4, 2018
src/cargo/core/resolver/context.rs
Outdated
| if !reqs.deps.is_empty() { | ||
| let unknown = reqs.deps.keys().map(|s| &s[..]).collect::<Vec<&str>>(); | ||
| let features = unknown.join(", "); | ||
| // Any entries in `reqs.dep` which wrent' used are bugs in that the |
tests/testsuite/build_script.rs
Outdated
| ); | ||
| let p = p.build(); | ||
|
|
||
| // assert_that(p.cargo("run"), execs().with_status(0).with_stdout("0\n")); |
There was a problem hiding this comment.
Why is this commented out? We probably should uncomment or delete it?
|
@bors: r=matklad Indeed, typos! |
|
📌 Commit 5f0bf04 has been approved by |
|
⌛ Testing commit 5f0bf0472af01ae891e55d53bc3d844d55d45962 with merge 4993ff1dcaacfb722acdcd0135858a4ccbf9bf6f... |
|
💔 Test failed - status-travis |
This commit fixes an issue where an optional dependency was listed multiple times in a manifest (multiple sections). This regression was introduced by rust-lang#5415 and happened because in the resolver we didn't record a `Dependency` as it was accidentally deduplicated too soon. The fix here was to ensure that all `Dependency` annotations make their way into `Resolve` now that we rely on the listed `Dependency` values for correctness. Closes rust-lang#5475
|
@bors: r=matklad |
|
📌 Commit 2f88a70 has been approved by |
bors
added a commit
that referenced
this pull request
May 4, 2018
Fix optional deps in multiple sections This commit fixes an issue where an optional dependency was listed multiple times in a manifest (multiple sections). This regression was introduced by #5415 and happened because in the resolver we didn't record a `Dependency` as it was accidentally deduplicated too soon. The fix here was to ensure that all `Dependency` annotations make their way into `Resolve` now that we rely on the listed `Dependency` values for correctness. Closes #5475
|
☀️ Test successful - status-appveyor, status-travis |
bors
added a commit
that referenced
this pull request
May 5, 2018
FIx false positive warning We warn if a feature was specified corresponding to a dependency which is not optional. However, a dependency can be both optional and required, and we shouldn't warn in that case. cc #5480 r? @alexcrichton
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes an issue where an optional dependency was listed multiple
times in a manifest (multiple sections). This regression was introduced by #5415
and happened because in the resolver we didn't record a
Dependencyas it wasaccidentally deduplicated too soon.
The fix here was to ensure that all
Dependencyannotations make their way intoResolvenow that we rely on the listedDependencyvalues for correctness.Closes #5475