[API] Abstract over repository roots to simplify the addition of new repository formats #6680

kit-ty-kate · 2025-09-09T10:33:32Z

Extracted from #6625
~~Queued on #6679~~
To be merged at the same time as ocaml-opam/opam-rt#86

As mentioned in #6625, adding abstract types over repository roots allows us to simplify and ensure correctness when adding a new format of repository.

In its current form, this PR also changes a couple of unrelated thing that i failed to change back when extracting from #6625 (removal of some use and logic associated with OpamRepositoryConfig.repo_tarring) so it needs at least a cleanup before it is in a mergable state (on top of being queued on the already substantial #6679)

rjbou · 2025-10-09T17:35:22Z

src/client/opamCommands.ml

+           let target = OpamRepositoryRoot.Dir.backup ~tmp_dir dir in
+           OpamRepositoryRoot.Dir.copy ~src:dir ~dst:target;
+           fun () -> OpamRepositoryRoot.Dir.copy ~src:target ~dst:dir)


Why use Dir here instead of the abstraction OpamRepositoryRoot.t ?

This code is guarded under the else branch of if OpamFilename.exists tar so in this case we know that we're working with directories

Yes, i saw. What i wanted to highlight is that (almost) the same code is used for tar & dir repositories : copy in tmp dir the do the copy and return the function that does the copy back. I was wondering if it is possible to factorise it using the abstraction.

if OpamFilename.exists tar then (let target = OpamFilename.create tmp_dir (OpamFilename.basename tar) in OpamFilename.copy ~src:tar ~dst:target; fun () -> OpamFilename.copy ~src:target ~dst:tar) else ( [...] let target = OpamRepositoryRoot.Dir.backup ~tmp_dir dir in OpamRepositoryRoot.Dir.copy ~src:dir ~dst:target; fun () -> OpamRepositoryRoot.Dir.copy ~src:target ~dst:dir)

rjbou · 2025-10-09T17:57:30Z

src/state/opamUpdate.ml

+let safe_read_repo_file = function
+  | OpamRepositoryRoot.Dir dir ->
+    OpamFile.Repo.safe_read (OpamRepositoryPath.repo dir)


better have this function is OpamRepositoryState (cf related comment in 6625)

I used the already existing OpamRepositoryRoot.delayed_read_repo function instead

src/state/opamUpdate.ml

rjbou · 2025-10-09T18:08:36Z

I've pushed some commits that contains some reviews proposals, left some comments. It is a very preliminary review comments, i still have some blind spot (that's why i refer to 6625 comments too).

…s called everywhere

rjbou · 2025-11-13T17:01:05Z

src/client/opamCommands.ml

+           let target = OpamRepositoryRoot.Dir.backup ~tmp_dir dir in
+           OpamRepositoryRoot.Dir.copy ~src:dir ~dst:target;
+           fun () -> OpamRepositoryRoot.Dir.copy ~src:target ~dst:dir)


Yes, i saw. What i wanted to highlight is that (almost) the same code is used for tar & dir repositories : copy in tmp dir the do the copy and return the function that does the copy back. I was wondering if it is possible to factorise it using the abstraction.

if OpamFilename.exists tar then (let target = OpamFilename.create tmp_dir (OpamFilename.basename tar) in OpamFilename.copy ~src:tar ~dst:target; fun () -> OpamFilename.copy ~src:target ~dst:tar) else ( [...] let target = OpamRepositoryRoot.Dir.backup ~tmp_dir dir in OpamRepositoryRoot.Dir.copy ~src:dir ~dst:target; fun () -> OpamRepositoryRoot.Dir.copy ~src:target ~dst:dir)

rjbou · 2025-11-14T16:30:27Z

src/state/opamUpdate.ml

      | Some (new_url, f) ->
-        OpamFilename.cleandir repo_root;
+        OpamRepositoryRoot.remove repo_root;
+        OpamRepositoryRoot.make_empty repo_root;


Do we need to create an empty directory here ? OpamHTTP and OpamVCS fetch_repo_update does not need the directory to exist, but OpamLocal requires it. I think it should be in there that the change should be applied. Plus, it'll make the functions more homogeneous.

~~And like that, there is only the need to have OpamRepositoryRoot.Dir.make_empty and no OpamRepositoryRoot.make_empty~~ Seems to be used in 6625 - OpamHTTP.fetch_repo_update

rjbou · 2025-11-14T16:35:19Z

src/repository/opamRepositoryRoot.ml

+  let copy = OpamFilename.copy_dir
+  let copy_except_vcs = OpamFilename.copy_dir_except_vcs


Doesn't worth to have only 1 function ?

Suggested change

let copy = OpamFilename.copy_dir

let copy_except_vcs = OpamFilename.copy_dir_except_vcs

let copy ?(include_vcs=true) =

if include_vcs then OpamFilename.copy_dir

else OpamFilename.copy_dir_except_vcs

rjbou · 2025-11-14T16:39:00Z

src/repository/opamRepositoryRoot.mli

+    implementation (could be a database, a file, a directory, etc.) *)
+
+(** Repository root implemented as a directory *)
+module Dir : sig


Some functions are only present in the Dir module, and not meant to be abstracted via OpamRepositoryRoot module. In that case, they will be used only in the match case OpamRepositoryRoot.Dir dir, does it worth to work directly with OpamFilename functions for those part instead of having the redirections?
A good example of that is the change in OpamLocal lines 160-171 (new file)

rjbou · 2025-11-14T16:39:23Z

src/repository/opamRepositoryRoot.mli

+
+val remove : t -> unit
+val is_empty : t -> bool option
+val make_empty : t -> unit


If not removed

Suggested change

val make_empty : t -> unit

(* Creates the root of the repository *)

val make_empty : t -> unit

rjbou · 2025-11-14T16:44:04Z

src/client/opamArg.ml

-      "Enables option `--require-checksums' when available \
-       (e.g. for `opam install').";
-      "REPOSITORYTARRING", cli_between cli2_2 cli2_5,
+      "REPOSITORYTARRING", cli_from cli2_2,


Is the removal of repository tarring moved to 6625 ?

kit-ty-kate added this to the 2.5.0~alpha1 milestone Sep 9, 2025

kit-ty-kate added PR: WIP Not for merge at this stage PRIORITY AREA: API PR: QUEUED Pending pull request, waiting for other work to be merged or closed labels Sep 9, 2025

kit-ty-kate mentioned this pull request Sep 9, 2025

[WIP] Improve performance of opam update/init by changing the structure of the internal http opam repositories (use the tar.gz as-is) #6625

Draft

rjbou reviewed Oct 9, 2025

View reviewed changes

rjbou force-pushed the abstract-repository-root branch from 283ee7a to 17fd765 Compare October 10, 2025 17:28

kit-ty-kate and others added 13 commits October 10, 2025 23:32

add new module

d358abe

fixup! add new module

b8401db

tmp

909be9d

fixup! tmp

79fd646

fixing compil

14a83e7

start adding some doc

be25bd1

some variable def / facto

708c708

remove tar dep

bb45e5d

remove via cli OPAMREPOTARRING env var

7e0f475

add Op modul in repo root modules

80e29e3

admincommand: factorise cwd repo root creation with a function that i…

ba224cc

…s called everywhere

admincommand: factorise some to_dir functions calls

445b882

amdin repo upgrade: use / operators to simplify code

baf0725

kit-ty-kate force-pushed the abstract-repository-root branch from 17fd765 to baf0725 Compare October 10, 2025 22:33

kit-ty-kate added 7 commits October 10, 2025 23:50

cwd

74254fb

norepotarringchange

b9cb4f5

lean

b913bbe

rename make_empty

b9cdd2c

rm clean

fd6bbda

doc delayed_read_repo

628fd8a

columns

872bc27

kit-ty-kate added 3 commits October 11, 2025 00:39

reduce diff

0e0c0bd

resolve todos

facbe39

fixup src/tools

836e917

kit-ty-kate marked this pull request as ready for review October 11, 2025 00:04

kit-ty-kate removed PR: QUEUED Pending pull request, waiting for other work to be merged or closed PR: WIP Not for merge at this stage labels Oct 11, 2025

kit-ty-kate added 2 commits October 11, 2025 01:27

reintroduce repo_tarring

c2e8e44

fixup-no-diff

49cf0d2

kit-ty-kate added a commit to kit-ty-kate/opam-rt that referenced this pull request Oct 11, 2025

Apply API change from ocaml/opam#6680

24584c3

kit-ty-kate mentioned this pull request Oct 11, 2025

Apply API change from ocaml/opam#6680 ocaml-opam/opam-rt#86

Closed

doc index

30c80d8

kit-ty-kate mentioned this pull request Oct 11, 2025

Apply API change from ocaml/opam#6680 ocaml-opam/opam-rt#87

Open

rm safe_read_repo_file

76b1706

kit-ty-kate requested a review from rjbou October 11, 2025 22:10

kit-ty-kate modified the milestones: 2.5.0~alpha1, 2.6.0~alpha1 Oct 14, 2025

rjbou reviewed Nov 17, 2025

View reviewed changes

		let copy = OpamFilename.copy_dir
		let copy_except_vcs = OpamFilename.copy_dir_except_vcs

-  let copy = OpamFilename.copy_dir
-  let copy_except_vcs = OpamFilename.copy_dir_except_vcs
+  let copy ?(include_vcs=true) =
+    if include_vcs then OpamFilename.copy_dir
+    else OpamFilename.copy_dir_except_vcs

	val make_empty : t -> unit
	(* Creates the root of the repository *)
	val make_empty : t -> unit

[API] Abstract over repository roots to simplify the addition of new repository formats #6680

Are you sure you want to change the base?

[API] Abstract over repository roots to simplify the addition of new repository formats #6680

Conversation

kit-ty-kate commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjbou commented Oct 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kit-ty-kate commented Sep 9, 2025 •

edited

Loading