irmin-pack: move pack_(key|value) unix specifics #2084

metanivek · 2022-09-12T17:41:15Z

This PR is similar to #2081 in that it is moving specifics of the irmin-pack unix implementation into irmin-pack.unix. Additionally, it sets us up to remove the optint dep in irmin-pack (see #1931).

samoht · 2022-09-12T17:45:35Z

src/irmin-tezos/irmin_tezos.mli

-     and type contents_key = Schema.Hash.t Irmin_pack.Pack_key.t
-     and type node_key = Schema.Hash.t Irmin_pack.Pack_key.t
-     and type commit_key = Schema.Hash.t Irmin_pack.Pack_key.t
+     and type contents_key = Schema.Hash.t Irmin_pack_unix.Pack_key.t


isn't it an issue to compile this codebase to javascript? (not sure exactly which part is required to compile to JS for Tezos)

Hmm, that's a good question. I hadn't thought about JS compilation. Any pointers on where/how octez is compiled to JS? Also, is irmin-tezos used by octez?

Thinking about this some more...

This constraint is a part of a module Store = Irmin_pack_unix.S with ... so it is already in the context of irmin-pack.unix (and therefore likely JS incompatible).

The current Irmin_pack.Pack_key is not implementation independent anyway -- it represents specifics of the unix implementation (with direct and indexed keys and inspection capabilities). This PR (hopefully) moves towards addressing this mixture.

So is this particular line an issue? I'm not sure but lean towards thinking it is not.

Also, here is some information I found for octez + JS

https://tezos.gitlab.io/developer/javascript.html

https://gitlab.com/tezos/tezos/-/blob/master/manifest/main.ml#L180-185

irmin-tezos/dune already contains irmin-pack.unix. I think this is fine.

codecov-commenter · 2022-09-12T17:52:25Z

Codecov Report

Merging #2084 (2f3b7fe) into main (b1c706a) will decrease coverage by 0.00%.
The diff coverage is 70.45%.

@@            Coverage Diff             @@
##             main    #2084      +/-   ##
==========================================
- Coverage   64.73%   64.72%   -0.01%     
==========================================
  Files         131      131              
  Lines       15658    15658              
==========================================
- Hits        10136    10135       -1     
- Misses       5522     5523       +1

Impacted Files	Coverage Δ
src/irmin-pack/pack_value.ml	`81.31% <ø> (ø)`
src/irmin-pack/unix/gc.ml	`40.00% <0.00%> (ø)`
src/irmin-pack/unix/gc_intf.ml	`100.00% <ø> (ø)`
src/irmin-pack/unix/import.ml	`88.88% <ø> (ø)`
src/irmin-pack/unix/inode.ml	`33.33% <ø> (ø)`
src/irmin-pack/unix/pack_key.ml	`74.35% <74.35%> (ø)`
src/irmin-pack/unix/ext.ml	`61.94% <100.00%> (ø)`
src/irmin-fs/unix/irmin_fs_unix.ml	`67.09% <0.00%> (-0.65%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Ngoguey42

All this looks good

Ngoguey42 · 2022-09-15T11:57:15Z

src/irmin-pack/pack_key_intf.ml

@@ -29,77 +29,5 @@ module type S = sig
 end

 module type Sigs = sig


I think you could collapse the 3 pack_key files into the ml file.

Ah, good spot. Fixed.

Ngoguey42 · 2022-09-15T12:00:14Z

src/irmin-tezos/irmin_tezos.mli

-     and type contents_key = Schema.Hash.t Irmin_pack.Pack_key.t
-     and type node_key = Schema.Hash.t Irmin_pack.Pack_key.t
-     and type commit_key = Schema.Hash.t Irmin_pack.Pack_key.t
+     and type contents_key = Schema.Hash.t Irmin_pack_unix.Pack_key.t


irmin-tezos/dune already contains irmin-pack.unix. I think this is fine.

Ngoguey42

Thanks :)

@patricoferris

…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2) CHANGES: ### Added - **irmin** - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris) - **irmin-graphql** - Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris) - Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris) - **irmin-pack** - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w) - **irmin-tezos-utils** - Add package `irmin-tezos-utils` containing a graphical tool for manual pack files analysis. (mirage/irmin#1939, @clecat) ### Changed - **irmin-pack** - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081, @icristescu) - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack` to `irmin-pack.unix` (mirage/irmin#2084, @metanivek) - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w) ### Fixed - **irmin-pack** - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42) - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106, @icristescu) ### Fixed - **irmin-pack** - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with gced stores. (mirage/irmin#1919, @icristescu)

@patricoferris

…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2) CHANGES: ### Added - **irmin** - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris) - **irmin-graphql** - Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris) - Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris) - **irmin-pack** - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w) - **irmin-tezos-utils** - Add package `irmin-tezos-utils` containing a graphical tool for manual pack files analysis. (mirage/irmin#1939, @clecat) ### Changed - **irmin-pack** - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081, @icristescu) - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack` to `irmin-pack.unix` (mirage/irmin#2084, @metanivek) - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w) ### Fixed - **irmin-pack** - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42) - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106, @icristescu) - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with gced stores. (mirage/irmin#1919, @icristescu)

@patricoferris

…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2) CHANGES: ### Added - **irmin** - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris) - **irmin-graphql** - Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris) - Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris) - **irmin-pack** - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w) - **irmin-tezos-utils** - Add package `irmin-tezos-utils` containing a graphical tool for manual pack files analysis. (mirage/irmin#1939, @clecat) ### Changed - **irmin-pack** - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081, @icristescu) - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack` to `irmin-pack.unix` (mirage/irmin#2084, @metanivek) - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w) ### Fixed - **irmin-pack** - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42) - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106, @icristescu) - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with gced stores. (mirage/irmin#1919, @icristescu)

@patricoferris

…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2) CHANGES: ### Added - **irmin** - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris) - **irmin-graphql** - Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris) - Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris) - **irmin-pack** - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w) - **irmin-tezos-utils** - Add package `irmin-tezos-utils` containing a graphical tool for manual pack files analysis. (mirage/irmin#1939, @clecat) ### Changed - **irmin-pack** - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081, @icristescu) - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack` to `irmin-pack.unix` (mirage/irmin#2084, @metanivek) - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w) ### Fixed - **irmin-pack** - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42) - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106, @icristescu) - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with gced stores. (mirage/irmin#1919, @icristescu)

samoht reviewed Sep 12, 2022

View reviewed changes

metanivek changed the title ~~irmin-pack: move pack_(key|value) unix specifics~~ (WIP) irmin-pack: move pack_(key|value) unix specifics Sep 12, 2022

Ngoguey42 reviewed Sep 15, 2022

View reviewed changes

irmin-pack: move pack_(key|value) unix specifics

bac32eb

metanivek force-pushed the mv-pack-key-value branch from 2f3b7fe to bac32eb Compare September 15, 2022 13:25

metanivek changed the title ~~(WIP) irmin-pack: move pack_(key|value) unix specifics~~ irmin-pack: move pack_(key|value) unix specifics Sep 15, 2022

Ngoguey42 approved these changes Sep 15, 2022

View reviewed changes

metanivek merged commit b72a868 into mirage:main Sep 15, 2022

metanivek deleted the mv-pack-key-value branch September 15, 2022 15:49

metanivek added a commit to metanivek/irmin that referenced this pull request Sep 15, 2022

Update changelog for mirage#2084

77eb0ff

icristescu mentioned this pull request Sep 26, 2022

Collection of IO and GC improvements #2039

Open

metanivek mentioned this pull request Oct 6, 2022

[new release] irmin project (3.4.2) ocaml/opam-repository#22219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

irmin-pack: move pack_(key|value) unix specifics #2084

irmin-pack: move pack_(key|value) unix specifics #2084

metanivek commented Sep 12, 2022

samoht Sep 12, 2022

metanivek Sep 12, 2022

metanivek Sep 12, 2022

Ngoguey42 Sep 15, 2022

codecov-commenter commented Sep 12, 2022

Ngoguey42 left a comment

Ngoguey42 Sep 15, 2022

metanivek Sep 15, 2022

Ngoguey42 Sep 15, 2022

Ngoguey42 left a comment

		@@ -29,77 +29,5 @@ module type S = sig
		end

		module type Sigs = sig

irmin-pack: move pack_(key|value) unix specifics #2084

irmin-pack: move pack_(key|value) unix specifics #2084

Conversation

metanivek commented Sep 12, 2022

samoht Sep 12, 2022

Choose a reason for hiding this comment

metanivek Sep 12, 2022

Choose a reason for hiding this comment

metanivek Sep 12, 2022

Choose a reason for hiding this comment

Ngoguey42 Sep 15, 2022

Choose a reason for hiding this comment

codecov-commenter commented Sep 12, 2022

Codecov Report

Ngoguey42 left a comment

Choose a reason for hiding this comment

Ngoguey42 Sep 15, 2022

Choose a reason for hiding this comment

metanivek Sep 15, 2022

Choose a reason for hiding this comment

Ngoguey42 Sep 15, 2022

Choose a reason for hiding this comment

Ngoguey42 left a comment

Choose a reason for hiding this comment