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

[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. We decided to split this out to its own PR #16765 as it is less controversial.

[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.

[weihanglo]

@TanmayArya-1p While this is still in FCP, we were discussing whether the target-dir-is-file check can be a separate commit, or even a separate PR. That one turns out to be less controversial within the Cargo team.

[TanmayArya-1p]

cool, i'll open another pr for the file validation.

[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.

[epage]

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.

The choice when we last discussed this wasn't between future-incompat OR error but do-nothing OR future-incompat.

[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.

The choice when we last discussed this wasn't between future-incompat OR error but do-nothing OR future-incompat.

I was re-reading #9192 (comment) and the meeting note. My understanding is that, for the env/config cases, the conclusion was not to hard-error right away. Instead the direction was to use a future-incompat warning first and then revisit based on user feedback.

I wasn't in the meeting, though I feel like without a warning there is no action we can take in the future. The drawback of the warning is no way to suppress it though.

[0xPoe]

Do we need to consider the new build directory layout here? I am not sure if this is out of scope. However, for the new build dir layout, I believe cargo clean also deletes the build directory. Should we perform the same check for the build directory?

Even if it is out of scope, it may be better to create an issue to track it if necessary.

cc: @ranger-ross @weihanglo

View changes since this review

[ranger-ross]

Should we perform the same check for the build directory?

I think that seems reasonable to also check the build-dir assuming target-dir != build-dir.
Note that for build-dir there is no --build-dir CLI flag, so if we matched the target-dir behavior in this PR Cargo would only warn.
Perhaps this could be added in a separate PR?

Do we need to consider the new build directory layout here?

The validation performed by validate_target_dir_tag() should be compatible for both the v1 and v2 build-dir layouts as it does not change anything outside <build-dir>/{platform-profile}/. The layout changes do not touch CACHEDIR.TAG so I don't think there should be any issues here.

[weihanglo]

Note that for build-dir there is no --build-dir CLI flag, so if we matched the target-dir behavior in this PR Cargo would only warn.
Perhaps this could be added in a separate PR?

Either in this or a separate PR is fine for me, though I would suggest doing it after the FCP concludes