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

zstd-sys: impl wasm32-unknown-unknown support #139

Merged
merged 6 commits into from
Jan 28, 2022

Conversation

quininer
Copy link
Contributor

This PR adds a simple wasm shim to zstd-sys to support wasm32-unknown-unknown arch.

The legacy and dict_builder cannot be compiled on this shim because they depend on stdio.h, so I added a new feature cfg to turning off dict_builder code.

You can test it with wasmtime

> cd zstd-safe/zstd-sys/
> cargo build --target wasm32-unknown-unknown --no-default-features --example it_work
> wasmtime target/wasm32-unknown-unknown/debug/examples/it_work.wasm --invoke test_compress

@gyscos
Copy link
Owner

gyscos commented Jan 26, 2022

Hi, and thanks for the work!

I think without dictBuilder, the functions from zdict don't have a definition anymore, and should not be used.

This means that the bindings file needs to depend on this. In practice, the zstd.h file used to generate the bindings should not include zdict.h when the dict builder feature is not enabled.

Rather than adding yet another dimension to the selection of the bindings file, we could split this bindings into the main zstd part and the dict part, and include each file conditionally. I pushed something like that to the bindings_split branch if you want to have a look.
If it looks fine, I'll merge the split, and rebase this PR to not include the zdict bindings without the feature.

We may also want to forward this dict_builder feature from zstd and zstd-safe, and there too conditionally enable the relevant functions.

@quininer
Copy link
Contributor Author

Yes, it would be nice to split them. Thanks for your quick work.

zstd-safe/zstd-sys/build.rs Outdated Show resolved Hide resolved
zstd-safe/zstd-sys/build.rs Outdated Show resolved Hide resolved
zstd-safe/zstd-sys/build.rs Outdated Show resolved Hide resolved
zstd-safe/zstd-sys/build.rs Outdated Show resolved Hide resolved
@gyscos
Copy link
Owner

gyscos commented Jan 28, 2022

Looking good! One last thing: we'll want to add dict_builder features to the zstd-safe and zstd crates, and forward it downstream.

We'll also want to conditionally hide the relevant types in these crates depending on this feature.

Comment on lines 33 to 39
#[cfg(feature = "zdict_builder")]
{
let dict = zstd::dict::from_files(&files, size).unwrap();

let mut dict_reader: &[u8] = &dict;
io::copy(&mut dict_reader, &mut io::stdout()).unwrap();
let mut dict_reader: &[u8] = &dict;
io::copy(&mut dict_reader, &mut io::stdout()).unwrap();
}
Copy link
Owner

@gyscos gyscos Jan 28, 2022

Choose a reason for hiding this comment

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

You can specify that an example requires some features. In Cargo.toml:

[[example]]
name = "train"
required-features = ["zdict_builder"]

This way, in the example itself, you can just assume the feature is set.

@gyscos gyscos merged commit d6bfa32 into gyscos:master Jan 28, 2022
@gyscos
Copy link
Owner

gyscos commented Jan 28, 2022

Thanks again for the work!

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