Migrate libtest-thread-limit run-make test to rmake

[Oneirical]

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

Please try, but only if normal CI is green:

// try-job: armhf-gnu // <- failed on this
try-job: aarch64-gnu

[rustbot]

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]

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

[Oneirical]

ulimit was not found? How is that possible, if the original test uses it and passes...

[jieyouxu]

ulimit was not found? How is that possible, if the original test uses it and passes...

Argh, you see the $(shell ulimit)? ulimit is a shell command implemented by the shell, not a separate program...

ulimit provides control over the resources available to the shell and to processes started by it, on systems that allow such control.

See bash(1) and search for ulimit and its -p flag.

In particular, the -p flag is to specify

the pipe size in 512-byte blocks (this may not be set)

[jieyouxu]

Also note that the -p flag on ulimit means different things depending on which shell is used, it's pipe size for bash but process limit for dash...

[bors]

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

[jieyouxu]

@rustbot author

[rustbot]

The run-make-support library was changed

cc @jieyouxu

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[Oneirical]

@rustbot review

This feels like progress, but it feels as if every possible permutation of 0 and 1 in all 3 arguments result in either a test failure or a coredump.

[jieyouxu]

I'm stuck on this libtest one as well, so I'm asking for help in #t-libs.

@rustbot blocked (waiting for elp)

[jieyouxu]

In https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Problem.20porting.20.60tests.2Frun-make.2Flibtest-thread-limit.60:

• The test.rs is always going to fail, because libtest upon thread exhaustion (failure to spawn new threads) will attempt (after that PR which introduced the test) to run the tests sequentially, in alphabetical test name order, meaning that spawn_thread_would_block which sets THREAD_ID will be executed after run_in_same_thread, causing the test to always fail.
• We don't need such execution order guarantee or that they even run on the same exact thread, we just want libtest to not crash here due to inability to spawn new threads. Thus, we can just have two empty tests:

#[test] fn foo() {}
#[test] fn bar() {}

[bors]

⌛ Testing commit 318dfb4 with merge 91e4b4a...

[rust-log-analyzer]

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] miri test:false 4.575
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Fri, Aug 23, 2024  4:43:48 PM
  network time: Fri, 23 Aug 2024 16:43:49 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[bors]

💔 Test failed - checks-actions

[jieyouxu]

@bors retry (msvc ci spurious failure)

[bors]

⌛ Testing commit 318dfb4 with merge eef00c8...

[bors]

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

[rust-timer]

Finished benchmarking commit (eef00c8): 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 1.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)	-	-	0
Regressions ❌ (secondary)	3.5%	[2.5%, 4.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 750.941s -> 751.017s (0.01%)
Artifact size: 338.96 MiB -> 338.95 MiB (-0.00%)

[bjorn3]

Using cmd.pre_exec(|| setrlimit(...)) would have worked too, right?

[jieyouxu]

Yes, it probably would've worked too. I was not aware that existed.

[bjorn3]

I think the old never properly worked at all. With -Cpanic=abort -Zpanic-abort-tests the test fail (which is to be expected as -Zpanic-abort-tests will always spawn a new process for each test). Before this PR it never failed in cg_clif's CI though. It should probably get a //@ needs-unwind annotation.

[Oneirical]

I think the old never properly worked at all.

That is what we found out, it constantly failed because of the test.rs alphabetical order problem, and that failure was silenced.

With -Cpanic=abort -Zpanic-abort-tests the test fail (which is to be expected as -Zpanic-abort-tests will always spawn a new process for each test). Before this PR it never failed in cg_clif's CI though. It should probably get a //@ needs-unwind annotation.

I'm not sure I understand, is it accurate if I rephrase what you said as:

"Adding -Cpanic=abort -Zpanic-abort-tests to the new rmake.rs test causes it to fail, which is normal because -Zpanic-abort-tests will always spawn a new process for each test, causing the process limit to be exceeded."

Before this PR it never failed in cg_clif's CI though.

Are you saying that the new rmake.rs test is failing in Cranelift's CI in its current form, or only if you add the unwind flags?

It should probably get a //@ needs-unwind annotation.

Do you think the test should also be expanded to check that adding -Cpanic=abort -Zpanic-abort-tests causes a failure?

[bjorn3]

I'm not sure I understand, is it accurate if I rephrase what you said as:

Yes

Are you saying that the new rmake.rs test is failing in Cranelift's CI in its current form, or only if you add the unwind flags?

In it's current form it fails. Adding //@ needs-unwind would cause it to be ignored in cg_clif's CI and anywhere else that runs tests with panic=abort.

[Oneirical]

I'm not sure I understand, is it accurate if I rephrase what you said as:

Yes

Are you saying that the new rmake.rs test is failing in Cranelift's CI in its current form, or only if you add the unwind flags?

In it's current form it fails. Adding //@ needs-unwind would cause it to be ignored in cg_clif's CI and anywhere else that runs tests with panic=abort.

Alright, added in #129690.

[jieyouxu]

I think the old never properly worked at all.

Yes. Unfortunately the test never properly worked ever since it was added due to makefile syntax shenangians. The actual test itself always failed, but the Makefile never fails.