Cleanup rmake.rs setup in compiletest #127958

jieyouxu · 2024-07-19T10:38:54Z

While debugging rmake.rs tests I realized that the rmake.rs setup itself in compiletest is very messy and confused. Now that I know some of the bootstrap steps and the rmake.rs tests themselves better, I realized there are cleanups that are possible:

Rework how source_root and build_root are calculated. They should now be less fragile then before.
Shuffle around path calculations to make them more logically grouped and closer to eventual use site(s).
Cleanup executable extension calculation with std::env::consts::EXE_EXTENSION.
Cleanup various dylib search path handling: renamed variables to better reflect their purpose, minimized mutability scope of said variables.
Prune useless env vars passed to both rustc and recipe binary commands.
Vastly improve the documentation for the setup of rmake.rs tests, including assumed bootstrap-provided build layouts, rmake.rs test layout, dylib search paths, intended purpose of passed env vars and the COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0 stage0 sysroot special handling.

This PR is best reviewed commit-by-commit.

Fixes #127920.

r? bootstrap (or Kobzol, or Mark, or T-compiler)

try-job: aarch64-apple
try-job: armhf-gnu
try-job: dist-x86_64-linux
try-job: test-various
try-job: x86_64-mingw
try-job: x86_64-msvc
try-job: x86_64-gnu-llvm-18

…r=<try> Cleanup rmake.rs setup in compiletest While debugging rmake.rs tests I realized that the rmake.rs setup itself in compiletest is very messy and confused. Now that I know some of the bootstrap steps and the rmake.rs tests themselves better, I realized there are cleanups that are possible: - Rework how `source_root` and `build_root` are calculated. They should now be less fragile then before. - Shuffle around path calculations to make them more logically grouped and closer to eventual use site(s). - Cleanup executable extension calculation with `std::env::consts::EXE_EXTENSION`. - Cleanup various dylib search path handling: renamed variables to better reflect their purpose, minimized mutability scope of said variables. - Prune useless env vars passed to both `rustc` and recipe binary commands. - Vastly improve the documentation for the setup of rmake.rs tests, including assumed bootstrap-provided build layouts, rmake.rs test layout, dylib search paths, intended purpose of passed env vars and the `COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0` stage0 sysroot special handling. r? bootstrap (or Kobzol, or Mark, or T-compiler) try-job: aarch64-apple try-job: armhf-gnu try-job: dist-x86_64-linux try-job: test-various try-job: x86_64-mingw try-job: x86_64-msvc try-job: x86_64-gnu-llvm-18

jieyouxu · 2024-07-19T10:53:46Z

@rustbot author

src/tools/compiletest/src/runtest.rs

…r=<try> Cleanup rmake.rs setup in compiletest While debugging rmake.rs tests I realized that the rmake.rs setup itself in compiletest is very messy and confused. Now that I know some of the bootstrap steps and the rmake.rs tests themselves better, I realized there are cleanups that are possible: - Rework how `source_root` and `build_root` are calculated. They should now be less fragile then before. - Shuffle around path calculations to make them more logically grouped and closer to eventual use site(s). - Cleanup executable extension calculation with `std::env::consts::EXE_EXTENSION`. - Cleanup various dylib search path handling: renamed variables to better reflect their purpose, minimized mutability scope of said variables. - Prune useless env vars passed to both `rustc` and recipe binary commands. - Vastly improve the documentation for the setup of rmake.rs tests, including assumed bootstrap-provided build layouts, rmake.rs test layout, dylib search paths, intended purpose of passed env vars and the `COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0` stage0 sysroot special handling. This PR is best reviewed commit-by-commit. r? bootstrap (or Kobzol, or Mark, or T-compiler) try-job: aarch64-apple try-job: armhf-gnu try-job: dist-x86_64-linux try-job: test-various try-job: x86_64-mingw try-job: x86_64-msvc try-job: x86_64-gnu-llvm-18

Kobzol

Uff, this code was really convoluted. It still is, but it's better, and documented now, so thanks for doing this! Looking forward to deleting the "v1" version of this code 😆

Left a few nits, you can r=me after taking a look at them.

src/tools/compiletest/src/runtest.rs

…rs passed to recipes

This is important for other tests that have various things like modes, revisions and the like. These features are not supported in run-make tests, so we don't need the double layering.

jieyouxu · 2024-07-20T09:51:56Z

@bors try (trying again just to be safe)

jieyouxu · 2024-07-20T09:52:03Z

@rustbot author

…r=<try> Cleanup rmake.rs setup in compiletest While debugging rmake.rs tests I realized that the rmake.rs setup itself in compiletest is very messy and confused. Now that I know some of the bootstrap steps and the rmake.rs tests themselves better, I realized there are cleanups that are possible: - Rework how `source_root` and `build_root` are calculated. They should now be less fragile then before. - Shuffle around path calculations to make them more logically grouped and closer to eventual use site(s). - Cleanup executable extension calculation with `std::env::consts::EXE_EXTENSION`. - Cleanup various dylib search path handling: renamed variables to better reflect their purpose, minimized mutability scope of said variables. - Prune useless env vars passed to both `rustc` and recipe binary commands. - Vastly improve the documentation for the setup of rmake.rs tests, including assumed bootstrap-provided build layouts, rmake.rs test layout, dylib search paths, intended purpose of passed env vars and the `COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0` stage0 sysroot special handling. This PR is best reviewed commit-by-commit. Fixes rust-lang#127920. r? bootstrap (or Kobzol, or Mark, or T-compiler) try-job: aarch64-apple try-job: armhf-gnu try-job: dist-x86_64-linux try-job: test-various try-job: x86_64-mingw try-job: x86_64-msvc try-job: x86_64-gnu-llvm-18

bors · 2024-07-20T09:53:08Z

⌛ Trying commit a8463be with merge ee6c7a1...

jieyouxu · 2024-07-20T09:56:44Z

Uff, this code was really convoluted. It still is, but it's better, and documented now, so thanks for doing this!

It's more like "Uff, ~~this code was~~ compiletest is really convoluted". I want to do some simplifications/cleanups/disentanglement some time soon. For example, runtest.rs is critical to our testing infra but it's this extremely convoluted 4000 lines file that I'm fairly sure nobody fully understands.

bors · 2024-07-20T12:00:31Z

☀️ Try build successful - checks-actions
Build commit: ee6c7a1 (ee6c7a11c6bd92d55384c85473c3f80e052316a3)

jieyouxu · 2024-07-20T12:46:10Z

@bors r=@Kobzol

bors · 2024-07-20T12:46:12Z

📌 Commit a8463be has been approved by Kobzol

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#127720 ([`macro_metavar_expr_concat`] Allow `concat` in repetitions) - rust-lang#127734 (Windows: move BSD socket shims to netc) - rust-lang#127752 (Ignore allocation bytes in one more mir-opt test) - rust-lang#127839 (Fix git safe-directory path for docker images) - rust-lang#127867 (Add `wasm32-wasip2` to `build-manifest` tool) - rust-lang#127958 (Cleanup rmake.rs setup in compiletest) - rust-lang#127975 (Fix trait bounds display) - rust-lang#128005 (Remove _tls_used hack) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#127958 - jieyouxu:compiletest-rmake-cleanup, r=Kobzol Cleanup rmake.rs setup in compiletest While debugging rmake.rs tests I realized that the rmake.rs setup itself in compiletest is very messy and confused. Now that I know some of the bootstrap steps and the rmake.rs tests themselves better, I realized there are cleanups that are possible: - Rework how `source_root` and `build_root` are calculated. They should now be less fragile then before. - Shuffle around path calculations to make them more logically grouped and closer to eventual use site(s). - Cleanup executable extension calculation with `std::env::consts::EXE_EXTENSION`. - Cleanup various dylib search path handling: renamed variables to better reflect their purpose, minimized mutability scope of said variables. - Prune useless env vars passed to both `rustc` and recipe binary commands. - Vastly improve the documentation for the setup of rmake.rs tests, including assumed bootstrap-provided build layouts, rmake.rs test layout, dylib search paths, intended purpose of passed env vars and the `COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0` stage0 sysroot special handling. This PR is best reviewed commit-by-commit. Fixes rust-lang#127920. r? bootstrap (or Kobzol, or Mark, or T-compiler) try-job: aarch64-apple try-job: armhf-gnu try-job: dist-x86_64-linux try-job: test-various try-job: x86_64-mingw try-job: x86_64-msvc try-job: x86_64-gnu-llvm-18

rustbot assigned Kobzol Jul 19, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2024

This comment was marked as duplicate.

Sign in to view

This comment was marked as outdated.

Sign in to view

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 Jul 19, 2024

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

jieyouxu commented Jul 19, 2024

View reviewed changes

src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved

rust-cloud-vms bot force-pushed the compiletest-rmake-cleanup branch 2 times, most recently from 8b8e154 to c3bcec2 Compare July 19, 2024 11:40

This comment was marked as outdated.

Sign in to view

rust-cloud-vms bot force-pushed the compiletest-rmake-cleanup branch from c3bcec2 to 24e6599 Compare July 19, 2024 11:57

This comment was marked as outdated.

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 Jul 19, 2024

Kobzol reviewed Jul 19, 2024

View reviewed changes

jieyouxu added 4 commits July 20, 2024 09:20

compiletest/rmake: prune unused RUST_BUILD_STAGE and explain env va…

3e77f7c

…rs passed to recipes

compiletest/rmake: improve comments

c863525

compiletest/rmake: avoid double test directory for rmake.rs tests

2c867d0

This is important for other tests that have various things like modes, revisions and the like. These features are not supported in run-make tests, so we don't need the double layering.

compiletest/rmake: simplify path calculations

a8463be

rust-cloud-vms bot force-pushed the compiletest-rmake-cleanup branch from ef2d0dd to a8463be Compare July 20, 2024 09:30

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 Jul 20, 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 20, 2024

matthiaskrgr mentioned this pull request Jul 20, 2024

Rollup of 8 pull requests #128011

Merged

bors merged commit ae28d5c into rust-lang:master Jul 21, 2024
7 checks passed

rustbot added this to the 1.81.0 milestone Jul 21, 2024

jieyouxu deleted the compiletest-rmake-cleanup branch July 21, 2024 06:03

jieyouxu mentioned this pull request Jul 22, 2024

Migrate run make issue 15460 #125886

Merged

cuviper modified the milestones: 1.81.0, 1.82.0 Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup rmake.rs setup in compiletest #127958

Cleanup rmake.rs setup in compiletest #127958

jieyouxu commented Jul 19, 2024 •

edited

Loading

This comment was marked as duplicate.

This comment was marked as outdated.

jieyouxu commented Jul 19, 2024

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Kobzol left a comment

jieyouxu commented Jul 20, 2024

jieyouxu commented Jul 20, 2024

bors commented Jul 20, 2024

jieyouxu commented Jul 20, 2024 •

edited

Loading

bors commented Jul 20, 2024

jieyouxu commented Jul 20, 2024

bors commented Jul 20, 2024

Cleanup rmake.rs setup in compiletest #127958

Cleanup rmake.rs setup in compiletest #127958

Conversation

jieyouxu commented Jul 19, 2024 • edited Loading

This comment was marked as duplicate.

This comment was marked as outdated.

jieyouxu commented Jul 19, 2024

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Kobzol left a comment

Choose a reason for hiding this comment

jieyouxu commented Jul 20, 2024

jieyouxu commented Jul 20, 2024

bors commented Jul 20, 2024

jieyouxu commented Jul 20, 2024 • edited Loading

bors commented Jul 20, 2024

jieyouxu commented Jul 20, 2024

bors commented Jul 20, 2024

jieyouxu commented Jul 19, 2024 •

edited

Loading

jieyouxu commented Jul 20, 2024 •

edited

Loading