build_helper::compiletest module factored out of compiletest for use by bootstrap by lolbinarycat · Pull Request #135653 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation19 Commits2 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
r? @wesleywiser
rustbot has assigned @wesleywiser.
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
Area: The compiletest test runner
Area: The testsuite used to check the correctness of rustc
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
labels
Some changes occurred in src/tools/compiletest
cc @jieyouxu
This comment has been minimized.
| } |
|---|
| impl Mode { |
| pub fn aux_dir_disambiguator(self) -> &'static str { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel like these two helpers belong in build_helpers?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would require turning them into free functions. do you want me to do that?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please do move them as free functions into compiletest, this is an impl detail that doesn't feel like they belong here
| string_enum! { |
| #[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)] |
| pub enum Mode { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please call this TestMode
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? the command line flag is called --mode, no? so why shouldn't the struct match?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because build_helpers is a shared dep between bootstrap, compiletest, and some other bootstrap tools. bootstrap itself already has a Mode, not to mention Mode is a very generic term.
--mode for compiletest is fine because it's very clear from context as its a flag of the compiletest binary itself (only matters when bootstrap is actually trying to run compiletest binary).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would think its position in the compiletest module would be adequate context.
i can if you really want me to, it's just gonna take a bit since it's refactoring across several crates.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I would prefer if this is called TestMode and not just Mode.
The one thing that feels slightly iffy to me is that now the compiletest test mode definitions are no longer in compiletest, which makes it more indirect if you want to wire up or remove a test mode, or adjust existing test modes.
But I suppose that's fine.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
The one thing that feels slightly iffy to me is that now the compiletest test mode definitions are no longer in compiletest, which makes it more indirect if you want to wire up or remove a test mode, or adjust existing test modes.
Fair concern as directives are compiletest-first dependency.
How about placing directives module (without any helper functions or additional components) into compiletest and using it in bootstrap with manually specified path mod?
The one thing that feels slightly iffy to me is that now the compiletest test mode definitions are no longer in compiletest, which makes it more indirect if you want to wire up or remove a test mode, or adjust existing test modes.
Fair concern as directives are compiletest-first dependency.
How about placing directives module (without any helper functions or additional components) into
compiletestand using it in bootstrap with manually specified path mod?
Not just directives, but also the concept of test modes and test suites. They are centric to what compiletest is responsible for handling.
I think keeping a very lean TestMode + TestSuite enum definition (no additional helpers modulo the string enum stuff) module in compiletest, then depending on that via path mod in bootstrap is probably more logical, yes. (We can just leave a doc comment on this specific definition module that one must not depend on other parts of compiletest, as bootstrap is designed to consume the enum definitions via #[path] module.)
I've always seen #[path] trickery as a bit of a hack outside of rust's normal dependency model, so i'd like to leave this open for bit to get feedback from other t-bootstrap members before i commit to any specific implementation, if that's alright.
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!
@JohnCSimon waiting for consensus on what should be done, i guess?
I guess people are pretty clear that they want to use the #[path] hack, but I was expecting to get some further feedback.
I'll try to get around to finishing this, but I am fairly busy, so if someone wants to take this over, I wouldn't mind.
Actually, can you hold off on pulling things out from compiletest? Several of us are working on several somewhat invasive changes to compiletest, and I'd like to keep compiletest types in compiletest in the short term to make it less places to adjust.
Eventually we'd probably move the key types to build_helpers, but preferably not at the moment.
Status: Awaiting decision from the relevant subteam (see the T- label).
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Labels
Area: The compiletest test runner
Area: The testsuite used to check the correctness of rustc
Status: Awaiting decision from the relevant subteam (see the T- label).
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)