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

Mark .cargo/git and .cargo/registry as cache dirs #10553

Merged
merged 9 commits into from
Apr 27, 2022
Merged

Mark .cargo/git and .cargo/registry as cache dirs #10553

merged 9 commits into from
Apr 27, 2022

Conversation

sourcefrog
Copy link
Contributor

@sourcefrog sourcefrog commented Apr 10, 2022 •
edited
Loading

Fixes #10457

What does this PR try to resolve?

As we previously discussed in #10457 this marks ~/.cargo/git and ~/.cargo/registry as not to be included in backups, and not subject to content indexing. These directories can be fairly large and frequently changed, and should only contain content that can be re-downloaded if necessary.

How should we test and review this PR?

I did two manual tests:

  1. Using the binary built from this tree, run cargo update in a source tree that has a git dependency, and observe that afterwards, there is a CACHEDIR.TAG in ~/.cargo/git.
  2. Similarly, run cargo update and observe that there's a CACHEDIR.TAG in ~/.cargo/registry.

(I ran this on Linux. This code should also trigger OS-specific behavior on macOS and Windows that's the same as is currently applied to target/.)

I added some test assertions.

@rust-highfive
Copy link

r? @ehuss

(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 Apr 10, 2022
@sourcefrog sourcefrog changed the title [WIP] Mark .cargo/git and .cargo/registry as cache dirs Mark .cargo/git and .cargo/registry as cache dirs Apr 16, 2022
@sourcefrog
Copy link
Contributor Author

OK I added some tests and this is ready for review.

weihanglo
weihanglo reviewed Apr 19, 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.

Thanks and glad to see you post this PR!

src/cargo/sources/git/source.rs Show resolved Hide resolved
tests/testsuite/git.rs Outdated Show resolved Hide resolved
src/cargo/sources/git/source.rs Outdated Show resolved Hide resolved
src/cargo/sources/registry/mod.rs Outdated Show resolved Hide resolved
@sourcefrog
Copy link
Contributor Author

I'm not sure at this point whether these errors https://github.com/rust-lang/cargo/runs/6096576521?check_suite_focus=true are related or just noise...

weihanglo
weihanglo reviewed Apr 24, 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.

Looks great! Though I don't expect you remove all the tests 😅 Sorry for misleading. Could you bring them back?

@sourcefrog
Copy link
Contributor Author

Oh of course, sorry. How about this?

@sourcefrog
exclude_from_backups_and_indexing can't fail
ae5fd5e 
Ignore errors creating the registry directory
@sourcefrog
Actually ignore errors
8f04a65
@sourcefrog
Ignore errors creating the git directory
4bcfd9e 
Probably can't fail, but just in case
@weihanglo weihanglo added the T-cargo Team: Cargo label Apr 26, 2022
@weihanglo
Copy link
Member

@rust-lang/cargo This pull request marks ~/.cargo/git and ~/.cargo/registry with a file CACHEDIR.TAG just before source checkout (in Source::block_until_ready, which should already acquire the .pacakge-cache lock). All errors during this process are ignored to avoid failures on a read-only filesystem. This seems to be a straightforward and less intrusive timing to mark them, so I am going to propose a merge.

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 26, 2022 •
edited by ehuss
Loading

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Apr 26, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 26, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@weihanglo
Copy link
Member

Here we come to a consensus and this is not actually a big change, so I am going to merge it without waiting the FCP.

Thanks @sourcefrog for the works!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 27, 2022

📌 Commit 4bcfd9e has been approved by weihanglo

@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 Apr 27, 2022
@bors
Copy link
Contributor

bors commented Apr 27, 2022

⌛ Testing commit 4bcfd9e with merge 7a3b56b...

@bors
Copy link
Contributor

bors commented Apr 27, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 7a3b56b to master...

@bors bors merged commit 7a3b56b into rust-lang:master Apr 27, 2022
@ehuss ehuss mentioned this pull request Apr 28, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2022
@bors
Auto merge of rust-lang#96503 - ehuss:update-cargo, r=ehuss
71cd460 
Update cargo

8 commits in edffc4ada3d77799e5a04eeafd9b2f843d29fc23..f63f23ff1f1a12ede8585bbd1bbf0c536e50293d
2022-04-19 17:38:29 +0000 to 2022-04-28 03:15:50 +0000
- move workspace inheritance untable docs to the correct place (rust-lang/cargo#10609)
- Cargo add support for workspace inheritance (rust-lang/cargo#10606)
- chore: Upgrade toml_edit (rust-lang/cargo#10603)
- Mark .cargo/git and .cargo/registry as cache dirs (rust-lang/cargo#10553)
- fix(yank): Use '--version' like install (rust-lang/cargo#10575)
- Disallow setting registry tokens with --config (rust-lang/cargo#10580)
- Set cargo --version git hash length to 9 (rust-lang/cargo#10579)
- Prefer `key.workspace = true` to `key = { workspace = true }` (rust-lang/cargo#10584)
@ehuss ehuss added this to the 1.62.0 milestone Apr 29, 2022
matthiaskrgr added a commit to matthiaskrgr/cargo-cache that referenced this pull request Apr 29, 2022
@matthiaskrgr
tests: update CARGO_HOME_subdirs_are_known()
9b917a8 
adapt to newly added CACHEDIR.TAG files.

see rust-lang/cargo#10553
@fredrikekre fredrikekre mentioned this pull request May 2, 2022
Open
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels May 6, 2022
@henkkuli henkkuli mentioned this pull request Aug 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo left review comments

Assignees

@ehuss ehuss

Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.

Mark ~/.cargo or some subdirectories with CACHEDIR.TAG
6 participants
@sourcefrog @rust-highfive @weihanglo @rfcbot @bors @ehuss