Don't rely on Hash for Package deduplicating #1709
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
The fix in rust-lang#1697 was to alter the `Hash` implementation of `Package` to take into account the source root instead of just the package id (so packages at the same source would have the same hash). There's a spot, however, where we normalize some paths and not others, causing two paths which logically point the same location end up having different hashes, so the same package ended up being stored multiple times, later leading to an assertion that there was only one stored. This commit alters the behavior of read_packages to keep a hash map of ID => Package instead of just a hash set of Package as the deduplication/equality needs to be based on the ID, not the literal path to the source root. This specific bug shows up when you have an override and a normal dependency pointing at the same location (and the override has some inter-dependencies). Not exactly a normal use case, but it showed up in Servo's build!
|
r? @brson |
|
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
|
@bors r+ |
|
📌 Commit 06a0605 has been approved by |
bors
added a commit
that referenced
this pull request
Jun 12, 2015
The fix in #1697 was to alter the `Hash` implementation of `Package` to take into account the source root instead of just the package id (so packages at the same source would have the same hash). There's a spot, however, where we normalize some paths and not others, causing two paths which logically point the same location end up having different hashes, so the same package ended up being stored multiple times, later leading to an assertion that there was only one stored. This commit alters the behavior of read_packages to keep a hash map of ID => Package instead of just a hash set of Package as the deduplication/equality needs to be based on the ID, not the literal path to the source root. This specific bug shows up when you have an override and a normal dependency pointing at the same location (and the override has some inter-dependencies). Not exactly a normal use case, but it showed up in Servo's build!
|
☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64 |
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.
The fix in #1697 was to alter the
Hashimplementation ofPackageto takeinto account the source root instead of just the package id (so packages at the
same source would have the same hash). There's a spot, however, where we
normalize some paths and not others, causing two paths which logically point the
same location end up having different hashes, so the same package ended up being
stored multiple times, later leading to an assertion that there was only one
stored.
This commit alters the behavior of read_packages to keep a hash map of ID =>
Package instead of just a hash set of Package as the deduplication/equality
needs to be based on the ID, not the literal path to the source root.
This specific bug shows up when you have an override and a normal dependency
pointing at the same location (and the override has some inter-dependencies).
Not exactly a normal use case, but it showed up in Servo's build!