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

WASM support #31

Merged
merged 8 commits into from
May 12, 2020
Merged

WASM support #31

merged 8 commits into from
May 12, 2020

Conversation

stoically
Copy link
Contributor

@stoically stoically commented May 6, 2020

This starts support for compiling to the wasm32-unknown-unknown webassembly target.

  • Find solution to handle sleeping
  • Handle User-Agent correctly with regards to CORS restrictions
  • Try to get encryption to work
  • Try to get tests working as is: Doesn't work because of reliance on either mockito (tcplistener) or filesystem
  • Try to get the tests that rely on filesystem working by inlining the json files
  • Add wasm target build to travis
  • cjson changes
  • libolm changes
  • olm-sys changes

To run the example in matrix_sdk/examples/wasm_send_message

  • Install emscripten and make it available on PATH
  • Adjust credentials and room id in src/lib.js
  • npm install
  • npm run serve
  • Visit http://localhost:8080
  • Should send an message to the room

@codecov-io
Copy link

codecov-io commented May 7, 2020

Codecov Report

Merging #31 into master will decrease coverage by 0.45%.
The diff coverage is 60.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
- Coverage   75.13%   74.67%   -0.46%     
==========================================
  Files          21       23       +2     
  Lines        3998     4040      +42     
==========================================
+ Hits         3004     3017      +13     
- Misses        994     1023      +29     
Impacted Files Coverage Δ
matrix_sdk/examples/wasm_command_bot/src/lib.rs 0.00% <0.00%> (ø)
matrix_sdk_base/src/models/message.rs 0.00% <ø> (ø)
matrix_sdk_base/src/state/mod.rs 91.66% <ø> (ø)
matrix_sdk_base/src/state/state_store.rs 93.70% <ø> (ø)
matrix_sdk_crypto/src/machine.rs 74.40% <ø> (ø)
matrix_sdk_crypto/src/memory_stores.rs 97.22% <ø> (ø)
matrix_sdk_crypto/src/olm.rs 94.80% <ø> (ø)
matrix_sdk_crypto/src/store/memorystore.rs 97.50% <ø> (ø)
matrix_sdk_crypto/src/store/sqlite.rs 75.84% <ø> (ø)
matrix_sdk_test_macros/src/lib.rs 0.00% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6c0d4e...1241fd7. Read the comment docs.

@poljar
Copy link
Contributor

poljar commented May 7, 2020

I renamed the types subproject now to matrix-sdk-common. The locks should go in there, and anything that's shared between the different crates to make wasm work.

@stoically
Copy link
Contributor Author

stoically commented May 7, 2020

While trying to get encryption to work I hit the roadblock that libolm requires the cstddef (c++'s stdlib). The wasm32-unknown-unknown target doesn't provide stdlib's, so that won't work.

I've tried to work around that by compiling just olm-sys with emcc in its build.rs, but that seems to not work either.

Another alternative would be to try to compile against the wasm32-unknown-emscripten target, but unfortunately only stdweb supports emscripten, while wasm-bindgen does not. It seems that the Rust ecosystem is moving away from stdweb (last commit to the repo over 6 months ago) and emscripten, so I'm not sure it makes sense to put in the work to support that - but I'm not sure either what an viable alternative could be.

@poljar
Copy link
Contributor

poljar commented May 7, 2020

While trying to get encryption to work I hit the roadblock that libolm requires the cstddef (c++'s stdlib). The wasm32-unknown-unknown target doesn't provide stdlib's, so that won't work.

I've tried to work around that by compiling just olm-sys with emcc in its build.rs, but that seems to not work either.

Another alternative would be to try to compile against the wasm32-unknown-emscripten target, but unfortunately only stdweb supports emscripten, while wasm-bindgen does not. It seems that the Rust ecosystem is moving away from stdweb (last commit to the repo over 6 months ago) and emscripten, so I'm not sure it makes sense to put in the work to support that - but I'm not sure either what an viable alternative could be.

That's a shame, perhaps asking at the wasm-bindgen repo or wasm-pack repo would give us some answers if this will ever be possible.

@stoically
Copy link
Contributor Author

Turns out I was wrong. It is possible to compile an archive with emscripten and use it with the wasm32-unknown-unknown target, yay.

@stoically stoically marked this pull request as ready for review May 11, 2020 11:26
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

So I played around with this for a bit and a couple of issues came up:

  • While futures-timer does compile under wasm, calling sleep panics.
  • Instant::now() from std panics when called.
  • The example never syncs so it won't send encrypted messages correctly.
  • It doesn't seem to be possible to use an event emitter implementation.

I fixed all of those in the wasm-upstream branch. Sleeping has been disabled for now, but we might want to use wasm-timer which uses the tokio timers on non-wasm targets.

The Instant usage has been replaced with the instant crate which is a drop in replacement. We might consider using wasm-timer instead of this since it also provides an Instant replacement.

The example has been modified to be similar to the command bot example. It now receives messages and responds if a message starts with !party.

I did not investigate what's up with the event emitter but I think that the huge compile time error meant that it's not possible to implement one because of the Send + Sync requirement this trait has. We'll possibly need a non Send + Sync version of it under wasm, as well as for the CryptoStore trait.

The only remaining issue is that we're depending on git repos again, but that's only required for encryption to work. I think we can remove the git dependencies and not support encryption for now.

If you're fine with this please merge my changes into your branch and remove the git deps.

@stoically stoically mentioned this pull request May 12, 2020
8 tasks
@stoically
Copy link
Contributor Author

stoically commented May 12, 2020

Thanks for the quick and thorough review. I've made the requested changes and a small fix regarding a missing continue. The (upstream) git dep for the futures-locks crate is required since it includes an refactoring to std futures.

I've opened an tracking ticket for the remaining issues here: #35

@poljar
Copy link
Contributor

poljar commented May 12, 2020

Ah ok, since futures-locks is only used for wasm I think I'm going to merge this as is. Btw that one commit message is a bit misleading, encryption does work under wasm if the deps are set to the git repos.

@stoically
Copy link
Contributor Author

Removed the misleading comment in the commit message.

Ah ok, since futures-locks is only used for wasm I think I'm going to merge this as is.

Sounds good to me, thanks!

@poljar poljar merged commit 1241fd7 into matrix-org:master May 12, 2020
@stoically stoically deleted the wasm branch May 12, 2020 16:32
alinradut pushed a commit to alinradut/olm that referenced this pull request May 26, 2020
Allows building an WASM-ready archive with emscripten.

This allows e.g. to compile to the `wasm32-unknown-unknown`
target with Rust.

Related matrix-rust-sdk PR:
matrix-org/matrix-rust-sdk#31

Signed-off-by: stoically <stoically@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants