compiletest: use `std::fs::remove_dir_all` now that it is available #129302

jieyouxu · 2024-08-20T09:29:44Z

It turns out aggressive_rm_rf is not sufficiently aggressive (RAGEY) on Windows and obviously handles Windows symlinks incorrectly. Instead of rolling our own version, let's use std::fs::remove_dir_all now that it's available (well, it's been available for a good while, but probably wasn't available when this helper was written).

cc #129187 since basically this is failing due to similar problems.

Blocker for #128562.
Fixes #129155.
Fixes #126334.

rustbot · 2024-08-20T09:29:52Z

r? @Kobzol

rustbot has assigned @Kobzol.
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

jieyouxu · 2024-08-20T09:32:25Z

src/tools/compiletest/src/runtest.rs

-                // Remove readonly files as well on windows (by default we can't)
-                fs::remove_file(&path).or_else(|e| {
-                    if cfg!(windows) && e.kind() == io::ErrorKind::PermissionDenied {
-                        let mut meta = entry.metadata()?.permissions();
-                        meta.set_readonly(false);
-                        fs::set_permissions(&path, meta)?;
-                        fs::remove_file(&path)
-                    } else {
-                        Err(e)
-                    }
-                })?;


Remark: cc #128562 (comment) for why this is wrong with respect to Windows symlinks.

compiler-errors · 2024-08-20T15:23:20Z

@bors r+

bors · 2024-08-20T15:23:23Z

📌 Commit 75ed089 has been approved by compiler-errors

It is now in the queue for this repository.

Kobzol · 2024-08-20T18:29:09Z

Does std::fs::remove_dir_all work with read-only files/dirs on Windows (and Linux)?

jieyouxu · 2024-08-20T18:42:12Z

Hmm good point, let me triple-check, I thought it did.
@bors r-

Kobzol · 2024-08-20T18:47:56Z

I'm asking because https://docs.rs/remove_dir_all/latest/remove_dir_all/ explicitly mentions that it handles the readonly flag on Windows, so it seems like the std::fs version might not do that.

jieyouxu · 2024-08-20T18:49:58Z

I'm asking because docs.rs/remove_dir_all/latest/remove_dir_all explicitly mentions that it handles the readonly flag on Windows, so it seems like the std::fs version might not do that.

Based on

rust/library/std/src/sys/pal/windows/fs.rs

Lines 649 to 664 in 4d5b3b1

    
           /// Delete using POSIX semantics. 
        
           /// 
        
           /// Files will be deleted as soon as the handle is closed. This is supported 
        
           /// for Windows 10 1607 (aka RS1) and later. However some filesystem 
        
           /// drivers will not support it even then, e.g. FAT32. 
        
           /// 
        
           /// If the operation is not supported for this filesystem or OS version 
        
           /// then errors will be `ERROR_NOT_SUPPORTED` or `ERROR_INVALID_PARAMETER`. 
        
           fn posix_delete(&self) -> io::Result<()> { 
        
               let info = c::FILE_DISPOSITION_INFO_EX { 
        
                   Flags: c::FILE_DISPOSITION_FLAG_DELETE 
        
                       | c::FILE_DISPOSITION_FLAG_POSIX_SEMANTICS 
        
                       | c::FILE_DISPOSITION_FLAG_IGNORE_READONLY_ATTRIBUTE, 
        
               }; 
        
               api::set_file_information_by_handle(self.handle.as_raw_handle(), &info).io_result() 
        
           }

I believe it is bypassing the read-only flag with FILE_DISPOSITION_FLAG_IGNORE_READONLY_ATTRIBUTE ~~(undocumented API on MSDN). But cc @ChrisDenton as you know way more about this...~~

However, that seems only to be true for the posix delete semantics path which is available for Windows 10 RS1 and beyond, but not Windows 7.

At least in #129187 when I added the test cases (I later removed them because std already has tests for them), I tested combination of symlinks and read-onlyn files/directories on Windows, and std::fs::remove_dir_all handled them no problem locally on Windows 11.

EDIT: asked Chris, yes it does, see https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntddk/ns-ntddk-_file_disposition_information_ex, specifically

FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE
Allows read-only files to be deleted.

EDIT 2: for Unix, read-only files can be deleted without special handling. Note that unlink the syscall is not rm; the rm command does extra read-only checking.

jieyouxu · 2024-08-20T19:25:45Z

Based on the investigation above, I think std::fs::remove_dir_all does handle read-only files and directories on both Windows (at least beyond and incl. Windows 10 RS1) and Unix. Let me know if you have concerns @Kobzol.

@rustbot ready

jieyouxu · 2024-08-20T19:32:57Z

I'm asking because docs.rs/remove_dir_all/latest/remove_dir_all explicitly mentions that it handles the readonly flag on Windows, so it seems like the std::fs version might not do that.

I think those docs were true, but no longer accurate since #93112.

Kobzol · 2024-08-20T20:00:12Z

Thanks for checking. No other concerns from me.

jieyouxu · 2024-08-20T20:04:19Z

@bors r=@compiler-errors (reapproving as concern resolved)

bors · 2024-08-20T20:04:21Z

📌 Commit 75ed089 has been approved by compiler-errors

It is now in the queue for this repository.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#127279 (use old ctx if has same expand environment during decode span) - rust-lang#127945 (Implement `debug_more_non_exhaustive`) - rust-lang#128941 ( Improve diagnostic-related lints: `untranslatable_diagnostic` & `diagnostic_outside_of_impl`) - rust-lang#129070 (Point at explicit `'static` obligations on a trait) - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation) - rust-lang#129231 (improve submodule updates) - rust-lang#129264 (Update `library/Cargo.toml` in weekly job) - rust-lang#129284 (rustdoc: animate the `:target` highlight) - rust-lang#129302 (compiletest: use `std::fs::remove_dir_all` now that it is available) r? `@ghost` `@rustbot` modify labels: rollup

…iler-errors compiletest: use `std::fs::remove_dir_all` now that it is available It turns out `aggressive_rm_rf` is not sufficiently aggressive (RAGEY) on Windows and obviously handles Windows symlinks incorrectly. Instead of rolling our own version, let's use `std::fs::remove_dir_all` now that it's available (well, it's been available for a good while, but probably wasn't available when this helper was written). cc rust-lang#129187 since basically this is failing due to similar problems. Blocker for rust-lang#128562. Fixes rust-lang#129155. Fixes rust-lang#126334.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#127279 (use old ctx if has same expand environment during decode span) - rust-lang#127945 (Implement `debug_more_non_exhaustive`) - rust-lang#128941 ( Improve diagnostic-related lints: `untranslatable_diagnostic` & `diagnostic_outside_of_impl`) - rust-lang#129070 (Point at explicit `'static` obligations on a trait) - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation) - rust-lang#129231 (improve submodule updates) - rust-lang#129264 (Update `library/Cargo.toml` in weekly job) - rust-lang#129284 (rustdoc: animate the `:target` highlight) - rust-lang#129302 (compiletest: use `std::fs::remove_dir_all` now that it is available) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129302 - jieyouxu:compiletest-RAGEY, r=compiler-errors compiletest: use `std::fs::remove_dir_all` now that it is available It turns out `aggressive_rm_rf` is not sufficiently aggressive (RAGEY) on Windows and obviously handles Windows symlinks incorrectly. Instead of rolling our own version, let's use `std::fs::remove_dir_all` now that it's available (well, it's been available for a good while, but probably wasn't available when this helper was written). cc rust-lang#129187 since basically this is failing due to similar problems. Blocker for rust-lang#128562. Fixes rust-lang#129155. Fixes rust-lang#126334.

…mpiler-errors Revert rust-lang#129187 and rust-lang#129302 The two PRs naively switched to `std::fs::remove_dir_all`, but failed to gracefully handle the failure case where the top-level directory entry does not exist, causing rust-lang#129187 (comment) `./x clean` to fail locally when `tmp` does not exist. I plan to reland the two PRs with fixed top-level dir entry handling and more testing, but let's quickly revert to unblock people. Reverts rust-lang#129187. Reverts rust-lang#129302. r? bootstrap

compiletest: use std::fs::remove_dir_all now that it is available

75ed089

rustbot assigned Kobzol Aug 20, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 20, 2024

jieyouxu commented Aug 20, 2024

View reviewed changes

jieyouxu added the O-windows Operating system: Windows label Aug 20, 2024

compiler-errors approved these changes Aug 20, 2024

View reviewed changes

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 Aug 20, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 20, 2024

This comment was marked as resolved.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 20, 2024

jieyouxu mentioned this pull request Aug 20, 2024

bootstrap: fix clean's remove_dir_all implementation #129187

Merged

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 Aug 20, 2024

matthiaskrgr mentioned this pull request Aug 21, 2024

Rollup of 9 pull requests #129365

Merged

jieyouxu mentioned this pull request Aug 22, 2024

Rollup of 8 pull requests #129393

Closed

bors merged commit 33dc313 into rust-lang:master Aug 22, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 22, 2024

jieyouxu deleted the compiletest-RAGEY branch August 22, 2024 09:29

jieyouxu mentioned this pull request Aug 22, 2024

Revert #129187 and #129302 #129413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiletest: use `std::fs::remove_dir_all` now that it is available #129302

compiletest: use `std::fs::remove_dir_all` now that it is available #129302

jieyouxu commented Aug 20, 2024 •

edited

Loading

rustbot commented Aug 20, 2024

jieyouxu Aug 20, 2024 •

edited

Loading

compiler-errors commented Aug 20, 2024

bors commented Aug 20, 2024

Kobzol commented Aug 20, 2024

jieyouxu commented Aug 20, 2024

Kobzol commented Aug 20, 2024 •

edited

Loading

jieyouxu commented Aug 20, 2024 •

edited

Loading

This comment was marked as resolved.

jieyouxu commented Aug 20, 2024

jieyouxu commented Aug 20, 2024

Kobzol commented Aug 20, 2024

jieyouxu commented Aug 20, 2024

bors commented Aug 20, 2024

compiletest: use std::fs::remove_dir_all now that it is available #129302

compiletest: use std::fs::remove_dir_all now that it is available #129302

Conversation

jieyouxu commented Aug 20, 2024 • edited Loading

rustbot commented Aug 20, 2024

jieyouxu Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

compiler-errors commented Aug 20, 2024

bors commented Aug 20, 2024

Kobzol commented Aug 20, 2024

jieyouxu commented Aug 20, 2024

Kobzol commented Aug 20, 2024 • edited Loading

jieyouxu commented Aug 20, 2024 • edited Loading

This comment was marked as resolved.

jieyouxu commented Aug 20, 2024

jieyouxu commented Aug 20, 2024

Kobzol commented Aug 20, 2024

jieyouxu commented Aug 20, 2024

bors commented Aug 20, 2024

compiletest: use `std::fs::remove_dir_all` now that it is available #129302

compiletest: use `std::fs::remove_dir_all` now that it is available #129302

jieyouxu commented Aug 20, 2024 •

edited

Loading

jieyouxu Aug 20, 2024 •

edited

Loading

Kobzol commented Aug 20, 2024 •

edited

Loading

jieyouxu commented Aug 20, 2024 •

edited

Loading