Skip to content

implement remaining public functions #49

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

Open
39 of 44 tasks
folkertdev opened this issue Feb 22, 2024 · 11 comments
Open
39 of 44 tasks

implement remaining public functions #49

folkertdev opened this issue Feb 22, 2024 · 11 comments

Comments

@folkertdev
Copy link
Collaborator

folkertdev commented Feb 22, 2024

these are missing versus zlib

deflate

inflate

general

gz

@brian-pane
Copy link

I'm interested in working on the gz functions, if someone can give me a few hints on where to put them. Should type declarations (like gzFile and gzFile_s) and functions (like gzopen and gzclose) live in zlib-rs, or lib-rs-sys, or somewhere else?

@folkertdev
Copy link
Collaborator Author

Thanks for your interest!

organization

I think these api's will just be for C consumption, so code that is only used by them should live just in libz-rs-sys. You could add libz-rs-sys/src/gz.rs and put all of the relevant types/functions/docs in there. Then in libz-rs-sys/src/lib.rs, use mod gz; pub use gz::*; to export everything. In test-libz-rs-sys I'd similarly like to see a separate file with the tests for these apis.

We currently don't have libc as a dependency. I'd like to keep it that way when building as a rust crate, so the gz functionality should live behind a feature flag that only conditionally enables libc. So I think that needs something like this:

[features]
gz = ["dep:libc"]

[dependencies]
libc = { ..., optional = true }

We'd enable that flag when building a cdylib, but disable it by default when using libz-rs-sys from rust.

method & priorities

Our idea (but the funding has so far not materialized for us to look at this ourselves) was to use c2rust for this part of the api, and then clean that up but not spend too much time ttrying to make it idiomatic rust (e.g. by trying to use std::fs::File). see https://trifectatech.org/blog/translating-bzip2-with-c2rust/ for our experiences with using that approach for bzip2.

So, idk if you want to go that route or try something more manual (up to you), but our priorities here are 1) compatibility and 2) decent rust code. Given that the source material uses libc functions a bunch, it likely won't be possible to just use std and have equivalent behavior. That's fine: our unidiomatic rust code is no worse than the existing C code.

final thoughs

We merged a PR yesterday that means all extern functions are now annotated with

#[cfg_attr(feature = "export-symbols", export_name = prefix!(<function name>))]

Finally, we have limited review capacity at the moment (again, funding etc.), so small PRs are much easier for us to handle than bigger ones.

@brian-pane
Copy link

Should libz-rs-sys also provide deflateInit2? While working on the gzwrite port just now, I found that deflateInit2_ is implemented but deflateInit isn't.

@folkertdev
Copy link
Collaborator Author

Those are not really symbols, but defines in the header file, e.g.

https://github.com/zlib-ng/zlib-ng/blob/46fc33f39d0de9b85330c275b26391645a04ffa5/zlib.h.in#L1767

So if you look at the object file, deflateInit is not in there, just deflateInit_. I suppose for rust users we could add it but never export it as a C symbol (and then also just use the rust calling convention)

@brian-pane
Copy link

The zlib.h provided by zlib-ng declares a few additional public gz functions that I see being exported in libz.a. I'll plan on porting these as well, unless any of them are no longer needed:

@folkertdev
Copy link
Collaborator Author

After thinking about this some more, and asking for feedback, here's my plan for actually releasing the gzip functions:

  • for now, we move the gz.rs file into libz-rs-sys-cdylib: the functions are probably only relevant for users of the cdylib anyway
  • in libz-rs-sys-cdylib, we can add a flag that adds gzprintf (that will require a nightly compiler for the time being, after some conversations at rustweek, I'll look into moving c varargs forward in the compiler itself)
  • we can then get some production feedback on these functions, improve documentation, testing, etc.
  • when we're confident in the quality of the implementation, we can move the functions back into libz-rs-sys and re-export them from libz-rs-sys-cdylib

I think this minimizes risk, unblocks some other processes (releases, security audits)

@brian-pane
Copy link

I attempted to make a patch that moves gz.rs into libz-rs-sys-cdylib, but I couldn't figure out the Cargo configuration to make the test dependencies work for the cdylib.

@folkertdev
Copy link
Collaborator Author

hmm, anything in particular that is problematic? (you can open a draft PR if that's easier, though currently no tests are run on CI for the cdylib crate)

@folkertdev
Copy link
Collaborator Author

It turns out that this cannot work, because a cdylib crate cannot be used like a standard rust module. It can be a dependency, but you just can't import the symbols. Sorry about that.

We'd like to release soon, so here are the steps that I plan to take:

  • explicitly document the gz feature flag as unstable
  • fix the codecov issue (probably by just running some of the tests again)
  • hold off on gzprintf for this release, add it afterwards with its own feature flag

@brian-pane
Copy link

I was able to work around that in my dev environment by having libz-rs-sys-cdylib generate both a cdylib and a lib. I'm not sure whether that will break anything else, though.

@folkertdev
Copy link
Collaborator Author

really? can you push that to the PR?

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

No branches or pull requests

2 participants