Frommi / miniz_oxide

Rust replacement for miniz
MIT License
174 stars 49 forks source link

Publish miniz_oxide on crates.io #10

Closed oyvindln closed 6 years ago

oyvindln commented 7 years ago

Now that the C API stuff is split off, we should prepare and publish the rust crate to crates.io. Possibly also the C API.

Frommi commented 6 years ago

Okay, last chance to painlessly change API. Do you think that everything is in order? Is there any sense in grouping functions like compress_to_vec into impl, for example? Or making compression level into enum?

Frommi commented 6 years ago

I thought about our APIs for a bit and think that this bunch of mz_*_oxide functions don't make much sense. When I wrote them, they were only meant to be as inner layer to extract all unsafe-y stuff of C API from logic and so it doesn't work as pub that good: there even no new method for StreamOxide without C mz_stream!

I think that this level of finesse won't be useful for anyone even if we cleaned it up, because there is always flate2. If you need to go lower than there are compress and decompress_oxide (need to make the names consistent). If higher -- compress_to_vec and decompress_to_vec.

So, I think we should make them private and move to miniz_oxide_c_api.

Frommi commented 6 years ago

Oh, we have two copies of same functions for some of mz_*_oxide: https://github.com/Frommi/miniz_oxide/blob/5980df341727c412cdd71a78f2ddbff99c0b9652/src/lib_oxide.rs#L120 https://github.com/Frommi/miniz_oxide/blob/5980df341727c412cdd71a78f2ddbff99c0b9652/miniz_oxide/src/lib_oxide.rs#L358

oyvindln commented 6 years ago

I propose we use rustdoc and look at the documentation to see if the current exported API is okayish and doesn't expose things it's shouldn't. I think it would be a good idea to have some sort of enum or builder struct for compression settings, but we could also leave that for later. The most important things to consider is stuff that could cause breaks. I think most potential API changes are going to be additions or renamings where the old naming can be kept around for compatibility anyhow, which won't cause breaking changes. I.e we need to make things consistent as suggested, and we cold probably get rid of the mz_/tdefl/tinfl/ and similar prefixes outside of non-mangled c exports as we don't have name collision issues in Rust like in C.

As for the mz_oxide stuff, having analogues to the functions that it uses from miniz/zlib might be useful to make integration simple, at least initially (granted maybe we could use some simple wrappers in flate2 instead.) If we don't move them yet we could also hide them from the documentation. I agree that they don't have much purpose outside of that and the C API though.

EDIT:

Oh, we have two copies of same functions for some of mz_*_oxide:

Ah lol, we should probably sort that too.

Frommi commented 6 years ago

So, I think that main crate can have like 4 functions: compress, decompress, compress_to_vec, decompress_to_vec. First two for handcrafting streaming if someone wants and quick and simple uses for latter two. We can even hide compress and decompress under something like core:: mod to dissuade someone casual from using it. And the second crate miniz_oxide_c_api (maybe rename to miniz_oxide and current miniz_oxide to miniz_oxide_core?) to have all the miniz C functions.

Frommi commented 6 years ago

I am more or less happy with rustdoc output now. I think it's ready for crates.io. I decided that those mz_oxide functions will be private: right now PR to flate2 we can do as a drop-in of miniz and in the near future we can add nice and tidy streaming interface with new and drop instead of init and end to purge unsafe's.

oyvindln commented 6 years ago

Yeah it's looking good, nice work.

oyvindln commented 6 years ago

I looked a bit at adding it as a back-end to flate2. As the API it looks like it would be simplest to use the miniz_c_api part as the interface for now since it replicates the miniz functions. We might want to have a feature that avoids exporting stuff as non-mangled C functions though to avoid potential symbol conflicts.

Frommi commented 6 years ago

Yes, just replacing miniz with miniz_oxide_c_api as I did in https://github.com/Frommi/flate2-rs. But instead of extern using use. Yes, and mangling it up can also replace objcopy magic that is done for benching and fuzzing right now.

oyvindln commented 6 years ago

There were some missing constants in the C_API, I'm working on re-adding them now.

Frommi commented 6 years ago

Okay, I am only publishing miniz_oxide without c_api for now.

oyvindln commented 6 years ago

I have a flate2-fork with the miniz_oxide_c_api backend here: https://github.com/oyvindln/flate2-rs/tree/rust-backend It seems to work fine, though at the moment it still builds the miniz_c files so we have to add some feature combination to avoid that.

Frommi commented 6 years ago

The cause of warning to link against native libs seems to be because we have staticlib in crate-type in Cargo.toml. It is needed so that C code could use miniz_oxide, like miniz_tester.cpp. But it is not needed for flate2 or other Rust projects. Sadly, it can't be conditioned on features as far as I know. The fuzzer had problems with that too, so I made hack-y sed remove and add of staticlib in run_fuzz.sh and build.sh. If we remove it by default everything should probably work.

oyvindln commented 6 years ago

It's possible to specify the crate type using "RUSTFLAGS="--crate-type type" when building if it's needed.

Frommi commented 6 years ago

Should we delete data for benches from the package? It bloats size a bit.

oyvindln commented 6 years ago

Yeah it's not used in the package anyhow.

Frommi commented 6 years ago

https://github.com/alexcrichton/flate2-rs/pull/128

oyvindln commented 6 years ago

It's been up on crates.io for a while now, so closing this.