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.

Sign up for GitHub

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

Jump to bottom

Cache index files based on contents hash #11044

Merged
merged 2 commits into from
Sep 8, 2022
Merged

Cache index files based on contents hash #11044

merged 2 commits into from
Sep 8, 2022

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Sep 1, 2022

Since #10507 Cargo has known how to read registry cached files whose index version starts with the hash of the file contents. Git makes it very cheap to determine the hash of a file. This PR switches cargo to start writing the new format.

Cargoes from before #10507 will not know how to read, and therefore overwrite, cached files written by Cargos after this PR.

Cargos after this PR can still read, and will consider up-to-date cached files written by all older Cargos.

As I'm writing this out I'm thinking that there may not be any point in writing a file that has both. An alternative implementation just writes the file contents hash. 🤔

@rust-highfive
Copy link

r? @weihanglo

(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 Sep 1, 2022
@Eh2406
Copy link
Contributor Author

Eh2406 commented Sep 1, 2022

I like the alternative implementation better. So added it as a second commit.

@digitalillusion
Copy link

digitalillusion commented Sep 2, 2022 •
edited
Loading

I have this situation where the same CrateA is imported transitively from two git repos:

MyProject
 - CrateA (https://github.com/cratea)
 - CrateB
   - CrateA (https://gitlab.com/cratea)

Problem is that the compiler see them as two different crates (different types, etc). The solution to this is adding a patch to the cargo.toml of MyProject to resolve all CrateA from the same git source. But the "two" CrateA are actually identical. Will cache based on content hash allow me to avoid the patch?

@Eh2406 Eh2406 changed the title Cache based on contents hash Cache index files based on contents hash Sep 2, 2022
@Eh2406
Copy link
Contributor Author

Eh2406 commented Sep 2, 2022

No. Unfortunately, this is a very targeted change at the index files for registries like crates.io.
Sorry that the title of this PR was overly broad.

@woss
Copy link

woss commented Sep 2, 2022

No. Unfortunately, this is a very targeted change at the index files for registries like crates.io. Sorry that the title of this PR was overly broad.

Is there a way now or in the future when we can have dependencies resolved properly?
As far as i can see that the CrateA is the same so why is cargo treating it differently? Especially when it is required from the same URL and commit?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Sep 2, 2022

Especially when it is required from the same URL and commit?

That sounds like a bug. If you are specifying the dependency as coming from the same place, it should be unified. Please open an issue with a reproducible example.

More generally the request to change how resolution works, or even discussion of a particular bug with resolution, should happen on a dedicated issue and not some random PR.

weihanglo
weihanglo reviewed Sep 3, 2022
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how I understand the index_version format compatibility matrix.

  • ✅: Read side understands index_version from that cargo version and is able to hit the cache.
  • ❌: Read side cannot understand index_version. Cache is always invalid under that circumstance.
Read\Write old #10507 e24222e
old ✅ ✅ ❌
#10507 ✅ ✅ ✅
e24222e ✅ ✅ ✅

If that is correct, this patch only affects a small portion of users (e.g. use 1.65 to generate cache index and read it from 1.61 or older). I won't concern with that, especially the old index_version was only based on git commit hash, which was too restricted and not much useful than the new one.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Sep 5, 2022

I believe your analysis is correct.

weihanglo
weihanglo approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the consensus from the last Cargo team meeting, this change really makes sense. A cache miss for the old version of Cargo doesn't harm much. It will then just get an object from the local index repo.

I am going to merge it. Thank you all for helping this!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 8, 2022

📌 Commit e24222e has been approved by weihanglo

It is now in the queue for this repository.

@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 Sep 8, 2022
@bors
Copy link
Contributor

bors commented Sep 8, 2022

⌛ Testing commit e24222e with merge 404889d...

@bors
Copy link
Contributor

bors commented Sep 8, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 404889d to master...

@bors bors merged commit 404889d into rust-lang:master Sep 8, 2022
@Eh2406 Eh2406 deleted the file_hash branch September 8, 2022 02:32
weihanglo added a commit to weihanglo/rust that referenced this pull request Sep 13, 2022
@weihanglo
Update cargo
3dfdfdf 
10 commits in 646e9a0b9ea8354cc409d05f10e8dc752c5de78e..082503982ea0fb7a8fd72210427d43a2e2128a63
2022-09-02 14:29:28 +0000 to 2022-09-13 17:49:38 +0000
- Take priority into account within the pending queue (rust-lang/cargo#11032)
- fix(add): Clarify which version the features are added for (rust-lang/cargo#11075)
- doc: clarify config-relative paths for `--config <path>` (rust-lang/cargo#11079)
- Do not add home bin path to PATH if it's already there (rust-lang/cargo#11023)
- Don't use `for` loop on an `Option` (rust-lang/cargo#11081)
- Remove dead code (rust-lang/cargo#11080)
- Change progress indicator for sparse registries (rust-lang/cargo#11068)
- chore(ci): Ensure intradoc links are valid (rust-lang/cargo#11055)
- Cache index files based on contents hash (rust-lang/cargo#11044)
- fix: specifies the max length for crate name (rust-lang/cargo#11051)
@weihanglo weihanglo mentioned this pull request Sep 13, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2022
@bors
Auto merge of rust-lang#101776 - weihanglo:update-cargo, r=ehuss
88a1922 
Update cargo

10 commits in 646e9a0b9ea8354cc409d05f10e8dc752c5de78e..082503982ea0fb7a8fd72210427d43a2e2128a63 2022-09-02 14:29:28 +0000 to 2022-09-13 17:49:38 +0000
- Take priority into account within the pending queue (rust-lang/cargo#11032)
- fix(add): Clarify which version the features are added for (rust-lang/cargo#11075)
- doc: clarify config-relative paths for `--config <path>` (rust-lang/cargo#11079)
- Do not add home bin path to PATH if it's already there (rust-lang/cargo#11023)
- Don't use `for` loop on an `Option` (rust-lang/cargo#11081)
- Remove dead code (rust-lang/cargo#11080)
- Change progress indicator for sparse registries (rust-lang/cargo#11068)
- chore(ci): Ensure intradoc links are valid (rust-lang/cargo#11055)
- Cache index files based on contents hash (rust-lang/cargo#11044)
- fix: specifies the max length for crate name (rust-lang/cargo#11051)
@ehuss ehuss added this to the 1.65.0 milestone Sep 21, 2022
@Eh2406 Eh2406 mentioned this pull request Oct 5, 2022
Closed
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
@bors
Auto merge of #101776 - weihanglo:update-cargo, r=ehuss
98e1e6b 
Update cargo

10 commits in 646e9a0b9ea8354cc409d05f10e8dc752c5de78e..082503982ea0fb7a8fd72210427d43a2e2128a63 2022-09-02 14:29:28 +0000 to 2022-09-13 17:49:38 +0000
- Take priority into account within the pending queue (rust-lang/cargo#11032)
- fix(add): Clarify which version the features are added for (rust-lang/cargo#11075)
- doc: clarify config-relative paths for `--config <path>` (rust-lang/cargo#11079)
- Do not add home bin path to PATH if it's already there (rust-lang/cargo#11023)
- Don't use `for` loop on an `Option` (rust-lang/cargo#11081)
- Remove dead code (rust-lang/cargo#11080)
- Change progress indicator for sparse registries (rust-lang/cargo#11068)
- chore(ci): Ensure intradoc links are valid (rust-lang/cargo#11055)
- Cache index files based on contents hash (rust-lang/cargo#11044)
- fix: specifies the max length for crate name (rust-lang/cargo#11051)
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
@bors
Auto merge of #101776 - weihanglo:update-cargo, r=ehuss
2d2ab83 
Update cargo

10 commits in 646e9a0b9ea8354cc409d05f10e8dc752c5de78e..082503982ea0fb7a8fd72210427d43a2e2128a63 2022-09-02 14:29:28 +0000 to 2022-09-13 17:49:38 +0000
- Take priority into account within the pending queue (rust-lang/cargo#11032)
- fix(add): Clarify which version the features are added for (rust-lang/cargo#11075)
- doc: clarify config-relative paths for `--config <path>` (rust-lang/cargo#11079)
- Do not add home bin path to PATH if it's already there (rust-lang/cargo#11023)
- Don't use `for` loop on an `Option` (rust-lang/cargo#11081)
- Remove dead code (rust-lang/cargo#11080)
- Change progress indicator for sparse registries (rust-lang/cargo#11068)
- chore(ci): Ensure intradoc links are valid (rust-lang/cargo#11055)
- Cache index files based on contents hash (rust-lang/cargo#11044)
- fix: specifies the max length for crate name (rust-lang/cargo#11051)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Eh2406 @rust-highfive @digitalillusion @woss @weihanglo @bors @ehuss