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.

Sign up for GitHub

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

Jump to bottom

[compiletest-related cleanups 2/7] Feed stage number to compiletest directly #136472

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[compiletest-related cleanups 2/7] Feed stage number to compiletest directly #136472

wants to merge 2 commits into from
+41 −38

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Feb 3, 2025 •
edited
Loading

Reference for overall changes: #136437
Part 2 of 7 of the compiletest-related cleanups PR series.

Summary

  • Pass stage number via new --stage compiletest flag directly from bootstrap, instead of deriving that info in compiletest by doing gymnastics on --stage-id.
  • Just a cleanup, should have no functional changes.

r? bootstrap

@jieyouxu
bootstrap: feed stage number via --stage to compiletest
7d29a2e
@jieyouxu
compiletest: use --stage number directly instead of deriving from `…
8a0dc27 
…--stage-id`

Notably, this avoids having to do hacky string splitting based on
`--stage-id`.
@rustbot rustbot added A-compiletest Area: The compiletest test runner 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 Feb 3, 2025
@jieyouxu jieyouxu mentioned this pull request Feb 3, 2025
Draft
jieyouxu
jieyouxu commented Feb 3, 2025
View reviewed changes
src/tools/compiletest/src/runtest/run_make.rs
Comment on lines -262 to -264
// FIXME(jieyouxu): improve the communication between bootstrap and compiletest here so
// we don't have to hack out a `stageN`.
let stage = self.config.stage_id.split('-').next().unwrap();
Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly to fix this

This was referenced Feb 3, 2025
Open
Open
@Mark-Simulacrum
Copy link
Member

This doesn't seem obviously better (we're still hardcoding knowledge in compiletest about the bootstrap folder hierarchy, just adjusting how the base stageN gets threaded). But I'm fine with approving it; my own sense is that tight coupling here is unavoidable given the constraints so I don't care much either way.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 9, 2025

📌 Commit 8a0dc27 has been approved by Mark-Simulacrum

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 Feb 9, 2025
@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 9, 2025

This doesn't seem obviously better (we're still hardcoding knowledge in compiletest about the bootstrap folder hierarchy, just adjusting how the base stageN gets threaded). But I'm fine with approving it; my own sense is that tight coupling here is unavoidable given the constraints so I don't care much either way.

Yeah I really don't think we can avoid assuming knowledge about the folder hierarchy. In fact, tests rely on knowing about this structure for normalization purposes. This PR isn't to get rid of that, it's just make the it slightly less hacky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-compiletest Area: The compiletest test runner 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jieyouxu @Mark-Simulacrum @bors @rustbot