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

Deprecate PosixTypes #624

Open
toots opened this issue Jan 5, 2020 · 10 comments
Open

Deprecate PosixTypes #624

toots opened this issue Jan 5, 2020 · 10 comments

Comments

@toots
Copy link

toots commented Jan 5, 2020

Hi,

Following a conversation in #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: yallop/ocaml-posix-types#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

@toots toots changed the title Deprecate PosixType Deprecate PosixTypes Jan 5, 2020
@yallop
Copy link
Owner

yallop commented Jan 6, 2020

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
Copy link
Author

toots commented Jan 6, 2020

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 (mwweissmann/ocaml-posix-time#1 for instance).

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

Thanks again!

@toots
Copy link
Author

toots commented Feb 4, 2020

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
Copy link
Contributor

avsm commented Mar 18, 2020

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
Copy link
Author

toots commented Mar 19, 2020

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
Copy link
Contributor

avsm commented Mar 20, 2020

@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
Copy link
Author

toots commented Mar 20, 2020

Excellent. I'm down!

@toots
Copy link
Author

toots commented Mar 21, 2020

@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
Copy link
Author

toots commented Apr 12, 2020

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
Copy link
Author

toots commented Apr 18, 2020

@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?

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

3 participants