-
Notifications
You must be signed in to change notification settings - Fork 149
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
datakit: update to latest versions of Irmin/Mirage/Cdmliner #529
Conversation
Note: this needs the dev version of hvsock (to support MirageOS 3), + mirage/irmin#432 and mirage/irmin#431 to be merged first (and Irmin 1.1 to be released) |
CI is unhappy because the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far :-)
src/datakit/ivfs.ml
Outdated
@@ -2,11 +2,14 @@ open Astring | |||
open Rresult | |||
open Lwt.Infix | |||
|
|||
let src = Logs.Src.create "ivfs" ~doc:"Irmin VFS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "DataKit" here?
@@ -674,43 +677,38 @@ module Make (Store : Ivfs_tree.STORE) = struct | |||
|
|||
(* Note: can't use [Store.fast_forward_head] because it can | |||
sometimes return [false] on success (when already up-to-date). *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still true?
src/datakit/ivfs_merge.ml
Outdated
|
||
type path = Path.t | ||
type step = Path.Step.t | ||
let src = Logs.Src.create "ivfs.merge" ~doc:"Irmin VFS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"DataKit"?
I've updated the PR with @talex5 comments, but I haven't fixed the CI yet. Working on that now. |
7516cdf
to
d33a472
Compare
src/datakit/ivfs_blob.ml
Outdated
let pp_buf ppf buf = Fmt.pf ppf "%S" (Cstruct.to_string buf) | ||
let pp ppf t = Fmt.pf ppf "%a" Fmt.(Dump.list pp_buf) !t | ||
let pp_buf ppf buf = Fmt.string ppf (Cstruct.to_string buf) | ||
let pp ppf t = Fmt.pf ppf "%a" Fmt.(list ~sep:(unit "") pp_buf) !t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the unit ""
can be replaced by nop
from Fmt
src/datakit/main.ml
Outdated
let merge _ = merge [] | ||
module Path = Ivfs_tree.Path | ||
end | ||
Irmin.Info.v ~date ~author:"datakit <[email protected]>" msg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we remove the [email protected]
email entirely? This is now a moby/datakit
repo, but could the default commit could also just be datakit
without an email address if that's valid in Git?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Email is mandatory, so Irmin will add the default [email protected]
email to it. But yes @docker.com
should go away.
src/datakit_io.ml
Outdated
protect (Lwt_unix.mkdir dir) 0o755; | ||
) in | ||
Lwt_pool.use mkdir_pool (fun () -> aux dirname) | ||
|
||
let file_exists f = | ||
Lwt.catch (fun () -> Lwt_unix.file_exists f) (function | ||
(* See https://github.com/ocsigen/lwt/issues/316 *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue is now fixed in Lwt>=2.7.1 -- update constraint and remove this workaround?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Note that this is a copy/paste from Irmin-unix to avoid depending on nocrypto and hence libgmp, so it needs to be fixed upstream too.
4d2895f
to
32152f2
Compare
The |
5dfd0c2
to
6b14d2f
Compare
Yay, all green! Need a rebase of protocol-9p (that @talex5 is working on) before it can be merged. |
Try pinning my |
This patch make Irmin 1.1 works with DataKit. As Irmin 1.1 only works with cmdliner >= 1.0 and MirageOS >= 3.0, also update these dependencies. The main change is the removal of the body Ivfs_tree to use the new Irmin API. The new Irmin Tree API is pretty similar (on purpose) but it seems that we are loosing caching of file hashes. I plan to re-add that in a later commit, after checking it is really needed. Ideally datakit-client will also move to something closer to the Irmin API but it is not a blocker for API compatibility. Signed-off-by: Thomas Gazagnaire <[email protected]>
Signed-off-by: Thomas Gazagnaire <[email protected]>
…xed upstream Signed-off-by: Thomas Gazagnaire <[email protected]>
Signed-off-by: Thomas Gazagnaire <[email protected]>
Signed-off-by: Thomas Gazagnaire <[email protected]>
Signed-off-by: Thomas Gazagnaire <[email protected]>
Signed-off-by: Thomas Gazagnaire <[email protected]>
Signed-off-by: Thomas Gazagnaire <[email protected]>
Signed-off-by: Thomas Gazagnaire <[email protected]>
Signed-off-by: Thomas Gazagnaire <[email protected]>
Signed-off-by: Thomas Gazagnaire <[email protected]>
All green, merging! |
This patch make Irmin 1.1 works with DataKit. As Irmin 1.1 only
works with cmdliner >= 1.0 and MirageOS >= 3.0, also update
these dependencies.
The main change is the removal of the body Ivfs_tree to use the new
Irmin API. The new Irmin Tree API is pretty similar (on purpose) but
it seems that we are loosing caching of file hashes. I plan to re-add
that in a later commit, after checking it is really needed.
Ideally datakit-client will also move to something closer to the Irmin API
but it is not a blocker for API compatibility.
Signed-off-by: Thomas Gazagnaire [email protected]