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 empty test suite tests/run-make-fulldeps #126155

Merged
merged 4 commits into from
Jun 9, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Jun 8, 2024

After #109770, there were only a handful of tests left in the run-make-fulldeps suite.

As of #126111, there are no longer any run-make-fulldeps tests, so now we can:

  • Remove the directory
  • Remove related bootstrap/compiletest code
  • Remove various other references in CI scripts and documentation.

By removing this suite, we also no longer need to worry about discrepancies between it and ui-fulldeps, and we don't have to worry about porting tests from Makefile to rmake (or whether rmake even works with fulldeps).

@rustbot
Copy link
Collaborator

rustbot commented Jun 8, 2024

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jun 8, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 8, 2024

Some changes occurred in src/doc/rustc/src/platform-support

cc @Nilstrieb

Comment on lines 47 to 49
ENV SCRIPT \
python3 ../x.py --stage 2 build && \
python3 ../x.py --stage 2 test tests/run-make-fulldeps --test-args clang
Copy link
Contributor Author

@Zalathar Zalathar Jun 8, 2024

Choose a reason for hiding this comment

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

This one is a bit of a head-scratcher for me, because with the removal of run-make-fulldeps it's unclear to me what this job is actually supposed to be testing.

(But I'm also not very familiar with how these Dockerfiles are used.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line that tests run-make-fulldeps was originally added in #57514 (specifically 48cb04f).

Copy link
Contributor

@Kobzol Kobzol Jun 8, 2024

Choose a reason for hiding this comment

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

Hmm, this image wasn't actually running any useful tests for some time, because there were no tests marked with needs-matching-clang left in the run-make-fulldeps directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we should just change this invocation to tests/run-make, for now. In the longer term, I'm planning to document and inspect all our CI jobs to make sure that we don't miss any tests, and that we don't do useless work, because apparently some of them are very old :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the history of this job, I think it was originally intended to just build the compiler with --enable-debug, to check that the build succeeds.

Then the clang-specific stuff was grafted on later.

Copy link
Contributor Author

@Zalathar Zalathar Jun 9, 2024

Choose a reason for hiding this comment

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

But since the clang-specific stuff has been moved over into run-make (#109770), replacing this with run-make still makes sense, probably.

@Zalathar Zalathar force-pushed the run-make-fulldeps branch from 133a403 to 0a258af Compare June 8, 2024 12:12
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

r=me once #126155 (comment) is addressed.

Zalathar added 2 commits June 9, 2024 12:49
This test never actually checked anything useful, so presumably it only existed
to silence the tidy check for feature gate tests, with the real checks being
performed elsewhere (in tests that have since been deleted).
@Zalathar Zalathar force-pushed the run-make-fulldeps branch 3 times, most recently from 8a3067f to 0bdc68d Compare June 9, 2024 03:05
@Zalathar
Copy link
Contributor Author

Zalathar commented Jun 9, 2024

I made the suggested changes to x86_64-gnu-debug (diff), and also added a brief comment with my current (imperfect) understanding of what this job is trying to do.

Note that I have removed the --test-args clang part, because it was failing to run some tests that use the //@ needs-matching-clang header but don't have clang in their name. Running the whole suite is overkill, but it seems preferable to accidentally missing some tests that otherwise wouldn't run anywhere.

@Zalathar
Copy link
Contributor Author

Zalathar commented Jun 9, 2024

I've also set up a run of the debug job over in #114917, because it looks like some of the affected tests haven't actually been running for a while, so I want to see whether any of them fail.

@Zalathar
Copy link
Contributor Author

Zalathar commented Jun 9, 2024

Looks like my test run found some tests that are broken:

      [run-make] tests/run-make/cross-lang-lto-pgo-smoketest
      [run-make] tests/run-make/cross-lang-lto-riscv-abi
      [run-make] tests/run-make/issue-84395-lto-embed-bitcode

In light of this, I'll update the Dockerfile to continue passing --test-args clang, and we can follow up these other tests in a separate issue.

@Zalathar Zalathar force-pushed the run-make-fulldeps branch from 0bdc68d to 1131680 Compare June 9, 2024 04:36
Zalathar added 2 commits June 9, 2024 14:37
It looks like this job was intending to run all of the `needs-matching-clang`
tests (since they don't run without `RUSTBUILD_FORCE_CLANG_BASED_TESTS`), but
over time developed two problems:

- The tests it cares about were moved from run-make-fulldeps to run-make.
- Some of the relevant tests don't actually have "clang" in their name.

Switching to run-make solves the first problem, but we still don't run the
tests without "clang" in their name, because some of them are currently broken.
@Zalathar Zalathar force-pushed the run-make-fulldeps branch from 1131680 to 5223bf4 Compare June 9, 2024 04:38
@onur-ozkan
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2024

📌 Commit 5223bf4 has been approved by onur-ozkan

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 Jun 9, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#126137 (tests: Add ui/higher-ranked/trait-bounds/normalize-generic-arg.rs)
 - rust-lang#126146 (std::unix::process adding few specific freebsd signals to be able to id.)
 - rust-lang#126155 (Remove empty test suite `tests/run-make-fulldeps`)
 - rust-lang#126168 (std::unix::os current_exe implementation simplification for haiku.)
 - rust-lang#126175 (Use --quiet flag when installing pip dependencies)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 246fc28 into rust-lang:master Jun 9, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2024
Rollup merge of rust-lang#126155 - Zalathar:run-make-fulldeps, r=onur-ozkan

Remove empty test suite `tests/run-make-fulldeps`

After rust-lang#109770, there were only a handful of tests left in the run-make-fulldeps suite.

As of rust-lang#126111, there are no longer *any* run-make-fulldeps tests, so now we can:

- Remove the directory
- Remove related bootstrap/compiletest code
- Remove various other references in CI scripts and documentation.

By removing this suite, we also no longer need to worry about discrepancies between it and ui-fulldeps, and we don't have to worry about porting tests from Makefile to [rmake](rust-lang#121876) (or whether rmake even works with fulldeps).
@Zalathar Zalathar deleted the run-make-fulldeps branch June 9, 2024 11:45
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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants