Migrate `incr-add-rust-src-component` to rmake #134656

jieyouxu · 2024-12-22T12:22:09Z

This PR partially supersedes #128562, and ports the Makefile-based tests/run-make/incr-add-rust-src-component to use rmake.rs infra.

Part of #121876.

This run-make test is a regression test for #70924. It (tries to) checks that if we add the rust-src component in between two incremental compiles, that the compiler doesn't ICE on the second invocation.

Original issue:'rustc' panicked at 'failed to lookup SourceFile in new context' #70924
Fix PR: Track devirtualized filenames #72767
PR adding this regression test: run-make regression test for issue #70924. #72952

However, the Makefile version of this used $SYSROOT/lib/rustlib/src/rust/src/libstd/lib.rs, but that actually got moved around and reorganized over the years. As of Dec 2024, the rust-src component is more like (specific for our purposes):

$SYSROOT/lib/rustlib/src/rust/
    library/std/src/lib.rs
    src/

However, this run-make test is ancient and it exercises incr-comp system logic. I'm not sure if this test would actually catch the original regression.

This PR was co-authored with @Oneirical.

r? incremental

try-job: i686-msvc
try-job: x86_64-mingw-1
try-job: x86_64-msvc
try-job: aarch64-apple

jieyouxu · 2024-12-22T17:46:53Z

@bors try

…omponent, r=<try> Migrate `incr-add-rust-src-component` to rmake **BLOCKED: this PR depends on rust-lang#134659** **TODO: actually write some useful PR description** r? `@ghost` try-job: i686-msvc try-job: x86_64-mingw-1 try-job: x86_64-msvc try-job: aarch64-apple

bors · 2024-12-22T17:48:04Z

⌛ Trying commit b334ba5 with merge 30b610b...

jieyouxu · 2024-12-22T18:57:51Z

@bors try

bors · 2024-12-22T18:59:02Z

⌛ Trying commit 41b2c3d with merge c56f737...

…omponent, r=<try> Migrate `incr-add-rust-src-component` to rmake **BLOCKED: this PR depends on rust-lang#134659** **TODO: actually write some useful PR description** r? `@ghost` try-job: i686-msvc try-job: x86_64-mingw-1 try-job: x86_64-msvc try-job: aarch64-apple

bors · 2024-12-22T21:32:44Z

☀️ Try build successful - checks-actions
Build commit: c56f737 (c56f737a666ba836105e48d9e405a5a6dfbca35e)

…Denton test-infra: improve compiletest and run-make-support symlink handling I was trying to implement rust-lang#134656 to port `tests/run-make/incr-add-rust-src-component.rs`, but found some blockers related to symlink handling, so in this PR I tried to resolve them by improving symlink handling in compiletest and run-make-support (particularly for native windows msvc environment). Key changes: - I needed to copy symlinks (duplicate a symlink pointing to the same file), so I pulled out the copy symlink logic and re-exposed it as `run_make_support::rfs::copy_symlink`. This helper correctly accounts for the Windows symlink-to-file vs symlink-to-dir distinction (hereafter "Windows symlinks") when copying symlinks. - `recursive_remove`: - I needed a way to remove symlinks themselves (no symlink traversal). `std::fs::remove_dir_all` handles them, but only if the root path is a directory. So I wrapped `std::fs::remove_dir_all` to also handle when the root path is a non-directory entity (e.g. file or symlink). Again, this properly accounts for Windows symlinks. - I wanted to use this for both compiletest and run-make-support, so I put the implementation and accompanying tests in `build_helper`. - In this sense, it's a reland of rust-lang#129302 with proper test coverage. - It's a thin wrapper around `std::fs::remove_dir_all` (`remove_dir_all` correctly handles read-only entries on Windows). The helper has additional permission-setting logic for when the root path is a non-dir entry on Windows to handle read-only non-dir entry. Fixes rust-lang#126334.

Rollup merge of rust-lang#134659 - jieyouxu:recursive-remove, r=ChrisDenton test-infra: improve compiletest and run-make-support symlink handling I was trying to implement rust-lang#134656 to port `tests/run-make/incr-add-rust-src-component.rs`, but found some blockers related to symlink handling, so in this PR I tried to resolve them by improving symlink handling in compiletest and run-make-support (particularly for native windows msvc environment). Key changes: - I needed to copy symlinks (duplicate a symlink pointing to the same file), so I pulled out the copy symlink logic and re-exposed it as `run_make_support::rfs::copy_symlink`. This helper correctly accounts for the Windows symlink-to-file vs symlink-to-dir distinction (hereafter "Windows symlinks") when copying symlinks. - `recursive_remove`: - I needed a way to remove symlinks themselves (no symlink traversal). `std::fs::remove_dir_all` handles them, but only if the root path is a directory. So I wrapped `std::fs::remove_dir_all` to also handle when the root path is a non-directory entity (e.g. file or symlink). Again, this properly accounts for Windows symlinks. - I wanted to use this for both compiletest and run-make-support, so I put the implementation and accompanying tests in `build_helper`. - In this sense, it's a reland of rust-lang#129302 with proper test coverage. - It's a thin wrapper around `std::fs::remove_dir_all` (`remove_dir_all` correctly handles read-only entries on Windows). The helper has additional permission-setting logic for when the root path is a non-dir entry on Windows to handle read-only non-dir entry. Fixes rust-lang#126334.

The Makefile version seems to contain a bug. Over the years, the directory structure of the `rust-src` component changed as the source tree directory structure changed. `libstd` is no longer a thing directly under `root/lib/rustlib/src/rust/src/`, it is moved to `root/lib/rustlib/src/rust/library/std`. Co-authored-by: Oneirical <[email protected]>

jieyouxu · 2024-12-23T14:08:35Z

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

+//! Regression test for rust-lang/rust#70924. Check that if we add the `rust-src` component in
+//! between two incremental compiles, that the compiler doesn't ICE on the second invocation.
+//!
+//! This test uses symbolic links to save testing time.
+//!
+//! The way this test works is that, for every prefix in `root/lib/rustlib/src`, link all of prefix
+//! parent content, then remove the prefix, then loop on the next prefix. This way, we basically
+//! create a copy of the context around `root/lib/rustlib/src`, and can freely add/remove the src
+//! component itself.


Question: ... is this test actually testing what it thinks it's testing? Be it the original Makefile version or this version...

I think so... in either case, your translation appears to preserve the intended behavior of the test (but perhaps that behavior itself isn't entirely correct?)

jieyouxu · 2024-12-23T14:14:31Z

tests/run-make/incr-add-rust-src-component/Makefile

-# This test uses `ln -s` rather than copying to save testing time, but its
-# usage doesn't work on windows. So ignore windows.
-
-# ignore-windows


Remark: by usage of pure rust test file, we're able to expand the test coverage of this to Windows as well. The symlink handling was fixed and improved in #134659.

wesleywiser · 2024-12-26T17:54:31Z

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

+//! Regression test for rust-lang/rust#70924. Check that if we add the `rust-src` component in
+//! between two incremental compiles, that the compiler doesn't ICE on the second invocation.
+//!
+//! This test uses symbolic links to save testing time.
+//!
+//! The way this test works is that, for every prefix in `root/lib/rustlib/src`, link all of prefix
+//! parent content, then remove the prefix, then loop on the next prefix. This way, we basically
+//! create a copy of the context around `root/lib/rustlib/src`, and can freely add/remove the src
+//! component itself.


I think so... in either case, your translation appears to preserve the intended behavior of the test (but perhaps that behavior itself isn't entirely correct?)

wesleywiser · 2024-12-26T17:54:53Z

@bors r+ rollup

bors · 2024-12-26T17:54:55Z

📌 Commit 7ee5204 has been approved by wesleywiser

It is now in the queue for this repository.

…llaumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#134656 (Migrate `incr-add-rust-src-component` to rmake) - rust-lang#134664 (Account for removal of multiline span in suggestion) - rust-lang#134772 (Improve/cleanup rustdoc code) - rust-lang#134781 (Add more `begin_panic` normalizations to panic backtrace tests) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#134656 - jieyouxu:migrate-incr-add-rust-src-component, r=wesleywiser Migrate `incr-add-rust-src-component` to rmake This PR partially supersedes rust-lang#128562, and ports the Makefile-based `tests/run-make/incr-add-rust-src-component` to use rmake.rs infra. Part of rust-lang#121876. This run-make test is a regression test for rust-lang#70924. It (tries to) checks that if we add the `rust-src` component in between two incremental compiles, that the compiler doesn't ICE on the second invocation. - Original issue:rust-lang#70924 - Fix PR: rust-lang#72767 - PR adding this regression test: rust-lang#72952 However, the Makefile version of this used `$SYSROOT/lib/rustlib/src/rust/src/libstd/lib.rs`, but that actually got moved around and reorganized over the years. As of Dec 2024, the `rust-src` component is more like (specific for our purposes): ``` $SYSROOT/lib/rustlib/src/rust/ library/std/src/lib.rs src/ ``` However, this run-make test is ancient and it exercises incr-comp system logic. I'm not sure if this test would actually catch the original regression. This PR was co-authored with `@Oneirical.` r? incremental try-job: i686-msvc try-job: x86_64-mingw-1 try-job: x86_64-msvc try-job: aarch64-apple

…-component, r=wesleywiser Migrate `incr-add-rust-src-component` to rmake This PR partially supersedes rust-lang#128562, and ports the Makefile-based `tests/run-make/incr-add-rust-src-component` to use rmake.rs infra. Part of rust-lang#121876. This run-make test is a regression test for rust-lang#70924. It (tries to) checks that if we add the `rust-src` component in between two incremental compiles, that the compiler doesn't ICE on the second invocation. - Original issue:rust-lang#70924 - Fix PR: rust-lang#72767 - PR adding this regression test: rust-lang#72952 However, the Makefile version of this used `$SYSROOT/lib/rustlib/src/rust/src/libstd/lib.rs`, but that actually got moved around and reorganized over the years. As of Dec 2024, the `rust-src` component is more like (specific for our purposes): ``` $SYSROOT/lib/rustlib/src/rust/ library/std/src/lib.rs src/ ``` However, this run-make test is ancient and it exercises incr-comp system logic. I'm not sure if this test would actually catch the original regression. This PR was co-authored with `@Oneirical.` r? incremental try-job: i686-msvc try-job: x86_64-mingw-1 try-job: x86_64-msvc try-job: aarch64-apple

…llaumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#134656 (Migrate `incr-add-rust-src-component` to rmake) - rust-lang#134664 (Account for removal of multiline span in suggestion) - rust-lang#134772 (Improve/cleanup rustdoc code) - rust-lang#134781 (Add more `begin_panic` normalizations to panic backtrace tests) r? `@ghost` `@rustbot` modify labels: rollup

This comment was marked as resolved.

Sign in to view

rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2024

jieyouxu force-pushed the migrate-incr-add-rust-src-component branch from 6e2f793 to b9f4dd2 Compare December 22, 2024 15:44

rustbot added the A-compiletest Area: The compiletest test runner label Dec 22, 2024

This comment has been minimized.

Sign in to view

jieyouxu force-pushed the migrate-incr-add-rust-src-component branch from b9f4dd2 to c6931e4 Compare December 22, 2024 15:56

jieyouxu mentioned this pull request Dec 22, 2024

test-infra: improve compiletest and run-make-support symlink handling #134659

Merged

jieyouxu force-pushed the migrate-incr-add-rust-src-component branch from c6931e4 to b334ba5 Compare December 22, 2024 17:42

This comment has been minimized.

Sign in to view

jieyouxu force-pushed the migrate-incr-add-rust-src-component branch from b334ba5 to 41b2c3d Compare December 22, 2024 18:49

This comment was marked as off-topic.

Sign in to view

jieyouxu force-pushed the migrate-incr-add-rust-src-component branch from 41b2c3d to 7ee5204 Compare December 23, 2024 14:01

rustbot assigned wesleywiser Dec 23, 2024

jieyouxu removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Dec 23, 2024

jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Dec 23, 2024

jieyouxu marked this pull request as ready for review December 23, 2024 14:07

jieyouxu commented Dec 23, 2024

View reviewed changes

jieyouxu removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Dec 23, 2024

This was referenced Dec 23, 2024

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

Open

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

Closed

wesleywiser approved these changes Dec 26, 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 Dec 26, 2024

GuillaumeGomez mentioned this pull request Dec 26, 2024

Rollup of 4 pull requests #134795

Merged

bors merged commit c016cd8 into rust-lang:master Dec 26, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 26, 2024

jieyouxu deleted the migrate-incr-add-rust-src-component branch December 27, 2024 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `incr-add-rust-src-component` to rmake #134656

Migrate `incr-add-rust-src-component` to rmake #134656

jieyouxu commented Dec 22, 2024 •

edited

Loading

This comment was marked as resolved.

This comment has been minimized.

jieyouxu commented Dec 22, 2024

bors commented Dec 22, 2024

This comment has been minimized.

This comment was marked as off-topic.

jieyouxu commented Dec 22, 2024

bors commented Dec 22, 2024

bors commented Dec 22, 2024

jieyouxu Dec 23, 2024 •

edited

Loading

wesleywiser Dec 26, 2024

jieyouxu Dec 23, 2024

wesleywiser Dec 26, 2024

wesleywiser commented Dec 26, 2024

bors commented Dec 26, 2024

Migrate incr-add-rust-src-component to rmake #134656

Migrate incr-add-rust-src-component to rmake #134656

Conversation

jieyouxu commented Dec 22, 2024 • edited Loading

This comment was marked as resolved.

This comment has been minimized.

jieyouxu commented Dec 22, 2024

bors commented Dec 22, 2024

This comment has been minimized.

This comment was marked as off-topic.

jieyouxu commented Dec 22, 2024

bors commented Dec 22, 2024

bors commented Dec 22, 2024

jieyouxu Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

wesleywiser Dec 26, 2024

Choose a reason for hiding this comment

jieyouxu Dec 23, 2024

Choose a reason for hiding this comment

wesleywiser Dec 26, 2024

Choose a reason for hiding this comment

wesleywiser commented Dec 26, 2024

bors commented Dec 26, 2024

Migrate `incr-add-rust-src-component` to rmake #134656

Migrate `incr-add-rust-src-component` to rmake #134656

jieyouxu commented Dec 22, 2024 •

edited

Loading

jieyouxu Dec 23, 2024 •

edited

Loading