compiletest: split rmake.rs executable from scratch directory #125827

jieyouxu · 2024-05-31T18:00:24Z

When implementing support for rmake.rs, I copied over the $TMPDIR directory logic from the legacy Makefile setup. In doing so, I also compiled recipe rmake.rs into executables which unfortunately are placed into $TMPDIR as well.

This causes a problem on Windows (as observed in PRs like #125752 (comment)) where:

The rmake.exe executable is placed in $TMPDIR.
We run the rmake.exe as a process.
The process uses rmake.exe inside $TMPDIR.
Windows prevents the .exe file from being deleted when the process is still alive.
The recipe test code tries to remove_dir_all($TMPDIR), which fails with access denied because rmake.exe is still being used.

We fix this by separating the recipe executable and the output artifacts directory:

base_dir/
    rmake.exe
    rmake_out/

We construct a base directory, unique to each run-make test, under which we place rmake.exe alongside a rmake_out/ directory. This rmake_out/ directory is what is passed to rmake.rs tests as $TMPDIR, so now remove_dir_all($TMPDIR) has a chance to succeed because it no longer contains rmake.exe.

This wasn't a problem for Makefile tests because there's no exe file under $TMPDIR whose process is still running when rm -rf $TMPDIR is called.

try-job: x86_64-msvc

rustbot · 2024-05-31T18:00:32Z

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
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-05-31T18:17:49Z

@bors try

…try> compiletest: split rmake.rs executable from scratch directory When implementing support for rmake.rs, I copied over the `$TMPDIR` directory logic from the legacy Makefile setup. In doing so, I also compiled recipe `rmake.rs` into executables which unfortunately are placed into `$TMPDIR` as well. This causes a problem on Windows (as observed in PRs like rust-lang#125752 (comment)) where: - The `rmake.exe` executable is placed in `$TMPDIR`. - We run the `rmake.exe` as a process. - The process uses `rmake.exe` inside `$TMPDIR`. - Windows prevents the .exe file from being deleted when the process is still alive. - The recipe test code tries to `remove_dir_all($TMPDIR)`, which fails with access denied because `rmake.exe` is still being used. We fix this by separating the recipe executable and the scratch directory: ``` base_dir/ rmake.exe scratch/ ``` We construct a base directory, unique to each run-make test, under which we place rmake.exe alongside a `scratch/` directory. This `scratch/` directory is what is passed to rmake.rs tests as `$TMPDIR`, so now `remove_dir_all($TMPDIR)` has a chance to succeed because it no longer contains `rmake.exe`. This wasn't a problem for Makefile tests because there's no exe file under `$TMPDIR` whose process is still running when `rm -rf $TMPDIR` is called. try-job: x86_64-msvc

bors · 2024-05-31T18:20:12Z

⌛ Trying commit 2163e31 with merge 092eb27...

bors · 2024-05-31T20:05:34Z

☀️ Try build successful - checks-actions
Build commit: 092eb27 (092eb273642f28ffb4b329694c1f577f3c64bc25)

src/tools/compiletest/src/runtest.rs

onur-ozkan · 2024-06-01T12:54:13Z

r? onur-ozkan

When implementing support for rmake.rs, I copied over the `$TMPDIR` directory logic from the legacy Makefile setup. In doing so, I also compiled recipe `rmake.rs` into executables which unfortunately are placed into `$TMPDIR` as well. This causes a problem on Windows where: - The `rmake.exe` executable is placed in `$TMPDIR`. - We run the `rmake.exe` as a process. - The process uses `rmake.exe` inside `$TMPDIR`. - Windows prevents the .exe file from being deleted when the process is still alive. - The recipe test code tries to `remove_dir_all($TMPDIR)`, which fails with access denied because `rmake.exe` is still being used. We fix this by separating the recipe executable and the sratch directory: ``` base_dir/ rmake.exe scratch/ ``` We construct a base directory, unique to each run-make test, under which we place rmake.exe alongside a `scratch/` directory. This `scratch/` directory is what is passed to rmake.rs tests as `$TMPDIR`, so now `remove_dir_all($TMPDIR)` has a chance to succeed because it no longer contains `rmake.exe`. Oops. This was a fun one to try figure out.

jieyouxu · 2024-06-02T06:35:07Z

Changes since last reviewed:

Renamed rmake output artifact directory to rmake_out.
Removed redundant create_dir_all.

@rustbot ready

onur-ozkan

Thanks!

onur-ozkan · 2024-06-02T07:33:59Z

@bors r+

bors · 2024-06-02T07:34:02Z

📌 Commit 4562245 has been approved by onur-ozkan

It is now in the queue for this repository.

bors · 2024-06-02T08:05:53Z

⌛ Testing commit 4562245 with merge 13e2d72...

bors · 2024-06-02T10:14:24Z

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing 13e2d72 to master...

rust-timer · 2024-06-02T11:30:36Z

Finished benchmarking commit (13e2d72): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary -3.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 667.268s -> 666.563s (-0.11%)
Artifact size: 318.90 MiB -> 318.88 MiB (-0.01%)

fbstj · 2024-06-02T13:55:38Z

src/tools/compiletest/src/runtest.rs

+        // ```
+        // base_dir/
+        //     rmake.exe
+        //     scratch/


looks like this comment is out of date now with the rmake_out change?

I updated the PR description but I forgor to update the comment, thanks, I'll send a follow-up PR.

Opened #125896

…, r=compiler-errors compiletest: fix outdated rmake.rs comment Noticed in rust-lang#125827 (comment). I fixed the PR description but forgot to update the comment.

Rollup merge of rust-lang#125896 - jieyouxu:compiletest-rmake-comment, r=compiler-errors compiletest: fix outdated rmake.rs comment Noticed in rust-lang#125827 (comment). I fixed the PR description but forgot to update the comment.

rustbot assigned albertlarsan68 May 31, 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 May 31, 2024

This was referenced May 31, 2024

run-make: arm command wrappers with drop bombs #125752

Merged

Rewrite emit, mixing-formats and bare-outfile run-make tests in rmake.rs format #125383

Merged

This comment was marked as resolved.

Sign in to view

onur-ozkan reviewed Jun 1, 2024

View reviewed changes

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

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

rustbot assigned onur-ozkan and unassigned albertlarsan68 Jun 1, 2024

jieyouxu mentioned this pull request Jun 2, 2024

Migrate run-make/c-link-to-rust-dylib to rmake.rs #125808

Merged

rust-cloud-vms bot force-pushed the rmake-separate-tmp-dir branch from 2163e31 to 4562245 Compare June 2, 2024 06:32

onur-ozkan approved these changes Jun 2, 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 Jun 2, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 2, 2024

bors merged commit 13e2d72 into rust-lang:master Jun 2, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone Jun 2, 2024

jieyouxu deleted the rmake-separate-tmp-dir branch June 2, 2024 10:18

fbstj reviewed Jun 2, 2024

View reviewed changes

jieyouxu mentioned this pull request Jun 2, 2024

compiletest: fix outdated rmake.rs comment #125896

Merged

This was referenced Jun 3, 2024

run-make-support: add wrapper for fs operations #125736

Merged

run-make-support: rename tmp_dir() and clarify its semantics #125726

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiletest: split rmake.rs executable from scratch directory #125827

compiletest: split rmake.rs executable from scratch directory #125827

jieyouxu commented May 31, 2024 •

edited

Loading

rustbot commented May 31, 2024

jieyouxu commented May 31, 2024

This comment was marked as resolved.

bors commented May 31, 2024

This comment was marked as resolved.

bors commented May 31, 2024

onur-ozkan commented Jun 1, 2024

jieyouxu commented Jun 2, 2024

onur-ozkan left a comment

onur-ozkan commented Jun 2, 2024

bors commented Jun 2, 2024

bors commented Jun 2, 2024

bors commented Jun 2, 2024

rust-timer commented Jun 2, 2024

fbstj Jun 2, 2024

jieyouxu Jun 2, 2024 •

edited

Loading

jieyouxu Jun 2, 2024

compiletest: split rmake.rs executable from scratch directory #125827

compiletest: split rmake.rs executable from scratch directory #125827

Conversation

jieyouxu commented May 31, 2024 • edited Loading

rustbot commented May 31, 2024

jieyouxu commented May 31, 2024

This comment was marked as resolved.

bors commented May 31, 2024

This comment was marked as resolved.

bors commented May 31, 2024

onur-ozkan commented Jun 1, 2024

jieyouxu commented Jun 2, 2024

onur-ozkan left a comment

Choose a reason for hiding this comment

onur-ozkan commented Jun 2, 2024

bors commented Jun 2, 2024

bors commented Jun 2, 2024

bors commented Jun 2, 2024

rust-timer commented Jun 2, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

fbstj Jun 2, 2024

Choose a reason for hiding this comment

jieyouxu Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu Jun 2, 2024

Choose a reason for hiding this comment

jieyouxu commented May 31, 2024 •

edited

Loading

jieyouxu Jun 2, 2024 •

edited

Loading