build_helper::compiletest module factored out of compiletest for use by bootstrap

[lolbinarycat]

closes #135645

[rustbot]

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

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[onur-ozkan]

r? onur-ozkan

[jieyouxu]

I don't feel like these two helpers belong in build_helpers?

[lolbinarycat]

that would require turning them into free functions. do you want me to do that?

[jieyouxu]

Yes, please do move them as free functions into compiletest, this is an impl detail that doesn't feel like they belong here

[jieyouxu]

Please call this TestMode

[lolbinarycat]

why? the command line flag is called --mode, no? so why shouldn't the struct match?

[jieyouxu]

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).

[lolbinarycat]

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.

[jieyouxu]

Yes. I would prefer if this is called TestMode and not just Mode.

[jieyouxu]

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.

[onur-ozkan]

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?

[jieyouxu]

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?

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.)

[lolbinarycat]

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.