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

lz4-sys: add libc shim for wasm targets #49

Merged
merged 6 commits into from
Sep 6, 2024

Conversation

james-rms
Copy link

@james-rms james-rms commented Sep 4, 2024

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 .

  • Adds C header files that are included from the parts of liblz4 that lz4-sys depends on. This includes stdlib.h, string.h and assert.h. The new experimental lz4file component of liblz4 depends also on stdio.h, but this library does not wrap that component.
  • Adds basic rust implementations only for the functions used within liblz4. These are malloc, free, memcmp, memmove, memcpy. One file also uses the assert macro- this patch defines the macro as a no-op.
  • exposes the C primitive types used in the lz4-sys API publically from that crate, and uses them from lz4-rs instead of depending on libc separately. This shouldn't have a real effect on what dependencies are built when, but helps consolidate the platform-specific logic only to lz4-sys.
  • Adds a check in CI to build and check for wasm32.

@pmarks
Copy link

pmarks commented Sep 4, 2024

Very cool, thanks! Is there an easy way to also run the unit tests in CI?

@pmarks
Copy link

pmarks commented Sep 4, 2024

Also the wasm build doesn't seem to work on MacOS.

@james-rms
Copy link
Author

@pmarks I have it working on my local mac, but the Apple-vendored clang binary doesn't build wasm32 out of the box, you need to install llvm separately. Will try to get this working on the CI runner.

Running tests is a good idea, i'll try to figure out how.

@james-rms
Copy link
Author

@pmarks the steps to get tests working in WASM are (roughly following this guide):

  • install wasm-pack and node on the CI runner
  • write new tests specifically for wasm and wrap them with #[wasm-bindgen-test]. These maybe don't need to be exhaustive, just enough to exercise the APIs.
  • run them in CI with wasm-pack test

IMO this is overkill, but i'm willing to give it a go if you think it's neccessary.

@pmarks
Copy link

pmarks commented Sep 5, 2024

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.

@james-rms
Copy link
Author

@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.

@james-rms james-rms force-pushed the jrms/ape-zstd branch 4 times, most recently from 059e2c0 to 3725ab2 Compare September 6, 2024 02:33
@james-rms
Copy link
Author

@pmarks OK, macOS CI should be green.

@pmarks pmarks merged commit eec50ee into 10XGenomics:master Sep 6, 2024
3 checks passed
@pmarks
Copy link

pmarks commented Sep 6, 2024

@james-rms Thanks!!

@james-rms
Copy link
Author

@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.

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.

2 participants