Exclude the target directory from backups using CACHEDIR.TAG

[jstasiak]

This patch follows the lead of #4386 (which excludes target directories
from Time Machine backups) and is motived by the same reasons listen
in #3884. CACHEDIR.TAG is an OS-independent mechanism supported by Borg,
restic, GNU Tar and other backup/archiving solutions.

See https://bford.info/cachedir/ for more information about the
specification. This has been discussed in Rust Internals earlier this
year[1] and it seems like it's an uncontroversial improvement so I went
ahead with the patch.

One thing I'm wondering is whether this should maybe cover the whole main target directory (right now it applies to target/debug, target/release etc. but not to target root).

[1] https://internals.rust-lang.org/t/pre-rfc-put-cachedir-tag-into-target/12262/11

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Eh2406]

Josh you were active on the internals thread:
@bors r? @joshtriplett

[jstasiak]

Ah yeah, I forgot to mention Josh explicitly, thanks for that. Also I'll rerun the test suite locally and address the build failures.

[jstasiak]

I'm unable to reproduce the resolver job failure (https://dev.azure.com/rust-lang/cargo/_build/results?buildId=32288&view=logs&j=f961518a-cff1-54aa-915a-1a77d90c89d9&t=b666365a-259d-50d7-5aed-f8ff536de640) locally (Mac OS 10.14), all tests pass for me and I'm kinda stuck on this.

[jstasiak]

I also have the following concern: I imagine it's possible for cargo to be interrupted right after creating the target directory (whether we consider target/ or target/debug/) but before applying this backup-exclusion login. Then this logic

        if !dest.as_path_unlocked().exists() {
            dest.create_dir()?;
            exclude_from_backups(dest.as_path_unlocked());
        }

would not trigger on the re-run and the directory would be included in backups until next working directory cleanup/target directory recreation. However small the chance of that is I'm wondering if maybe the exclude_from_backups() step should always happen (assuming its idempotence). Granted, that would counter user's actions to actually make that directory included in backups (by removing the flags) so I don't have a strong opinion on this, maybe it's ok to be slightly flaky here.

[joshtriplett]

I also have the following concern: I imagine it's possible for cargo to be interrupted right after creating the target directory (whether we consider target/ or target/debug/) but before applying this backup-exclusion login.

I can see a straightforward way to fix that. When creating a new target directory:

• Create a temporary directory.
• Create the CACHEDIR.TAG file in that temporary directory.
• Rename the temporary directory to target.

[jstasiak]

Yeah I decided against it before but you convinced me to give it a shot – I pushed a commit that does just what you suggest.

[jstasiak]

FWIW I had a quick look around and the only other open source backup solution I found (I only looked at open source ones) that supports this out of the box is attic, others (like rsync and bacula) support this in a exclude-directory-if-a-file-named-like-this-exists-in-it way – so while my initial commit message may be technically correct I wanted the above to be known, I don't want to oversell this solution on false premises.

[joshtriplett]

@jstasiak duplicity similarly has an --exclude-if-present option; I used that when I used duplicity, before I switched to restic.

[joshtriplett]

Could you please add a test that confirms this creates CACHEDIR.TAG, and a second test that confirms this doesn't create CACHEDIR.TAG in an existing target dir?

[jstasiak]

Sure, done. I wasn't sure if `testsuite/build.rs`` was the right place for them but I didn't find a better one, let me know if it's ok.

[jstasiak]

I'm still wondering whether this shouldn't maybe cover the whole target root. Right now I have the following subdirectories in target in one of my projects:

target% du -d 1 -h
121M	./rls
 48M	./release
 67M	./doc
240M	./debug
476M	.

debug and release is covered by this, but rls and doc I've no idea. If they're not I'm happy to add a patch to cover doc in this or a separate PR, rls is a separate repository I think so I can open a PR there, but that'd still mean the target subdirectories are excluded, each one on its own, not the target wholesale.

[joshtriplett]

@jstasiak I think it would be appropriate to add a CACHEDIR.TAG in doc, and for the code creating rls to do the same there.

If we were doing this from day 1, I would suggest covering all of target with a single CACHEDIR.TAG. But since we're introducing this now, when many other uses may have arisen for target, it does seem safer to handle only the target subdirs cargo creates, and let other tools handle the target subdirs they create.

[jstasiak]

Ok, fair. I added a patch to add this to target/doc as well + some reorganization to reuse the code that's already there. I wasn't sure about the name for the new create_dir_ function, feel free to change it or I can change it if there's a better suggestion.

If this is accepted I'll create a patch for RLS to match this.

[ehuss]

I think it is reasonable to consider marking the entire target directory as a cache directory. Unfortunately I don't see much of a discussion about why the current behavior was chosen (#4386 and #3884). @alexcrichton do you have any opinion on marking the entire target directory as a cache directory?

I'm pretty certain we do not want to include the CACHEDIR.TAG in target/doc. That directory is a "final artifact" that is often used for publishing, which is definitely not a temporary cache. Usually that is done by copying or compressing the entire directory wholesale to some output, and we don't want people pull in that file by accident.

Also, I'd prefer to avoid adding additional tests for each command. I think it's fine to just have a single test that runs multiple commands as needed. (Each additional test has a cost, so it helps to avoid adding too many unless necessary.)

(I'm amused to see Josh has experience with this from 9 years ago.)

[jstasiak]

OK, I went ahead and added two more commits: one changing the logic to exclude whole target/ from backups (I didn't move the new created_dir_..._atomic function back into layout (now that it's used only there) as I didn't want to add even more lines to the diff but I can do that if it's desired. Given that the tests pass there should be some variant of this patch (cherry-picking involved) in this PR to get this merged, I hope. :)

PS. I also saw that Firefox thread ;)

[joshtriplett]

@jstasiak Thank you for working on this, and for dealing with all the back-and-forth in requirements.

I think this is ready to merge.

@rfcbot merge

[rfcbot]

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

• @Eh2406
• @alexcrichton
• @ehuss
• @joshtriplett

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]

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

[ehuss]

Thanks @jstasiak!

@bors r+

[bors]

📌 Commit 5f2ba2b has been approved by ehuss

[bors]

⌛ Testing commit 5f2ba2b with merge cf3bfc9...

[bors]

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing cf3bfc9 to master...

[jstasiak]

Thank you all for the comments and patience, I'm really glad this managed to land. :) I'm still slightly unsure if maybe errors on writing to CACHEDIR.TAG shouldn't be silenced (I followed the lead of Time Machine-specific code already there) but on the other hand I can't imagine too many scenarios in which creating and renaming a directory will work but writing a small number of bytes to a file inside the directory won't so I guess it's not worth worrying about this too much?

Thanks again!