Implement escaping to allow clean -p to delete all files when directory contains glob characters

[russweas]

Closes #10068.

Runs glob::Pattern::escape on path input to rm_rf_glob(). This fixes a bug in which cargo clean -p fails to delete a number of files from target/ if the path to target contains glob characters.

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

Please see the contribution instructions for more information.

[jonhoo]

Ah, you need to make sure to escape only the part of the path that isn't intended to be a pattern. The code that calls rm_rf_glob specifically expects one part of the pattern to actually be a glob (something like {pkg.name()}-*), and we'll need to keep supporting that (which is probably what the failing test is indicating).

[russweas]

Ah, you need to make sure to escape only the part of the path that isn't intended to be a pattern. The code that calls rm_rf_glob specifically expects one part of the pattern to actually be a glob (something like {pkg.name()}-*), and we'll need to keep supporting that (which is probably what the failing test is indicating).

Sorry about that! I was under the impression that only I could see draft PR's... I just pushed the correct solution.

Here's the output of running your script on my machine:

➜  cargo_test rm -rf glob-in-rm && ./reproduce.sh
~/Projects/cargo_test/glob-in-rm/hello ~/Projects/cargo_test/glob-in-rm
     Created binary (application) `foo` package
    Checking foo v0.1.0 (/Users/russweas/Projects/cargo_test/glob-in-rm/hello/foo)
    Finished dev [unoptimized + debuginfo] target(s) in 0.65s
~/Projects/cargo_test/glob-in-rm
~/Projects/cargo_test/glob-in-rm/[hello] ~/Projects/cargo_test/glob-in-rm
     Created binary (application) `foo` package
    Checking foo v0.1.0 (/Users/russweas/Projects/cargo_test/glob-in-rm/[hello]/foo)
    Finished dev [unoptimized + debuginfo] target(s) in 0.33s
~/Projects/cargo_test/glob-in-rm
Files [hello]/foo/target/.rustc_info.json and hello/foo/target/.rustc_info.json differ

[jonhoo]

Nice! Draft PRs are visible to everyone, they are just marked as "not ready to merge". The idea of a draft PR is to get feedback on work that's not yet completed, or just to have a place to allow discussion on a fix before it's done. I think it was totally fine to open this as a draft :)

[russweas]

Nice! Draft PRs are visible to everyone, they are just marked as "not ready to merge". The idea of a draft PR is to get feedback on work that's not yet completed, or just to have a place to allow discussion on a fix before it's done. I think it was totally fine to open this as a draft :)

Now I know 😆. At my internship I used draft PR's like a Todo list of things I was working on, so I would make a tiny change then push it.

[russweas]

Note: would also need to incorporate these changes into #10070

cargo/src/cargo/ops/cargo_clean.rs

Lines 176 to 184 in 2f375da

	rm_rf_glob(&src_path.as_path_unlocked().join(&pkg_dir), config)?;
	}
	if let Some(cache_path) = registry.dot_crate_cache() {
	rm_rf_glob(
	&cache_path
	.as_path_unlocked()
	.join(&format!("{}.crate", pkg_dir)),
	config,
	)?;

[russweas]

Tests passed, pushed final commit to undo superfluous formatting changes

[alexcrichton]

Thanks for this! Would you be up for adding some tests for this as well?

[russweas]

@alexcrichton @jonhoo Test added! LMK if I need more coverage, but I think this + existing tests should cover everything. The only other thing I can think of is checking for each kind of build artifact, but I feel that's already covered in other tests, since all this PR changed is top-level glob escaping.

[russweas]

And just squashed the commits, I noticed it was getting excessive

[russweas]

Looks like the test works on windows-latest, nightly-gnu, i686-pc-windows-gnu , but not windows-latest, stable-msvc, i686-pc-windows-msvc. Specifically, the build artifacts look like they're being generated differently...
EDIT: found the reason, it's the code in #7400. Should be able to fix this.

[alexcrichton]

@bors: r+

Thanks!

[bors]

📌 Commit effc720 has been approved by alexcrichton

[bors]

⌛ Testing commit effc720 with merge adf601d...

[bors]

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing adf601d to master...