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

Refactor RegistryData::load to handle management of the index cache #10482

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Mar 16, 2022

Enables registry implementations to signal if the cache is valid on a per-request basis.

Fixes a bug introduced by #10064 that caused Cargo not to update for several cases in a release build because it believed the index cache to be valid when it was not. The issue only occurred in release builds because debug builds verify that the cache contents is correct (by refreshing it).

Previously current_version was called by the index to determine whether the cache was valid. In the new model, RegistryData::load is passed the current version of the cache and returns an enum to indicate the status of the cached data.

r? @Eh2406
cc @ehuss

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2022
Copy link
Contributor

@Eh2406 Eh2406 left a comment

Choose a reason for hiding this comment

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

I overall like the direction this is going. And with a quick look most of this makes sense.

Will this allow HTTP-based registries to use the existing caching mechanism?

src/cargo/sources/registry/index.rs Show resolved Hide resolved
src/cargo/sources/registry/index.rs Show resolved Hide resolved
@arlosi
Copy link
Contributor Author

arlosi commented Mar 16, 2022

Will this allow HTTP-based registries to use the existing caching mechanism?

Yes. I'll update the http registry PR after this is in.

…tself.

This enables registry implementations to signal if the cache is valid
on a per-request basis.

Fixes a bug introduced by rust-lang#10064 that caused Cargo not to update for
several cases in a release build because it believed the index cache to
be valid when it was not.
@Eh2406
Copy link
Contributor

Eh2406 commented Mar 17, 2022

I confirmed that the cargo test --release worked for me.
Hopefully I can give this a full review after my next meeting.

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 17, 2022

Thank you!

@bors r+

By the way, I don't know if this is an optimization, but a git index could return the sha of the last commit when that file was updated instead of the currently checked out sha. Calculating last modified sha may not be worth the trouble, but it would allow the reuse of cached files if all of the changes are related to other files.

@bors
Copy link
Contributor

bors commented Mar 17, 2022

📌 Commit a4a882d has been approved by Eh2406

@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 Mar 17, 2022
@bors
Copy link
Contributor

bors commented Mar 17, 2022

⌛ Testing commit a4a882d with merge 109bfbd...

@bors
Copy link
Contributor

bors commented Mar 17, 2022

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 109bfbd to master...

@bors bors merged commit 109bfbd into rust-lang:master Mar 17, 2022
@bors bors mentioned this pull request Mar 17, 2022
5 tasks
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2022
Update cargo

9 commits in 65c82664263feddc5fe2d424be0993c28d46377a..109bfbd055325ef87a6e7f63d67da7e838f8300b
2022-03-09 02:32:56 +0000 to 2022-03-17 21:43:09 +0000
- Refactor RegistryData::load to handle management of the index cache (rust-lang/cargo#10482)
- Separate VCS command paths with "--" (rust-lang/cargo#10483)
- Fix panic when artifact target is used for `[target.'cfg(<target>)'.dependencies` (rust-lang/cargo#10433)
- Bump [email protected] and [email protected] (rust-lang/cargo#10479)
- vendor: Don't allow multiple values for --sync (rust-lang/cargo#10448)
- Use types to make clere (credential process || token) (rust-lang/cargo#10471)
- Warning on conflicting keys (rust-lang/cargo#10316)
- Registry functions return Poll to enable parallel fetching of index data (rust-lang/cargo#10064)
- Refine the contributor guide (rust-lang/cargo#10468)
@ehuss
Copy link
Contributor

ehuss commented Mar 20, 2022

BTW, I wanted to say thanks for fixing this so quickly. Sorry, I'm really behind on things. These changes should be in the latest nightly now (2022-03-20).

bors added a commit that referenced this pull request Apr 5, 2022
File Cache is valid if checkout or contents hasn't changed

#10482 allow the registry to have cashe keys at a per file level.
On master if the currently checked out commit changes all cached files are regenerated.
After this commit we also check if GITs hash of the particular file has changed.

To avoid thrashing cached files this PR only checks for the presence of both hashes, but does not write them both. This means that nightly after this branch will still be able to share file caches with older versions, specifically current stable.

When sufficient versions of stable know how to read the new format, a one line change can be made to start writing the new format.
@ehuss ehuss added this to the 1.61.0 milestone Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants