-
Notifications
You must be signed in to change notification settings - Fork 1
Build libpressio for libpressio-sys #1
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
base: master
Are you sure you want to change the base?
Conversation
|
@robertu94 Fantastic, thank you! |
|
I can either take over again, or you can complete the PR (whatever you prefer). For CI, we could use something similar to my setup for tthresh-rs: https://github.com/juntyr/tthresh-rs/tree/main/.github/workflows |
|
I just built a minimum viable LibPressio. This has a few known limitations:
|
I can add the CI part; just breaking up the work into separate commits :) I also would appreciate your thoughts on the scoping limitations and which ones are important to solve |
I'll quickly draft a minimal PR in numcodecs-rs that doesn't wrap anything yet but checks if we can build with this PR. That should give us info on if there are any problems we would have to fix. Only bundling core compressors makes sense. We can always add a README line saying that one can always build dynlibs for the other ones manually using normal libpressio. |
|
Currently I'm getting the following error on juntyr/numcodecs-rs#23 So presumably you updated libpressio to size_t in the meantime, and somehow we're not emitting the correct link path in the build script yet. |
|
@robertu94 I made some progress and got the latest ref of this PR to build locally and on CI. Next, I tried to build it for WASM with numcodecs-wasm-builder (checkout juntyr/numcodecs-rs#23) cargo run --bin numcodecs-wasm-builder -- --crate numcodecs-pressio --version 0.1.0 --codec IdentityCodec --output pressio.wasm --path MY-LOCAl-PATH/numcodecs-rs/codecs/pressioNow we get an error when building std_compat. I think it's because its build doesn't recognize WASI and enables |
|
The cmake output includes so my guess is that you require some newer C++ features that the wasi-sdk doesn't include. |
|
If there's no way around using boost, Pyodide has a working build for it at https://github.com/pyodide/pyodide/blob/main/packages/boost-cpp/meta.yaml, so we could try to build on that |
+ PERF: modified libpressio-sys/build.rs to ignore spurious rebuilds caused by generated files (e.g. pressio_version.h) that are always considered fresh + FIX: better handled u64 vs usize differences on MacOS where these are different types, by ensuring that the correct type (usize) is used instead + FIX: silence spurious warnings about non standard naming conventions in low-level bindgen generated rust code that calls libpressio + BUG: the mechanism that libpressio for dynamic module linkage does not work in static rust because the symbols are being stripped from the resulting binary. This needs to be resolved by using a explicit registration pattern
|
The problems are std::shared_mutex and std::variant. The former makes sense since threading on WASI is limited. It is also easier to work around as it is only used in 3 optional places we are not using right now. I could add a libstdcompat disable threading configuration option that could remove this dependency and then check for it in cmake for libpressio to disable the 4 files that need it. The latter, does not make sense and is much harder to fix. Do you know what libstdc++ or libc++ is being used, or can you provide me a command to inspect the sysroot so I can check myself? std::variant I believe is part of freestanding and should have been include in clang since clang/libc++4 so it should be compilable on WASI |
|
WASM is usually quite good at hiding its lack of threads until you try to spawn one, so even the std::shared_mutex should be fine. I think it's a case of an outdated libc++. Ok, it's weird ... When you run the numcodecs-wasm-builder, it spits out all the env variables it passes to the eventual cargo build. In it, I get When I then inspect /nix/store/ki1m25h4iqsqv1vgm874br77hwfz13wf-wasi-sysroot-25.0/include/wasm32-wasip1/c++/v1, it contains the files named |
|
Ok, I'll explore further then why it failed in the libstdcompat check, and try a fix there. Robert |
|
I wanted to provide an update on this. I've been working on a series of modifications to the libpressio cmake code to dynamically generate a "statics export" function to ensure that the linker does not eliminate the code used to register the compressors. I'll be pushing an update to robertu94/libpressio when that is completed. |
|
Thank you for the update! |
I'm opening this draft early so that I can get some help with building libpressio.
The main idea is to update this repo in preparation for publishing to crates.io. The most important step here is to ensure that libpressio can be built automatically by Rust. To start, we should aim for supporting a minimal built with a few statically linked compressors, once that works we can add Rust features corresponding to other compressors we want to support building.
Currently the build doesn't work yet since
@robertu94 Perhaps you could help with including std_compat in this build and configuring cmake for a minimal build?
Once we've got libpressio-sys working, I'd also like to make a quick pass over the higher-level wrapper.