Audit usage of command_output methods in rmake.rs tests
#125617
Assignees
Labels
A-testsuite
Area: The testsuite used to check the correctness of rustc
C-bug
Category: This is a bug.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
jieyouxu
added
A-testsuite
rustbot
added
the
needs-triage
jieyouxu
removed
the
needs-triage
This was referenced May 27, 2024
jieyouxu
added a commit
to jieyouxu/rust
that referenced
this issue
May 31, 2024
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
added a commit
to rust-lang-ci/rust
that referenced
this issue
May 31, 2024
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
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 3, 2024
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.
try-job: x86_64-msvc
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 3, 2024
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.
try-job: x86_64-msvc
github-project-automation
bot
moved this from In progress
to Done
in Oxidizing run-make tests
|
I found a case in #126231 where the existing script does want the command output, so that it can report multiple failures instead of stopping immediately on the first one. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
They are typically not the semantics that's desired, i.e. they only provide the command output but does not assert that the command status is success. We should make sure we're not silently letting failed commands through.
Do we even want to allow
command_outputmethods? It's likely less footgunny if we only providedCommandWrapper::{run, run_fail, run_fail_assert_exit_code}methods which perform asserts on the command exit status.The text was updated successfully, but these errors were encountered: