-
Notifications
You must be signed in to change notification settings - Fork 119
Support compiling to wasm32-wasi target #113
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
Conversation
@@ -61,7 +62,13 @@ impl Build { | |||
{ | |||
Command::new("gmake") | |||
} else { | |||
Command::new("make") | |||
if target.contains("wasm") { |
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.
You can wrap this in a single else if statement
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.
Thanks! Can you add a builder to CI so we can track whether it continues to work over time or not?
if target.contains("wasm") { | ||
let mut cmd = Command::new("wasimake"); | ||
|
||
std::mem::replace(cmd.arg("make"), Command::new("wasimake")) |
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.
What is wasimake
and wasiconfigure
? I don't think these are provided by OpenSSL and otherwise I don't want to rely on too many external programs. (I've also never heard of these myself)
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.
What is
wasimake
andwasiconfigure
? I don't think these are provided by OpenSSL and otherwise I don't want to rely on too many external programs. (I've also never heard of these myself)
Yes these commands are included in the wasienv
toolchain https://docs.wasmer.io/ecosystem/wasienv and right now i only found this way to compile openssl for wasm.
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.
Can this patch be written without requiring those tools? Without knowing what they do it seems like they could introduce hidden dependencies we aren't aware of.
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.
Can this patch be written without requiring those tools? Without knowing what they do it seems like they could introduce hidden dependencies we aren't aware of.
I dont think so, but this only requires the user to install the wasienv toolchain.
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.
Personally I do not want to land this with a dependency on the wasienv toolchain. I don't know why it's necessary as opposed to the standard wasi-sdk.
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.
Personally I do not want to land this with a dependency on the wasienv toolchain. I don't know why it's necessary as opposed to the standard wasi-sdk.
I tried compiling using the standard wasi-sdk but it didnt work no matter what i tried, this seems to be the proper way to configure the build for wasi as far as i know.
@@ -155,7 +169,7 @@ impl Build { | |||
// Avoid multilib-postfix for build targets that specify it | |||
.arg("--libdir=lib"); | |||
|
|||
if cfg!(not(feature = "legacy")) { | |||
if cfg!(not(feature = "legacy")) && !target.contains("wasm") { |
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.
Why is this condition here added for wasm?
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.
Why is this condition here added for wasm?
Ah my bad, i accidentally left that condition there.
This condition is required for building OpenSSL 1.1.1d
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.
No this repository only has one version of OpenSSL via a submodule, which is currently 3+
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.
No this repository only has one version of OpenSSL via a submodule, which is currently 3+
Yes this repo only contains openssl 3, but right now only OpenSSL 1.1.1d is supported with specific patches for wasm32-wasi, see #113 (comment)
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 that's the case then I don't think this can land right now.
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 that's the case then I don't think this can land right now.
I will try patching the latest OpenSSL 3 and see if this works with that or not.
configure.arg("-DSIG_IGN=0"); | ||
configure.arg("-DHAVE_FORK=0"); | ||
configure.arg("-DOPENSSL_NO_AFALGENG=1"); | ||
configure.arg("--with-rand-seed=getrandom"); |
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.
Can you add some comments for what's going on here for wasm?
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.
Can you add some comments for what's going on here for wasm?
Sure
OpenSSL 3 isnt compatible with wasm32-wasi target by default, in order to compile it you need to apply patches and i only found patches for OpenSSL 1.1.1d and compiled it, but the openssl-src-rs repo always fetches the latest openssl, is there any way to fetch only OpenSSL 1 for the builder? |
Closed because of #119 |
Adds support for compiling to wasm32-wasi target
Changes
wasiconfigure
andwasimake
when targeting wasmIssues