From 4e91684e17bcf6925f82e1145f27b9117c442c8c Mon Sep 17 00:00:00 2001 From: Marek Kubica Date: Mon, 15 Aug 2022 14:55:47 +0200 Subject: [PATCH 1/5] Read package flags from OPAM metadata This changes the logic from hardcoding the compiler "base packages" to instead read the OPAM flags to check whether the package in question is a compiler in which case it will be considered a base package. It also adds code to check for packages whether they are `conf` packages, which could be used to check for virtual packages. Inspired by #327, this would allow `dkml-base-compiler` to be considered a base package and not accidentally vendor it. However it doesn't yet allow using the DKML compiler, because the name of the `ocaml` conf package is still hardcoded with no way to override it (yet). --- lib/config.ml | 12 +----------- lib/opam.ml | 40 +++++++++++++++++++++++++++++--------- lib/opam.mli | 1 + test/lib/test_duniverse.ml | 3 ++- 4 files changed, 35 insertions(+), 21 deletions(-) diff --git a/lib/config.ml b/lib/config.ml index 078887d7b..53eea1ed8 100644 --- a/lib/config.ml +++ b/lib/config.ml @@ -16,17 +16,7 @@ open Import let base_packages = - [ - "jbuilder"; - "dune"; - "ocamlbuild"; - "ocamlmod"; - "oasis"; - "ocamlify"; - "ocaml"; - "ocaml-base-compiler"; - "ocaml-variants"; - ] + [ "jbuilder"; "dune"; "ocamlbuild"; "ocamlmod"; "oasis"; "ocamlify" ] |> List.map ~f:OpamPackage.Name.of_string |> OpamPackage.Name.Set.of_list diff --git a/lib/opam.ml b/lib/opam.ml index a8a3920c9..0cf91eae7 100644 --- a/lib/opam.ml +++ b/lib/opam.ml @@ -174,6 +174,20 @@ 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; @@ -181,18 +195,19 @@ module Package_summary = struct 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 "@[{ 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,25 @@ 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_base_package v = + let in_base_pkgs { package; _ } = + OpamPackage.Name.Set.mem package.name Config.base_packages + in + let is_compiler_pkg { package; _ } = + OpamPackage.Name.equal package.name Config.compiler_package_name + in + is_compiler v || in_base_pkgs v || is_compiler_pkg v end module Dependency_entry = struct diff --git a/lib/opam.mli b/lib/opam.mli index daf28a880..1a93366ff 100644 --- a/lib/opam.mli +++ b/lib/opam.mli @@ -20,6 +20,7 @@ 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 diff --git a/test/lib/test_duniverse.ml b/test/lib/test_duniverse.ml index fdbea1f49..ce3519b90 100644 --- a/test/lib/test_duniverse.ml +++ b/test/lib/test_duniverse.ml @@ -25,7 +25,8 @@ let opam_factory ~name ~version = let summary_factory ?(name = "undefined") ?(version = "1") ?dev_repo ?url_src ?(hashes = []) ?(depexts = []) () = let package = opam_factory ~name ~version in - { Opam.Package_summary.package; dev_repo; url_src; hashes; depexts } + let flags = [] in + { Opam.Package_summary.package; dev_repo; url_src; hashes; depexts; flags } let dependency_factory ?(vendored = true) ?name ?version ?dev_repo ?url_src ?hashes ?depexts () = From b89ea097ff368ad93824c06ec4c8e00c0681a480 Mon Sep 17 00:00:00 2001 From: Marek Kubica Date: Mon, 15 Aug 2022 15:06:13 +0200 Subject: [PATCH 2/5] Describe change in changelog --- CHANGES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index cb4be11a1..fdf57ad8f 100644 --- a/CHANGES.md +++ b/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 From 81a424141e1a225e9bb5a4c8723bb72da88999bb Mon Sep 17 00:00:00 2001 From: Marek Kubica Date: Tue, 22 Nov 2022 15:10:42 +0100 Subject: [PATCH 3/5] Add tests for when packages are flagged as compiler --- test/lib/test_duniverse.ml | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/test/lib/test_duniverse.ml b/test/lib/test_duniverse.ml index ce3519b90..a9ba2d132 100644 --- a/test/lib/test_duniverse.ml +++ b/test/lib/test_duniverse.ml @@ -17,21 +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 - let flags = [] in { 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 } @@ -273,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 = From 37bda0967ca96852dba14dc7a1377c22e3b4d037 Mon Sep 17 00:00:00 2001 From: Marek Kubica Date: Fri, 25 Nov 2022 11:02:27 +0100 Subject: [PATCH 4/5] Replace `base/virtual/compiler` with `safe` The codebase always checks for base and virtual at the same time (sometimes using `is_virtual`, sometimes reimplementing `is_virtual`) so it can be folded into a single function that now also checks for the flags of the package to determine whether it is a compiler. --- lib/duniverse.ml | 21 ++++++++++++++------- lib/lockfile.ml | 3 +-- lib/opam.ml | 16 ++++++++-------- lib/opam.mli | 7 ++++--- lib/opam_solve.ml | 3 +-- 5 files changed, 28 insertions(+), 22 deletions(-) diff --git a/lib/duniverse.ml b/lib/duniverse.ml index 3342eb8e0..feab1068f 100644 --- a/lib/duniverse.ml +++ b/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 = { diff --git a/lib/lockfile.ml b/lib/lockfile.ml index 05e7a0a63..0060b97cf 100644 --- a/lib/lockfile.ml +++ b/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 }) diff --git a/lib/opam.ml b/lib/opam.ml index 0cf91eae7..b9ca8ea69 100644 --- a/lib/opam.ml +++ b/lib/opam.ml @@ -230,14 +230,14 @@ module Package_summary = struct | { dev_repo = None | Some ""; _ } -> true | _ -> false - let is_base_package v = - let in_base_pkgs { package; _ } = - OpamPackage.Name.Set.mem package.name Config.base_packages - in - let is_compiler_pkg { package; _ } = - OpamPackage.Name.equal package.name Config.compiler_package_name - in - is_compiler v || in_base_pkgs v || is_compiler_pkg v + let is_compiler_package { package; _ } = + OpamPackage.Name.equal package.name Config.compiler_package_name + + let is_base_package { package; _ } = + OpamPackage.Name.Set.mem package.name Config.base_packages + + let is_safe_package v = + is_compiler v || is_compiler_package v || is_base_package v || is_virtual v end module Dependency_entry = struct diff --git a/lib/opam.mli b/lib/opam.mli index 1a93366ff..9dd410534 100644 --- a/lib/opam.mli +++ b/lib/opam.mli @@ -26,10 +26,11 @@ module Package_summary : sig 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 diff --git a/lib/opam_solve.ml b/lib/opam_solve.ml index 5ea010b11..18aff4b4c 100644 --- a/lib/opam_solve.ml +++ b/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 From 497d558628dac9902a1fb8349c9d43c041061ab1 Mon Sep 17 00:00:00 2001 From: Marek Kubica Date: Fri, 25 Nov 2022 15:45:09 +0100 Subject: [PATCH 5/5] Rename base packages to skippable packages Also includes a comment on *why* these packages are excluded. Now with the compiler being auto-detected calling them "base" packages was confusing since it was unclear what makes them "base". --- lib/config.ml | 5 ++++- lib/opam.ml | 7 ++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/config.ml b/lib/config.ml index 53eea1ed8..8ef8dfe1d 100644 --- a/lib/config.ml +++ b/lib/config.ml @@ -15,7 +15,10 @@ *) open Import -let base_packages = +(* 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 diff --git a/lib/opam.ml b/lib/opam.ml index b9ca8ea69..b83944d65 100644 --- a/lib/opam.ml +++ b/lib/opam.ml @@ -233,11 +233,12 @@ module Package_summary = struct let is_compiler_package { package; _ } = OpamPackage.Name.equal package.name Config.compiler_package_name - let is_base_package { package; _ } = - OpamPackage.Name.Set.mem package.name Config.base_packages + 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_base_package v || is_virtual v + is_compiler v || is_compiler_package v || is_skippable_package v + || is_virtual v end module Dependency_entry = struct