Skip to content

Fix: merge multiple identical opam pins on a local package #13232

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.

Sign up for GitHub

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

Merged
ElectreAAS merged 2 commits into from
Jan 9, 2026
Merged

Fix: merge multiple identical opam pins on a local package #13232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aee8b4e
Expand test to show how different pin types interact
ElectreAAS Jan 6, 2026
5884666
Fix: multiple identical local pins couldn't be merged.
ElectreAAS Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 33 additions & 11 deletions src/dune_pkg/pinned_package.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,40 @@
open Import
open Fiber.O

let collect (local_packages : Local_package.t Package_name.Map.t) =
match
Package_name.Map.values local_packages
|> List.concat_map ~f:(fun (local_package : Local_package.t) ->
Package_name.Map.to_list_map local_package.pins ~f:(fun name pin -> name, pin))
|> Package_name.Map.of_list
with
| Ok s -> s
| Error (_, pin, _) ->
let merge_pins name (pin1 : Local_package.pin) (pin2 : Local_package.pin) =
let loc1, url1 = pin1.url in
let loc2, url2 = pin2.url in
if Package_version.equal pin1.version pin2.version
then
if OpamUrl.equal url1 url2
then pin1
else
User_error.raise
~loc:loc1
[ Pp.textf
"local package %s is pinned twice with different URLs"
(Package_name.to_string name)
; Pp.textf "it is also defined in %s" (Loc.to_file_colon_line loc2)
]
else
User_error.raise
~loc:pin.loc
[ Pp.textf "local package %s cannot be pinned" (Package_name.to_string pin.name) ]
~loc:loc1
[ Pp.textf
"local package %s is pinned here with version %s, but also version %s in %s"
(Package_name.to_string name)
(Package_version.to_string pin1.version)
(Package_version.to_string pin2.version)
(Loc.to_file_colon_line loc2)
]
;;

let collect (local_packages : Local_package.t Package_name.Map.t) =
Package_name.Map.values local_packages
|> List.concat_map ~f:(fun (local_package : Local_package.t) ->
Package_name.Map.to_list_map local_package.pins ~f:(fun name pin -> name, pin))
|> List.sort ~compare:(fun (_, (pin1 : Local_package.pin)) (_, pin2) ->
Loc.compare pin1.loc pin2.loc)
|> Package_name.Map.of_list_reducei ~f:merge_pins
;;

(* There's many layouts for pinned packages:
Expand Down
88 changes: 86 additions & 2 deletions test/blackbox-tests/test-cases/pkg/pin-stanza/pin-depends-multiple.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Having multiple opam packages that all depend on one pinned pkg in
the workspace causes confusion
the workspace caused confusion

$ mkrepo
$ add_mock_repo_if_needed
Expand Down Expand Up @@ -28,7 +28,91 @@ Main left & right both depend on the trouble maker package in their opam file
> pin-depends: [ "trouble.1.0.0" "file://$PWD/trouble" ]
> EOF

This now works!
$ dune_pkg_lock_normalized
Solution for dune.lock:
- trouble.1.0.0

If the versions disagree, we give a clear error
$ cat > main_left.opam << EOF
> opam-version: "2.0"
> build: [ "echo" "main_left" ]
> depends: [ "trouble" ]
> pin-depends: [ "trouble.9.9.9" "file://$PWD/trouble" ]
> EOF

$ dune_pkg_lock_normalized
File "main_left.opam", line 1, characters 0-0:
Error: local package trouble is pinned here with version 9.9.9, but also
version 1.0.0 in main_right.opam:1
[1]

If the urls disagree, we also give a clear error
$ cat > main_left.opam << EOF
> opam-version: "2.0"
> build: [ "echo" "main_left" ]
> depends: [ "trouble" ]
> pin-depends: [ "trouble.1.0.0" "file://$PWD/other" ]
> EOF

$ dune_pkg_lock_normalized
File "main_left.opam", line 1, characters 0-0:
Error: local package trouble is pinned twice with different URLs
it is also defined in main_right.opam:1
[1]

If instead of two opam pins we have one opam pin and one dune pin
$ cat > main_left.opam << EOF
> opam-version: "2.0"
> build: [ "echo" "main_left" ]
> depends: [ "trouble" ]
> pin-depends: [ "trouble.1.0.0" "file://$PWD/trouble" ]
> EOF

$ rm main_right.opam
$ mkdir right
$ cat > right/dune-project << EOF
> (lang dune 3.14)
> (pin
> (url "file://$PWD/trouble")
> (package
> (name trouble)
> (version "1.0.0")))
> (package
> (name main_right)
> (depends trouble))
> EOF

This works!
$ dune_pkg_lock_normalized
Solution for dune.lock:
- trouble.1.0.0

If the versions disagree, we favor the opam info?
$ cat > main_left.opam << EOF
> opam-version: "2.0"
> build: [ "echo" "main_left" ]
> depends: [ "trouble" ]
> pin-depends: [ "trouble.0.0.1" "file://$PWD/trouble" ]
> EOF

We pick the opam version here.
$ dune_pkg_lock_normalized
Solution for dune.lock:
- trouble.0.0.1
Copy link
Collaborator Author

@ElectreAAS ElectreAAS Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very dubious to me, especially since there is this comment in a related test:

dune/test/blackbox-tests/test-cases/pkg/pin-stanza/mixed-opam-dune.t

Lines 1 to 2 in 4cc2b86

We try to use a project that has both opam files and a dune-project file. We
should favor the dune metadata in such a case.

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is quite dubious. I would say if the versions disagree we should error and not pick sides, to be consistent with how it works when it is pinned twice via dune.

But this isn't introduced by this code, right? In such case it could be fixed in a separate PR.

Copy link
Collaborator Author

@ElectreAAS ElectreAAS Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this wasn't introduced by this PR, it was already present behaviour

Copy link
Member

@rgrinberg rgrinberg Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it dubious? This is exactly what we do everywhere else. The opam pins are there for opam users presumably.

Copy link
Collaborator Author

@ElectreAAS ElectreAAS Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment in mixed-opam-dune says we should favor the dune metadata when there is both opam and dune info
Here that is not what is happening, we favor the opam info

If you think that's fine, I'll leave it as is


If the urls disagree, we give a somewhat clear error
$ cat > main_left.opam << EOF
> opam-version: "2.0"
> build: [ "echo" "main_left" ]
> depends: [ "trouble" ]
> pin-depends: [ "trouble.1.0.0" "file://$PWD/other" ]
> EOF

$ dune_pkg_lock_normalized
File "main_left.opam", line 1, characters 0-0:
Error: local package trouble cannot be pinned
Error: unable to discover an opam file for package trouble
[1]

For a test covering the case where we have two pins in dune-projects,
check override-single-workspace.t
Loading