Merge pull request #328 from Leonidas-from-XIV/base-packages-from-met…

…adata

Read package flags from OPAM metadata

CHANGES.md

@@ -10,6 +10,8 @@
 
 - Fix resolving refs of locally pinned repositories (#326, #332, @hannesm,
   @Leonidas-from-XIV)
+- Read the `compiler` flag from OPAM metadata thus classifying more packages
+  correctly as base packages (#328, @Leonidas-from-XIV)
 
 ### Removed
 

lib/config.ml

@@ -15,18 +15,11 @@
  *)
 open Import
 
-let base_packages =
-  [
-    "jbuilder";
-    "dune";
-    "ocamlbuild";
-    "ocamlmod";
-    "oasis";
-    "ocamlify";
-    "ocaml";
-    "ocaml-base-compiler";
-    "ocaml-variants";
-  ]
+(* These packages are usually used only for legacy building. Since
+   opam-monorepo requires everything to build with dune (which is special
+   cased) we can ignore these packages if they're in the dependency formula *)
+let skip_packages =
+  [ "jbuilder"; "dune"; "ocamlbuild"; "ocamlmod"; "oasis"; "ocamlify" ]
   |> List.map ~f:OpamPackage.Name.of_string
   |> OpamPackage.Name.Set.of_list
 

lib/duniverse.ml

@@ -83,13 +83,20 @@ module Repo = struct
             let* ref = get_default_branch repo in
             Ok (Url.Git { repo; ref })
       in
-      match ps with
-      | _ when is_base_package ps -> Ok None
-      | { url_src = None; _ } | { dev_repo = None; _ } -> Ok None
-      | { url_src = Some url_src; package; dev_repo = Some dev_repo; hashes; _ }
-        ->
-          let* url = url url_src in
-          Ok (Some { opam = package; dev_repo; url; hashes })
+      match is_safe_package ps with
+      | true -> Ok None
+      | false -> (
+          match ps with
+          | {
+           url_src = Some url_src;
+           package;
+           dev_repo = Some dev_repo;
+           hashes;
+           _;
+          } ->
+              let* url = url url_src in
+              Ok (Some { opam = package; dev_repo; url; hashes })
+          | _ -> Ok None)
   end
 
   type 'ref t = {

lib/lockfile.ml

@@ -100,8 +100,7 @@ module Depends = struct
     List.map dependency_entries
       ~f:(fun Opam.Dependency_entry.{ vendored; package_summary } ->
         let vendored =
-          (not @@ Opam.Package_summary.is_base_package package_summary)
-          && (not @@ Opam.Package_summary.is_virtual package_summary)
+          (not @@ Opam.Package_summary.is_safe_package package_summary)
           && vendored
         in
         { vendored; package = package_summary.package })

lib/opam.ml

@@ -174,25 +174,40 @@ module Pp = struct
   let url = Fmt.using OpamUrl.to_string Fmt.string
 end
 
+module Package_flag = struct
+  type t = OpamTypes.package_flag
+
+  let pp pps (v : t) =
+    match v with
+    | Pkgflag_LightUninstall -> Fmt.pf pps "light-uninstall"
+    | Pkgflag_Verbose -> Fmt.pf pps "verbose"
+    | Pkgflag_Plugin -> Fmt.pf pps "plugin"
+    | Pkgflag_Compiler -> Fmt.pf pps "compiler"
+    | Pkgflag_Conf -> Fmt.pf pps "conf"
+    | Pkgflag_AvoidVersion -> Fmt.pf pps "avoid-version"
+    | Pkgflag_Unknown unknown -> Fmt.pf pps "unknown(%s)" unknown
+end
+
 module Package_summary = struct
   type t = {
     package : OpamPackage.t;
     url_src : Url.t option;
     hashes : OpamHash.t list;
     dev_repo : string option;
     depexts : (OpamSysPkg.Set.t * OpamTypes.filter) list;
+    flags : Package_flag.t list;
   }
 
-  let pp fmt { package; url_src; hashes; dev_repo; depexts } =
+  let pp fmt { package; url_src; hashes; dev_repo; depexts; flags } =
     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 }@]"
+       dev_repo = %a;@ depexts = %a;@ flags = %a }@]"
       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
+      dev_repo Depexts.pp depexts (list Package_flag.pp) flags
 
   let from_opam package opam_file =
     let url_field = OpamFile.OPAM.url opam_file in
@@ -204,18 +219,26 @@ module Package_summary = struct
       Option.map ~f:OpamUrl.to_string (OpamFile.OPAM.dev_repo opam_file)
     in
     let depexts = OpamFile.OPAM.depexts opam_file in
-    { package; url_src; hashes; dev_repo; depexts }
+    let flags = OpamFile.OPAM.flags opam_file in
+    { package; url_src; hashes; dev_repo; depexts; flags }
+
+  let has_flag flag { flags; _ } = List.mem flag ~set:flags
+  let is_compiler v = has_flag OpamTypes.Pkgflag_Compiler v
 
   let is_virtual = function
     | { url_src = None; _ } -> true
     | { dev_repo = None | Some ""; _ } -> true
     | _ -> false
 
-  let is_base_package = function
-    | { package; _ }
-      when OpamPackage.Name.Set.mem package.name Config.base_packages ->
-        true
-    | _ -> false
+  let is_compiler_package { package; _ } =
+    OpamPackage.Name.equal package.name Config.compiler_package_name
+
+  let is_skippable_package { package; _ } =
+    OpamPackage.Name.Set.mem package.name Config.skip_packages
+
+  let is_safe_package v =
+    is_compiler v || is_compiler_package v || is_skippable_package v
+    || is_virtual v
 end
 
 module Dependency_entry = struct

lib/opam.mli

@@ -20,15 +20,17 @@ module Package_summary : sig
     hashes : OpamHash.t list;
     dev_repo : string option;
     depexts : (OpamSysPkg.Set.t * OpamTypes.filter) list;
+    flags : OpamTypes.package_flag list;
   }
 
   val pp : t Fmt.t
   val from_opam : OpamPackage.t -> OpamFile.OPAM.t -> t
 
-  val is_virtual : t -> bool
-  (** A package is considered virtual if it has no url.src or no dev-repo. *)
+  val is_safe_package : t -> bool
+  (** A package is safe when it is clear that it can be added to the lockfile
+      without disruption, as it will not get pulled.
 
-  val is_base_package : t -> bool
+      The OCaml compiler is one such package. *)
 end
 
 module Dependency_entry : sig

lib/opam_solve.ml

@@ -97,8 +97,7 @@ module Opam_monorepo_context (Base_context : BASE_CONTEXT) :
     in
     let summary = Opam.Package_summary.from_opam pkg opam_file in
     let is_valid_dune_wise =
-      Opam.Package_summary.is_base_package summary
-      || Opam.Package_summary.is_virtual summary
+      Opam.Package_summary.is_safe_package summary
       || (not require_dune) || uses_dune
     in
     match is_valid_dune_wise with

test/lib/test_duniverse.ml

@@ -17,20 +17,25 @@ module Testable = struct
   end
 end
 
+module Flag = struct
+  let compiler = OpamTypes.Pkgflag_Compiler
+  let conf = OpamTypes.Pkgflag_Conf
+end
+
 let opam_factory ~name ~version =
   let name = OpamPackage.Name.of_string name in
   let version = OpamPackage.Version.of_string version in
   OpamPackage.create name version
 
 let summary_factory ?(name = "undefined") ?(version = "1") ?dev_repo ?url_src
-    ?(hashes = []) ?(depexts = []) () =
+    ?(hashes = []) ?(depexts = []) ?(flags = []) () =
   let package = opam_factory ~name ~version in
-  { Opam.Package_summary.package; dev_repo; url_src; hashes; depexts }
+  { Opam.Package_summary.package; dev_repo; url_src; hashes; depexts; flags }
 
 let dependency_factory ?(vendored = true) ?name ?version ?dev_repo ?url_src
-    ?hashes ?depexts () =
+    ?hashes ?depexts ?flags () =
   let package_summary =
-    summary_factory ?name ?version ?dev_repo ?url_src ?hashes ?depexts ()
+    summary_factory ?name ?version ?dev_repo ?url_src ?hashes ?depexts ?flags ()
   in
   { Opam.Dependency_entry.vendored; package_summary }
 
@@ -272,6 +277,22 @@ let test_from_dependency_entries =
              };
            ])
       ();
+    make_test ~name:"Excludes compilers"
+      ~dependency_entries:
+        [
+          dependency_factory ~name:"mycaml" ~version:"5" ~url_src:(Other "u")
+            ~dev_repo:"d" ~flags:[ Flag.compiler ] ();
+        ]
+      ~expected:(Ok []) ();
+    make_test ~name:"Excludes anything that is a compiler"
+      ~dependency_entries:
+        [
+          dependency_factory ~name:"conf-mycaml" ~version:"5"
+            ~url_src:(Other "u") ~dev_repo:"d"
+            ~flags:[ Flag.compiler; Flag.conf ]
+            ();
+        ]
+      ~expected:(Ok []) ();
   ]
 
 let suite =