fix: use build and install instructions from opam on pinning (#11513)

* fix: use build and install instructions from opam on pinning

When building instructions, pinning packages relied on dune for all the
cases. This commit changes this behavior: if there is an opam file, it
extracts the instructions from it.

Signed-off-by: Etienne Marais <[email protected]>
Co-authored-by: ArthurW <[email protected]>
Signed-off-by: Etienne Marais <[email protected]>

* test: upgrade test with expected output

Signed-off-by: Etienne Marais <[email protected]>

* test: add missing test case

Signed-off-by: Etienne Marais <[email protected]>

* fix: use sum type instead of list

Signed-off-by: Etienne Marais <[email protected]>

* doc: add type explanation

Signed-off-by: Etienne Marais <[email protected]>

* refactor(naming): change to clearer names

Signed-off-by: Etienne Marais <[email protected]>

---------

Signed-off-by: Etienne Marais <[email protected]>
Co-authored-by: ArthurW <[email protected]>

Author: maiste

bin/lock_dev_tool.ml

@@ -61,6 +61,7 @@ let make_local_package_wrapping_dev_tool ~dev_tool ~dev_tool_version ~extra_depe
   ; pins = Package_name.Map.empty
   ; conflict_class = []
   ; loc = Loc.none
+  ; command_source = Opam_file { build = []; install = [] }
   }
 ;;
 

src/dune_pkg/local_package.ml

@@ -12,6 +12,13 @@ type pin =
 
 type pins = pin Package_name.Map.t
 
+type command_source =
+  | Assume_defaults
+  | Opam_file of
+      { build : OpamTypes.command list
+      ; install : OpamTypes.command list
+      }
+
 type t =
   { name : Package_name.t
   ; version : Package_version.t option
@@ -21,6 +28,7 @@ type t =
   ; depopts : Package_dependency.t list
   ; pins : pins
   ; loc : Loc.t
+  ; command_source : command_source
   }
 
 module Dependency_hash = struct
@@ -56,9 +64,21 @@ module For_solver = struct
     ; depopts : Package_dependency.t list
     ; conflict_class : Package_name.t list
     ; pins : pins
+    ; build : OpamTypes.command list
+    ; install : OpamTypes.command list
     }
 
-  let to_opam_file { name; dependencies; conflicts; conflict_class; depopts; pins = _ } =
+  let to_opam_file
+        { name
+        ; dependencies
+        ; conflicts
+        ; conflict_class
+        ; depopts
+        ; pins = _
+        ; build
+        ; install
+        }
+    =
     (* CR-rgrinberg: it's OK to ignore pins here since the solver doesn't touch
        them *)
     OpamFile.OPAM.empty
@@ -72,6 +92,8 @@ module For_solver = struct
     |> OpamFile.OPAM.with_depopts
          (List.map depopts ~f:Package_dependency.to_opam_filtered_formula
           |> OpamFormula.ands)
+    |> OpamFile.OPAM.with_install install
+    |> OpamFile.OPAM.with_build build
   ;;
 
   let non_local_dependencies local_deps =
@@ -93,9 +115,23 @@ let for_solver
       ; loc = _
       ; depopts
       ; pins
+      ; command_source
       }
   =
-  { For_solver.name; dependencies; conflicts; conflict_class; depopts; pins }
+  let build, install =
+    match command_source with
+    | Assume_defaults -> [], []
+    | Opam_file { build; install } -> build, install
+  in
+  { For_solver.name
+  ; dependencies
+  ; conflicts
+  ; conflict_class
+  ; depopts
+  ; pins
+  ; build
+  ; install
+  }
 ;;
 
 let of_package (t : Dune_lang.Package.t) =
@@ -114,11 +150,18 @@ let of_package (t : Dune_lang.Package.t) =
     ; loc
     ; conflict_class = []
     ; pins = Package_name.Map.empty
+    ; command_source = Assume_defaults
     }
   | Some { file; contents = opam_file_string } ->
     let opam_file =
       Opam_file.read_from_string_exn ~contents:opam_file_string (Path.source file)
     in
+    let command_source =
+      Opam_file
+        { build = opam_file |> OpamFile.OPAM.build
+        ; install = opam_file |> OpamFile.OPAM.install
+        }
+    in
     let dependencies =
       opam_file |> OpamFile.OPAM.depends |> Dependency_formula.of_filtered_formula
     in
@@ -150,5 +193,14 @@ let of_package (t : Dune_lang.Package.t) =
           ~loc:pkg.loc
           [ Pp.textf "package %s is already pinned" (Package_name.to_string pkg.name) ]
     in
-    { name; version; dependencies; conflicts; depopts; loc; conflict_class; pins }
+    { name
+    ; version
+    ; dependencies
+    ; conflicts
+    ; depopts
+    ; loc
+    ; conflict_class
+    ; pins
+    ; command_source
+    }
 ;;

src/dune_pkg/local_package.mli

@@ -10,6 +10,18 @@ type pin =
 
 type pins = pin Package_name.Map.t
 
+(** Describe the commands to execute from the source. [Assume_defaults] means
+    that no opam file is available and dune will use [dune] to build the
+    package. [Opam_file { build ; install }]  describe where there is an opam
+    file from which we extract the build and the install command. Note that
+    both of them can be empty. *)
+type command_source =
+  | Assume_defaults
+  | Opam_file of
+      { build : OpamTypes.command list
+      ; install : OpamTypes.command list
+      }
+
 (** Information about a local package that's relevant for package management.
     This is intended to represent local packages defined in a dune-project file
     (rather than packages in a lockdir). This is distinct from a
@@ -26,6 +38,7 @@ type t =
   ; depopts : Package_dependency.t list
   ; pins : pins
   ; loc : Loc.t
+  ; command_source : command_source
   }
 
 module Dependency_hash : sig
@@ -48,6 +61,8 @@ module For_solver : sig
     ; depopts : Package_dependency.t list
     ; conflict_class : Package_name.t list
     ; pins : pins
+    ; build : OpamTypes.command list
+    ; install : OpamTypes.command list
     }
 
   (** [to_opam_file t] returns an [OpamFile.OPAM.t] whose fields are based on the

src/dune_pkg/pin_stanza.ml

@@ -349,8 +349,9 @@ let resolve (t : DB.t) ~(scan_project : Scan_project.t)
         ]
     | Some pkg ->
       let resolved_package =
+        let local_package = Local_package.of_package pkg in
         let opam_file =
-          Local_package.of_package pkg
+          local_package
           |> Local_package.for_solver
           |> Local_package.For_solver.to_opam_file
           |> OpamFile.OPAM.with_url (OpamFile.URL.create (snd package.url))
@@ -360,7 +361,11 @@ let resolve (t : DB.t) ~(scan_project : Scan_project.t)
             (Package_name.to_opam_package_name package.name)
             (Package_version.to_opam_package_version package.version)
         in
-        Resolved_package.dune_package package.loc opam_file opam_package
+        Resolved_package.local_package
+          ~command_source:local_package.command_source
+          package.loc
+          opam_file
+          opam_package
       in
       resolve package.name resolved_package
   in

src/dune_pkg/resolved_package.ml

@@ -84,10 +84,15 @@ let scan_files_entries path =
          ])
 ;;
 
-let dune_package loc opam_file opam_package =
+let local_package ~command_source loc opam_file opam_package =
+  let dune_build =
+    match (command_source : Local_package.command_source) with
+    | Assume_defaults -> true
+    | Opam_file _ -> false
+  in
   let opam_file = add_opam_package_to_opam_file opam_package opam_file in
   let package = OpamFile.OPAM.package opam_file in
-  { dune_build = true; opam_file; package; loc; extra_files = Inside_files_dir None }
+  { dune_build; opam_file; package; loc; extra_files = Inside_files_dir None }
 ;;
 
 open Fiber.O

src/dune_pkg/resolved_package.mli

@@ -23,5 +23,11 @@ val local_fs
   -> files_dir:Path.Local.t option
   -> t
 
-val dune_package : Loc.t -> OpamFile.OPAM.t -> OpamPackage.t -> t
+val local_package
+  :  command_source:Local_package.command_source
+  -> Loc.t
+  -> OpamFile.OPAM.t
+  -> OpamPackage.t
+  -> t
+
 val get_opam_package_files : t list -> File_entry.t list list Fiber.t

test/blackbox-tests/test-cases/pkg/pin-stanza/build-command-dune-project.t

@@ -5,20 +5,20 @@ Demonstrate the build command we construct for different types of projects:
   $ mkrepo
   $ add_mock_repo_if_needed
 
-  $ mkdir _template _dune-only _mixed
+  $ mkdir _template _dune-only _mixed _opam-only
 
   $ cat >_template/dune-project <<EOF
   > (lang dune 3.13)
   > (generate_opam_files true)
-  > (package (name template))
+  > (package (name template)  (allow_empty))
   > EOF
   $ cat >_template/mixed.opam.template <<EOF
   > build: [ "echo" "template" ]
   > EOF
 
   $ cat >_dune-only/dune-project <<EOF
   > (lang dune 3.13)
-  > (package (name dune-only))
+  > (package (name dune-only)  (allow_empty))
   > EOF
 
   $ cat >_mixed/dune-project <<EOF
@@ -29,6 +29,11 @@ Demonstrate the build command we construct for different types of projects:
   > build: [ "echo" "mixed" ]
   > EOF
 
+  $ cat > _opam-only/opam-only.opam <<EOF
+  > opam-version: "2.0"
+  > build: [ "echo" "opam only" ]
+  > EOF
+
   $ cat >dune-project <<EOF
   > (lang dune 3.13)
   > (pin
@@ -40,22 +45,35 @@ Demonstrate the build command we construct for different types of projects:
   > (pin
   >  (url "$PWD/_mixed")
   >  (package (name mixed)))
+  > (pin
+  >  (url "$PWD/_opam-only")
+  >  (package (name opam-only)))
   > (package
   >  (name main)
-  >  (depends dune-only mixed template))
+  >  (allow_empty)
+  >  (depends dune-only mixed template opam-only))
   > EOF
 
   $ dune pkg lock
   Solution for dune.lock:
   - dune-only.dev
   - mixed.dev
+  - opam-only.dev
   - template.dev
   $ build_command() {
-  > grep "(dune)" dune.lock/$1.pkg
+  > grep "$1" dune.lock/$2.pkg
   > }
-  $ build_command dune-only
-  (dune)
-  $ build_command mixed
+  $ build_command "(dune)" dune-only
   (dune)
-  $ build_command template
+  $ build_command "(dune)" template
   (dune)
+  $ build_command "(build" mixed
+  (build
+  $ build_command "(build" opam-only
+  (build
+
+If we build the deps, everything works fine and we see the output of the opam
+pins:
+  $ dune build @pkg-install
+  mixed
+  opam only