Use `rmake` for `windows-` run-make tests #125613

ChrisDenton · 2024-05-27T11:51:56Z

Convert some Makefile tests to recipes.

I renamed "issue-85441" to "windows-ws2_32" as I think it's slightly more descriptive. EDIT: llvm-readobj seems to work for reading DLL imports so I've used that instead of objdump.

cc #121876

rustbot · 2024-05-27T11:52:03Z

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-05-27T11:52:04Z

Some changes occurred in run-make tests.

cc @jieyouxu

rustbot · 2024-05-27T12:50:52Z

The run-make-support library was changed

cc @jieyouxu

jieyouxu

Thanks for the test comments, they are very informative :3
I left two suggestions, r=me with or without addressing them.

tests/run-make/windows-safeseh/rmake.rs

tests/run-make/windows-subsystem/rmake.rs

jieyouxu · 2024-05-27T14:27:20Z

tests/run-make/windows-ws2_32/rmake.rs

+    rustc().input("empty.rs").run();
+    let empty = tmp_dir().join("empty.exe");
+    let output = llvm_readobj().input(empty).coff_imports().command_output();
+    assert!(output.status.success());


Reminder for myself: cc #125617 we should add run() that asserts success for llvm-readobj as well, so it's less of a footgun to forgor checking cmd status

Oh, wait there is a run I think. I missed that.

Yeah, I think we probably need to un-provide the command_output helper because it's probably almost never the right helper to use

Sounds helpful. I should probably also read the docs for the support lib.

I think it wasn't mentioned explicitly in command_output, I'll clean it up soon.

Oh, that gives me a thought: it might be nice to make ./x doc run-make-support --open work.

Yes, that would indeed be very useful

ChrisDenton · 2024-05-27T15:53:32Z

Ok, I'll stop fiddling now 😛There are maybe some more improvements to be made but I'll leave that to follow-ups. Like, maybe it would make sense to use the object crate for enumerating imports rather than llvm_readobj.

@bors r=jieyouxu

bors · 2024-05-27T15:53:35Z

📌 Commit ad5dce5 has been approved by jieyouxu

It is now in the queue for this repository.

…youxu Use `rmake` for `windows-` run-make tests Convert some Makefile tests to recipes. I renamed "issue-85441" to "windows-ws2_32" as I think it's slightly more descriptive. EDIT: `llvm-readobj` seems to work for reading DLL imports so I've used that instead of `objdump`. cc rust-lang#121876

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#125339 (The number of tests does not depend on the architecture's pointer width) - rust-lang#125539 (crashes: increment the number of tracked ones) - rust-lang#125542 (Migrate rustdoc verify output files) - rust-lang#125613 (Use `rmake` for `windows-` run-make tests) - rust-lang#125616 (MIR validation: ensure that downcast projection is followed by field projection) Failed merges: - rust-lang#125573 (Migrate `run-make/allow-warnings-cmdline-stability` to `rmake.rs`) r? `@ghost` `@rustbot` modify labels: rollup

…youxu Use `rmake` for `windows-` run-make tests Convert some Makefile tests to recipes. I renamed "issue-85441" to "windows-ws2_32" as I think it's slightly more descriptive. EDIT: `llvm-readobj` seems to work for reading DLL imports so I've used that instead of `objdump`. cc rust-lang#121876

Rollup of 4 pull requests Successful merges: - rust-lang#125411 (check host's libstdc++ version when using ci llvm) - rust-lang#125598 (Make `ProofTreeBuilder` actually generic over `Interner`) - rust-lang#125609 (Always use the general case char count with `optimize_for_size`) - rust-lang#125613 (Use `rmake` for `windows-` run-make tests) r? `@ghost` `@rustbot` modify labels: rollup

workingjubilee · 2024-05-28T05:48:44Z

i686-windows-gnu funtimes ahead?

@bors rollup=iffy

Use `rmake` for `windows-` run-make tests Convert some Makefile tests to recipes. I renamed "issue-85441" to "windows-ws2_32" as I think it's slightly more descriptive. EDIT: `llvm-readobj` seems to work for reading DLL imports so I've used that instead of `objdump`. cc rust-lang#121876

bors · 2024-05-28T23:10:55Z

⌛ Testing commit ad5dce5 with merge 4b42964...

bors · 2024-05-29T00:04:12Z

💔 Test failed - checks-actions

bors · 2024-05-29T02:05:32Z

⌛ Testing commit ad5dce5 with merge fb51598...

bors · 2024-05-29T03:54:15Z

💔 Test failed - checks-actions

ChrisDenton · 2024-05-29T04:27:28Z

Ok so the no external deps test is failing on mingw due to a missing dll (but it succeeds for msvc). The weird thing is it was obviously succeeding in the Makefile version. Which perhaps suggest make or mingw bash is doing something behind our backs so we weren't testing what we thought we were testing. Either that or I missed something else.

I'll investigate tomorrow.

jieyouxu · 2024-05-29T06:56:42Z

Ok so the no external deps test is failing on mingw due to a missing dll (but it succeeds for msvc). The weird thing is it was obviously succeeding in the Makefile version. Which perhaps suggest make or mingw bash is doing something behind our backs so we weren't testing what we thought we were testing. Either that or I missed something else.

I'll investigate tomorrow.

run() sets extra env vars for the executable, e.g.

rust/src/tools/run-make-support/src/run.rs

Lines 27 to 34 in 7516912

    
           if is_windows() { 
        
               let mut paths = vec![]; 
        
               for p in env::split_paths(&std::env::var("PATH").unwrap_or(String::new())) { 
        
                   paths.push(p.to_path_buf()); 
        
               } 
        
               paths.push(Path::new(&std::env::var("TARGET_RPATH_DIR").unwrap()).to_path_buf()); 
        
               cmd.env("PATH", env::join_paths(paths.iter()).unwrap()); 
        
           }

maybe that is why in the original Makefile they used

$(TMPDIR)/hello.exe

instead of

$(call RUN,hello)

Not entirely sure.

bors · 2024-05-29T07:08:29Z

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

ChrisDenton · 2024-05-29T12:41:32Z

I had a look at the original issue that provoked adding this test. It seems we only need to be testing the final binary, not rustc itself. That does make things easier.

I still need to rebase and squish. And while I'm at it, I might rewrite the windows-ws2_32 test to use object. I think it'll be easy enough.

ChrisDenton · 2024-05-29T13:41:44Z

I've rebased and made the commits easier to navigate. There are two changed tests.

5ec0c00 fixes the failure as discussed above by only setting PATH when running the final binary

For f08e00f I switched to using object rather than depending on llvm-readobj and searching the command output. I also added an extra test case to make sure the actual test works. The problem with depending on the absence of something (in this case WS2_32.dll) is that it could be absent for a number of reasons (e.g. the test is buggy or the standard library switches to another DLL) so testing the test still works is useful, I think. Therefore the new test confirms that TcpListener::bind causes WS2_32.dll to be linked.

jieyouxu · 2024-05-29T15:28:31Z

tests/run-make/windows-binary-no-external-deps/rmake.rs

+    if !status.success() {
+        panic!("Command failed!\noutput status: `{status}`");
+    }


Remark (to myself): we should probably expose the handle_failed_output helper to make this less repetitive.

jieyouxu · 2024-05-29T15:38:35Z

tests/run-make/windows-ws2_32/rmake.rs

+    let binary_data = fs::read(path).unwrap();
+    let file = object::File::parse(&*binary_data).unwrap();
+    for import in file.imports().unwrap() {
+        if import.library().eq_ignore_ascii_case(b"WS2_32.dll") {


I like how this test reads :3

jieyouxu · 2024-05-29T15:39:34Z

Thanks!
@bors r+

bors · 2024-05-29T15:39:37Z

📌 Commit f34bbde has been approved by jieyouxu

It is now in the queue for this repository.

bors · 2024-05-29T18:03:58Z

⌛ Testing commit f34bbde with merge e9b7aa0...

bors · 2024-05-29T20:10:01Z

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing e9b7aa0 to master...

rust-timer · 2024-05-29T21:27:39Z

Finished benchmarking commit (e9b7aa0): 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)

Results (primary -0.8%, secondary -3.6%)

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)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-2.6%, -1.5%]	3
Improvements ✅ (secondary)	-3.6%	[-4.3%, -3.1%]	3
All ❌✅ (primary)	-0.8%	[-2.6%, 3.2%]	4

Cycles

Results (primary -2.5%, secondary -3.0%)

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)	-2.5%	[-3.1%, -1.9%]	6
Improvements ✅ (secondary)	-3.0%	[-3.9%, -2.4%]	14
All ❌✅ (primary)	-2.5%	[-3.1%, -1.9%]	6

Binary size

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

Bootstrap: 667.715s -> 667.139s (-0.09%)
Artifact size: 318.83 MiB -> 318.84 MiB (0.00%)

rustbot assigned jieyouxu May 27, 2024

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

This comment has been minimized.

Sign in to view

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 27, 2024

ChrisDenton force-pushed the windows-recipie branch from e71d23b to dabfbe9 Compare May 27, 2024 12:50

jieyouxu approved these changes May 27, 2024

View reviewed changes

tests/run-make/windows-safeseh/rmake.rs Outdated Show resolved Hide resolved

tests/run-make/windows-subsystem/rmake.rs Outdated Show resolved Hide resolved

ChrisDenton force-pushed the windows-recipie branch from da5deed to f705a22 Compare May 27, 2024 13:56

jieyouxu reviewed May 27, 2024

View reviewed changes

ChrisDenton force-pushed the windows-recipie branch from dc0843c to ad5dce5 Compare May 27, 2024 14:36

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 May 27, 2024

matthiaskrgr mentioned this pull request May 27, 2024

Rollup of 5 pull requests #125624

Closed

jieyouxu mentioned this pull request May 28, 2024

Rollup of 4 pull requests #125641

Closed

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 29, 2024

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 29, 2024

ChrisDenton added 6 commits May 29, 2024 13:04

Move run-make windows_subsystem tests to ui tests

1e6544a

Rename run-make/issue-85441 and convert to rmake

f08e00f

Convert run-make/windows-spawn to rmake

e463f6f

Convert run-make/windows-safeseh to rmake

e03f9cb

Convert windows-binary-no-external-deps to rmake

5ec0c00

Remove Makefiles from allowed_run_make_makefiles

f34bbde

ChrisDenton force-pushed the windows-recipie branch from 0034ce5 to f34bbde Compare May 29, 2024 13:35

jieyouxu approved these changes May 29, 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 May 29, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label May 29, 2024

bors merged commit e9b7aa0 into rust-lang:master May 29, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone May 29, 2024

ChrisDenton deleted the windows-recipie branch May 29, 2024 20:11

jieyouxu mentioned this pull request Jun 9, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `rmake` for `windows-` run-make tests #125613

Use `rmake` for `windows-` run-make tests #125613

ChrisDenton commented May 27, 2024 •

edited

Loading

rustbot commented May 27, 2024

rustbot commented May 27, 2024

This comment has been minimized.

rustbot commented May 27, 2024

jieyouxu left a comment

jieyouxu May 27, 2024

ChrisDenton May 27, 2024

jieyouxu May 27, 2024

ChrisDenton May 27, 2024

jieyouxu May 27, 2024

ChrisDenton May 27, 2024

jieyouxu May 27, 2024

ChrisDenton commented May 27, 2024

bors commented May 27, 2024

workingjubilee commented May 28, 2024

bors commented May 28, 2024

This comment has been minimized.

bors commented May 29, 2024

bors commented May 29, 2024

This comment has been minimized.

bors commented May 29, 2024

ChrisDenton commented May 29, 2024

jieyouxu commented May 29, 2024

bors commented May 29, 2024

ChrisDenton commented May 29, 2024

ChrisDenton commented May 29, 2024 •

edited

Loading

jieyouxu May 29, 2024

jieyouxu May 29, 2024

jieyouxu commented May 29, 2024

bors commented May 29, 2024

bors commented May 29, 2024

bors commented May 29, 2024

rust-timer commented May 29, 2024

Use rmake for windows- run-make tests #125613

Use rmake for windows- run-make tests #125613

Conversation

ChrisDenton commented May 27, 2024 • edited Loading

rustbot commented May 27, 2024

rustbot commented May 27, 2024

This comment has been minimized.

rustbot commented May 27, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisDenton commented May 27, 2024

bors commented May 27, 2024

workingjubilee commented May 28, 2024

bors commented May 28, 2024

This comment has been minimized.

bors commented May 29, 2024

bors commented May 29, 2024

This comment has been minimized.

bors commented May 29, 2024

ChrisDenton commented May 29, 2024

jieyouxu commented May 29, 2024

bors commented May 29, 2024

ChrisDenton commented May 29, 2024

ChrisDenton commented May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented May 29, 2024

bors commented May 29, 2024

bors commented May 29, 2024

bors commented May 29, 2024

rust-timer commented May 29, 2024

Overall result: no relevant changes - no action needed

Use `rmake` for `windows-` run-make tests #125613

Use `rmake` for `windows-` run-make tests #125613

ChrisDenton commented May 27, 2024 •

edited

Loading

ChrisDenton commented May 29, 2024 •

edited

Loading