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

[RFC]: Convert to direct-style with Eio #2149

Draft
wants to merge 97 commits into
base: main
Choose a base branch
from

Conversation

patricoferris
Copy link
Contributor

@patricoferris patricoferris commented Nov 29, 2022

What's ported?

This draft PR ports parts of Irmin to https://github.com/ocaml-multicore/eio. This includes irmin, irmin-fs, irmin-pack, irmin-containers and irmin-chunk along with the tests. Most of the other libraries are not yet ported because of the effort to try and port the dependencies in particular graphql, ocaml-git and webmachine (although I think irmin-http might be on its way out).

I'm opening this draft PR to let people know of the existence of this work (in case they want to do some hacking ;)) and to also let people comment on it. It might be interesting to see if there are impacts on the benchmarks.

Some issues

One problem that has come up a few times, is Irmin's use of top-level values (even if lazy). In the Eio world this becomes a little tricky because they often need either access to some capability or a Eio.Switch.t. There is a port of irmin-watcher for example that requires the installation of a handler in order to be able to get a switch to the hooks. irmin-fs also requires something similar so that users would have to do this (see the fs unix tests):

let () =
  Eio_main.run @@ fun env ->
  Irmin_fs.run env#fs @@ fun () ->
  Irmin_watcher.run @@ fun () ->
  Irmin_test.Store.run "irmin-fs.unix" ~slow:false ~sleep:Eio_unix.sleep
       ~misc:[]
       [ (`Quick, Test_fs_unix.suite) ]

Which I'm definitely not a fan of! As pointed out in ocaml-multicore/eio#364 (comment) fiber-local storage could be used but comes with it's own problems. The Irmin_fs handler was more about getting the fs capability to the repo constructor that can only take a configuration because I wasn't sure how to do that otherwise. The only way to get a capability is if it is passed to you from Eio's standard environment.

Porting to Eio already is a massive breaking change, so perhaps some of this could be reworked to avoid all of this :)) Would love to know your thoughts?

@art-w
Copy link
Contributor

art-w commented Jun 29, 2023

Thanks a lot @patricoferris, this is fantastic work! We've rebased your branch and completed the missing libraries using lwt_eio here: https://github.com/art-w/irmin/tree/direct-style (... it got a bit messy, but all the tests are working now) Do you mind if I update your PR with the new patches? :)

@patricoferris
Copy link
Contributor Author

Awesome! Sorry for the slow reply, please do whatever is easiest (pushing the changes here, new PR etc.)

@art-w art-w force-pushed the direct-style branch 2 times, most recently from ca52020 to a0e31eb Compare April 9, 2024 12:28
@art-w
Copy link
Contributor

art-w commented Apr 9, 2024

Thanks a lot for your patience! I've updated your PR with all the work done by @ElectreAAS and @clecat to make irmin-pack domain-safe, updates for Eio 1.0, bugfixes etc, and rebased on top of the concurrent changes to irmin-client/server by zshipko and metanivek :)

So this PR now has feature parity with main and is pretty solidly tested... There are still issues that we need to address, but I'm hoping that we could do this in follow-up PRs as this work is getting hard to track:

  • We have a couple of Irmin backends in the work that are actually using Eio for I/O (without lwt_eio), which are targetting this branch (and getting rid of the effect handlers to pass the Eio capabilities, as this doesn't really work with fibers)
  • Some platform-specific issues reported by the CI on macos/freebsd and ppc64, that deserve special attention (only the OCaml 5.2~alpha1 compilation issue of libirmin has been fixed, thanks to dra27)

WDYT @samoht ? If we can merge this wip, should we add a disclaimer in the readme that the repo is migrating to Eio and that the public API will still change?

@samoht
Copy link
Member

samoht commented Apr 11, 2024

Could we add temporary workarounds for the failing tests? I'd be very happy to merge that PR but I also would like to keep the CI green to detect future regressions.

@art-w art-w force-pushed the direct-style branch 2 times, most recently from 0f1ff3f to e70e4dd Compare May 6, 2024 18:12
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.

None yet

5 participants