Migrate `incr-add-rust-src-component` and `issue-84395-lto-embed-bitcode` `run-make` tests to rmake #128562

Oneirical · 2024-08-02T17:43:36Z

Part of #121876 and the associated Google Summer of Code project.

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

rustbot · 2024-08-02T17:43:43Z

r? @jieyouxu

rustbot has assigned @jieyouxu.
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-02T17:43:45Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

Kobzol · 2024-08-02T19:22:37Z

@bors try

Migrate `incr-add-rust-src-component` and `issue-84395-lto-embed-bitcode` `run-make` tests to rmake Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). In `incr-add-rust-src-component`, I directly modify the sysroot, unlike the original test which used a symlink. This is probably a very bad idea, but I want to see what happens. try-job: x86_64-gnu-debug

bors · 2024-08-02T19:23:49Z

⌛ Trying commit 25380bc with merge 5de06a2...

bors · 2024-08-02T20:56:13Z

💔 Test failed - checks-actions

bors · 2024-08-02T21:49:52Z

☔ The latest upstream changes (presumably #128361) made this pull request unmergeable. Please resolve the merge conflicts.

Oneirical · 2024-08-05T17:36:34Z

@bors try

(the incr-add-rust-src-component is still broken, but the try job should only run issue-84395-lto-embed-bitcode)

@rustbot review

About the sysroot problem: the original test creates a symbolic link to the sysroot and starts editing it - how does this not just follow the link to the real sysroot and break the test?

The syslink feature in run_make_support seems much more limited. One clear example is when I tried to port the test libs-through-symlinks, which has this line: ln -nsf outdir/$(NAME) $(TMPDIR).

This means it's supposed to make the current working directory a link to another place while simultaneously keeping all its contents. This is some quantum physics wormhole stuff.

bors · 2024-08-05T17:36:37Z

@Oneirical: 🔑 Insufficient privileges: not in try users

Kobzol · 2024-08-05T18:04:04Z

@bors try

bors · 2024-08-05T18:10:29Z

⌛ Trying commit 76a7e72 with merge 67d4129...

Migrate `incr-add-rust-src-component` and `issue-84395-lto-embed-bitcode` `run-make` tests to rmake Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). In `incr-add-rust-src-component`, I directly modify the sysroot, unlike the original test which used a symlink. This is probably a very bad idea, but I want to see what happens. try-job: x86_64-gnu-debug

Oneirical · 2024-08-14T16:58:40Z

our windows CI jobs actually have symlink privileges

The try job has failed due to a permissions error. Is this a case where creating the symlink is authorized, but removing it isn't? @rustbot ready

jieyouxu · 2024-08-14T17:01:06Z

I'll get back to you after I try it locally later.

jieyouxu

I am the bearer of bad news, so here are some caveats for your caveats:

jieyouxu · 2024-08-16T14:00:26Z

tests/run-make/incr-add-rust-src-component/rmake.rs

+fn symlink_all_entries<P: AsRef<std::path::Path>>(dir: P, fakepath: &str) {
+    for found_path in rfs::shallow_find_dir_entries(dir) {
+        rfs::create_symlink(&found_path, path(fakepath).join(found_path.file_name().unwrap()));
+    }
+}


Problem [REV1 (1/2)]: recall that rfs::create_symlink on Windows actually diverges into two function calls conditioned on if found_path is a file or if it's a directory:

If it's a directory, then a symlink-to-dir is created via std::os::windows::fs::symlink_dir. This underneath calls CreateSymbolicLinkW with the SYMBOLIC_LINK_FLAG_DIRECTORY flag set.

If it's a file, then a symlink-to-file is created via std::os::windows::fs::symlink_file. This underneath calls CreateSymbolicLinkW with no flags set.

jieyouxu · 2024-08-16T14:12:55Z

tests/run-make/incr-add-rust-src-component/rmake.rs

+    rfs::create_dir("fakeroot");
+    symlink_all_entries(&sysroot, "fakeroot");
+    rfs::remove_file("fakeroot/lib");


Problem [REV1 (2/2)]: unfortunately, there's a whole rabbit hole for symlinks, with different tunnel branches for each platform. The following description is only true for your bog-standard symbolic link ("soft links" that is unofficially called to distinguish against hard links), not junction points, nor hard links.

On Windows

But young padawan (myself included), the API contract for CreateSymlinkW on Windows says:

To remove a symbolic link, delete the file (using DeleteFile or similar APIs) or remove the directory (using RemoveDirectory or similar APIs) depending on what type of symbolic link is used.

Those are the underlying Win32 APIs called by std::fs::remove_file and std::fs::remove_dir respectively.

If we don't know the kind of symlink on Windows beforehand, we can check it with std::os::windows::fs::FileTypeExt with is_symlink_dir and is_symlink_file. Thank you so much @ChrisDenton for pointing this out to me!

This remove_file and several below it is wrong (on Windows) because it's trying to call DeleteFileW on a directory.

On Linux / macOS

But on Linux/macOS, we need to use std::fs::remove_file because that corresponds to unlink on Unix, and there's no distinction for unlink with regard to symbolic links in terms of if the symbolic links point to files or directories.

Ah but wait, there's another caveat. If you want to interrogate the metadata of the symlink itself, you must use std::fs::symlink_metadata and not std::fs::metadata which will follow through symlinks to the linked directory/file.

std::fs::FileType: The underlying Metadata struct needs to be retrieved with the fs::symlink_metadata function and not the fs::metadata function. The fs::metadata function follows symbolic links, so is_symlink would always return false for the target file.

Suggestion: we probably want to implement our very own remove_symlink helper that handles these specialness.

jieyouxu · 2024-08-16T14:32:22Z

This will also need to be blocked on a compiletest bug that I need to fix regarding trying to remove symlinks in the per-test rmake_out/ directory via aggressive_rm_rf...

@rustbot blocked (on #129155: compiletest aggressive_rm_rf bug regarding symlink removal on windows)

jieyouxu · 2024-08-17T05:22:10Z

It turns out the compiletest fix is going to be blocked on a bootstrap fix for similar problems with symlink on windows.

…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

…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

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

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

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

rustbot assigned jieyouxu Aug 2, 2024

Oneirical force-pushed the no-contest-expected branch from 68eaebb to 84d0649 Compare August 2, 2024 17:44

This comment has been minimized.

Sign in to view

Oneirical force-pushed the no-contest-expected branch from 84d0649 to 25380bc Compare August 2, 2024 18:41

Kobzol mentioned this pull request Aug 2, 2024

Tracking Issue for porting run-make tests to use Rust #121876

Open

This comment has been minimized.

Sign in to view

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

Oneirical force-pushed the no-contest-expected branch from 25380bc to 76a7e72 Compare August 5, 2024 17:35

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

This comment has been minimized.

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

jieyouxu requested changes Aug 16, 2024

View reviewed changes

rustbot 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 16, 2024

rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 16, 2024

jieyouxu mentioned this pull request Aug 16, 2024

compiletest::runtest: aggressive_rm_rf does not like symlinks on Windows #129155

Open

jieyouxu mentioned this pull request Aug 17, 2024

bootstrap: fix clean's remove_dir_all implementation #129187

Merged

jieyouxu mentioned this pull request Aug 20, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `incr-add-rust-src-component` and `issue-84395-lto-embed-bitcode` `run-make` tests to rmake #128562

Migrate `incr-add-rust-src-component` and `issue-84395-lto-embed-bitcode` `run-make` tests to rmake #128562

Oneirical commented Aug 2, 2024 •

edited

Loading

rustbot commented Aug 2, 2024

rustbot commented Aug 2, 2024

This comment has been minimized.

Kobzol commented Aug 2, 2024

bors commented Aug 2, 2024

This comment has been minimized.

This comment has been minimized.

bors commented Aug 2, 2024

bors commented Aug 2, 2024

Oneirical commented Aug 5, 2024 •

edited

Loading

bors commented Aug 5, 2024

Kobzol commented Aug 5, 2024

bors commented Aug 5, 2024

This comment has been minimized.

This comment has been minimized.

Oneirical commented Aug 14, 2024 •

edited

Loading

jieyouxu commented Aug 14, 2024

jieyouxu left a comment

jieyouxu Aug 16, 2024

jieyouxu Aug 16, 2024 •

edited

Loading

jieyouxu Aug 16, 2024

jieyouxu commented Aug 16, 2024 •

edited

Loading

jieyouxu commented Aug 17, 2024

Migrate incr-add-rust-src-component and issue-84395-lto-embed-bitcode run-make tests to rmake #128562

Are you sure you want to change the base?

Migrate incr-add-rust-src-component and issue-84395-lto-embed-bitcode run-make tests to rmake #128562

Conversation

Oneirical commented Aug 2, 2024 • edited Loading

rustbot commented Aug 2, 2024

rustbot commented Aug 2, 2024

This comment has been minimized.

Kobzol commented Aug 2, 2024

bors commented Aug 2, 2024

This comment has been minimized.

This comment has been minimized.

bors commented Aug 2, 2024

bors commented Aug 2, 2024

Oneirical commented Aug 5, 2024 • edited Loading

bors commented Aug 5, 2024

Kobzol commented Aug 5, 2024

bors commented Aug 5, 2024

This comment has been minimized.

This comment has been minimized.

Oneirical commented Aug 14, 2024 • edited Loading

jieyouxu commented Aug 14, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu Aug 16, 2024

Choose a reason for hiding this comment

jieyouxu Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

On Windows

On Linux / macOS

jieyouxu Aug 16, 2024

Choose a reason for hiding this comment

jieyouxu commented Aug 16, 2024 • edited Loading

jieyouxu commented Aug 17, 2024

Migrate `incr-add-rust-src-component` and `issue-84395-lto-embed-bitcode` `run-make` tests to rmake #128562

Migrate `incr-add-rust-src-component` and `issue-84395-lto-embed-bitcode` `run-make` tests to rmake #128562

Oneirical commented Aug 2, 2024 •

edited

Loading

Oneirical commented Aug 5, 2024 •

edited

Loading

Oneirical commented Aug 14, 2024 •

edited

Loading

jieyouxu Aug 16, 2024 •

edited

Loading

jieyouxu commented Aug 16, 2024 •

edited

Loading