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

Wasm: remove libc dependency #26

Closed
ctjhoa opened this Issue Jun 19, 2018 · 14 comments

Comments

Projects
None yet
5 participants
@ctjhoa
Contributor

ctjhoa commented Jun 19, 2018

Hi,

Currently, miniz_oxide is not buildable with wasm32-unknown-unknown as target.
I would like to help to remove libc to be able to use flate2 inside a browser.
Unfortunately I have no knowledge in miniz/zlib api.

Do you think it's doable? Can you give me guidance to do it?
From what I understand it's highly related to #3 .

Thanks.

@Frommi

This comment has been minimized.

Owner

Frommi commented Jun 19, 2018

Hi. There are miniz_oxide and miniz_oxide_c_api packages. It would be good to eliminate libc dependency for the first one and not for the second. I think it is doable and was already mostly done, actually. Most of c stuff like zlib API and default c allocation was moved to miniz_oxide_c_api.

The relevant subdirectory is this: https://github.com/Frommi/miniz_oxide/tree/master/miniz_oxide.
There are some remnants of libc in miniz_oxide. Regrettably, API depends on it, e.g. here: https://docs.rs/miniz_oxide/0.1.2/miniz_oxide/deflate/core/struct.CallbackFunc.html. It would be good to purge that once and for all, probably with a bump to 0.2.

@oyvindln

This comment has been minimized.

Collaborator

oyvindln commented Jun 22, 2018

libc has been used for C interoperability and memory allocation.
Interfacing with c is only for the miniz_oxide_c_api crate, so we shouldn't need it in the core crate. I'm not sure how to best provide a callback function though, maybe we can change the API to take a rust function pointer, and for the c api supply a wrapper function that calls the requested c function.
As for memory allocation, we've already dropped the custom allocation functionality that miniz has for now, so replacing that should be fine as long as we make sure to avoid large stack copies.

Doing the same for flate2 may be a bit more involved, currently it interfaces miniz_oxide with a C api which mimics the ones for miniz and zlib, so avoiding that would require some larger changes to flate2 internals. I do think it would be a good idea though to avoid unsafetyness that comes with C code. Maybe it would even make sense to split the C api and the flate2 brigding code.

@ctjhoa

This comment has been minimized.

Contributor

ctjhoa commented Jul 3, 2018

@oyvindln Thanks for this detailed explanation. I will try to help the best I could :)

@myfreeweb

This comment has been minimized.

myfreeweb commented Jul 5, 2018

maybe we can change the API to take a rust function pointer, and for the c api supply a wrapper function that calls the requested c function

Just doing s/c_void/() and s/c_int/i32 in lib.rs and deflate/core.rs (in addition to #27) makes it compile. So,

pub type PutBufFuncPtrNotNull = unsafe extern "C" fn(*const (), i32, *mut ()) -> bool;

does compile just fine with wasm32-unknown-unknown. Not sure how good this is for the C side though...

@oyvindln

This comment has been minimized.

Collaborator

oyvindln commented Jul 13, 2018

libc defines c_int as i32 and *c_void as a pointer to a u8 on all platforms outside of wasm, so that may be safe. Just in case though I think it's probably safest to redefine it only on wasm conditionally for now. A proper fix later should be to alter the rust API to use a safe rust callback.

@oyvindln

This comment has been minimized.

Collaborator

oyvindln commented Jul 13, 2018

I changed the function signature on wasm32 (similar to how libc defines nothing on the target) in the latest commit, and changed MZFlush and TDEFFLush new functions to take an i32. It compiles to the wasm32-unknown-unknown target now. I haven't done any tests with it though as cargo test isn't implemented for wasm32 yet as far as I know.

@Frommi

This comment has been minimized.

Owner

Frommi commented Jul 16, 2018

In this commit I replaced callback function pointer by closure and removed libc as the dependency for miniz_oxide. Thanks everyone!

There are breaking changes, so before version bump there is an opportunity to introduce other breaking changes, so there will be some time before this hits a crate release.

@Frommi Frommi closed this Jul 16, 2018

@eminence

This comment has been minimized.

Contributor

eminence commented Jul 16, 2018

It looks like there was a pretty big history re-write/rebase (force push) with your latest change. The entire history was re-written. For example: 81fa450 became 828fbef.

Just checking to see if this was intended and deliberate.

@Frommi

This comment has been minimized.

Owner

Frommi commented Jul 16, 2018

No, it is a mistake. Thanks for pointing out. I intended to rewrite only last commit. I made it from other computer and wanted to set author to this acount. Any idea how to revert?

@eminence

This comment has been minimized.

Contributor

eminence commented Jul 16, 2018

I would reset your master branch to 81fa450 (this is what the master branch pointed to before your latest commit) and then cherry-pick 62489b7 on top of that. And then force-push to github.

@Frommi

This comment has been minimized.

Owner

Frommi commented Jul 16, 2018

I think I restored old history. Thanks again.

@eminence

This comment has been minimized.

Contributor

eminence commented Jul 16, 2018

Look good to me!

@eminence

This comment has been minimized.

Contributor

eminence commented Jul 16, 2018

By the way, with only a modest amount of hacking, I was able to use this latest code (via the zip-rs crate) to decode zip files using the wasm32-unknown-unknown target in a browser. Pretty cool!

@ctjhoa

This comment has been minimized.

Contributor

ctjhoa commented Jul 17, 2018

@eminence Can you share your hacks on this? Maybe a github repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment