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

Don't rely on Hash for Package deduplicating #1709

Merged
merged 1 commit into from Jun 12, 2015

Conversation

Projects
None yet
5 participants
@alexcrichton
Copy link
Member

alexcrichton commented 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!

Don't rely on Hash for Package deduplicating
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!
@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jun 12, 2015

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned huonw Jun 12, 2015

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Jun 12, 2015

r? @huonw

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

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jun 12, 2015

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 12, 2015

📌 Commit 06a0605 has been approved by brson

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 12, 2015

⌛️ Testing commit 06a0605 with merge d406d25...

bors added a commit that referenced this pull request Jun 12, 2015

Auto merge of #1709 - alexcrichton:fix-some-overrides, r=brson
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!
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 12, 2015

@bors bors merged commit 06a0605 into rust-lang:master Jun 12, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@alexcrichton alexcrichton deleted the alexcrichton:fix-some-overrides branch Jun 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.