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

OCaml 5.00 compatibility #923

Closed
patricoferris opened this issue Jan 23, 2022 · 7 comments
Closed

OCaml 5.00 compatibility #923

patricoferris opened this issue Jan 23, 2022 · 7 comments

Comments

@patricoferris
Copy link

patricoferris commented Jan 23, 2022

OCaml 5.00 (at the moment trunk) has removed lots of functions that were deprecated including String.uppercase that is used in the unix library's discover executable.

let uppercase = String.uppercase [@ocaml.warning "-3"]

This needs fixed so lwt can be installed with OCaml 5.00. I have a fix here which aimed to keep things working on 4.02 (I think uppercase_ascii was added in 4.03). By the looks of it, lwt supports 4.02 but isn't tested against it so maybe there's a plan to drop support, in which case uppercase_ascii would probably suffice?

EDIT: I just noticed there are tests for 4.02 and they fail for my "fix" because an older version of dune is used. It probably needs something like https://github.com/janestreet/result? At any rate the issue still stands :))

@EduardoRFS
Copy link
Contributor

There is also the problem of Unix.fork on Lwt_process, which I guess could be replaced by create_process.

@raphael-proust
Copy link
Collaborator

@EduardoRFS Is the issue that we can't use Unix.fork at all when we have domains spawned? How is create_process handled under the hood in 5.00?

@EduardoRFS
Copy link
Contributor

EduardoRFS commented Feb 4, 2022

@raphael-proust yes that's the issue. Unix.create_process uses spawn which is implemented in C directly https://github.com/ocaml/ocaml/blob/trunk/otherlibs/unix/unix.ml#L894

But that doesn't work as Lwt_process actually cannot be described on create_process because there is no way to close fd's on the child process, so this line cannot be implemented. https://github.com/ocsigen/lwt/blob/master/src/unix/lwt_process.ml#L159

And this changes behavior considerably, so likely lwt will need to implement it's own spawn in C. Like https://github.com/ocaml/ocaml/blob/trunk/otherlibs/unix/spawn.c#L31

@patricoferris
Copy link
Author

patricoferris commented Feb 11, 2022

If I understand correctly, I think something needs done with the bigarray library in lwt.unix too. The ocamlfind package is gone from 5.00 so on a 5.00.0+trunk switch it would seem lwt.unix will be hidden because it is optional and has unmet dependencies.

@raphael-proust
Copy link
Collaborator

Lwt is now OCaml.5-compatible. Closing.

@EduardoRFS
Copy link
Contributor

It doesn't seems to have solved my problem at least from looking at code.

Lwt_process still calls Lwt_unix.fork

match Lwt_unix.fork () with

Which calls Unix.fork https://github.com/ocsigen/lwt/blob/master/src/unix/lwt_unix.cppo.ml#L2307

Which then will fail at
https://github.com/ocaml/ocaml/blob/trunk/otherlibs/unix/fork.c#L41

@raphael-proust
Copy link
Collaborator

@EduardoRFS I have opened a separate issue to track this problem.

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