-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Don't panic if WASI_SDK_PATH not set when detecting compiler #143752
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi, how does it help you if you return In any case, if it does help you, I'd appreciate printing a warning containing the |
If not specified, it falls back to the Also, Fedora uses |
I see. Then use the fallback to the default |
If <target>.wasi-root is set, don't panic. It may be the case the default compiler is sufficient or not needed. This is the case when packaging wasm targets on fedora today.
aca4980
to
b859fbd
Compare
Would it be ok to restrict panicking to a missing |
So if I understand it correctly, you are using There is already a function in bootstrap ( Line 1359 in 2fbc642
cc detection logic should also first be accessing the wasi root and only then the WASI_SDK_PATH - assuming that the wasi-root is actually useful for the cc detection (maybe that's not the case).
|
I even see that we already assert this (
|
Yes, I don't think |
.wasi_sdk_path | ||
.as_ref() | ||
.expect("WASI_SDK_PATH mut be configured for a -wasi target"); | ||
let root = build.wasi_sdk_path.as_ref(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the WASI SDK path is missing here, let's just print a warning like "WARNING: WASI_SDK_PATH is not configured, falling back to default C/C++ compiler". If we're on CI (build.config.is_running_on_ci
), then panic instead with the original error message.
The case where both the SDK and the root are missing should be handled elsewhere in bootstrap, not here, and it is already handled when copying compiler artifacts, so I think it should be fine.
The fedora packaging builds the wasm sysroot outside of the rust build system. Fedora applies a couple of patches related to wasm which I think make this possible. Not panicking seems consistent with the detection logic of other targets when they cannot find cc.
r? @Kobzol