Skip to content

Update how WASI toolchains are used in CI and bootstrap #123978

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

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

alexcrichton
Copy link
Member

This commit updates how the WASI targets are configured with their toolchain. Long ago a config.toml option of wasi-root was added to enable building with the WASI files produced by wasi-libc. Additionally for CI testing and release building the Rust toolchain has been using a hard-coded commit of wasi-libc which is bundled with the release of the wasm32-wasip1 target, for example.

Nowadays though the wasi-sdk project, the C/C++ toolchain for WASI, is the go-to solution for compiling/linking WASI code and contains the more-or-less official releases of wasi-libc. This commit migrates CI to using wasi-sdk releases and additionally updates bootstrap to recognize when this is configured. This means that with $WASI_SDK_PATH configured there's no further configuration necessary to get a working build. Notably this also works better for the new targets of WASI as well, such as wasm32-wasip2 and wasm32-wasip1-threads where the wasi-sdk release now has libraries for all targets bundled within it.

@rustbot
Copy link
Collaborator

rustbot commented Apr 15, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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 A-testsuite Area: The testsuite used to check the correctness of rustc 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Apr 15, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 15, 2024

Some changes occurred in src/doc/rustc/src/platform-support

cc @Nilstrieb

This commit updates how the WASI targets are configured with their
toolchain. Long ago a `config.toml` option of `wasi-root` was added to
enable building with the WASI files produced by wasi-libc. Additionally
for CI testing and release building the Rust toolchain has been using a
hard-coded commit of wasi-libc which is bundled with the release of the
`wasm32-wasip1` target, for example.

Nowadays though the wasi-sdk project, the C/C++ toolchain for WASI, is
the go-to solution for compiling/linking WASI code and contains the
more-or-less official releases of wasi-libc. This commit migrates CI to
using wasi-sdk releases and additionally updates `bootstrap` to
recognize when this is configured. This means that with `$WASI_SDK_PATH`
configured there's no further configuration necessary to get a working
build. Notably this also works better for the new targets of WASI as
well, such as `wasm32-wasip2` and `wasm32-wasip1-threads` where the
wasi-sdk release now has libraries for all targets bundled within it.
@alexcrichton alexcrichton force-pushed the update-wasi-toolchain branch from cfba7ed to 8bb9d30 Compare April 15, 2024 21:27
let root = PathBuf::from(std::env::var_os("WASI_SDK_PATH")?);
let compiler = match compiler {
Language::C => format!("{t}-clang"),
Language::CPlusPlus => format!("{t}-clang++"),
Copy link
Member

Choose a reason for hiding this comment

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

Out of interest, why is clang/clang++ prefixed with the target? That seems like an odd choice for the usually cross-compiling nature of clang/LLVM?

(Obviously not blocking here since it's not something we can change on the Rust side :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not necessarily required but it helps configure clang with the --target value. Clang has a feature that a target-prefixed executable doesn't need --target passed, and the wasi-sdk sysroot has a bunch of symlinks for wasi targets that all point at a single clang executable, so it's all the same executable just a name-based-dispatch executable.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I didn't know about that feature. Sounds good.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Apr 16, 2024

📌 Commit 8bb9d30 has been approved by Mark-Simulacrum

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 Apr 16, 2024
@cuviper
Copy link
Member

cuviper commented Apr 16, 2024

I just want to note that for Fedora and RHEL, we don't have the full wasi-sdk available, so we still need wasi-root to work (and our general LLVM toolchain has WebAssembly support enabled). AFAICT that still looks ok in this PR, so I'm saying this more for any future changes you might be considering. If that's going to be a problem, let's talk!

@alexcrichton
Copy link
Member Author

Thanks for the heads up, good to know! The one caveat with this PR is that the lookup within wasi-root is changing to use the raw wasi target's name directly. For example .join(target.to_string().replace("-preview1", "").replace("p2", "").replace("p1", "") was removed. That means that the wasi-root configured will need to have a target-named directory with the contents inside.

Will that cause problems though? If so I can rename wasi-root to wasi-libdir and avoid having any .join(..) logic entirely within bootstrap.

@cuviper
Copy link
Member

cuviper commented Apr 16, 2024

Yeah, a direct wasi-libdir config sounds cleaner and would be more convenient for us too, so we can continue sharing the same wasi-libc build for now.

@alexcrichton
Copy link
Member Author

Ok cool, I'll add that as an option on top of wasi-root which can be specified.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2024
…r=Mark-Simulacrum

Update how WASI toolchains are used in CI and bootstrap

This commit updates how the WASI targets are configured with their toolchain. Long ago a `config.toml` option of `wasi-root` was added to enable building with the WASI files produced by wasi-libc. Additionally for CI testing and release building the Rust toolchain has been using a hard-coded commit of wasi-libc which is bundled with the release of the `wasm32-wasip1` target, for example.

Nowadays though the wasi-sdk project, the C/C++ toolchain for WASI, is the go-to solution for compiling/linking WASI code and contains the more-or-less official releases of wasi-libc. This commit migrates CI to using wasi-sdk releases and additionally updates `bootstrap` to recognize when this is configured. This means that with `$WASI_SDK_PATH` configured there's no further configuration necessary to get a working build. Notably this also works better for the new targets of WASI as well, such as `wasm32-wasip2` and `wasm32-wasip1-threads` where the wasi-sdk release now has libraries for all targets bundled within it.
@bors
Copy link
Collaborator

bors commented Apr 17, 2024

⌛ Testing commit 8bb9d30 with merge 5a1441c...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@alexcrichton
Copy link
Member Author

Looking over the logs there it seems like the job was cancelled although I am not certain of this as I know I've seen failured on GitHub Actions look like cancellations in the past. Is there perhaps a better way to figure out what failed? Or failing that, could this be queued for a retry?

@cuviper
Copy link
Member

cuviper commented Apr 17, 2024

As long as it's not a timeout, which this doesn't appear to be, then I think we can just retry.

@bors retry

@bors
Copy link
Collaborator

bors commented Apr 17, 2024

⌛ Testing commit 8bb9d30 with merge becebb3...

@bors
Copy link
Collaborator

bors commented Apr 17, 2024

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing becebb3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 17, 2024
@bors bors merged commit becebb3 into rust-lang:master Apr 17, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 17, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (becebb3): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.8% [3.8%, 3.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.8% [3.8%, 3.8%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 677.262s -> 677.135s (-0.02%)
Artifact size: 316.13 MiB -> 316.09 MiB (-0.01%)

@alexcrichton alexcrichton deleted the update-wasi-toolchain branch May 3, 2024 20:32
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 merged-by-bors This PR was explicitly merged by bors. 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants