bootstrap: fix clean's remove_dir_all implementation #129187

jieyouxu · 2024-08-17T09:38:02Z

It turns out bootstrap's clean.rs's hand-rolled rm_rf (which probably comes before std::fs::remove_dir_all was stable) is very broken on native Windows around both read-only files/directories and especially symbolic links. So instead of rolling our own, just use std::fs::remove_dir_all.

This is a related to compiletest's own rm_rf implementation #129155 which happens to be also buggy, which in turn is a blocker for the rmake.rs test port #128562 that heavily exercises symlinks (I was reviewing #128562 and testing it on native Windows which is how I found out).

Fixes #112544 (because now we handle Windows symlinks properly).

try-job: x86_64-msvc
try-job: i686-mingw
try-job: test-various
try-job: armhf-gnu
try-job: aarch64-apple
try-job: aarch64-gnu

rustbot · 2024-08-17T09:38:10Z

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

rustbot · 2024-08-17T09:38:11Z

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

…nks, r=<try> bootstrap: fix clean's remove_dir_all implementation It turns out bootstrap's `clean.rs`'s hand-rolled `rm_rf` (which probably comes before `std::fs::remove_dir_all` was stable) is very broken on native Windows around both read-only files/directories and especially symbolic links. So instead of rolling our own, just use `std::fs::remove_dir_all`. Because I wasn't super sure about `std::fs::remove_dir_all`'s behavior and to catch `std::fs::remove_dir_all`'s behavioral changes here on forward, I added a collection of tests that checks if our expectation of the behavior of `std::fs::remove_dir_all` and its underlying Unix syscalls and Win32 APIs matches with its actual behavior. This is a blocker for compiletest's own `rm_rf` implementation rust-lang#129155 which happens to be also buggy, which in turn is a blocker for the rmake.rs test port rust-lang#128562 that heavily exercises symlinks (I was reviewing rust-lang#128562 and testing it on native Windows which is how I found out). I also left a FIXME for `detect_src_and_out` due to a failing assertion on native Windows: ``` ---- core::config::tests::detect_src_and_out stdout ---- thread 'core::config::tests::detect_src_and_out' panicked at src\core\config\tests.rs:72:13: assertion `left == right` failed left: "E:\\tmp" right: "C:\\tmp" ``` Fixes rust-lang#112544 (because now we handle Windows symlinks properly). try-job: x86_64-msvc try-job: i686-mingw try-job: test-various try-job: armhf-gnu try-job: aarch64-apple try-job: aarch64-gnu

src/bootstrap/src/core/build_steps/clean/tests.rs

src/bootstrap/src/core/config/tests.rs

ChrisDenton · 2024-08-17T10:30:44Z

I feel like the std::fs::remove_dir_all tests should be std tests rather than bootstrap tests.

jieyouxu · 2024-08-17T10:46:50Z

I feel like the std::fs::remove_dir_all tests should be std tests rather than bootstrap tests.

I might actually remove the tests because std already has some for remove_dir_all. The set of tests here make simplifying assumptions which would be subtly wrong for std (fallback behavior for Windows targets less than Windows 10 RS1 are not accounted for in this set of tests).

src/bootstrap/src/core/build_steps/clean/tests.rs

jieyouxu · 2024-08-17T13:22:33Z

@rustbot ready

Kobzol · 2024-08-17T13:33:08Z

@bors try

…nks, r=<try> bootstrap: fix clean's remove_dir_all implementation It turns out bootstrap's `clean.rs`'s hand-rolled `rm_rf` (which probably comes before `std::fs::remove_dir_all` was stable) is very broken on native Windows around both read-only files/directories and especially symbolic links. So instead of rolling our own, just use `std::fs::remove_dir_all`. This is a blocker for compiletest's own `rm_rf` implementation rust-lang#129155 which happens to be also buggy, which in turn is a blocker for the rmake.rs test port rust-lang#128562 that heavily exercises symlinks (I was reviewing rust-lang#128562 and testing it on native Windows which is how I found out). I also left a FIXME for `detect_src_and_out` due to a failing assertion on native Windows (opened rust-lang#129188): ``` ---- core::config::tests::detect_src_and_out stdout ---- thread 'core::config::tests::detect_src_and_out' panicked at src\core\config\tests.rs:72:13: assertion `left == right` failed left: "E:\\tmp" right: "C:\\tmp" ``` Fixes rust-lang#112544 (because now we handle Windows symlinks properly). try-job: x86_64-msvc try-job: i686-mingw try-job: test-various try-job: armhf-gnu try-job: aarch64-apple try-job: aarch64-gnu

bors · 2024-08-17T13:34:18Z

⌛ Trying commit 8af5c0a with merge ca401c6...

bors · 2024-08-17T15:48:40Z

☀️ Try build successful - checks-actions
Build commit: ca401c6 (ca401c6512833e829cbb2f14741b81fb9fff8d0a)

bors · 2024-08-20T20:06:09Z

📌 Commit 1687c55 has been approved by Kobzol

It is now in the queue for this repository.

…links, r=Kobzol bootstrap: fix clean's remove_dir_all implementation It turns out bootstrap's `clean.rs`'s hand-rolled `rm_rf` (which probably comes before `std::fs::remove_dir_all` was stable) is very broken on native Windows around both read-only files/directories and especially symbolic links. So instead of rolling our own, just use `std::fs::remove_dir_all`. This is a blocker for compiletest's own `rm_rf` implementation rust-lang#129155 which happens to be also buggy, which in turn is a blocker for the rmake.rs test port rust-lang#128562 that heavily exercises symlinks (I was reviewing rust-lang#128562 and testing it on native Windows which is how I found out). I also left a FIXME for `detect_src_and_out` due to a failing assertion on native Windows (opened rust-lang#129188): ``` ---- core::config::tests::detect_src_and_out stdout ---- thread 'core::config::tests::detect_src_and_out' panicked at src\core\config\tests.rs:72:13: assertion `left == right` failed left: "E:\\tmp" right: "C:\\tmp" ``` Fixes rust-lang#112544 (because now we handle Windows symlinks properly). try-job: x86_64-msvc try-job: i686-mingw try-job: test-various try-job: armhf-gnu try-job: aarch64-apple try-job: aarch64-gnu

Rollup of 7 pull requests Successful merges: - rust-lang#128432 (WASI: forbid `unsafe_op_in_unsafe_fn` for `std::{os, sys}`) - rust-lang#128627 (Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.) - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation) - rust-lang#129257 (Allow rust staticlib to work with MSVC's /WHOLEARCHIVE) - rust-lang#129264 (Update `library/Cargo.toml` in weekly job) - rust-lang#129284 (rustdoc: animate the `:target` highlight) - rust-lang#129332 (Avoid extra `cast()`s after `CStr::as_ptr()`) 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

…links, r=Kobzol bootstrap: fix clean's remove_dir_all implementation It turns out bootstrap's `clean.rs`'s hand-rolled `rm_rf` (which probably comes before `std::fs::remove_dir_all` was stable) is very broken on native Windows around both read-only files/directories and especially symbolic links. So instead of rolling our own, just use `std::fs::remove_dir_all`. This is a blocker for compiletest's own `rm_rf` implementation rust-lang#129155 which happens to be also buggy, which in turn is a blocker for the rmake.rs test port rust-lang#128562 that heavily exercises symlinks (I was reviewing rust-lang#128562 and testing it on native Windows which is how I found out). I also left a FIXME for `detect_src_and_out` due to a failing assertion on native Windows (opened rust-lang#129188): ``` ---- core::config::tests::detect_src_and_out stdout ---- thread 'core::config::tests::detect_src_and_out' panicked at src\core\config\tests.rs:72:13: assertion `left == right` failed left: "E:\\tmp" right: "C:\\tmp" ``` Fixes rust-lang#112544 (because now we handle Windows symlinks properly). try-job: x86_64-msvc try-job: i686-mingw try-job: test-various try-job: armhf-gnu try-job: aarch64-apple try-job: aarch64-gnu

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

Rollup merge of rust-lang#129187 - jieyouxu:squeaky-clean-windows-symlinks, r=Kobzol bootstrap: fix clean's remove_dir_all implementation It turns out bootstrap's `clean.rs`'s hand-rolled `rm_rf` (which probably comes before `std::fs::remove_dir_all` was stable) is very broken on native Windows around both read-only files/directories and especially symbolic links. So instead of rolling our own, just use `std::fs::remove_dir_all`. This is a blocker for compiletest's own `rm_rf` implementation rust-lang#129155 which happens to be also buggy, which in turn is a blocker for the rmake.rs test port rust-lang#128562 that heavily exercises symlinks (I was reviewing rust-lang#128562 and testing it on native Windows which is how I found out). I also left a FIXME for `detect_src_and_out` due to a failing assertion on native Windows (opened rust-lang#129188): ``` ---- core::config::tests::detect_src_and_out stdout ---- thread 'core::config::tests::detect_src_and_out' panicked at src\core\config\tests.rs:72:13: assertion `left == right` failed left: "E:\\tmp" right: "C:\\tmp" ``` Fixes rust-lang#112544 (because now we handle Windows symlinks properly). try-job: x86_64-msvc try-job: i686-mingw try-job: test-various try-job: armhf-gnu try-job: aarch64-apple try-job: aarch64-gnu

krasimirgg · 2024-08-22T14:42:03Z

For some reason, after this we're getting some failures with x.py clean. Initially I saw this in our rust+llvm@head build bot: https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/30548#01917a4a-a850-4dd1-9c7a-38da213bc8ea.

I tried it with a freshly-cloned version of rust and was able to reproduce as:

git clone https://github.com/rust-lang/rust
cd rust
git clean -fdx
python3 x.py clean --all
...
thread 'main' panicked at src/core/build_steps/clean.rs:178:9:
failed to `remove_dir_all` at `tmp`: No such file or directory (os error 2)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Creating a tmp directory under the checked out rust seems to fix it.

ChrisDenton · 2024-08-22T15:24:48Z

Ah, I believe that's fixed in std by #127623

In the meantime bootstrap will need a workaround that skips deleting directories that don't exist.

EDIT: Oh, maybe not fixed as it would still error on the top-level directory. So the workaround would need to be permanent.

jieyouxu · 2024-08-22T15:41:30Z

Oh wait. I think I didn't consider the top-level directory missing but was ultrafocused on read-only/symlinks of nested directory entries.

I'm going to revert the two PRs then reland the PRs properly accounting for missing top-level directories.

jieyouxu · 2024-08-22T15:42:54Z

For some reason, after this we're getting some failures with x.py clean. Initially I saw this in our rust+llvm@head build bot: buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/30548#01917a4a-a850-4dd1-9c7a-38da213bc8ea.

Thanks for the heads up! This PR and the compiletest PR is indeed the culprit. My apologies for borking the buildsystem.

jieyouxu · 2024-08-22T16:04:39Z

For some reason, after this we're getting some failures with x.py clean.

Revert PR is up at #129413.

…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

…rors Revert #129187 and #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/rust#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 #129187. Reverts #129302. r? bootstrap

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

rustbot assigned Kobzol Aug 17, 2024

rustbot added 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 17, 2024

This comment was marked as outdated.

Sign in to view

jieyouxu 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2024

jieyouxu commented Aug 17, 2024

View reviewed changes

src/bootstrap/src/core/build_steps/clean/tests.rs Outdated Show resolved Hide resolved

jieyouxu commented Aug 17, 2024

View reviewed changes

src/bootstrap/src/core/config/tests.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

jieyouxu force-pushed the squeaky-clean-windows-symlinks branch from e736cb4 to 1ef814a Compare August 17, 2024 10:50

This comment has been minimized.

Sign in to view

jieyouxu force-pushed the squeaky-clean-windows-symlinks branch from 1ef814a to c762aa6 Compare August 17, 2024 11:06

jieyouxu commented Aug 17, 2024

View reviewed changes

src/bootstrap/src/core/build_steps/clean/tests.rs Outdated Show resolved Hide resolved

jieyouxu force-pushed the squeaky-clean-windows-symlinks branch from c762aa6 to 8af5c0a Compare August 17, 2024 11:27

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 17, 2024

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

tgross35 mentioned this pull request Aug 21, 2024

Rollup of 7 pull requests #129349

Closed

tgross35 mentioned this pull request Aug 21, 2024

Rollup of 7 pull requests #129351

Closed

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 9fd8a2c 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 squeaky-clean-windows-symlinks branch August 22, 2024 09:28

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

bootstrap: fix clean's remove_dir_all implementation #129187

bootstrap: fix clean's remove_dir_all implementation #129187

jieyouxu commented Aug 17, 2024 •

edited

Loading

rustbot commented Aug 17, 2024

rustbot commented Aug 17, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment has been minimized.

ChrisDenton commented Aug 17, 2024

This comment has been minimized.

This comment was marked as outdated.

jieyouxu commented Aug 17, 2024 •

edited

Loading

This comment has been minimized.

jieyouxu commented Aug 17, 2024

Kobzol commented Aug 17, 2024

bors commented Aug 17, 2024

bors commented Aug 17, 2024

bors commented Aug 20, 2024

krasimirgg commented Aug 22, 2024

ChrisDenton commented Aug 22, 2024 •

edited

Loading

jieyouxu commented Aug 22, 2024

jieyouxu commented Aug 22, 2024

jieyouxu commented Aug 22, 2024

bootstrap: fix clean's remove_dir_all implementation #129187

bootstrap: fix clean's remove_dir_all implementation #129187

Conversation

jieyouxu commented Aug 17, 2024 • edited Loading

rustbot commented Aug 17, 2024

rustbot commented Aug 17, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment has been minimized.

ChrisDenton commented Aug 17, 2024

This comment has been minimized.

This comment was marked as outdated.

jieyouxu commented Aug 17, 2024 • edited Loading

This comment has been minimized.

jieyouxu commented Aug 17, 2024

Kobzol commented Aug 17, 2024

bors commented Aug 17, 2024

bors commented Aug 17, 2024

bors commented Aug 20, 2024

krasimirgg commented Aug 22, 2024

ChrisDenton commented Aug 22, 2024 • edited Loading

jieyouxu commented Aug 22, 2024

jieyouxu commented Aug 22, 2024

jieyouxu commented Aug 22, 2024

jieyouxu commented Aug 17, 2024 •

edited

Loading

jieyouxu commented Aug 17, 2024 •

edited

Loading

ChrisDenton commented Aug 22, 2024 •

edited

Loading