Reject manifest with duplicate dependencies in different targets #2491
Conversation
|
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
|
This solution looks a bit mouthful and its correctness depends on the order of
|
|
I think that we may want to tweak this a bit actually. So right now Cargo actually supports duplicate names just fine (between dev, build, deps, target-specific deps, etc), so long as the same name resolves to the same crate. Maybe this check could be placed after all the dependencies have been shoved into a vector? That way it could ensure that all dependencies with the same name come from the same source (regardless of kind) |
|
@alexcrichton ok, I'll try to do that. |
|
Oops! Should be fixed in #2496 |
|
Would it be enough to check that all dependencies with the same |
|
The weird and funny thing about #2023 is that it builds first time, and fails second time when we have |
| for dep in deps.iter() { | ||
| let name = dep.name(); | ||
| let sources = names_sources.entry(name).or_insert(HashSet::new()); | ||
| if sources.insert(dep.source_id()) && sources.len() > 1 { |
There was a problem hiding this comment.
Couldn't this be if !sources.insert(source_id)? I think insert returns false if it was already present
There was a problem hiding this comment.
Yes, it is. But this line means "if insertion was successful, and we now has more than 1 source". So we check sources length only if we inserted something. Actually, I thought to split this line on two because we mostly care about number of sources (second part of the condition).
We can miss different sources if we will check it with !sources.insert(source_id). Say, we have source1 and source2 for the same dependency name. On both iterations it will return false and if-branch will not be executed.
So, would it be better to leave it as is or split it on two lines?
There was a problem hiding this comment.
Oh, right! In that case we don't need the value here to be a HashSet<SourceId>, right? If it's just a SourceId we could just compare against what was already there.
There was a problem hiding this comment.
Ah, indeed. Will fix it asap.
|
Looks good to me! Just one minor nit. |
|
@alexcrichton any update on this? =) I've removed unneeded |
Reject manifest with duplicate dependencies in different targets Closes #2023
|
💔 Test failed - cargo-win-gnu-64 |
|
@bors: retry On Thu, Mar 31, 2016 at 10:00 AM, bors notifications@github.com wrote:
|
|
⚡ Previous build results for cargo-linux-64 are reusable. Rebuilding only cargo-cross-linux, cargo-linux-32, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64... |
|
☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64 |
|
What if the targets are mutually exclusive? It seems like that should be allowed. |
|
@wagenet oh this was definitely not intended to error out on legitimate cases, if that's happening please feel free to file an issue! |
|
@alexcrichton see #3195. Thanks! |
Closes #2023