-
Notifications
You must be signed in to change notification settings - Fork 30
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
lz4-sys: add libc shim for wasm targets #49
Conversation
9b6fc9f
to
6ecc19b
Compare
Very cool, thanks! Is there an easy way to also run the unit tests in CI? |
Also the wasm build doesn't seem to work on MacOS. |
@pmarks I have it working on my local mac, but the Apple-vendored Running tests is a good idea, i'll try to figure out how. |
@pmarks the steps to get tests working in WASM are (roughly following this guide):
IMO this is overkill, but i'm willing to give it a go if you think it's neccessary. |
I'm ok merging without having the unit tests in CI. Seems a bit risky that we could somehow break WASM if we're not running the tests since it's the only 32bit target so you can have bugs from e.g. assuming usize is 64 bits. Doesn't seem too bad to run the tests? Node should already be installed - maybe we can do it in a follow up PR. Also I'm OK disabling the WASM build on Mac if it's a pain to get working. |
@pmarks i think it's not a pain, but it's hard to iterate on when every CI run requires approval. I'll see if I can get CI running in my fork first. |
059e2c0
to
3725ab2
Compare
1806445
to
d0e32c3
Compare
@pmarks OK, macOS CI should be green. |
@james-rms Thanks!! |
@pmarks Thanks for the review! I'd appreciate it if you drop an @ here when you get around to cutting a release with this commit. |
This crate does not build for WASM targets currently. This patch is intended to get builds for
wasm32-unknown-unknown
working. It follows a similar approach to that used in https://github.com/gyscos/zstd-rs .liblz4
thatlz4-sys
depends on. This includesstdlib.h
,string.h
andassert.h
. The new experimentallz4file
component of liblz4 depends also onstdio.h
, but this library does not wrap that component.liblz4
. These aremalloc
,free
,memcmp
,memmove
,memcpy
. One file also uses theassert
macro- this patch defines the macro as a no-op.lz4-sys
API publically from that crate, and uses them fromlz4-rs
instead of depending onlibc
separately. This shouldn't have a real effect on what dependencies are built when, but helps consolidate the platform-specific logic only tolz4-sys
.