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

Remove valgrind test suite and support from compiletest, bootstrap and opt-dist #131351

Merged
merged 4 commits into from
Oct 7, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Oct 7, 2024

The run-pass-valgrind test suite is not exercised in CI, and as far as I'm aware nobody runs it (asked in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Are.20the.20valgrind.20tests.20even.20used.20by.20anyone.3F). What's remaining of valgrind support in compiletest isn't even properly hooked up with bootstrap.

The existing valgrind logic in compiletest is also straight up questionable, i.e.

// FIXME(jieyouxu): does this really make any sense? If a valgrind test isn't testing
// valgrind, what is it even testing?
if self.config.valgrind_path.is_none() {
assert!(!self.config.force_valgrind);
return self.run_rpass_test();
}

It just runs valgrind tests as rpass if no valgrind path is provided to compiletest from bootstrap -- but bootstrap doesn't even pass a valgrind path to compiletest in the first place, so this always ran as rpass tests. So what is this even testing?

So if it's not testing anything, let's delete it.

Closes #44816 by deleting the test suite :3

@rustbot
Copy link
Collaborator

rustbot commented Oct 7, 2024

r? @Kobzol

rustbot has assigned @Kobzol.
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 Oct 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 7, 2024

Some changes occurred in src/tools/opt-dist

cc @Kobzol

Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also "ignore-mode-run-pass-valgrind", but I see that's being removed in #131346.

In terms of the bootstrap and opt-dist changes, LGTM. You can r=me unless someone wants to do an MCP.

@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 7, 2024

I'll wait for the answer to if MCP is needed to come back from T-compiler.

And also purge mentions of run-pass-valgrind from dev-guide. EDIT: rust-lang/rustc-dev-guide#2091

MCP: rust-lang/compiler-team#792

@jieyouxu jieyouxu added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 7, 2024
@jieyouxu jieyouxu added S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 7, 2024
@fmease
Copy link
Member

fmease commented Oct 7, 2024

This would fix #44816 from 2017 (found via #44802 (comment) via tests/run-pass-valgrind/issue-44800.rs).

@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 7, 2024

I was not aware that this was broken since 2017, that is funny :3 Can't have bugs related to the test suite if the test suite doesn't exist!

@jieyouxu jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. labels Oct 7, 2024
@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 7, 2024

Since the MCP isn't strictly required (it's marked as accepted rust-lang/compiler-team#792 because it's not really necessary but still useful as a way to announce the change), and since this test suite never worked properly since 2017 (maybe even earlier),

@bors r=@Kobzol

@bors
Copy link
Contributor

bors commented Oct 7, 2024

📌 Commit fa3c25e 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 7, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 7, 2024
…kingjubilee

Rollup of 7 pull requests

Successful merges:

 - rust-lang#128721 (Don't allow the `#[pointee]` attribute where it doesn't belong)
 - rust-lang#130479 (skip in-tree compiler build for llvm-bitcode-linker if ci-rustc is on)
 - rust-lang#130899 (Couple of changes to make it easier to compile rustc for wasm)
 - rust-lang#131225 (`rustc_borrowck` memory management tweaks)
 - rust-lang#131351 (Remove valgrind test suite and support from compiletest, bootstrap and opt-dist)
 - rust-lang#131359 (Fix used_underscore_binding in rustc_serialize)
 - rust-lang#131367 (Mark Boxy as on vacation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f88bfa3 into rust-lang:master Oct 7, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 7, 2024
Rollup merge of rust-lang#131351 - jieyouxu:yeet-the-valgrind, r=Kobzol

Remove valgrind test suite and support from compiletest, bootstrap and opt-dist

The `run-pass-valgrind` test suite is not exercised in CI, and as far as I'm aware nobody runs it (asked in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Are.20the.20valgrind.20tests.20even.20used.20by.20anyone.3F). What's remaining of valgrind support in compiletest isn't even properly hooked up with bootstrap.

The existing valgrind logic in compiletest is also straight up questionable, i.e.

https://github.com/rust-lang/rust/blob/1b3b8e7b0265162853c650ead09905bc3cdaeae9/src/tools/compiletest/src/runtest/valgrind.rs#L7-L12

It just runs valgrind tests as `rpass` if no valgrind path is provided to compiletest from bootstrap -- but bootstrap doesn't even pass a valgrind path to compiletest in the first place, so this always ran as `rpass` tests. So what is this even testing?

So if it's not testing anything, let's delete it.

Closes rust-lang#44816 by deleting the test suite :3

<img src="https://github.com/user-attachments/assets/99525bf7-e85b-40ba-9281-e4e1e275c4e8" width=300 />
@jieyouxu jieyouxu deleted the yeet-the-valgrind branch October 7, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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
None yet
Development

Successfully merging this pull request may close these issues.

run-pass-valgrind tests don't actually run in valgrind
5 participants