Merge pull request #376 from Leonidas-from-XIV/virtual-pkg-install

Mark packages as virtual when they have neither `build` nor `install`

CHANGES.md

@@ -27,6 +27,10 @@
 - Fix assertion failure when prefix of "lock" subcommand is used (#381,
   @gridbugs)
 
+- Treat packages without build commands as virtual only if also lack install
+  commands, as some non-virtual packages might only have install commands.
+  (#376 @Leonidas-from-XIV, @gridbugs)
+
 ### Removed
 
 ### Security

lib/opam.ml

@@ -129,14 +129,6 @@ module Hash = struct
     Format.fprintf fmt "%s:%s" kind contents
 end
 
-module Command = struct
-  type t = OpamTypes.command
-
-  let pp fmt (t : t) =
-    OpamPp.print OpamFormat.V.command t
-    |> OpamPrinter.FullPos.value |> Format.pp_print_string fmt
-end
-
 module Depexts = struct
   let pp fmt t =
     let pp_filter fmt filter =
@@ -204,21 +196,32 @@ module Package_summary = struct
     dev_repo : string option;
     depexts : (OpamSysPkg.Set.t * OpamTypes.filter) list;
     flags : Package_flag.t list;
-    build_commands : OpamTypes.command list;
+    has_build_commands : bool;
+    has_install_commands : bool;
   }
 
   let pp fmt
-      { package; url_src; hashes; dev_repo; depexts; flags; build_commands } =
+      {
+        package;
+        url_src;
+        hashes;
+        dev_repo;
+        depexts;
+        flags;
+        has_build_commands;
+        has_install_commands;
+      } =
     let open Pp_combinators.Ocaml in
     Format.fprintf fmt
       "@[<hov 2>{ name = %a;@ version = %a;@ url_src = %a;@ hashes = %a;@ \
-       dev_repo = %a;@ depexts = %a;@ flags = %a;@ build_commands = %a }@]"
+       dev_repo = %a;@ depexts = %a;@ flags = %a;@ has_build_commands = %B;@ \
+       has_install_commands = %B}@]"
       Pp.package_name package.name Pp.version package.version
       (option ~brackets:true Url.pp)
       url_src (list Hash.pp) hashes
       (option ~brackets:true string)
-      dev_repo Depexts.pp depexts (list Package_flag.pp) flags (list Command.pp)
-      build_commands
+      dev_repo Depexts.pp depexts (list Package_flag.pp) flags
+      has_build_commands has_install_commands
 
   let from_opam package opam_file =
     let url_field = OpamFile.OPAM.url opam_file in
@@ -231,8 +234,21 @@ module Package_summary = struct
     in
     let depexts = OpamFile.OPAM.depexts opam_file in
     let flags = OpamFile.OPAM.flags opam_file in
-    let build_commands = OpamFile.OPAM.build opam_file in
-    { package; url_src; hashes; dev_repo; depexts; flags; build_commands }
+    let has_commands = function [] -> false | _ -> true in
+    let has_build_commands = opam_file |> OpamFile.OPAM.build |> has_commands in
+    let has_install_commands =
+      opam_file |> OpamFile.OPAM.install |> has_commands
+    in
+    {
+      package;
+      url_src;
+      hashes;
+      dev_repo;
+      depexts;
+      flags;
+      has_build_commands;
+      has_install_commands;
+    }
 
   let has_flag flag { flags; _ } = List.mem flag ~set:flags
   let is_compiler v = has_flag OpamTypes.Pkgflag_Compiler v
@@ -241,7 +257,7 @@ module Package_summary = struct
   let is_virtual = function
     | { url_src = None; _ } -> true
     | { dev_repo = None | Some ""; _ } as pkg when is_conf pkg -> true
-    | { build_commands = []; _ } -> true
+    | { has_build_commands = false; has_install_commands = false; _ } -> true
     | _ -> false
 
   let is_compiler_package { package; _ } =

lib/opam.mli

@@ -21,7 +21,8 @@ module Package_summary : sig
     dev_repo : string option;
     depexts : (OpamSysPkg.Set.t * OpamTypes.filter) list;
     flags : OpamTypes.package_flag list;
-    build_commands : OpamTypes.command list;
+    has_build_commands : bool;
+    has_install_commands : bool;
   }
 
   val pp : t Fmt.t

test/lib/test_duniverse.ml

@@ -28,7 +28,8 @@ let opam_factory ~name ~version =
   OpamPackage.create name version
 
 let summary_factory ?(name = "undefined") ?(version = "1") ?dev_repo ?url_src
-    ?(hashes = []) ?(depexts = []) ?(flags = []) ?(build_commands = []) () =
+    ?(hashes = []) ?(depexts = []) ?(flags = []) ?(has_build_commands = false)
+    ?(has_install_commands = false) () =
   let package = opam_factory ~name ~version in
   {
     Opam.Package_summary.package;
@@ -37,14 +38,15 @@ let summary_factory ?(name = "undefined") ?(version = "1") ?dev_repo ?url_src
     hashes;
     depexts;
     flags;
-    build_commands;
+    has_build_commands;
+    has_install_commands;
   }
 
 let dependency_factory ?(vendored = true) ?name ?version ?dev_repo ?url_src
-    ?hashes ?depexts ?flags ?build_commands () =
+    ?hashes ?depexts ?flags ?has_build_commands ?has_install_commands () =
   let package_summary =
     summary_factory ?name ?version ?dev_repo ?url_src ?hashes ?depexts ?flags
-      ?build_commands ()
+      ?has_build_commands ?has_install_commands ()
   in
   { Opam.Dependency_entry.vendored; package_summary }
 
@@ -67,6 +69,21 @@ module Repo = struct
         in
         (test_name, `Quick, test_fun)
       in
+      let simple_summary ?has_build_commands ?has_install_commands =
+        summary_factory ~dev_repo:"d" ~url_src:(Other "u") ~name:"y"
+          ~version:"v" ~hashes:[] ?has_build_commands ?has_install_commands
+      in
+      let simple_package =
+        Ok
+          (Some
+             Duniverse.Repo.Package.
+               {
+                 opam = opam_factory ~name:"y" ~version:"v";
+                 dev_repo = "d";
+                 url = Other "u";
+                 hashes = [];
+               })
+      in
       [
         make_test ~name:"Base package"
           ~summary:(summary_factory ~name:"dune" ())
@@ -78,30 +95,29 @@ module Repo = struct
           ~summary:(summary_factory ?dev_repo:None ())
           ~expected:(Ok None) ();
         make_test ~name:"Regular"
+          ~summary:(simple_summary ~has_build_commands:true ())
+          ~expected:simple_package ();
+        make_test ~name:"Has only install commands"
+          ~summary:(simple_summary ~has_install_commands:true ())
+          ~expected:simple_package ();
+        make_test ~name:"Has both build & install commands"
           ~summary:
-            (summary_factory ~dev_repo:"d" ~url_src:(Other "u") ~name:"y"
-               ~version:"v" ~hashes:[]
-               ~build_commands:[ ([], None) ]
+            (simple_summary ~has_build_commands:true ~has_install_commands:true
                ())
-          ~expected:
-            (Ok
-               (Some
-                  {
-                    opam = opam_factory ~name:"y" ~version:"v";
-                    dev_repo = "d";
-                    url = Other "u";
-                    hashes = [];
-                  }))
-          ();
+          ~expected:simple_package ();
+        make_test
+          ~name:"Has neither build nor install commands: virtual package"
+          ~summary:
+            (simple_summary ~has_build_commands:false
+               ~has_install_commands:false ())
+          ~expected:(Ok None) ();
         make_test ~name:"Uses default branch when no tag"
           ~get_default_branch:(function
             | "r" -> Ok "master" | _ -> assert false)
           ~summary:
             (summary_factory ~dev_repo:"d"
                ~url_src:(Git { repo = "r"; ref = None })
-               ~name:"y" ~version:"v" ~hashes:[]
-               ~build_commands:[ ([], None) ]
-               ())
+               ~name:"y" ~version:"v" ~hashes:[] ~has_build_commands:true ())
           ~expected:
             (Ok
                (Some
@@ -260,9 +276,24 @@ let test_from_dependency_entries =
       ~dependency_entries:
         [
           dependency_factory ~name:"x" ~version:"v" ~url_src:(Other "u")
-            ~dev_repo:"d" ~hashes:[]
-            ~build_commands:[ ([], None) ]
-            ();
+            ~dev_repo:"d" ~hashes:[] ~has_build_commands:true ();
+        ]
+      ~expected:
+        (Ok
+           [
+             {
+               dir = "d";
+               url = Other "u";
+               hashes = [];
+               provided_packages = [ opam_factory ~name:"x" ~version:"v" ];
+             };
+           ])
+      ();
+    make_test ~name:"Package with just install commands"
+      ~dependency_entries:
+        [
+          dependency_factory ~name:"x" ~version:"v" ~url_src:(Other "u")
+            ~dev_repo:"d" ~hashes:[] ~has_install_commands:true ();
         ]
       ~expected:
         (Ok
@@ -286,13 +317,9 @@ let test_from_dependency_entries =
       ~dependency_entries:
         [
           dependency_factory ~name:"y" ~version:"v" ~url_src:(Other "u")
-            ~dev_repo:"d" ~hashes:[]
-            ~build_commands:[ ([], None) ]
-            ();
+            ~dev_repo:"d" ~hashes:[] ~has_build_commands:true ();
           dependency_factory ~name:"y-lwt" ~version:"v" ~url_src:(Other "u")
-            ~dev_repo:"d" ~hashes:[]
-            ~build_commands:[ ([], None) ]
-            ();
+            ~dev_repo:"d" ~hashes:[] ~has_build_commands:true ();
         ]
       ~expected:
         (Ok