Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

run-make: annotate library with #[must_use] and enforce unused_must_use in rmake.rs #126197

Merged
merged 5 commits into from
Jun 13, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Jun 9, 2024

This PR adds #[must_use] annotations to functions of the run_make_support library where it makes sense, and adjusts compiletest to compile rmake.rs with -Dunused_must_use.

The rationale is that it's highly likely that unused #[must_use] values in rmake.rs test files are bugs. For example, unused fs/io results are often load-bearing to the correctness of the test and often unchecked fs/io results allow the test to silently pass where it would've failed if the result was checked.

This PR is best reviewed commit-by-commit.

try-job: test-various
try-job: x86_64-msvc

@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2024

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

@rustbot 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 Jun 9, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2024

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.

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 9, 2024

r? @Kobzol since you also reviewed #125752

@rustbot rustbot assigned Kobzol and unassigned Mark-Simulacrum Jun 9, 2024
@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 9, 2024

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2024
run-make: annotate library with `#[must_use]` and enforce `unused_must_use` in rmake.rs

Commits up to 706abf6 are from rust-lang#125752.

This PR adds `#[must_use]` annotations to functions of the `run_make_support` library where it makes sense, and adjusts compiletest to compile rmake.rs with `-Dunused_must_use`.

The rationale is that it's highly likely that unused `#[must_use]` values in rmake.rs test files are bugs. For example, unused fs/io results are often crucial to the correctness of the test and often unchecked fs/io results allow the test to silently pass where it would've failed if the result was checked.

try-job: test-various
try-job: x86_64-msvc
@bors
Copy link
Contributor

bors commented Jun 9, 2024

⌛ Trying commit 8d182e3 with merge ef2e85e...

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 9, 2024

@bors rollup=never (likely conflicts with other rmake.rs PRs)

@bors
Copy link
Contributor

bors commented Jun 9, 2024

☀️ Try build successful - checks-actions
Build commit: ef2e85e (ef2e85e07ec543371158b7f45fc3e7440450acfd)

@Kobzol
Copy link
Contributor

Kobzol commented Jun 9, 2024

LGTM, feel free to r=me after #125752 is merged. It might also be good to only merge this after the FS wrapper PR.

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 9, 2024

@rustbot blocked (on #125752)

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 9, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Jun 11, 2024

@rustbot author

@rustbot 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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jun 11, 2024
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Jun 11, 2024
@jieyouxu
Copy link
Member Author

I'll try to land this after the fs wrapper PR (#125736), I'll block this PR on the fs wrapper PR and rebase after that and drop the reset-codegen-1 change.

@rustbot blocked (after #125736)

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. 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
Copy link
Contributor

bors commented Jun 11, 2024

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

@jieyouxu
Copy link
Member Author

Unblocked since #125736 is merged. @rustbot author

@rustbot 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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jun 11, 2024
@jieyouxu
Copy link
Member Author

Rebased and fixed tests, but otherwise no significant changes.

@bors r=@Kobzol rollup=never

@bors
Copy link
Contributor

bors commented Jun 11, 2024

📌 Commit 5b126ed has been approved by Kobzol

It is now in the queue for this repository.

@bors 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
@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 11, 2024

Oh hm this is going to need another test update if the current rollup passes. EDIT: actually it doesn't

@bors
Copy link
Contributor

bors commented Jun 13, 2024

⌛ Testing commit 5b126ed with merge 921645c...

@bors
Copy link
Contributor

bors commented Jun 13, 2024

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing 921645c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 13, 2024
@bors bors merged commit 921645c into rust-lang:master Jun 13, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 13, 2024
@jieyouxu jieyouxu deleted the rmake-must-use branch June 13, 2024 09:49
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (921645c): 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.5% [2.5%, 2.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary -2.8%)

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)
-2.8% [-2.8%, -2.8%] 1
All ❌✅ (primary) - - 0

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: 668.917s -> 671.475s (0.38%)
Artifact size: 319.72 MiB -> 320.37 MiB (0.20%)

@RalfJung
Copy link
Member

This PR contained an odd commit: c8e3eeb didn't change anything. How did that happen?

@Kobzol
Copy link
Contributor

Kobzol commented Jun 20, 2024

Probably some leftover from a rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants