pkg: allow arbitrary opam repo layouts #11616

gridbugs · 2025-04-08T06:58:42Z

Opam repos can have fairly arbitrary directory layouts. The only requirement is that all package manifests are under the top-level "packages" directory, and each manifest is in a file named "opam" whose parent directory is named like ".".

Prior to this change dune assumed a layout similar to the official opam-repository, where manifests are at
"packages//./opam".

This generalizes dune's expectations of opam repositories such that arbitrary directory layouts are supported.

Fixes #11615

Opam repos can have fairly arbitrary directory layouts. The only requirement is that all package manifests are under the top-level "packages" directory, and each manifest is in a file named "opam" whose parent directory is named like "<name>.<version>". Prior to this change dune assumed a layout similar to the official opam-repository, where manifests are at "packages/<name>/<name>.<version>/opam". This generalizes dune's expectations of opam repositories such that arbitrary directory layouts are supported. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

maiste

Thanks for the PR! I have noticed few typos, but I mostly have questions regarding the errors handling. My main question is shouldn't we prevent the users from using a corrupted opam repository?

maiste · 2025-04-08T08:51:32Z

test/blackbox-tests/test-cases/pkg/opam-repo-layouts.t

+  > opam-version: "2.0"
+  > EOF
+
+Make some packgaes at various depths in the directory hierarchy:


Suggested change

Make some packgaes at various depths in the directory hierarchy:

Make some packages at various depths in the directory hierarchy:

maiste · 2025-04-08T09:12:05Z

src/dune_pkg/opam_repo.ml

+          (Package_version.Map.update version_to_entry version ~f:(function
+             | None -> Some entry
+             | Some existing_entry ->
+               User_warning.emit


Don't we want to fail instead if such cases occur? It means that the repository doesn't respect the structure.

I think a warning is ok when loading but we should definitely refuse to lock a package like this. Especially since which entry is the existing_entry and which one is the new entry depends on Fpath.traverse which doesn't guarantee order, so you might potentially get different results depending e.g. on which FS the repo is on.

maiste · 2025-04-08T09:19:35Z

src/dune_pkg/opam_repo.ml

+            let opam_file_path_relative_to_repo_root =
+              Path.Local.relative (Path.Local.relative Paths.packages dir) file
+            in


Shouldn't this be computed at line 171 to make it closer to where it is used?

maiste · 2025-04-08T09:21:00Z

src/dune_pkg/opam_repo.ml

+          let entry =
+            { Entry.opam_file_path_relative_to_repo_root = path
+            ; rev_store_file_if_loaded_from_git_repo = Some file
+            }


Suggested change

let entry =

{ Entry.opam_file_path_relative_to_repo_root = path

; rev_store_file_if_loaded_from_git_repo = Some file

}

let entry : Entry.t =

{ opam_file_path_relative_to_repo_root = path

; rev_store_file_if_loaded_from_git_repo = Some file

}

maiste · 2025-04-08T09:26:41Z

src/dune_pkg/opam_repo.ml

+    match Path.exists (Path.append_local dir opam_file_path) with
+    | false -> None
+    | true ->
+      let files_dir = Opam_file_table.files_dir_path_of_package opam_file_table package in


Doesn't it call twice the opam_file_path_of_package function? Shouldn't it be simplified to avoid it?

maiste · 2025-04-08T09:33:41Z

test/blackbox-tests/test-cases/pkg/opam-repo-layouts.t

@@ -0,0 +1,105 @@
+Test that dune can solve dependencies with opam repos using a layout other than
+the one used by the official opam repo. Opam allows arbitrary dependency
+hierarchies inside the "package" directory.


Suggested change

hierarchies inside the "package" directory.

hierarchies inside the "packages" directory.

maiste · 2025-04-08T09:34:52Z

test/blackbox-tests/test-cases/pkg/opam-repo-layouts.t

+  - c.3
+
+
+Now test that dune can handle some degenerate cases.


Suggested change

Now test that dune can handle some degenerate cases.

Now test that dune can handle some problematic cases.

😄

Leonidas-from-XIV

So I think the code is fine mostly and conceptually it is the right thing, as it enables us to use the Jane Street OPAM repo and any repo really, which is definitely neat.

However, I wonder if the complexity and cost in loading the whole repo into memory is worth it. Realistically people are going to use opam-repository, our overlays whose layout we control ourselves. So maybe the Jane Street repo layout could be added when inspecting the repo (e.g. how many layers are there between package/**/<name.version>/opam and then forcing the rest of the repo to match instead of allowing any nesting for any opam file), thus changing very little about what we currently do, basically just some additional processing in Opam_repo.Paths.

It is less flexible but I would argue that the added flexibility is not all that useful (OPAM suffers from this already, where it allows a lot of flexibility, sometimes to its detriment) and the price might be quite high.

I don't have a strong opinion on this however, so if people think that the added complexity, processing time and memory usage is worth it, I am okay with that.

Leonidas-from-XIV · 2025-04-08T09:42:39Z

src/dune_pkg/opam_repo.ml

+module Opam_file_table = struct
+  module Entry = struct
+    type t =
+      { opam_file_path_relative_to_repo_root : Path.Local.t


This feels overly very verbose, maybe worth shortening it a little?

Leonidas-from-XIV · 2025-04-08T09:43:32Z

src/dune_pkg/opam_repo.ml

+    ;;
+  end
+
+  module Version_to_entry = struct


Version_map?

Leonidas-from-XIV · 2025-04-08T09:45:22Z

src/dune_pkg/opam_repo.ml

+
+  let files_dir_path_of_package (t : t) package =
+    Option.map (opam_file_path_of_package t package) ~f:(fun opam_file_path ->
+      Path.Local.relative (Path.Local.parent_exn opam_file_path) "files")


So files is always relative to the opam file? If so, maybe worth writing a short comment to explain the layout.

Leonidas-from-XIV · 2025-04-08T09:46:18Z

src/dune_pkg/opam_repo.ml

+        ~f:(fun (entry : Entry.t) -> entry.opam_file_path_relative_to_repo_root))
+  ;;
+
+  let files_dir_path_of_package (t : t) package =


Suggested change

let files_dir_path_of_package (t : t) package =

let files_dir_of_package (t : t) package =

Leonidas-from-XIV · 2025-04-08T09:49:56Z

src/dune_pkg/opam_repo.ml

+      Path.Local.relative (Path.Local.parent_exn opam_file_path) "files")
+  ;;
+
+  let all_package_versions_of_name (t : t) package_name =


Suggested change

let all_package_versions_of_name (t : t) package_name =

let all_package_versions (t : t) package_name =

of_name is kind of implied by the type of the argument, Package_name.t.

Leonidas-from-XIV · 2025-04-08T09:54:23Z

src/dune_pkg/opam_repo.ml

+          (Package_version.Map.update version_to_entry version ~f:(function
+             | None -> Some entry
+             | Some existing_entry ->
+               User_warning.emit


I think a warning is ok when loading but we should definitely refuse to lock a package like this. Especially since which entry is the existing_entry and which one is the new entry depends on Fpath.traverse which doesn't guarantee order, so you might potentially get different results depending e.g. on which FS the repo is on.

Leonidas-from-XIV · 2025-04-08T09:57:01Z

src/dune_pkg/opam_repo.ml

+  let of_repo_dir_path opam_repo_dir_path ~loc : t =
+    let repo_name_for_messages = Path.to_absolute_filename opam_repo_dir_path in
+    try
+      Fpath.traverse_files


I think I'd really want a Fiber version of this, as for large repos or repos on slow shares this will block Dune for a while. On my machine with an SSD just find -name opam in OPAM repository takes half a second.

Fiber or not, it doesn't seem like a good trade-off to introduce such an expensive step that would benefit so few users. Can you either make this optional or introduce a lazy loading scheme for the JST repo?

Leonidas-from-XIV · 2025-04-08T10:03:44Z

src/dune_pkg/opam_repo.ml

+     files named "opam". *)
+  let of_rev_store_at_rev at_rev ~repo_name_for_messages ~loc : t =
+    Rev_store.At_rev.directory_entries ~recursive:true at_rev Paths.packages
+    |> Rev_store.File.Set.fold ~init:empty ~f:(fun file acc ->


Does this materialize every opam file from a Git repo into memory? In the case of OPAM-Repository that's 30MB of data, most of which will never be looked at.

Leonidas-from-XIV · 2025-04-08T12:13:31Z

test/blackbox-tests/test-cases/pkg/opam-repo-layouts.t

+  - b.2
+  - c.3
+
+  $ rm custom-opam-repo/packages/opam


What is this action for? It seems like there should be some kind of test but isn't.

Leonidas-from-XIV · 2025-04-08T12:14:12Z

test/blackbox-tests/test-cases/pkg/opam-repo-layouts.t

+
+Create an opam file in a directory that doesn't have a version number. There's
+no way to refer to this package in dune-project but dune should still be able
+to parse the repo with crashing.


Suggested change

to parse the repo with crashing.

to parse the repo without crashing.

Maybe?

rgrinberg · 2025-04-08T12:53:52Z

src/dune_pkg/opam_repo.ml

+  let of_repo_dir_path opam_repo_dir_path ~loc : t =
+    let repo_name_for_messages = Path.to_absolute_filename opam_repo_dir_path in
+    try
+      Fpath.traverse_files


Fiber or not, it doesn't seem like a good trade-off to introduce such an expensive step that would benefit so few users. Can you either make this optional or introduce a lazy loading scheme for the JST repo?

gridbugs · 2025-04-09T07:10:10Z

Thanks for the PR! I have noticed few typos, but I mostly have questions regarding the errors handling. My main question is shouldn't we prevent the users from using a corrupted opam repository?

If a repo does not follow the convention set by opam-repository, that doesn't mean it is corrupt. The documentation on repo layout in the opam manual specifies that any layout where opam files are contained in directories named after the package is acceptable. The document also suggests a potential future change to the layout of opam-repository where another level of hierarchy is introduced to group packages by common prefixes, e.g. packages/p/pa/package-name/package-name.version.

I agree with the sentiment that in practice we're probably only ever going to see repos formatted like opam-repository or JST, and the complexity/runtime cost of fully general opam repos is too high to be worth it. I briefly thought about introducing a fast-path where if a repo is formatted like opam-repository then we can make some optimizing assumptions, but the only way to be sure that a repo is formatted like opam-repository is to do a full scan of all its opam files which is just as expensive as not having a fast-path to begin with.

So I'm thinking I'll defer solving #11615 until I have more time to work on it, and eventually we can use a solution more specialized to the layouts we see in opam-repository and JST.

Alternatively, @rgrinberg how feasible would it be to ask Jane Street to change the layout of their repos to match opam-repository?

maiste · 2025-04-09T07:43:18Z

If a repo does not follow the convention set by opam-repository, that doesn't mean it is corrupt.

It is a misunderstanding: I was referring to repositories with twice the same file or an opam file at the root.

rgrinberg · 2025-04-10T15:54:22Z

I'll ask, but I don't really see an issue with implementing an alternative lazy loading scheme. What we're doing is implementing the following function for a single repository:

val list_packages : Package_name.t -> Package_version.t list

So you could implement this function by:

Try to list packages/$package. If this succeeds, we finish our search.
If the above fails because packages/$package doesn't exist, you read can read all of packages and look for $package.*. The result is now the matches in 3
For subsequent lookups, you want the listing of packages/ to be cached. Otherwise it would be really slow for large repos that aren't from git.

With this scheme, the common users always get the optimized experience, while we still support this alternative layout.
For multiple repositories, we are sadly going to hit the 2nd step more often than we'd like. On the other hand, multiple repositories aren't all that common and the performance problem aren't relevant to opam repositories from git.

gridbugs · 2025-04-11T01:33:44Z

For multiple repositories, we are sadly going to hit the 2nd step more often than we'd like. On the other hand, multiple repositories aren't all that common and the performance problem aren't relevant to opam repositories from git.

What do you mean by "multiple repositories" here?

gridbugs · 2025-04-11T02:59:09Z

I just noticed that Jane Street's compiler extensions branch uses a mix of different layouts within its packages directory. Most packages are directly under packages but a few (e.g. ocaml-variants) get a subdirectory in the style of opam-repository. @rgrinberg is that what you mean by a "multiple repository"?

rgrinberg · 2025-04-11T16:35:30Z

Multiple repositories can be defined in dune's workspace file in order. Refer to tests like multiple-opam-repos.t for examples.

Mixing the two layouts defeats the ability to implement any lazy loading scheme. I'm going to ask about sticking to one scheme. For now, I think you could just make eager loading "opt-in" for some repos.

gridbugs · 2025-04-14T01:47:44Z

Is lazy-loading the opam repo something we're planning to implement in the future? Unless I'm mistaken it looks like we currently enumerate all the paths in the opam repo when creating a Rev_store.At_rev.t to populate the files: File.Set.t field (here). I timed dune pkg lock a few times with and without this change and didn't see a noticeable difference.

gridbugs requested review from Leonidas-from-XIV and maiste April 8, 2025 06:58

maiste reviewed Apr 8, 2025

View reviewed changes

Leonidas-from-XIV reviewed Apr 8, 2025

View reviewed changes

rgrinberg requested changes Apr 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg: allow arbitrary opam repo layouts #11616

pkg: allow arbitrary opam repo layouts #11616

gridbugs commented Apr 8, 2025

maiste left a comment

maiste Apr 8, 2025

maiste Apr 8, 2025

Leonidas-from-XIV Apr 8, 2025

maiste Apr 8, 2025

maiste Apr 8, 2025

maiste Apr 8, 2025

maiste Apr 8, 2025

maiste Apr 8, 2025

Leonidas-from-XIV left a comment

Leonidas-from-XIV Apr 8, 2025

Leonidas-from-XIV Apr 8, 2025

Leonidas-from-XIV Apr 8, 2025

Leonidas-from-XIV Apr 8, 2025

Leonidas-from-XIV Apr 8, 2025

Leonidas-from-XIV Apr 8, 2025

Leonidas-from-XIV Apr 8, 2025

rgrinberg Apr 8, 2025

Leonidas-from-XIV Apr 8, 2025

Leonidas-from-XIV Apr 8, 2025

Leonidas-from-XIV Apr 8, 2025

rgrinberg Apr 8, 2025

gridbugs commented Apr 9, 2025

maiste commented Apr 9, 2025

rgrinberg commented Apr 10, 2025

gridbugs commented Apr 11, 2025

gridbugs commented Apr 11, 2025

rgrinberg commented Apr 11, 2025

gridbugs commented Apr 14, 2025

	Make some packgaes at various depths in the directory hierarchy:
	Make some packages at various depths in the directory hierarchy:

	hierarchies inside the "package" directory.
	hierarchies inside the "packages" directory.

	Now test that dune can handle some degenerate cases.
	Now test that dune can handle some problematic cases.

	let files_dir_path_of_package (t : t) package =
	let files_dir_of_package (t : t) package =

	let all_package_versions_of_name (t : t) package_name =
	let all_package_versions (t : t) package_name =

	to parse the repo with crashing.
	to parse the repo without crashing.

pkg: allow arbitrary opam repo layouts #11616

Are you sure you want to change the base?

pkg: allow arbitrary opam repo layouts #11616

Conversation

gridbugs commented Apr 8, 2025

maiste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gridbugs commented Apr 9, 2025

maiste commented Apr 9, 2025

rgrinberg commented Apr 10, 2025

gridbugs commented Apr 11, 2025

gridbugs commented Apr 11, 2025

rgrinberg commented Apr 11, 2025

gridbugs commented Apr 14, 2025