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

Update irmin to the last version of ocaml-git #1065

Merged
merged 7 commits into from
Jan 11, 2021

Conversation

dinosaure
Copy link
Member

This is a draft about an update to the new version of git presented on mirage/ocaml-git#395 and the new version of conduit presented on mirage/ocaml-conduit#311 (including patches into mirage/ocaml-cohttp#692). I think the patch is small enough. The CI will fail but locally (with a pin of conduit and cohttp and a simple dune runtest) it's work (believe me).

This is a draft but people can start to comment it if some details are obscures. Currently, the biggest change is about the synchronization where I updated the interface which seems to be much more simpler.

@craigfe
Copy link
Member

craigfe commented Aug 17, 2020

The CI will fail but locally (with a pin of conduit and cohttp and a simple dune runtest) it's work (believe me).

If you want to get the CI to work, you can add pin-depends on specific commits as we're doing here. (Needs to be a commit hash, otherwise OCaml-CI will not accept it.) Also fine to just leave this failing until the upstream changes have been released 🤷

@dinosaure
Copy link
Member Author

dinosaure commented Sep 9, 2020

  • clean the PR
  • CI is happy

@dinosaure
Copy link
Member Author

Re-updated to the last version of ocaml-git and remove the update to conduit.3.0.0 (useless due to mirage/ocaml-git#428)

@dinosaure dinosaure force-pushed the ocaml-git branch 2 times, most recently from f4a1fcd to 933bea6 Compare January 6, 2021 15:26
@dinosaure dinosaure marked this pull request as ready for review January 6, 2021 15:26
@dinosaure
Copy link
Member Author

Ready to review 👍., ocaml-git is freezed.

irmin-graphql.opam Outdated Show resolved Hide resolved
src/irmin-graphql/dune Outdated Show resolved Hide resolved
@samoht
Copy link
Member

samoht commented Jan 7, 2021

Thanks for the patch. My main concern is that I would like to avoid mimic to leak in non-git related codebase. It also seems to me that Mimic should be renamed Git_conduit to avoid confusion (as it's a conduit clone only used by ocaml-git).

@dinosaure dinosaure force-pushed the ocaml-git branch 5 times, most recently from de0e98e to 369edbe Compare January 7, 2021 12:13
@dinosaure
Copy link
Member Author

It also seems to me that Mimic should be renamed Git_conduit to avoid confusion (as it's a conduit clone only used by ocaml-git).

I'm not sure, even if mimic seems specialized to ocaml-git, I move some of my codebase (SMTP and HTTP/AF) with this small library.

irmin-git.opam Outdated Show resolved Hide resolved
irmin-http.opam Outdated Show resolved Hide resolved
src/irmin-git/irmin_git.ml Outdated Show resolved Hide resolved
@samoht
Copy link
Member

samoht commented Jan 7, 2021

The diff in irmin-http.opam are weird but I can live with that :-) why does irmin-http now depends on Git? I am not sure I am good with this change.

Could you also add a line in the changelog (and point to https://github.com/mirage/irmin/issues?q=is%3Aissue+is%3Aopen+git+label%3Agit which would probably be fixed 🎉) ? After that I think we are good to merge.

@dinosaure
Copy link
Member Author

The diff in irmin-http.opam are weird but I can live with that :-) why does irmin-http now depends on Git? I am not sure I am good with this change.

I reverted the patch on irmin-http.opam however, dune-opam-lint still complains about missing dependencies even if I did not updated src/irmin-http/dune or the irmin-unix's codebase (according to the diff). So I don't really understand why such dependencies appears.

@dinosaure
Copy link
Member Author

So my revert seems good. However, I don't think that this PR fixes #914 or #917 (but #1001 is fixed according to a test in ocaml-git). I will add an entry into the Changelog 👍.

@dinosaure
Copy link
Member Author

According to the release of git.3.0.0 (see ocaml/opam-repository#17950), this PR is ready to merge.

@samoht
Copy link
Member

samoht commented Jan 11, 2021

Can you fix dune build @fmt to make the CI happy?

@dinosaure
Copy link
Member Author

It's done 👍.

@craigfe
Copy link
Member

craigfe commented Jan 11, 2021

LGTM, thanks!

@craigfe craigfe merged commit 71bd092 into mirage:master Jan 11, 2021
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.

3 participants