-
Notifications
You must be signed in to change notification settings - Fork 722
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
Support WASI #900
Support WASI #900
Conversation
This should (at the very least) add stuff to the CI to make sure that ring builds and the tests pass when run on WASI. |
I totally agree. I was actually just looking into doing this for both Wasm and WASI using Travis. |
NB: the newly added tests pass. CI appears to be failing because one of them is flaky. Update: that one works after a rebuild. Now a different one is randomly failing. |
@nhynes I'm not sure why the arm tests are acting up, but we also need to actually run the tests on WASI. It looks like your current CI changes just build the code but don't execute it. |
Good point. Turns out that wasm32 won't work without somewhat involved runtime support until the GFp functions are moved to not-assembly. |
@nhynes I reopened this because I also want to make forward progress on WASI support. It is true that some functionality won't be available yet in WebAssembly, but that should actually change soon. In the meantime, there is a lot of functionality that is available already: basically How are you yourself running (not just building) the tests in WASI? And which specific WASI environment are you targetting? (It looks like we don't have the wasm32-unknown-unknown tests running in CI in this repo now. I'll see what I can |
This is great news, thanks! I'm compiling to a standard wasi-unstable environment to be used in a custom (blockchain) WASI runtime. I was trying to use ECDSA which is why lld couldn't prune the offending symbols. Building the tests shows what's needed in not-asm: First, you'll need the wasi-sdk to build the ring C extensions. Then compile using cargo:
Finally, running wasm-nm shows the required functions that either need to be implemented in not-asm or provided by the runtime:
It goes without saying that getting the |
For running We use This lets us test the more conventional Obviously this is easier for us, as we have no C code, but if you can figure out how to build ring, you should be able to use something like our setup. |
Actually after applying mentioned changes by @josephlr tests still do not run due to dependency on C library which is required for ring to work.. It was mentioned above but I thought it is due to non standard setup: |
@nhynes @briansmith I still want to make it to WASI even with custom functions: at least it will allow to run TlsStream in a customized WASI runtime. Direct approach I am following so far is to compile test runner exporting C functions into WASI environment, so that tests compiled to wasm32-wasi target might call those functions via a wrapper over original I started building WASI runtime, though there are few complications:
I want to start discussion and get your opinion before spending more time on this, is it feasible approach? Is there any better way to work around those complexities? One of possible approaches would be to convert all crypto code to pure rust, though probably it won't provide required crypto warranties and is it too big effort for benefit? Another possible options might be for wasi runtime would just export custom top level cryptography functions so that TlsStream would use those functions to make it work - what would be candidates? Or probably WASI runtime should just export TlsStream Sink/Send... |
@@ -489,6 +490,16 @@ fn build_library( | |||
.and_then(|f| f.to_str()) | |||
.expect("No filename"), | |||
); | |||
|
|||
if &target.os == "wasi" { | |||
let ranlib_cmd = std::env::var("RANLIB"); |
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.
Please write a comment explaining why this chunk of code is here.
Wouldn't we need to output a cargo:rerun-if-env-changed=RANLIB
here?
@@ -87,6 +87,9 @@ | |||
#elif defined(__mips__) && defined(__LP64__) | |||
#define OPENSSL_64_BIT | |||
#define OPENSSL_MIPS64 | |||
#elif defined(__wasm32__) | |||
#define OPENSSL_32_BIT | |||
#define OPENSSL_X86_64 |
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.
It definitely isn't right to define OPENSSL_X86_64
for WASI.
We already have:
#elif defined(__wasm__)
#define OPENSSL_32_BIT
Is that not good enough? I think we can just revert this chunk.
@@ -102,6 +102,11 @@ else | |||
target_dir=target/$TARGET_X/debug | |||
fi | |||
|
|||
if [[ "$TARGET_X" =~ "wasm32-" ]]; then | |||
cargo build -vv -j2 ${mode-} ${FEATURES_X-} --target=$TARGET_X |
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.
I will leave a comment in the issue about the CI/CD.
|
||
#[inline] | ||
pub fn chunk(dest: &mut [u8]) -> Result<usize, error::Unspecified> { | ||
wasi::wasi_unstable::random_get(dest).map_err(|_| error::Unspecified)?; |
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.
It seems like, rather than adding that heavyweight dependency, we should just call __wasi_random_get
directly and avoid that crate.
My plan is to, soon, have C or Rust implementations of all features of ring, so that all ring features will work on WebAssembly, MIPS, etc. I understand that right now some people are working around the functions that ring is currently missing by substituting their own. This will break as we add the C/Rust versions of those functions to ring. Also, there are other changes in the pipeline that would break such an approach. And, such an approach has never been guaranteed to work; we frequently change the "ABI" of the internal functions of ring, so it's pretty unreasonable to plug in implementations from the outside. |
No description provided.