Use `git_repo` to pin git packages #13176

Leonidas-from-XIV · 2026-01-02T10:43:55Z

While trying to figure out which revision a package that is pinned via git is, I realized that for some reason packages pinned via Git are pinned as Resolved_package.local_package. I would have expected it to be git_repo which also gets an At_rev.t (thus keeping a reference to it that I can read later), so I updated the code to do so.

Tests seem to all work fine after fixing git_repo so that it can build packages with Dune. Detecting whether a package builds with Dune is a bit fiddly but before git_repo always assumed false so I've retained that for the other invocations of git_repo and in the pinning case I know that the repo to be pinned uses Dune so I can set it to true, thus I can avoid guessing.

Alizter · 2026-01-02T11:00:12Z

Can you add some tests?

Leonidas-from-XIV · 2026-01-02T11:01:26Z

The source-locking.t test is exercising this (and breaks when dune_build=false).

rgrinberg · 2026-01-02T11:12:08Z

I guess the description makes it sound like this is a fix, but is it just a refactoring?

Alizter · 2026-01-02T11:13:42Z

This refactoring is needed I guess in order to include the revision of the pinned git repo in the lock. Might be worth opening a PR with that fix so we can see the direction this is going.

rgrinberg · 2026-01-02T11:18:51Z

src/dune_pkg/pin.ml

+            opam_package
+        | Git at_rev ->
+          let opam_file_contents = OpamFile.OPAM.write_to_string opam_file in
+          (* when pinning there is no files dir nor does it make sense *)


This comment is incorrect. When pinning a package with a root directory opam/, the files directory is opam/files.

Actually, my comment is also incorrect. Because we're pinning a dune package, there's no files directory. Because at the moment, we don't allow users to define such a directory.

Nope, looks like my original comment was correct. The pin is what is from dune here, not the package type. And dune pins can be used on opam packages which may contain an opam/files directory.

Interesting. I've never encountered such a package (and it seems a bit pointless, because in such cases the files could just be part of the repo/tarball itself).

Where is the opam file in such case? Does it have to be in opam/ or can it be in the root?

Interesting. I've never encountered such a package (and it seems a bit pointless, because in such cases the files could just be part of the repo/tarball itself).

I recall running into this at some point. I agree it's exceedingly rare.

Interesting. I've never encountered such a package (and it seems a bit pointless, because in such cases the files could just be part of the repo/tarball itself).

In such a case, the opam file would be opam/opam.

I think we should add a test for this if we haven't one yet. We don't necessarily need to support this feature, but having a good error message that we don't support pinning such a package would be the least we could do.

rgrinberg · 2026-01-02T11:26:34Z

src/dune_pkg/pin.ml

+            opam_file
+            opam_package
+        | Git at_rev ->
+          let opam_file_contents = OpamFile.OPAM.write_to_string opam_file in


Something is off here. Why are we turning this value into a string just so that we can parse it back later?

To me it seems the issue is that Resolved_package.{local_package,local_fs,git_repo} have a very inconsistent API. local_package takes an OpamFile.OPAM.t whereas git_repo gets a string and a path and constructs an OpamFile.OPAM.t itself.

local_fs does not take an OpamFile.OPAM.t at all, it just takes the path and reads that path from the file system to get an OpamFile.OPAM.t.

In general I think that Resolved_package.{git_repo,local_fs} should take an OpamFile.OPAM.t as argument and how that is constructed (be it through reading from a local path or consulting the rev-store) is up to the caller.

Right, it might suck but let's fix it instead of piling on.

In general I think that Resolved_package.{git_repo,local_fs} should take an OpamFile.OPAM.t as argument and how that is constructed (be it through reading from a local path or consulting the rev-store) is up to the caller.

Yeah, that sounds fine. Make sure to maintain the other functionality that Resolved_package.read_opam_file does though.

Opened #13216 to implement that.

rgrinberg · 2026-01-02T11:49:17Z

Tests seem to all work fine after fixing git_repo so that it can build packages with Dune. Detecting whether a package builds with Dune is a bit fiddly but before git_repo always assumed false so I've retained that for the other invocations of git_repo and in the pinning case I know that the repo to be pinned uses Dune so I can set it to true, thus I can avoid guessing.

Unfortunately, this isn't correct. From reading the code, the origin variable controls from where the pin is coming from and not what kind of a package we're building. In other words, it tells you whether:

The pin originates from a dune-project file
The pin originates from an opam file

One can pin a dune package (a package built with dune) via an opam pin, so we cannot make this assumption:

the pinning case I know that the repo to be pinned uses Dune so I can set it to true, thus I can avoid guessing.

I think the tests are all passing because we simply don't have a test case for this.

The reason why we need to know what kind of a package it is is to generate the correct build command for packages that are built with dune.

Sorry for the confusing naming, I had to understand everything from scratch myself.

Leonidas-from-XIV · 2026-01-02T13:40:07Z

Thanks for the input; this is all very interesting to start to understand how the whole pinning mechanism works!

I guess the description makes it sound like this is a fix, but is it just a refactoring?

Yeah honestly I am a bit confused by the entirety of the code. The code in dune_package in Pin.resolve looks like something that Pinned_package.resolve_package should be doing (and opam_package calls Pinned_package.resolve_package), but when I tried to call it and it failed on discover_layout. So I do think there's something wrong somewhere but I can't really tell how it should actually work.

So maybe it fixes something, but as far I see there is nothing broken, just a Resolved_package.t that seems to be constructed slightly incorrectly. Or, as you point out, what is currently broken doesn't have a test.

One can pin a dune package (a package built with dune) via an opam pin, so we cannot make this assumption:

the pinning case I know that the repo to be pinned uses Dune so I can set it to true, thus I can avoid guessing.

Hmm, in that case I can also implement a similar guessing logic as Resolved_package.local_package does. It seems odd that Resolved_package.git_repo always sets dune_build to false and pinning git repos using Dune only works currently because it is not calling Resolved_package.git_repo but rather Resolved_package.local_package.

rgrinberg · 2026-01-02T15:23:24Z

The code in dune_package in Pin.resolve looks like something that Pinned_package.resolve_package should be doing (and opam_package calls Pinned_package.resolve_package), but when I tried to call it and it failed on discover_layout. So I do think there's something wrong somewhere but I can't really tell how it should actually work.

I think the challenge with that is that Resolved_package is also by packages that aren't pinned too. Briefly, the purpose of this module is a uniform representation of packages that works for packages from the local file system and the revision store. We need that for general repositories as well as pinning

So maybe it fixes something, but as far I see there is nothing broken, just a Resolved_package.t that seems to be constructed slightly incorrectly. Or, as you point out, what is currently broken doesn't have a test.

To re-iterate what i said earlier, although we have a massive amount of tests, they're by no means exhaustive. I think that pinning both dune and opam packages is simply not tested at the moment.

Hmm, in that case I can also implement a similar guessing logic as Resolved_package.local_package does. It seems odd that Resolved_package.git_repo always sets dune_build to false and pinning git repos using Dune only works currently because it is not calling Resolved_package.git_repo but rather Resolved_package.local_package.

That makes no sense to me either and I don't see how it can work. My understanding is that dune_build is what controls how we build a package and has nothing to do with the type of a pin it is. So it can only be determined by inspecting the package contents. I think it's just a bug that can be easily demonstrated.

src/dune_pkg/pin.ml

) Pulled out of #13176. --------- Signed-off-by: Marek Kubica <marek@tarides.com>

…13216) As a follow-up to #13176 (comment) this refactors the `Resolved_package` API so the functions that construct a `Resolved_package.t` take somewhat similar arguments, i.e. an `OpamFile.OPAM.t`. This should make it easier to call the right function at the right place. --------- Signed-off-by: Marek Kubica <marek@tarides.com>

Signed-off-by: Marek Kubica <marek@tarides.com>

…13297) This PR applies #13176 and adds the remaining code to implement the behavior discussed in #13110. The tests change accordingly with the changed semantics of `git` URLs. Closes #13110 --------- Signed-off-by: Marek Kubica <marek@tarides.com>

Leonidas-from-XIV · 2026-01-23T17:15:23Z

Turns out this wasn't really necessary (nor particularly helpful, as the At_rev.t would be pointing to the repo the OPAM file came from and not the source that is to be pinned) to implement #13297, so closing it.

Leonidas-from-XIV requested a review from Alizter January 2, 2026 10:43

Leonidas-from-XIV added the package management label Jan 2, 2026

rgrinberg reviewed Jan 2, 2026

View reviewed changes

rgrinberg requested changes Jan 2, 2026

View reviewed changes

Leonidas-from-XIV force-pushed the resolve-git-package-via-git branch from abb1c93 to 0c447b9 Compare January 2, 2026 14:51

rgrinberg reviewed Jan 2, 2026

View reviewed changes

src/dune_pkg/pin.ml Show resolved Hide resolved

rgrinberg reviewed Jan 2, 2026

View reviewed changes

src/dune_pkg/pin.ml Outdated Show resolved Hide resolved

Leonidas-from-XIV mentioned this pull request Jan 2, 2026

refactor: Rename pinning functions to make their meaning clearer #13178

Merged

rgrinberg pushed a commit that referenced this pull request Jan 2, 2026

refactor: Rename pinning functions to make their meaning clearer (#13178

02ebab6

) Pulled out of #13176. --------- Signed-off-by: Marek Kubica <marek@tarides.com>

Leonidas-from-XIV mentioned this pull request Jan 5, 2026

Make Resolved_package API consistently take an OpamFile.OPAM.t #13216

Merged

Leonidas-from-XIV added 2 commits January 9, 2026 16:30

Use git_repo to pin git packages

80fff2d

Signed-off-by: Marek Kubica <marek@tarides.com>

Hack in dune build detection

21ba4a4

Signed-off-by: Marek Kubica <marek@tarides.com>

Leonidas-from-XIV force-pushed the resolve-git-package-via-git branch from 0c447b9 to 21ba4a4 Compare January 9, 2026 15:33

This was referenced Jan 12, 2026

Implement detection of dune builds in resolved_package #13246

Closed

Write commit hashes into the lock dir when locking git repositories #13297

Merged

Leonidas-from-XIV closed this Jan 23, 2026

Use git_repo to pin git packages #13176

Use git_repo to pin git packages #13176

Conversation

Leonidas-from-XIV commented Jan 2, 2026

Uh oh!

Alizter commented Jan 2, 2026

Uh oh!

Leonidas-from-XIV commented Jan 2, 2026

Uh oh!

rgrinberg commented Jan 2, 2026

Uh oh!

Alizter commented Jan 2, 2026

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgrinberg commented Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Leonidas-from-XIV commented Jan 2, 2026

Uh oh!

rgrinberg commented Jan 2, 2026

Uh oh!

Uh oh!

Uh oh!

Leonidas-from-XIV commented Jan 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Use `git_repo` to pin git packages #13176

Use `git_repo` to pin git packages #13176

rgrinberg commented Jan 2, 2026 •

edited

Loading