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

russweas · 2021-11-12T01:58:09Z

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 · 2021-11-12T01:58:12Z

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 · 2021-11-12T03:12:30Z

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 · 2021-11-12T03:16:31Z

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 · 2021-11-12T03:19:40Z

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 :)

src/cargo/core/compiler/layout.rs

russweas · 2021-11-12T03:27:08Z

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 · 2021-11-12T03:37:55Z

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 · 2021-11-12T04:07:35Z

Tests passed, pushed final commit to undo superfluous formatting changes

src/cargo/ops/cargo_clean.rs

alexcrichton · 2021-11-15T14:26:39Z

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

russweas · 2021-11-16T21:54:24Z

@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 · 2021-11-16T22:01:47Z

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

russweas · 2021-11-16T22:36:21Z

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.

Implement glob escaping for clean -p Add pattern escape for glob matching cargo clean files Implement correct solution for rust-lang#10068 Removed superfluous formatting changes Update rm_rf_glob()'s error handling Remove dir_glob reference for non-glob function Added test Satisfy clippy Add MSVC support for test

alexcrichton · 2021-11-17T15:37:43Z

@bors: r+

Thanks!

bors · 2021-11-17T15:37:45Z

📌 Commit effc720 has been approved by alexcrichton

bors · 2021-11-17T16:00:19Z

⌛ Testing commit effc720 with merge adf601d...

bors · 2021-11-17T16:46:14Z

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

Update cargo 11 commits in 2e2a16e983f597da62bc132eb191bc3276d4b1bb..ad50d0d266213e0cc4f6e526a39d96faae9a3842 2021-11-08 15:13:38 +0000 to 2021-11-17 18:36:37 +0000 - Warn when alias shadows external subcommand (rust-lang/cargo#10082) - Implement escaping to allow clean -p to delete all files when directory contains glob characters (rust-lang/cargo#10072) - Match any error when failing to find executables (rust-lang/cargo#10092) - Enhance error message for target auto-discovery (rust-lang/cargo#10090) - Include note about bug while building on macOS in mdbook (rust-lang/cargo#10073) - Improve the help text of the --quiet args for all commands (rust-lang/cargo#10080) - `future-incompat-report` checks both stdout and stderr for color support (rust-lang/cargo#10024) - Remove needless borrow to make clippy happy (rust-lang/cargo#10081) - Describe the background color of the timing graph (rust-lang/cargo#10076) - Make ProfileChecking comments a doc comments (rust-lang/cargo#10077) - Fix test: hash value depends on endianness and bitness. (rust-lang/cargo#10011)

rust-highfive assigned Eh2406 Nov 12, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 12, 2021

jonhoo reviewed Nov 12, 2021

View reviewed changes

src/cargo/core/compiler/layout.rs Outdated Show resolved Hide resolved

russweas marked this pull request as ready for review November 12, 2021 04:06

russweas changed the title ~~Add pattern escape for glob matching cargo clean files~~ Implement escaping to allow clean -p to delete all files when directory contains glob characters Nov 12, 2021

jonhoo suggested changes Nov 12, 2021

View reviewed changes

src/cargo/ops/cargo_clean.rs Outdated Show resolved Hide resolved

russweas force-pushed the master branch from 8c33344 to 735ae03 Compare November 16, 2021 22:01

russweas force-pushed the master branch from a4e2edb to 21ed4f5 Compare November 16, 2021 22:02

russweas force-pushed the master branch from 21ed4f5 to effc720 Compare November 16, 2021 22:55

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2021

bors merged commit adf601d into rust-lang:master Nov 17, 2021

This was referenced Nov 17, 2021

cargo clean -p package can clear the document of a single crates #9841

Closed

clean: add --with-downloads option #10070

Closed

ehuss mentioned this pull request Nov 17, 2021

Update cargo rust-lang/rust#90991

Merged

ehuss added this to the 1.58.0 milestone Feb 6, 2022

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

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

Uh oh!

Conversation

russweas commented Nov 12, 2021

Uh oh!

rust-highfive commented Nov 12, 2021

Uh oh!

jonhoo commented Nov 12, 2021

Uh oh!

russweas commented Nov 12, 2021

Uh oh!

jonhoo commented Nov 12, 2021

Uh oh!

Uh oh!

russweas commented Nov 12, 2021

Uh oh!

russweas commented Nov 12, 2021

Uh oh!

russweas commented Nov 12, 2021

Uh oh!

Uh oh!

alexcrichton commented Nov 15, 2021

Uh oh!

russweas commented Nov 16, 2021

Uh oh!

russweas commented Nov 16, 2021

Uh oh!

russweas commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Nov 17, 2021

Uh oh!

bors commented Nov 17, 2021

Uh oh!

bors commented Nov 17, 2021

Uh oh!

bors commented Nov 17, 2021

Uh oh!

Uh oh!

russweas commented Nov 16, 2021 •

edited

Loading