search

yallop / ocaml-ctypes

Library for binding to C libraries using pure OCaml
MIT License
363 stars 95 forks source link

Deprecate `PosixTypes` #624

Open toots opened 4 years ago

toots commented 4 years ago

Hi,

Following a conversation in https://github.com/ocamllabs/ocaml-ctypes/pull/620 I have worked on a consolidated ocaml-posix module.

This module aims at giving a unified structure to all existing POSIX bindings, with support for both high-level consumer API as well as low-level ctypes APIs.

Also, these modules take advantage of the most recent dune support which, in particular, allows for powerful compilation configuration, including multi-layered builds and config tests. which makes it possible to write all these bindings without writing a single C file. The gist of this scaffolding is described here: https://medium.com/@romain.beauxis/advanced-c-binding-using-ocaml-ctypes-and-dune-cc3f4cbab302

The posix-types module included in ocaml-posix provides the exact same API as the PosixTypes, except for support for sigset_t.

Support for sigset_t is actually specified in POSIX in <signals.h> instead of <sys/types.h> and I would like to address it as part of a proper posix-signals.

Would you be okay to deprecate this module to be replaced by the new one? If dropping support for sigset_t is a problem, I could look at implementing posix-signals before that happen.

I have also submitted a similar request to ocaml-posix-types here: https://github.com/yallop/ocaml-posix-types/issues/5. I the maintainer agrees to it, this would lead to a single, unified modern implementation.

It is, of course, my intent to give all previous maintainer commit access to the new repository so the work can continue jointly there.

Let me know what you think! Romain

yallop commented 4 years ago

Would you be okay to deprecate this module to be replaced by the new one?

Yes, I'm fine with that. Would you like to submit a PR to deprecate it when the new package/module is available/released?

If dropping support for sigset_t is a problem, I could look at implementing posix-signals before that happen.

I'm not sure offhand where sigset_t is used (besides the README and examples), but it'd be good to ensure, if possible, that there aren't any uses in client code that would be broken by the change.

toots commented 4 years ago

Great, thanks!

I'll add at least a simple module with a sigset_t replacement. I would also like to write on documenting the APIs better.

Lastly, I need to coordinate with other modules, I might need to rename some of them I cannot convince some maintainers to switch to this one (https://github.com/mwweissmann/ocaml-posix-time/issues/1 for instance).

Nothing blocking, though, so I hope to be able to push a PR relatively soon.

Thanks again!

toots commented 4 years ago

Ok, I have added posix-signal, covering sigset_t even adding some functionalities for it. I'll wait a little bit to see if I get a response from mwweissmann/ocaml-posix-time#1 and will submit a PR. Thks!

avsm commented 4 years ago

It might make sense to maintain these in the ctypes repository itself as subpackages. We already have the burden of having to work with some posix types due to the tests and examples, and once bound, they should be fairly stable.

toots commented 4 years ago

I'm down for that as well. My current build system is all based on dune. Should I convert the rest of this repository to use dune as well?

avsm commented 4 years ago

@toots see #588 -- your review/fixes there would be much appreciated. In particular, a few test cases are failing than on the master branch, so finding the cause of those regressions would help that get merged.

toots commented 4 years ago

Excellent. I'm down!

toots commented 4 years ago

@avsm Sorry, one more question though: will releases of sub-packages be possible with this configuration? It would be nice to not e.g. tie up release of an important bug fix in one sub-package to a release of the biggest/whole set of packages..

toots commented 4 years ago

Ok, with the release of dune 2.5 we can now bump package's individual versions so I'll try to resume work on this front soon.

toots commented 4 years ago

@avsm I just had a look through your PR, it looks great. I hope we can fix the remaining tests and merge it soon!

I have to admit, however, that I am running a little short on spare developer time lately. I don't think that I can take over helping with this effort. Plus, I am still curious about why/how you would integrate posix-specific modules in a dune project that is multi-OSes, including OSes that do not support POSIX. Wouldn't that add a lot on unnecessary complexity?

I think that I'm gonna go ahead and submit a PR to deprecate here. We could perhaps revisit this one/when dune support is merged?