Skip to content

Match std RUSTFLAGS for host and target for mir-opt test suite to fix double std build/rebuilds #131442

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Oct 9, 2024

Previously the bootstrap compiletest Step::run flow had:

// ensure that `libproc_macro` is available on the host.
builder.ensure(compile::Std::new(compiler, compiler.host));

// ...

if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target));
} else {
    builder.ensure(compile::Std::new(compiler, target));
}

This can cause unnecessary std rebuilds (even on the same invocation) because if host == target then builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target)) will have different RUSTFLAGS than builder.ensure(compile::Std::new(compiler, compiler.host)).

This PR fixes that by matching up std RUSTFLAGS if the test suite is mir-opt:

if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, compiler.host));
} else {
    builder.ensure(compile::Std::new(compiler, compiler.host));
}

This is a short-term fix, the better fix is to enforce how RUSTFLAGS are handled as described in #131437 (comment).

Fixes #131437.

@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 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 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 Oct 9, 2024
@@ -1697,7 +1697,11 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
builder.ensure(TestHelpers { target: compiler.host });
Copy link
Member Author

Choose a reason for hiding this comment

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

Remark: the compiletest Step::run is some gigantic convoluted build step, lol. I might revisit that in the future and see if I can try to make it uhhh less insane.

@jieyouxu jieyouxu changed the title Match std RUSTFLAGS for host and target for mir-opt test suite Match std RUSTFLAGS for host and target for mir-opt test suite to fix double std build/rebuilds Oct 9, 2024
@onur-ozkan
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 10, 2024

📌 Commit becf664 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 Oct 10, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 10, 2024
…zkan

Match std `RUSTFLAGS` for host and target for `mir-opt` test suite to fix double std build/rebuilds

Previously the bootstrap compiletest `Step::run` flow had:

```rs
// ensure that `libproc_macro` is available on the host.
builder.ensure(compile::Std::new(compiler, compiler.host));

// ...

if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target));
} else {
    builder.ensure(compile::Std::new(compiler, target));
}
```

This can cause unnecessary std rebuilds (even on the same invocation) because if host == target then `builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target))` will have different `RUSTFLAGS` than `builder.ensure(compile::Std::new(compiler, compiler.host))`.

This PR fixes that by matching up std `RUSTFLAGS` if the test suite is `mir-opt`:

```rs
if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, compiler.host));
} else {
    builder.ensure(compile::Std::new(compiler, compiler.host));
}
```

This is a short-term fix, the better fix is to enforce how `RUSTFLAGS` are handled as described in rust-lang#131437 (comment).

Fixes rust-lang#131437.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 10, 2024
…zkan

Match std `RUSTFLAGS` for host and target for `mir-opt` test suite to fix double std build/rebuilds

Previously the bootstrap compiletest `Step::run` flow had:

```rs
// ensure that `libproc_macro` is available on the host.
builder.ensure(compile::Std::new(compiler, compiler.host));

// ...

if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target));
} else {
    builder.ensure(compile::Std::new(compiler, target));
}
```

This can cause unnecessary std rebuilds (even on the same invocation) because if host == target then `builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target))` will have different `RUSTFLAGS` than `builder.ensure(compile::Std::new(compiler, compiler.host))`.

This PR fixes that by matching up std `RUSTFLAGS` if the test suite is `mir-opt`:

```rs
if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, compiler.host));
} else {
    builder.ensure(compile::Std::new(compiler, compiler.host));
}
```

This is a short-term fix, the better fix is to enforce how `RUSTFLAGS` are handled as described in rust-lang#131437 (comment).

Fixes rust-lang#131437.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 10, 2024
…zkan

Match std `RUSTFLAGS` for host and target for `mir-opt` test suite to fix double std build/rebuilds

Previously the bootstrap compiletest `Step::run` flow had:

```rs
// ensure that `libproc_macro` is available on the host.
builder.ensure(compile::Std::new(compiler, compiler.host));

// ...

if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target));
} else {
    builder.ensure(compile::Std::new(compiler, target));
}
```

This can cause unnecessary std rebuilds (even on the same invocation) because if host == target then `builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target))` will have different `RUSTFLAGS` than `builder.ensure(compile::Std::new(compiler, compiler.host))`.

This PR fixes that by matching up std `RUSTFLAGS` if the test suite is `mir-opt`:

```rs
if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, compiler.host));
} else {
    builder.ensure(compile::Std::new(compiler, compiler.host));
}
```

This is a short-term fix, the better fix is to enforce how `RUSTFLAGS` are handled as described in rust-lang#131437 (comment).

Fixes rust-lang#131437.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Oct 10, 2024
…zkan

Match std `RUSTFLAGS` for host and target for `mir-opt` test suite to fix double std build/rebuilds

Previously the bootstrap compiletest `Step::run` flow had:

```rs
// ensure that `libproc_macro` is available on the host.
builder.ensure(compile::Std::new(compiler, compiler.host));

// ...

if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target));
} else {
    builder.ensure(compile::Std::new(compiler, target));
}
```

This can cause unnecessary std rebuilds (even on the same invocation) because if host == target then `builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target))` will have different `RUSTFLAGS` than `builder.ensure(compile::Std::new(compiler, compiler.host))`.

This PR fixes that by matching up std `RUSTFLAGS` if the test suite is `mir-opt`:

```rs
if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, compiler.host));
} else {
    builder.ensure(compile::Std::new(compiler, compiler.host));
}
```

This is a short-term fix, the better fix is to enforce how `RUSTFLAGS` are handled as described in rust-lang#131437 (comment).

Fixes rust-lang#131437.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#130308 (codegen_ssa: consolidate tied target checks)
 - rust-lang#130538 (Stabilize const `{slice,array}::from_mut`)
 - rust-lang#130741 (rustc_target: Add sme-b16b16 as an explicit aarch64 target feature)
 - rust-lang#131033 (Precise capturing in traits)
 - rust-lang#131442 (Match std `RUSTFLAGS` for host and target for `mir-opt` test suite to fix double std build/rebuilds)
 - rust-lang#131470 (add test infra to explicitely test rustc with autodiff/enzyme disabled)
 - rust-lang#131475 (Compiler & its UI tests: Rename remaining occurrences of "object safe" to "dyn compatible" )
 - rust-lang#131493 (Avoid redundant sysroot additions to `PATH` when linking)
 - rust-lang#131509 (Update .mailmap)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 28bc5a2 into rust-lang:master Oct 10, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2024
Rollup merge of rust-lang#131442 - jieyouxu:mir-opt-rebuild, r=onur-ozkan

Match std `RUSTFLAGS` for host and target for `mir-opt` test suite to fix double std build/rebuilds

Previously the bootstrap compiletest `Step::run` flow had:

```rs
// ensure that `libproc_macro` is available on the host.
builder.ensure(compile::Std::new(compiler, compiler.host));

// ...

if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target));
} else {
    builder.ensure(compile::Std::new(compiler, target));
}
```

This can cause unnecessary std rebuilds (even on the same invocation) because if host == target then `builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target))` will have different `RUSTFLAGS` than `builder.ensure(compile::Std::new(compiler, compiler.host))`.

This PR fixes that by matching up std `RUSTFLAGS` if the test suite is `mir-opt`:

```rs
if suite == "mir-opt" {
    builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, compiler.host));
} else {
    builder.ensure(compile::Std::new(compiler, compiler.host));
}
```

This is a short-term fix, the better fix is to enforce how `RUSTFLAGS` are handled as described in rust-lang#131437 (comment).

Fixes rust-lang#131437.
@jieyouxu jieyouxu deleted the mir-opt-rebuild branch October 12, 2024 01:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

Running ./x test mir-opt rebuilds std every time
4 participants