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

jieyouxu · 2024-05-30T02:49:27Z

This PR is one in a series of cleanups to run-make tests and the run-make-support library.

Summary

It's easy to forget to actually executed constructed command wrappers, e.g. rustc().input("foo.rs") but forget the run(), so to help catch these mistakes, we arm command wrappers with drop bombs on construction to force them to be executed by test code.

This PR also removes the Deref/DerefMut impl for our custom Command which derefs to std::process::Command because it can cause issues when trying to use a custom command:

htmldocck().arg().run()

fails to compile because the arg() is resolved to std::process::Command::arg, which returns &mut std::process::Command that doesn't have a run() command.

This PR also:

Removes env_var on the impl_common_helper macro that was wrongly named and is a footgun (no users).
Bumps the run-make-support library to version 0.1.0.
Adds a changelog to the support library.

Details

Especially for command wrappers like Rustc, it's very easy to build up
a command invocation but forget to actually execute it, e.g. by using
run(). This commit adds "drop bombs" to command wrappers, which are
armed on command wrapper construction, and only defused if the command
is executed (through run, run_fail).

If the test writer forgets to execute the command, the drop bomb will
"explode" and panic with an error message. This is so that tests don't
silently pass with constructed-but-not-executed command wrappers.

This PR is best reviewed commit-by-commit.

try-job: x86_64-msvc

rustbot · 2024-05-30T02:49:34Z

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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

Kobzol

Looks good, left a few comments.

r? @Kobzol

tests/run-make/rustdoc-error-lines/rmake.rs

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

src/tools/run-make-support/src/drop_bomb/mod.rs

src/tools/compiletest/src/runtest.rs

jieyouxu · 2024-05-30T14:16:03Z

@rustbot author

jieyouxu · 2024-05-31T13:01:09Z

@rustbot ready

tests/run-make/same-lib-two-locations-no-panic/rmake.rs

Kobzol · 2024-05-31T13:12:48Z

Great, this should help catch us subtle mistakes in runmake tests.

@bors r+

bors · 2024-05-31T13:12:51Z

📌 Commit 7867fb7 has been approved by Kobzol

It is now in the queue for this repository.

run-make: enforce `#[must_use]` and arm command wrappers with drop bombs This PR is one in a series of cleanups to run-make tests and the run-make-support library. ### Summary It's easy to forget to actually executed constructed command wrappers, e.g. `rustc().input("foo.rs")` but forget the `run()`, so to help catch these mistakes, we: - Add `#[must_use]` annotations to functions where suitable and compile rmake.rs recipes with `-Dunused_must_use`. - Arm command wrappers with drop bombs on construction to force them to be executed by test code. ### Details Especially for command wrappers like `Rustc`, it's very easy to build up a command invocation but forget to actually execute it, e.g. by using `run()`. This commit adds "drop bombs" to command wrappers, which are armed on command wrapper construction, and only defused if the command is executed (through `run`, `run_fail` or `run_fail_assert_exit_code`). If the test writer forgets to execute the command, the drop bomb will "explode" and panic with an error message. This is so that tests don't silently pass with constructed-but-not-executed command wrappers. We don't add `#[must_use]` for command wrapper helper methods because they return `&mut Self` and can be annoying e.g. if a helper method is conditionally called, such as ``` if condition { cmd.arg("-Cprefer-dynamic"); // <- unused_must_use fires } cmd.run(); // <- even though cmd is eventually executed ``` This PR is best reviewed commit-by-commit. Fixes rust-lang#125703. Because `command_output()` doesn't defuse the drop bomb, it also fixes rust-lang#125617.

Rollup of 6 pull requests Successful merges: - rust-lang#125652 (Revert propagation of drop-live information from Polonius) - rust-lang#125730 (Apply `x clippy --fix` and `x fmt` on Rustc) - rust-lang#125752 (run-make: enforce `#[must_use]` and arm command wrappers with drop bombs) - rust-lang#125756 (coverage: Optionally instrument the RHS of lazy logical operators) - rust-lang#125796 (Also InstSimplify `&raw*`) - rust-lang#125816 (Don't build the `rust-demangler` binary for coverage tests) r? `@ghost` `@rustbot` modify labels: rollup

run-make: enforce `#[must_use]` and arm command wrappers with drop bombs This PR is one in a series of cleanups to run-make tests and the run-make-support library. ### Summary It's easy to forget to actually executed constructed command wrappers, e.g. `rustc().input("foo.rs")` but forget the `run()`, so to help catch these mistakes, we: - Add `#[must_use]` annotations to functions where suitable and compile rmake.rs recipes with `-Dunused_must_use`. - Arm command wrappers with drop bombs on construction to force them to be executed by test code. ### Details Especially for command wrappers like `Rustc`, it's very easy to build up a command invocation but forget to actually execute it, e.g. by using `run()`. This commit adds "drop bombs" to command wrappers, which are armed on command wrapper construction, and only defused if the command is executed (through `run`, `run_fail` or `run_fail_assert_exit_code`). If the test writer forgets to execute the command, the drop bomb will "explode" and panic with an error message. This is so that tests don't silently pass with constructed-but-not-executed command wrappers. We don't add `#[must_use]` for command wrapper helper methods because they return `&mut Self` and can be annoying e.g. if a helper method is conditionally called, such as ``` if condition { cmd.arg("-Cprefer-dynamic"); // <- unused_must_use fires } cmd.run(); // <- even though cmd is eventually executed ``` This PR is best reviewed commit-by-commit. Fixes rust-lang#125703. Because `command_output()` doesn't defuse the drop bomb, it also fixes rust-lang#125617.

bors · 2024-05-31T14:36:38Z

⌛ Testing commit 7867fb7 with merge 7df2b08...

bors · 2024-05-31T16:11:51Z

💔 Test failed - checks-actions

bors · 2024-06-11T01:03:36Z

💔 Test failed - checks-actions

jieyouxu · 2024-06-11T01:29:30Z

@bors r-

This is incorrectly named (it's actually `env_clear`), and is itself a gigantic footgun: removing `TMPDIR` on Unix and `TMP`/`TEMP` on Windows basically wrecks anything that relies on `std::env::temp_dir` from functioning correctly. For example, this includes rustc's codegen.

- Update all command wrappers and command construction helpers with `#[track_caller]` where suitable to help the drop bomb panic message. - Remove `Deref`/`DerefMut` for `Command` because it was causing issues with resolving to `std::process::Command` in a method call chain.

jieyouxu · 2024-06-11T10:13:20Z

Fixed the broken intra-doc link.

@bors r=@Kobzol rollup=never p=1 (there are a couple of PRs blocked on this transitively)

bors · 2024-06-11T10:13:23Z

📌 Commit 5ec3eef has been approved by Kobzol

It is now in the queue for this repository.

bors · 2024-06-11T11:29:06Z

⌛ Testing commit 5ec3eef with merge 20ba13c...

bors · 2024-06-11T13:38:38Z

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing 20ba13c to master...

rust-timer · 2024-06-11T14:52:50Z

Finished benchmarking commit (20ba13c): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

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

Cycles

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

Binary size

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

Bootstrap: 678.233s -> 677.565s (-0.10%)
Artifact size: 320.05 MiB -> 319.41 MiB (-0.20%)

… r=jieyouxu Remove unused `llvm_readobj.rs` in `run-make-support` `llvm_readobj.rs` seems unused from the migration to `llvm.rs` in rust-lang#125165. Also, `llvm.rs` was missing the drop bombs (rust-lang#125752) in `llvm_readobj.rs`. Part of rust-lang#121876. r? `@jieyouxu`

Rollup merge of rust-lang#126536 - Rejyr:remove-unused-run-make-file, r=jieyouxu Remove unused `llvm_readobj.rs` in `run-make-support` `llvm_readobj.rs` seems unused from the migration to `llvm.rs` in rust-lang#125165. Also, `llvm.rs` was missing the drop bombs (rust-lang#125752) in `llvm_readobj.rs`. Part of rust-lang#121876. r? `@jieyouxu`

rustbot assigned Mark-Simulacrum May 30, 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 30, 2024

rust-cloud-vms bot force-pushed the kaboom branch 2 times, most recently from f9f3c42 to d9feedf Compare May 30, 2024 03:26

Kobzol reviewed May 30, 2024

View reviewed changes

rustbot assigned Kobzol and unassigned Mark-Simulacrum May 30, 2024

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

rust-cloud-vms bot force-pushed the kaboom branch from d9feedf to 7867fb7 Compare May 31, 2024 08:11

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

Kobzol reviewed May 31, 2024

View reviewed changes

tests/run-make/same-lib-two-locations-no-panic/rmake.rs Outdated Show resolved Hide resolved

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 31, 2024

jieyouxu mentioned this pull request May 31, 2024

Rollup of 6 pull requests #125817

Closed

This comment has been minimized.

Sign in to view

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 31, 2024

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 Jun 11, 2024

bors 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 Jun 11, 2024

jieyouxu added 6 commits June 11, 2024 08:21

run-make-support: add drop bomb module

a3feeb3

tests/run-make: update tests to use new API

ca95f78

run-make-support: bump version

d308a70

run-make-support: add changelog

5ec3eef

rust-cloud-vms bot force-pushed the kaboom branch from c1c06d6 to 5ec3eef Compare June 11, 2024 09:14

rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Jun 11, 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 Jun 11, 2024

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

bors merged commit 20ba13c into rust-lang:master Jun 11, 2024
7 checks passed

rustbot added this to the 1.81.0 milestone Jun 11, 2024

jieyouxu deleted the kaboom branch June 11, 2024 13:39

bors mentioned this pull request Jun 11, 2024

Migrate run-make/pgo-branch-weights to rmake #125165

Merged

4 tasks

Rejyr mentioned this pull request Jun 16, 2024

Remove unused llvm_readobj.rs in run-make-support #126536

Merged

jieyouxu mentioned this pull request Jul 18, 2024

run-make-support: add #[must_use] to helpers where suitable #125703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jieyouxu commented May 30, 2024 •

edited

Loading

rustbot commented May 30, 2024

Kobzol left a comment •

edited

Loading

jieyouxu commented May 30, 2024

jieyouxu commented May 31, 2024

Kobzol commented May 31, 2024

bors commented May 31, 2024

bors commented May 31, 2024

This comment has been minimized.

bors commented May 31, 2024

bors commented Jun 11, 2024

jieyouxu commented Jun 11, 2024

jieyouxu commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

rust-timer commented Jun 11, 2024

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

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

Conversation

jieyouxu commented May 30, 2024 • edited Loading

Summary

Details

rustbot commented May 30, 2024

Kobzol left a comment • edited Loading

Choose a reason for hiding this comment

jieyouxu commented May 30, 2024

jieyouxu commented May 31, 2024

Kobzol commented May 31, 2024

bors commented May 31, 2024

bors commented May 31, 2024

This comment has been minimized.

bors commented May 31, 2024

bors commented Jun 11, 2024

jieyouxu commented Jun 11, 2024

jieyouxu commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

rust-timer commented Jun 11, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

jieyouxu commented May 30, 2024 •

edited

Loading

Kobzol left a comment •

edited

Loading