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

Add an Eio backend #25

Merged
merged 4 commits into from
Apr 24, 2024
Merged

Add an Eio backend #25

merged 4 commits into from
Apr 24, 2024

Conversation

clecat
Copy link
Collaborator

@clecat clecat commented Apr 18, 2024

This PR aims to provide a backend for Eio in the same way it is currently done for Lwt: The same interface was used, with only minor changes in the code and was written by @patricoferris.

An optional dependency to Iomux was added as well for the Eio backend, as Eio itself does not (yet ?) provide a function to check if a file descriptor is readable: It was done here using Iomux.Poll.

Thank you for your time, Gwen

Copy link
Owner

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@whitequark
Copy link
Owner

Looks like the CI fails.

@clecat
Copy link
Collaborator Author

clecat commented Apr 19, 2024

My apologies, I tried to fix it however I ran into a more general problem with how dependencies are handled:

  • eio is a library made for ocaml-multicore, and therefore is only available on ocaml.5.x. Because the package inotify.eio is marked in dune as optional, this is not an issue in platforms running lower versions of ocaml: The package will simply not be built.
  • eio_main is a library only used in the tests, however they do not seem to allow for an optional tag and will always ask to be run. Which is why, as the testing platforms are set to 4.03.x, 4.08.x and 4.12.x, the CI fails.
  • I have tried marking eio_main as both a depopt & related to the tests, but it does not seem to have any impact. I have also tried other things to no avail.

The first and less friendly way to do things could be to only run the CI on with ocaml versions of 5.x, making sure that eio_main would be available, but I do not really like the idea of forcing such a decision for an optional package.

One of the more reasonable options that come to my mind, would be to dissociate from the way lwt was integrated in inotify, and instead of declaring inotify.eio as an optional library, another package inotify-eio with it's own opam file could be added. It would directly declare a strict dependency to ocaml.5.x and resolve much of the issues. A new CI running on 5.1.x could then be added to test the second package.

What sounds better to you ? Do you have any oppositions to such an idea ?
Cordially, Gwen

@whitequark
Copy link
Owner

No idea. I haven't used OCaml (somewhat regretfully) in years and I don't want to spend any more effort on this PR than is absolutely necessary.

@clecat
Copy link
Collaborator Author

clecat commented Apr 22, 2024

I see, thank you for your time, I'll go for what seems best and commit when it is ready

@clecat clecat force-pushed the eio branch 4 times, most recently from 8094802 to 7fb1ef2 Compare April 23, 2024 16:31
@clecat
Copy link
Collaborator Author

clecat commented Apr 24, 2024

This PR should be done, sorry for the noise, I fought for a bit with the github actions in order to make the CI run properly:

  • The work on eio now has a proper separate package
  • A new CI running on 5.1.x will run all the tests on both packages
  • Older CI's will only test the inotify package

Thank you for your time

@whitequark whitequark merged commit baf8922 into whitequark:master Apr 24, 2024
4 checks passed
@whitequark
Copy link
Owner

@clecat clecat mentioned this pull request Apr 25, 2024
@clecat
Copy link
Collaborator Author

clecat commented Apr 25, 2024

#26 should fix it, sorry

@akirak akirak mentioned this pull request Jul 24, 2024
@samoht
Copy link

samoht commented Jul 25, 2024

Any chance to make a release with the eio support?

@whitequark
Copy link
Owner

Yep, let me remember how to do it...

@samoht Would you like to become a collaborator in this repo? It's been a while since I actually used OCaml, and I trust you to do a good job.

@clecat
Copy link
Collaborator Author

clecat commented Jul 26, 2024

I can do it if we are ok with it

@whitequark
Copy link
Owner

If I'm handing somebody write access for a reasonably popular package I'd like it to be someone I know; I do know @samoht (I think we've been in the same OCaml circles for like a decade, at least?) but I don't know you. Nothing personal.

@clecat
Copy link
Collaborator Author

clecat commented Jul 26, 2024

Sorry, by that I meant if we are okay with the current state of the repository, I believe that I already have write access to the repository as a maintainer.

@whitequark
Copy link
Owner

Wow, my memory really is awful sometimes. I'm sorry, and you're right.

@clecat
Copy link
Collaborator Author

clecat commented Jul 26, 2024

No worries at all, would you prefer me to do it or @Samoth ? Both are obviously okay with me

@whitequark
Copy link
Owner

I'm fine either way. I don't remember most of the last 10 months because of a few unfortunate events that happened to me. I have now reconstructed what happened with this repository. Go ahead and make a release, please!

@clecat
Copy link
Collaborator Author

clecat commented Jul 26, 2024

Noted, I will do the release then. I hope everything will be okay for you, take care !

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

Successfully merging this pull request may close these issues.

4 participants