make it possible to use stage0 libtest on compiletest #139386

onur-ozkan · 2025-04-04T17:56:50Z

With #119899, building the library tree will require a stage 1 compiler. This is because compiletest is defined as a ToolStd (since #68019) in order to use the in-tree library. As a result, #119899 makes certain development workflows more difficult as changes on the compiler tree will now require recompiling compiletest each time.

This PR allows switching ToolStd to ToolBootstrap with a simple boolean option in bootstrap.toml to allow compiletest to use the stage 0 libtest instead.

The changes under src/ci are clearly intended to make sure that compiletest doesn't break during future bootstrap beta bumps.

rustbot · 2025-04-04T17:56:54Z

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 · 2025-04-04T17:56:57Z

This PR modifies bootstrap.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

Kobzol · 2025-04-04T18:27:55Z

src/ci/run.sh

@@ -134,6 +134,7 @@ if [ "$DEPLOY$DEPLOY_ALT" = "1" ]; then

  CODEGEN_BACKENDS="${CODEGEN_BACKENDS:-llvm}"
  RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set rust.codegen-backends=$CODEGEN_BACKENDS"
+  RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set build.compiletest-use-stage0-libtest=false"


I would set this to false in the dist profile, in order to be enabled globally on CI, and then have one runner that sets it to true, to test that compiletest works also with stage0, if we want to gate on this.

Not sure if using it in just one runner gives enough coverage.

Here's what I did: compiletest-use-stage0-libtest is set to false in the dist profile and run.sh script overrides it to true unless $DEPLOY or $DEPLOY_ALT is set.

I'm pretty sure that most dist builders don't even build compiletest, as they don't run any tests. So if you want to have partial coverage of both true/false, we should do the split inside the test builders, not between test and dist, IMO.

That being said, I'm not sure if we need that much coverage of the true value? It's just a hack to make local rebuilds faster. I would put it maybe into one runner per OS.

Although, if we have both true and false on CI as blocking, can't that break beta bumps? 🤔

That being said, I'm not sure if we need that much coverage of the true value? It's just a hack to make local rebuilds faster. I would put it maybe into one runner per OS.

Yeah, probably... I can do that for Linux, Mac and Windows (let me know if I am missing any other) as those are the most popular ones used by devs.

Although, if we have both true and false on CI as blocking, can't that break beta bumps? 🤔

~~compiletest is always built using the in-tree version of libtest. So if something breaks libtest, it would fail during development already; it wouldn't make it to beta without being caught.~~ I guess that could happen if libtest is changed before bumping stage0 compiler, which will require cfg(bootstrap) thing there, I am not sure if this is acceptable. cc @jieyouxu

Hm yeah, I think there's a window where indeed cfg(bootstrap) might be needed, a library/testing-devex contributor who wants to modify libtest may need to cfg(bootstrap)-gate bit.

That being said, following #139317, the libtest usages in compiletest should be mostly isolated in one single module. We could add a doc comment on that about possibly needing cfg(bootstrap)-gating libtest changes.

Personally, I think this approach (possibility of compiletest using stage0 libtest for local dev) is reasonable, and I can't really think of a better solution.

rustbot · 2025-04-05T06:05:38Z

This PR modifies src/bootstrap/defaults.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

Signed-off-by: onur-ozkan <[email protected]>

jieyouxu

This looks good to me, cc @Kobzol in case you still have concerns.

src/bootstrap/defaults/bootstrap.dist.toml

Signed-off-by: onur-ozkan <[email protected]>

Kobzol

Left one comment. The change otherwise looks fine. I'm not sure if we shouldn't do an MCP for this, but I think that libtest changed very rarely recently, so this shouldn't require a lot of cfg(bootstrap) places in compiletest. If it becomes a problem in the future, we can discuss it further.

Kobzol · 2025-04-07T10:41:55Z

src/ci/citool/tests/test-jobs.yml

@@ -27,7 +27,7 @@ runners:
    <<: *base-job
 envs:
  env-x86_64-apple-tests: &env-x86_64-apple-tests
-    SCRIPT: ./x.py --stage 2 test --skip tests/ui --skip tests/rustdoc -- --exact
+    SCRIPT: ./x.py check compiletest --set build.compiletest-use-stage0-libtest=true && ./x.py --stage 2 test --skip tests/ui --skip tests/rustdoc -- --exact


x check doesn't actually go through the same codepath as x build; I think that for x check the stage0 config is not used at all, all the tools are checked with Mode::ToolRustc (

rust/src/bootstrap/src/core/build_steps/check.rs

Line 424 in d3d7a6d

Mode::ToolRustc,

).

d2840fa makes x check compiletest to respect build.compiletest-use-stage0-libtest option.

Thanks. This specific file contains just tests mock data btw, it doesn't need to be updated 😆

I am aware, I just wanted them to be 1:1 equal.

onur-ozkan · 2025-04-07T11:57:30Z

Left one comment. The change otherwise looks fine. I'm not sure if we shouldn't do an MCP for this, but I think that libtest changed very rarely recently, so this shouldn't require a lot of cfg(bootstrap) places in compiletest.

In my opinion, the point we are discussing (whether compiletest should support stage0 with cfg(bootstrap) or require a stage1 compiler) seems more suitable for a Zulip thread (which is already ongoing). I'm not sure if this will change in the near future with other possible workarounds, so I wouldn't go for an MCP at this point.

Kobzol

Ok, I'm fine with this now, so no other concerns.

It seems like you committed a LLVM submodule update by accident.

Kobzol · 2025-04-07T12:47:15Z

src/ci/citool/tests/test-jobs.yml

@@ -27,7 +27,7 @@ runners:
    <<: *base-job
 envs:
  env-x86_64-apple-tests: &env-x86_64-apple-tests
-    SCRIPT: ./x.py --stage 2 test --skip tests/ui --skip tests/rustdoc -- --exact
+    SCRIPT: ./x.py check compiletest --set build.compiletest-use-stage0-libtest=true && ./x.py --stage 2 test --skip tests/ui --skip tests/rustdoc -- --exact


Thanks. This specific file contains just tests mock data btw, it doesn't need to be updated 😆

Kobzol · 2025-04-07T12:48:45Z

src/bootstrap/src/core/build_steps/check.rs

+            &[],
+        );
+
+        cargo.allow_features("test");


It kinds sucks that we have to duplicate the code in this way. Would be maybe nice to merge build/check logic into the same step. But that's not relevant for this PR, anyway.

I think the macro will be super complicated and ugly with compiletest specific changes, it's better to re-write the whole macro to make it more generic so...

Signed-off-by: onur-ozkan <[email protected]>

rustbot assigned Kobzol Apr 4, 2025

onur-ozkan force-pushed the configurable-compiletest-libtest branch from 15d164d to 7f3da8c Compare April 4, 2025 17:59

Kobzol reviewed Apr 4, 2025

View reviewed changes

onur-ozkan force-pushed the configurable-compiletest-libtest branch from 7f3da8c to 8c17501 Compare April 5, 2025 06:05

onur-ozkan force-pushed the configurable-compiletest-libtest branch 3 times, most recently from 2ceaed5 to 48e9c4a Compare April 5, 2025 11:19

onur-ozkan added 3 commits April 5, 2025 14:22

create new option build.compiletest-use-stage0-libtest

86a7ee6

Signed-off-by: onur-ozkan <[email protected]>

utilize compiletest_use_stage0_libtest option

17fad21

Signed-off-by: onur-ozkan <[email protected]>

add change-entry

afe3834

Signed-off-by: onur-ozkan <[email protected]>

onur-ozkan force-pushed the configurable-compiletest-libtest branch 4 times, most recently from c757f55 to c3cfdfc Compare April 5, 2025 11:25

jieyouxu self-assigned this Apr 5, 2025

jieyouxu approved these changes Apr 6, 2025

View reviewed changes

src/bootstrap/defaults/bootstrap.dist.toml Outdated Show resolved Hide resolved

enable in-tree std on some runners

c830665

Signed-off-by: onur-ozkan <[email protected]>

onur-ozkan force-pushed the configurable-compiletest-libtest branch from c3cfdfc to c830665 Compare April 7, 2025 07:09

Kobzol reviewed Apr 7, 2025

View reviewed changes

implement check step for compiletest separately

cae28b5

Signed-off-by: onur-ozkan <[email protected]>

onur-ozkan force-pushed the configurable-compiletest-libtest branch from d2840fa to cae28b5 Compare April 7, 2025 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make it possible to use stage0 libtest on compiletest #139386

make it possible to use stage0 libtest on compiletest #139386

onur-ozkan commented Apr 4, 2025

rustbot commented Apr 4, 2025

rustbot commented Apr 4, 2025

Kobzol Apr 4, 2025 •

edited

Loading

onur-ozkan Apr 5, 2025 •

edited

Loading

Kobzol Apr 5, 2025

Kobzol Apr 5, 2025

onur-ozkan Apr 5, 2025 •

edited

Loading

jieyouxu Apr 6, 2025

jieyouxu Apr 6, 2025

rustbot commented Apr 5, 2025

jieyouxu left a comment

Kobzol left a comment

Kobzol Apr 7, 2025

onur-ozkan Apr 7, 2025

Kobzol Apr 7, 2025

onur-ozkan Apr 7, 2025

onur-ozkan commented Apr 7, 2025

Kobzol left a comment

Kobzol Apr 7, 2025

Kobzol Apr 7, 2025

onur-ozkan Apr 7, 2025

make it possible to use stage0 libtest on compiletest #139386

Are you sure you want to change the base?

make it possible to use stage0 libtest on compiletest #139386

Conversation

onur-ozkan commented Apr 4, 2025

rustbot commented Apr 4, 2025

rustbot commented Apr 4, 2025

Kobzol Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

onur-ozkan Apr 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan Apr 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Apr 5, 2025

jieyouxu left a comment

Choose a reason for hiding this comment

Kobzol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan commented Apr 7, 2025

Kobzol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kobzol Apr 4, 2025 •

edited

Loading

onur-ozkan Apr 5, 2025 •

edited

Loading

onur-ozkan Apr 5, 2025 •

edited

Loading