-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix(package): exclude target/package from backups #16272
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
Conversation
|
Should we have automated tests for this? Note that our contrib guide normally recommends creating a passing test in a commit before the fix, showing the old behavior. Then the commit with the fix updates the test so it still passes, showing the new behavior. Then the diff will show how the behavior changed. |
4d928f4 to
5bb82db
Compare
|
Looks like this needs |
5bb82db to
d77851d
Compare
Thanks for all your help! I have did the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to update test snapshots accordingly (with env SNAPSHOTS=overwrite): https://github.com/rust-lang/cargo/actions/runs/19452875528/job/55661053577?pr=16272#step:15:4779
tests/testsuite/package.rs
Outdated
| // This test documents the current behavior where target directory is NOT excluded from backups. | ||
| // After the fix, this test will be updated to verify that CACHEDIR.TAG exists. | ||
| let p = project() | ||
| .file("src/main.rs", r#"fn main() { println!("hello"); }"#) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for faster test execution (avoid to binary linking)
| .file("src/main.rs", r#"fn main() { println!("hello"); }"#) | |
| .file("src/lib.rs", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip! Done.
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
d77851d to
454b4f5
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
454b4f5 to
4c118e5
Compare
tests/testsuite/package.rs
Outdated
| .file("src/lib.rs", "") | ||
| .build(); | ||
|
|
||
| p.cargo("package --allow-dirty -v") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need -v here, since we just want to check cachedir tag file existing
You might need to change that , regenerate snapshots, and cargo fmt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
4c118e5 to
8e2fcdf
Compare
Add a failing test that demonstrates the current buggy behavior where `cargo package` does not create a CACHEDIR.TAG file in the target directory. This causes backup tools to unnecessarily include temporary packaging artifacts. The test currently passes by asserting that CACHEDIR.TAG does NOT exist (the buggy behavior). The next commit will implement the fix and update this test to verify correct behavior. Relates to rust-lang#16238
Create CACHEDIR.TAG files in build directories during `cargo package` to exclude them from system backups. This ensures that temporary packaging artifacts in target/package are not unnecessarily included by backup tools like Time Machine on macOS. The fix calls `create_dir_all_excluded_from_backups_atomic` when creating the build directory and target directory for packaging, which automatically creates the CACHEDIR.TAG file. Update existing tests to verify that CACHEDIR.TAG files are now created in the expected locations. Fixes rust-lang#16238
8e2fcdf to
b8dc393
Compare
|
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
What does this PR try to resolve?
The target directory is not excluded from backups when created with
cargo packageorcargo publish. This causes thetarget/packagedirectory to be included in Time Machine backups on macOS and other
backup systems that respect CACHEDIR.TAG files.
The issue occurs because when
Filesystem::open()creates parentdirectories in
src/cargo/util/flock.rs:336, it usespaths::create_dir_all()instead of the backup-excluding version,bypassing the normal target directory creation logic.
This commit adds a new
create_dir_with_backup_exclusion()method toFilesystemand uses it to ensure thetarget/packagedirectory isproperly excluded from backups before creating files in it.
Fixes #16238
How to test and review this PR?
Run the following:
cargo new foo
cd foo
cargo package --allow-dirty
ls -al target/package/
Verify that
CACHEDIR.TAGnow exists intarget/package/.All existing package tests pass.