cargo clean: Add target directory validation

[TanmayArya-1p]

What does this PR try to resolve?

Fixes #9192

Implements the checks mentioned in this comment

To summarise, when cargo clean is run with a specified target directory:

• If a target directory is explicitly passed via --target-dir: check if a valid CACHEDIR.TAG exists in the target directory and hard error otherwise.
• In other cases where target directory is specified(via env vars or build config): emit a future incompat warning if the target directory does not contain a valid CACHEDIR.TAG

Tests

I've added 3 sets of unit tests for:

• When --target-dir is used explicitly
• When target directory is specified via the build config
• When target directory is specified via the CARGO_TARGET_DIR env variable

Let me know if there is a case I've missed or if i need to merge multiple tests into a single one.

[TanmayArya-1p]

I also noticed that if a file path is passed via --target-dir, it gets deleted without validation. I was wondering if we require checks for this as well?

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ehuss, @epage, @weihanglo
• @ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
• Random selection from ehuss, epage, weihanglo

[weihanglo]

Some nitpicks but general good. Thank you!

View changes since this review

[TanmayArya-1p]

thanks for the review :)
I made a few changes and fixed the things you pointed out, let me know if theres anything else that needs to be changed.

Also I noticed that the CI keeps failing in something totally unrelated. I see it's happening in other PRs as well. Any idea why this is happening?
Edit: Nevermind, I see #16714 addresses this

[weihanglo]

Also I noticed that the CI keeps failing in something totally unrelated. I see it's happening in other PRs as well. Any idea why this is happening?
Edit: Nevermind, I see #16714 addresses this

Feel free to rebase onto master!
Edit: actually you need to do that in order to make CI green and in a mergeable state

[rustbot]

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.

[weihanglo]

While this has been discussed within the team, this is a behavior change so I may start an FCP. Just FYI

View changes since this review

[TanmayArya-1p]

Added a check(and test) to make sure target_dir is a directory when it is specified. Also, I wasnt happy with how the code looked so i refactored it a bit.

[weihanglo]

Thanks!

View changes since this review

[weihanglo]

I knew we discussed this during a meeting last year, but for a behavior change it is still better to have a formal record and give members time to think about it offline. Hence

@rfcbot fcp merge cargo

This implements the target-dir validation described in #9192 (comment).

• If --target-dir is passed, cargo clean now hard-errors unless the directory contains a valid CACHEDIR.TAG.
• Otherwise, if the explicit target-dir comes from config, cargo clean emits a future-incompat warning if it doesn't contain a valid CACHEDIR.TAG

This also adds an extra validation that if target-dir points to a file, errors out.

[rust-rfcbot]

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

• @0xPoe
• @Eh2406
• @Muscraft
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

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.

[epage]

Otherwise, if the explicit target-dir comes from config, cargo clean emits a future-incompat warning if it doesn't contain a valid CACHEDIR.TAG

From #9192 (comment)

There was some uncertainty if that should also include setting target dir via environment variables and config files. One idea is to issue a future-incompatible warning that those situations may not be allowed in the future too, and then collect any feedback if people have issues with that.

What led to deciding in the direction of the future-incompat warning?

[weihanglo]

What led to deciding in the direction of the future-incompat warning?

For myself, to be conservative. I remembered rust-analyzer may create target-dir before running any cargo command, and that there will be no CACHEDIR.TAG. But probably this doesn't matter for this case.