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.

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

PR CI: check-build the library for more targets #121519

Open
RalfJung opened this issue Feb 23, 2024 · 15 comments
Open

PR CI: check-build the library for more targets #121519

RalfJung opened this issue Feb 23, 2024 · 15 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-feature-request Category: A feature request, i.e: not implemented / a PR. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@RalfJung
Copy link
Member

RalfJung commented Feb 23, 2024

@saethlin and @onur-ozkan made it so that ./x.py check library --target=aarch64-pc-windows-msvc now works on all hosts, without special magic Windows tooling being needed. We should use that in PR CI to catch some more standard library build issues. In particular we are currently not covering

  • Windows MSVC (we do check GNU)
  • macOS

(Or, well, it turns out we run Miri tests for these targets so we cover them in x86_64-gnu-tools. But that seems to be a bit too accidental. And Miri doesn't complain if there are warnings, only if there are errors. And this sets cfg(miri) which changes some codepaths.)

@rust-lang/infra which runner would be the best one to add this to? AFAIK the mingw-check runner exists mostly to give us some test coverage for library builds on a Windows target, so maybe that would be a good one?

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 23, 2024
@saethlin
Copy link
Member

saethlin commented Feb 23, 2024

Sysroot builds are nearly serial, and our runners have 8 vCPUs each. While Cargo supports multi-target builds and parallelizes them effectively, other tools like cargo-miri and bootstrap do not.

I've poked around the code in bootstrap for passing multiple targets down to the cargo invocation, but found it too challenging for me to do personally. I suspect the bootstrap team has enough understanding to do this much more easily.

@saethlin
Copy link
Member

For anyone who wants a demo of how close-at-hand this is, you can just run this locally today:

cargo +nightly check -Zbuild-std --target=aarch64-unknown-linux-gnu --target=i686-pc-windows-gnu --target=i686-pc-windows-msvc --target=i686-unknown-linux-gnu --target=x86_64-apple-darwin --target=x86_64-pc-windows-gnu --target=x86_64-pc-windows-msvc --target=x86_64-unknown-linux-gnu

@saethlin saethlin added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-feature-request Category: A feature request, i.e: not implemented / a PR. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 24, 2024
@onur-ozkan onur-ozkan added T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. and removed T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 26, 2024
@onur-ozkan
Copy link
Member

I believe this is more related to the infra team. Bootstrap team don't have any access to CI environments.

@RalfJung
Copy link
Member Author

It's not about access to any environments, all files that need changing are in this repo. It's about adjusting our CI scripts to run these checks. No idea who's usually maintaining them...

@onur-ozkan
Copy link
Member

It's not about access to any environments, all files that need changing are in this repo. It's about adjusting our CI scripts to run these checks. No idea who's usually maintaining them...

Right 🤦 my bad.

@onur-ozkan onur-ozkan added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) and removed T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Feb 26, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Mar 2, 2024

@saethlin hm, this way of cross-checking does not seem to work for all hosts though...

$ ./x.py check library --target i686-pc-windows-gnu
Building bootstrap
    Finished dev [unoptimized] target(s) in 0.03s
thread 'main' panicked at src/core/sanity.rs:58:13:


couldn't find required command: "i686-w64-mingw32-gcc"


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Build completed unsuccessfully in 0:00:00

@onur-ozkan
Copy link
Member

I think we can bypass target sanity check with BOOTSTRAP_SKIP_TARGET_SANITY for x check. Something like BOOTSTRAP_SKIP_TARGET_SANITY=1 x check library --target=i686-pc-windows-gnu should work I believe.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 2, 2024

Yeah, I just thought that wouldn't be necessary any more. I guess I misunderstood.

@saethlin
Copy link
Member

saethlin commented Mar 2, 2024

The target sanity checks were added so that you don't fail late when bootstrap does cc detection internally.

We removed the cc detection and use of its results in check builds. So the sanity checks aren't doing anything useful for check builds. Maybe there's enough context available when they're done to skip them for check builds.

@onur-ozkan
Copy link
Member

So the sanity checks aren't doing anything useful for check builds.

Is this true even in build scripts?

@saethlin
Copy link
Member

saethlin commented Mar 2, 2024

Aren't build scripts compiled for the host?

@onur-ozkan
Copy link
Member

onur-ozkan commented Mar 2, 2024

Aren't build scripts compiled for the host?

We do this sanity check for host too and bootstrap should complain about it if they are missing. I guess we can optimize it by performing the sanity check only for the host if it's a check build.

@ChrisDenton
Copy link
Member

After testing some more, BOOTSTRAP_SKIP_TARGET_SANITY=1 works for most targets but not all. E.g. I'm not sure why but aarch64-apple-ios doesn't seem to honour BOOTSTRAP_SKIP_TARGET_SANITY.

I guess we can optimize it by performing the sanity check only for the host if it's a check build.

I didn't test this properly but I did hack out the sanity check and linker configuration which seemed to work for all targets (albeit with check errors on some, which is not unexpected).

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 9, 2024
…eck, r=albertlarsan68

skip sanity check for non-host targets in `check` builds

For `check` builds, since we only need to perform a sanity check on the host target, this patch skips target sanity checks on non-host targets.

For more context, see rust-lang#121519 (comment)

cc `@saethlin`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 9, 2024
…eck, r=albertlarsan68

skip sanity check for non-host targets in `check` builds

For `check` builds, since we only need to perform a sanity check on the host target, this patch skips target sanity checks on non-host targets.

For more context, see rust-lang#121519 (comment)

cc ``@saethlin``
jhpratt added a commit to jhpratt/rust that referenced this issue Mar 11, 2024
…eck, r=albertlarsan68

skip sanity check for non-host targets in `check` builds

For `check` builds, since we only need to perform a sanity check on the host target, this patch skips target sanity checks on non-host targets.

For more context, see rust-lang#121519 (comment)

cc `@saethlin`
jhpratt added a commit to jhpratt/rust that referenced this issue Mar 11, 2024
…eck, r=albertlarsan68

skip sanity check for non-host targets in `check` builds

For `check` builds, since we only need to perform a sanity check on the host target, this patch skips target sanity checks on non-host targets.

For more context, see rust-lang#121519 (comment)

cc ``@saethlin``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 11, 2024
Rollup merge of rust-lang#121907 - onur-ozkan:better-target-sanity-check, r=albertlarsan68

skip sanity check for non-host targets in `check` builds

For `check` builds, since we only need to perform a sanity check on the host target, this patch skips target sanity checks on non-host targets.

For more context, see rust-lang#121519 (comment)

cc ``@saethlin``
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Mar 12, 2024
…bertlarsan68

skip sanity check for non-host targets in `check` builds

For `check` builds, since we only need to perform a sanity check on the host target, this patch skips target sanity checks on non-host targets.

For more context, see rust-lang/rust#121519 (comment)

cc ``@saethlin``
@tgross35
Copy link
Contributor

In https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/PSA.3A.20.2E.2Fx.20check.20can.20work.20for.20any.20target, running checks for T2 and T3 targets was discussed. Where would be the place to do this?

#122401 used the main mingw-check, but it's probably not worth running T2/T3 on every PR.

Cc @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

No strong opinion. We should aim to minimize impact on overall build times, but otherwise just picking shortest builders feels reasonable. There's probably also a discussion to be had about cost/benefit -- increasing checks to some extent is a shift in our target tiering.

Tier 2 targets already largely get build-tested as part of the dist builders for them, so not sure there's sufficient value in additional checks in PR CI there. Tier 3 targets aren't, but that's sort of by intent; that tier specifically does not come with any CI costs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-feature-request Category: A feature request, i.e: not implemented / a PR. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

No branches or pull requests

7 participants