cargo clean: Add target directory validation by TanmayArya-1p · Pull Request #16712 · rust-lang/cargo

TanmayArya-1p · 2026-03-05T23:20:32Z

View all comments

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~~ No longer this case. See cargo clean: Add target directory validation #16712 (comment)

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 · 2026-03-05T23:22:38Z

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 · 2026-03-05T23:27:20Z

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 · 2026-03-06T12:44:44Z

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 · 2026-03-06T17:09:52Z

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 · 2026-03-07T21:06:12Z

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 · 2026-03-11T01:10:20Z

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~~ See cargo clean: Add target directory validation #16712 (comment)

~~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 · 2026-03-11T01:10:22Z

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

Concerns:

~~future-incompat~~ resolved by cargo clean: Add target directory validation #16712 (comment)

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 · 2026-03-11T15:12:47Z

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 · 2026-03-11T17:29:21Z

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 · 2026-03-17T14:44:14Z

@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 · 2026-03-17T23:52:41Z

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

epage · 2026-04-02T20:09:02Z

@rfcbot fcp resolved future-incompat

rust-rfcbot · 2026-04-02T20:09:15Z

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

rust-rfcbot · 2026-04-12T20:10:30Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

…16878) ### What does this PR try to resolve? #16712 added some additional tests to `clean.rs` but did not add them to `clean_new_layout.rs` (which mirrors `clean.rs` with `-Zbuild-dir-new-layout` enabled) While these tests do not depend on the new build-dir layout, #16807 renames `clean.rs` to `clean_legacy_layout.rs` and `clean_new_layout.rs` to `clean.rs`. Having the tests be in both makes the switch easier and results in less diffs. I pulled this out into its own PR as the stabilization has grown pretty large. ### How to test and review this PR? `cargo test clean`

Update cargo submodule 26 commits in eb94155a9a60943bd7b1cb04abec42f5d0de6ddc..3bf1388b8823ec4eab622340036593381d8871aa 2026-04-09 13:10:20 +0000 to 2026-04-17 23:27:26 +0000 - Add global HTTP timeout tracking to http_async (rust-lang/cargo#16903) - feat(config): Stabilize `resolver.lockfile-path` config (rust-lang/cargo#16694) - chore(deps): update msrv (1 version) to v1.95 (rust-lang/cargo#16897) - fix: rebuild rustdoc json for different versions of a same crate (rust-lang/cargo#16773) - Add effective_url to http_async (rust-lang/cargo#16899) - Make crate downloads process as a queue rather than a stack (rust-lang/cargo#16898) - fix: sparse registry fetch progress reporting (rust-lang/cargo#16900) - chore: bump to 0.98.0; update changelog (rust-lang/cargo#16892) - docs(semver-check): update diagnostics to Rust 1.95 (rust-lang/cargo#16895) - docs(ref): Better highlight 'package --list' for include/exclude (rust-lang/cargo#16891) - docs: `include` doesnt apply gitignore from repo (rust-lang/cargo#16890) - fix: Update `bash` and `zsh` Autocompletion Documents to add `-m` (rust-lang/cargo#16888) - Add `-m` shorthand for `--manifest-path` (rust-lang/cargo#16858) - Fix a typo (rust-lang/cargo#16885) - Always take a shared lock on `.cargo-lock` (rust-lang/cargo#16886) - fix(toml): Teach users how to pin edition (rust-lang/cargo#16851) - fix: Prefer defined lint levels over default (rust-lang/cargo#16879) - chore(deps): bump rand from 0.10.0 to 0.10.1 (rust-lang/cargo#16883) - fix(lints): Ignore unused_crate_dependencies status (rust-lang/cargo#16877) - Copy cargo clean target-dir validation tests to clean_new_layout.rs (rust-lang/cargo#16878) - cargo clean: Add target directory validation (rust-lang/cargo#16712) - Docs: "api" in the configuration should not include a trailing slash (rust-lang/cargo#16869) - fix(core): use `closest_msg` to suggest similar member name for mistyped `-p` (rust-lang/cargo#16844) - chore(ci): Pin GitHub Actions to commit SHAs (rust-lang/cargo#16868) - chore(ci): Update `actions/checkout` from v5 to v6 (rust-lang/cargo#16867) - Add test for cargo install with github fastpath (rust-lang/cargo#16866) r? ghost

Update cargo submodule 28 commits in eb94155a9a60943bd7b1cb04abec42f5d0de6ddc..7ecf0285ebb408d596e4a8ac76a0980d8edb7005 2026-04-09 13:10:20 +0000 to 2026-04-18 15:34:11 +0000 - fix: use `portable-atomic` for 64-bit atomics (rust-lang/cargo#16906) - fix: rename incorrectly documented target-spec-json to json-target-sp… (rust-lang/cargo#16905) - Add global HTTP timeout tracking to http_async (rust-lang/cargo#16903) - feat(config): Stabilize `resolver.lockfile-path` config (rust-lang/cargo#16694) - chore(deps): update msrv (1 version) to v1.95 (rust-lang/cargo#16897) - fix: rebuild rustdoc json for different versions of a same crate (rust-lang/cargo#16773) - Add effective_url to http_async (rust-lang/cargo#16899) - Make crate downloads process as a queue rather than a stack (rust-lang/cargo#16898) - fix: sparse registry fetch progress reporting (rust-lang/cargo#16900) - chore: bump to 0.98.0; update changelog (rust-lang/cargo#16892) - docs(semver-check): update diagnostics to Rust 1.95 (rust-lang/cargo#16895) - docs(ref): Better highlight 'package --list' for include/exclude (rust-lang/cargo#16891) - docs: `include` doesnt apply gitignore from repo (rust-lang/cargo#16890) - fix: Update `bash` and `zsh` Autocompletion Documents to add `-m` (rust-lang/cargo#16888) - Add `-m` shorthand for `--manifest-path` (rust-lang/cargo#16858) - Fix a typo (rust-lang/cargo#16885) - Always take a shared lock on `.cargo-lock` (rust-lang/cargo#16886) - fix(toml): Teach users how to pin edition (rust-lang/cargo#16851) - fix: Prefer defined lint levels over default (rust-lang/cargo#16879) - chore(deps): bump rand from 0.10.0 to 0.10.1 (rust-lang/cargo#16883) - fix(lints): Ignore unused_crate_dependencies status (rust-lang/cargo#16877) - Copy cargo clean target-dir validation tests to clean_new_layout.rs (rust-lang/cargo#16878) - cargo clean: Add target directory validation (rust-lang/cargo#16712) - Docs: "api" in the configuration should not include a trailing slash (rust-lang/cargo#16869) - fix(core): use `closest_msg` to suggest similar member name for mistyped `-p` (rust-lang/cargo#16844) - chore(ci): Pin GitHub Actions to commit SHAs (rust-lang/cargo#16868) - chore(ci): Update `actions/checkout` from v5 to v6 (rust-lang/cargo#16867) - Add test for cargo install with github fastpath (rust-lang/cargo#16866)

After rust-lang/cargo#16712, `cargo clean` requires a CACHEDIR.TAG file to work. As we RmTree() the build dir a bit further up, it doesn't naturally exist for us, so put one there. (https://chromium-review.googlesource.com/c/chromium/src/+/7614540 added the `cargo clean` call -- I suppose `cargo clean` cleans things outside of the build dir? Else this would've been redundant after the RmTree.) Bug: 504672125 Change-Id: I673de8a9f395e69991aea43b5edfc35db4658544 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7782785 Commit-Queue: Nico Weber <thakis@chromium.org> Reviewed-by: Arthur Eubanks <aeubanks@google.com> Cr-Commit-Position: refs/heads/main@{#1618312}

… exist (rust-lang#16934) Fixes rust-lang#16925 Regressed in rust-lang#16712 which added validation for explicitly specified target directories by checking if they contain a valid `CACHEDIR.TAG`. This PR makes it so that cargo skips this validation if the target-dir doesn't exist.

rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-clean labels Mar 5, 2026

TanmayArya-1p marked this pull request as ready for review March 5, 2026 23:27

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2026

rustbot assigned weihanglo Mar 5, 2026

weihanglo reviewed Mar 6, 2026

View reviewed changes

TanmayArya-1p force-pushed the clean-target-validation branch from 94c1489 to 94a4d66 Compare March 6, 2026 11:15

TanmayArya-1p force-pushed the clean-target-validation branch from 94a4d66 to c6cdaa1 Compare March 6, 2026 17:12

This comment has been minimized.

Sign in to view

weihanglo reviewed Mar 7, 2026

View reviewed changes

Comment thread src/cargo/ops/cargo_clean.rs Outdated

Comment thread src/cargo/ops/cargo_clean.rs Outdated

TanmayArya-1p force-pushed the clean-target-validation branch 2 times, most recently from f72fbd1 to 23c225b Compare March 7, 2026 20:33

weihanglo approved these changes Mar 11, 2026

View reviewed changes

weihanglo added the T-cargo Team: Cargo label Mar 11, 2026

This comment was marked as duplicate.

Sign in to view

rust-rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Mar 11, 2026

ehuss added this to Cargo status tracker Mar 17, 2026

ehuss moved this to FCP merge in Cargo status tracker Mar 17, 2026

ehuss moved this from FCP merge to FCP blocked in Cargo status tracker Mar 31, 2026

rust-rfcbot added the final-comment-period FCP — a period for last comments before action is taken label Apr 2, 2026

rust-rfcbot removed the proposed-final-comment-period An FCP proposal has started, but not yet signed off. label Apr 2, 2026

rust-rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Apr 12, 2026

weihanglo approved these changes Apr 12, 2026

View reviewed changes

weihanglo added this pull request to the merge queue Apr 12, 2026

Merged via the queue into rust-lang:master with commit b54fe55 Apr 12, 2026
29 checks passed

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2026

rustbot mentioned this pull request Apr 12, 2026

Stabilize build-dir layout v2 #16807

Open

3 tasks

jm809 mentioned this pull request Apr 13, 2026

Add build profile. #6577

Closed

ranger-ross mentioned this pull request Apr 13, 2026

Copy cargo clean target-dir validation tests to clean_new_layout.rs #16878

Merged

weihanglo mentioned this pull request Apr 18, 2026

Update cargo submodule rust-lang/rust#155470

Merged

rustbot added this to the 1.97.0 milestone Apr 19, 2026

bjorn3 mentioned this pull request Apr 22, 2026

cargo clean --target-dir fails when target dir doesn't exist #16925

Closed

TanmayArya-1p mentioned this pull request Apr 23, 2026

cargo clean: do not error if explicitly specified target-dir does not exist #16934

Merged

Conversation

TanmayArya-1p commented Mar 5, 2026 • edited by weihanglo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

Tests

Uh oh!

TanmayArya-1p commented Mar 5, 2026

Uh oh!

rustbot commented Mar 5, 2026

Uh oh!

weihanglo left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TanmayArya-1p commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weihanglo commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

weihanglo left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TanmayArya-1p commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weihanglo left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

weihanglo commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-rfcbot commented Mar 11, 2026 • edited by epage Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epage commented Mar 11, 2026

Uh oh!

weihanglo commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weihanglo commented Mar 17, 2026

Uh oh!

TanmayArya-1p commented Mar 17, 2026

Uh oh!

epage commented Apr 2, 2026

Uh oh!

rust-rfcbot commented Apr 2, 2026

Uh oh!

rust-rfcbot commented Apr 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

TanmayArya-1p commented Mar 5, 2026 •

edited by weihanglo

Loading

weihanglo left a comment •

edited by rustbot

Loading

TanmayArya-1p commented Mar 6, 2026 •

edited

Loading

weihanglo commented Mar 6, 2026 •

edited

Loading

weihanglo left a comment •

edited by rustbot

Loading

TanmayArya-1p commented Mar 7, 2026 •

edited

Loading

weihanglo left a comment •

edited by rustbot

Loading

weihanglo commented Mar 11, 2026 •

edited

Loading

rust-rfcbot commented Mar 11, 2026 •

edited by epage

Loading

weihanglo commented Mar 11, 2026 •

edited

Loading