cargo clean --target-dir should check if the directory looks like a Cargo target directory before deleting it

[joshtriplett]

If you're in a Rust project directory (somewhere with a valid Cargo.toml), calling cargo clean --target-dir /some/path will delete everything in /some/path, whether it's a Cargo target directory or not.

Reports like #8866 and https://www.reddit.com/r/rust/comments/lo413x/novice_mistake_deleted_all_my_hobby_projectstests/ suggest that this can lead to problems and data loss.

I think it would make sense to add some checks to cargo clean --target-dir, to make sure it doesn't unintentionally remove more than the user would like it to.

A few ideas:

• We could look for some distinctive flag files in a target directory, and refuse to remove it if we don't find them.
• We could create a flag file and look for that. (This may cause transition issues, though, if people upgrade and then clean before building with the new cargo.)
• We could only remove subdirectories we know about, and if that doesn't leave the target directory empty, avoid removing everything else.

[BartMassey]

Alternatively, is there any reason we need cargo clean --target-dir d as an alias for rm -rf d at all? Could we just remove the --target-dir flag?

[joshtriplett]

@BartMassey Also an option. cargo clean without --target-dir makes sense even though it just does rm -rf target, but at the point where you're passing --target-dir, it seems like it has lower utility. An alternate target-dir might be shared (in which case you might not want to clean it lightly), for instance.

There may potentially be scripts out there relying on cargo clean to work with a target directory set. But it certainly seems like a problem waiting to happen.

[Andy-Python-Programmer]

This is how I think we can solve the problem:

Step 1:

Get the target directory provided by the --target-dir flag.

Step 2:

Check if the parent of the target directory has a Cargo.toml file. If Cargo.toml does not exist then its not a valid target directory.

Step 3:

Go into the cargo toml and grab the target dir set there and also grab the CARGO_TARGET_DIR env. If the env's or target's value == the target directory then we are all good.

[BartMassey]

@Andy-Python-Programmer Symlinks make the parent check a bit more exciting, though the good news is that this will fail-safe. Still doesn't take care of @joshtriplett's case of cleaning a shared target directory, though maybe that's OK.

I feel like Cargo needs (and may have, I haven't looked) to maintain a manifest at top level in the target directory with enough information to be able to GC all the files left by a particular project. That sounds like a really big undertaking, though. For now, I'd still just say don't allow specifying --target-dir with cargo clean.

[joshtriplett]

@BartMassey I'm much less worried about "I accidentally cleaned a shared target directory and now I have to rebuild lots of things", and much more worried about "I accidentally 'cleaned' a non-target directory and now I've lost data".

[agostbiro]

Wanted to add some user feedback here:

I accidentally ran cargo clean --target-dir . in my repo root while debugging some build issue and lost some unpushed commits. I'm in favor of removing the --target-dir flag.

[BartMassey]

I did the trivial patch to remove the --target-dir flag for cargo clean here. It looks like I'll need to get somebody to approve it before I can do a PR? @joshtriplett would it be OK to just go ahead with this? It seems like the safest way to go, and way less effort than trying some fancy heuristics to figure out safe-to-clean. I suppose somebody must be using this flag, but it seems easy enough to replace?

Thoughts welcome.

[epage]

Wouldn't removing --target-dir be a breaking change? It would also remove the symmetry with other cargo commands that take --target-dir which can be important for wrapping with other build processes.

Alternatively, is there any reason we need cargo clean --target-dir d as an alias for rm -rf d at all?

cargo clean gives more control than just rm -rf <target-dir> and even more if we merge in any of the features of third-party cleanup commands that are used for cases like cleaning up caches for CI

[BartMassey]

These are reasonable objections. Right now, I think the footgun is not worth the cost, though. We can't really have people accidentally removing important stuff, and it has happened to several of us.

Given that you think it's important, I'll try to figure out some reliable way for cargo clean to check. The obvious thing is to look for CACHEDIR.TAG in the target root, but I'm not sure how far back the existence of that file goes. I guess in the worst case you'd have to hand-clean an old target with rm -rf. Also, it would be nice if other cargo operations recreated it if they didn't find it…

Thoughts and suggestions welcome.

[BartMassey]

BartMassey-upstream/cargo:git-clean-check-target-dir now contains a preliminary set of commits to check for CACHEDIR.TAG in a target directory before trying to remove it. It fails some tests; if we're going in this direction I'll investigate and fix that. I will PR it if/when it is ready and desired, but no point cluttering the queue for now I think?

I split out the checking code because really cargo build should also check for CACHEDIR.TAG before trying to reuse a target directory to avoid various nastiness.

Should we also insist that target directory paths end in /target? Without something like that, Issue #9628 will still be a thing.

Honestly, while I think this patch is a good idea regardless, I'm still inclined to want to get rid of --target for cargo clean.