Skip to content

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

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
rgrinberg merged 2 commits into from
Jan 2, 2026
Merged

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

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
aa5fef2
refactor: Rename pinning functions to make their meaning clearer
Leonidas-from-XIV Jan 2, 2026
20db977
Make the origin type clearer
Leonidas-from-XIV Jan 2, 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
10 changes: 8 additions & 2 deletions src/dune_pkg/local_package.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
open Import
module Package_constraint = Dune_lang.Package_constraint

module Origin = struct
type t =
| Opam_file
| Pin_stanza
end

type pin =
{ loc : Loc.t
; version : Package_version.t
; url : Loc.t * OpamUrl.t
; name : Package_name.t
; origin : [ `Dune | `Opam ]
; origin : Origin.t
}

type pins = pin Package_name.Map.t
Expand Down Expand Up @@ -190,7 +196,7 @@ let of_package (t : Dune_lang.Package.t) =
Package_version.of_opam_package_version (OpamPackage.version pkg)
in
let loc = Loc.in_file (Path.source file) in
name, { loc; version; url = loc, url; name; origin = `Opam })
name, { loc; version; url = loc, url; name; origin = Opam_file })
|> Package_name.Map.of_list
with
| Ok x -> x
Expand Down
8 changes: 7 additions & 1 deletion src/dune_pkg/local_package.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
open Import

module Origin : sig
type t =
| Opam_file
| Pin_stanza
end

type pin =
{ loc : Loc.t
; version : Package_version.t
; url : Loc.t * OpamUrl.t
; name : Package_name.t
; origin : [ `Dune | `Opam ]
; origin : Origin.t
}

type pins = pin Package_name.Map.t
Expand Down
22 changes: 11 additions & 11 deletions src/dune_pkg/pin.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ module DB = struct
map =
Package_name.Map.fold pins ~init:t.map ~f:(fun (pin : Local_package.pin) acc ->
(match pin.origin with
| `Dune -> Code_error.raise "add_opam_pins: can only pin opam packages" []
| `Opam -> ());
| Pin_stanza -> Code_error.raise "add_opam_pins: can only pin opam packages" []
| Opam_file -> ());
Package_name.Map.update acc pin.name ~f:(function
| None -> Some (pin, t.context)
| Some _ as x -> x))
Expand All @@ -50,7 +50,7 @@ module DB = struct
; map =
Pin_stanza.Project.map pins
|> Package_name.Map.map ~f:(fun (url, pkg) ->
let pkg = local_package_of_pin pkg ~url ~origin:`Dune in
let pkg = local_package_of_pin pkg ~url ~origin:Pin_stanza in
pkg, context)
}
;;
Expand Down Expand Up @@ -117,7 +117,7 @@ module DB = struct
Dune_lang.Pin_stanza.Workspace.map pins
|> String.Map.map ~f:(fun map ->
Package_name.Map.map map ~f:(fun (url, package) ->
local_package_of_pin package ~url ~origin:`Dune))
local_package_of_pin package ~url ~origin:Pin_stanza))
;;

let extract (t : t) ~names =
Expand Down Expand Up @@ -261,7 +261,7 @@ let resolve (t : DB.t) ~(scan_project : Scan_project.t)
; Stack.pp stack
]
in
let opam_package stack (package : Local_package.pin) =
let pinned_via_opam stack (package : Local_package.pin) =
let* resolved_package = Pinned_package.resolve_package package in
resolve package.name resolved_package;
Resolved_package.opam_file resolved_package
Expand All @@ -276,7 +276,7 @@ let resolve (t : DB.t) ~(scan_project : Scan_project.t)
; version
; name
; loc = package.loc
; origin = `Opam
; origin = Opam_file
}
in
let stack = Stack.push stack package in
Expand All @@ -286,7 +286,7 @@ let resolve (t : DB.t) ~(scan_project : Scan_project.t)
|> Fiber.parallel_iter ~f:(fun package ->
Pinned_package.resolve_package package >>| resolve package.name)
in
let dune_package packages (package : Local_package.pin) =
let pinned_via_dune packages (package : Local_package.pin) =
match Package_name.Map.find packages package.name with
| None ->
User_error.raise
Expand Down Expand Up @@ -331,13 +331,13 @@ let resolve (t : DB.t) ~(scan_project : Scan_project.t)
|> Fiber.parallel_iter ~f:(fun (package : Local_package.pin) ->
let stack = Stack.push stack package in
match package.origin with
| `Opam -> opam_package stack package
| `Dune ->
| Opam_file -> pinned_via_opam stack package
| Pin_stanza ->
eval_url package.url
>>= (function
| None -> opam_package stack package
| None -> pinned_via_opam stack package
Copy link
Collaborator Author

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

Choose a reason for hiding this comment

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

@rgrinberg What I don't understand here is why this is calling the opam code path despite package.origin stating Dune?

Copy link
Member

@rgrinberg rgrinberg Jan 2, 2026

Choose a reason for hiding this comment

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

The suggestion here should clarify it #13176 (comment)

Because what is Opam | Dune here is not the type of package but the type of pin that we have. And we can pin both types of packages with both types of pins.

Copy link
Member

@rgrinberg rgrinberg Jan 2, 2026

Choose a reason for hiding this comment

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

To be more precise:

  • a dune package may have both opam pins and pin stanzas

  • an opam package may only have opam pins

  • A pin stanza may refer to a dune or an opam package

  • An opam file pin may refer to a dune or an opam package as well

| Some (more_sources, packages) ->
dune_package packages package;
pinned_via_dune packages package;
let more_sources = DB.add_opam_pins more_sources packages in
loop stack more_sources))
in
Expand Down
Loading