From e353136410e4816e7d0671cc8b54f473e8b1f90a Mon Sep 17 00:00:00 2001 From: Marek Kubica Date: Tue, 8 Mar 2022 15:38:28 +0100 Subject: [PATCH 1/4] Check OCaml version of lockfile against the switch version --- CHANGES.md | 2 ++ cli/pull.ml | 27 ++++++++++++++++++++++++--- lib/config.ml | 2 ++ lib/lockfile.ml | 7 +++++++ lib/lockfile.mli | 2 ++ stdext/result.ml | 6 ++++++ stdext/result.mli | 4 ++++ 7 files changed, 47 insertions(+), 3 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index fecb8a3e5..96f3cc1fe 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -12,6 +12,8 @@ `opam-monorepo`. To control this a new optional Opam file field, `x-opam-monorepo-opam-provided` is introduced. Its value is a list of package names that are to be excluded from being pulled (#234, @Leonidas-from-XIV) +- Show an error message when the OCaml version of the lock file does not match + the OCaml version of the switch (#267, #268, @Leonidas-from-XIV) ### Changed diff --git a/cli/pull.ml b/cli/pull.ml index 3baa4da3b..ec70cce75 100644 --- a/cli/pull.ml +++ b/cli/pull.ml @@ -65,9 +65,30 @@ let run (`Yes yes) (`Root root) (`Lockfile explicit_lockfile) Common.filter_duniverse ~to_consider:duniverse_repos duniverse in let* () = check_dune_lang_version ~yes ~root in - OpamGlobalState.with_ `Lock_none (fun global_state -> - Pull.duniverse ~global_state ~root ~full - ~trim_clone:(not keep_git_dir) duniverse) + OpamGlobalState.with_ `Lock_none @@ fun global_state -> + let* locked_ocaml_version = + Lockfile.ocaml_version lockfile + |> Result.of_option ~error:(`Msg "OCaml compiler not in lockfile") + in + OpamSwitchState.with_ `Lock_none global_state @@ fun switch_state -> + let switch_ocaml_version = + OpamSwitchState.get_package switch_state Config.compiler_package_name + |> OpamPackage.version + in + let* () = + Result.ok_if_true + (OpamPackage.Version.equal locked_ocaml_version switch_ocaml_version) + ~error: + (let msg = + Fmt.str + "OCaml versions do not match: %a in lockfile, but %a in switch" + Opam.Pp.version locked_ocaml_version Opam.Pp.version + switch_ocaml_version + in + `Msg msg) + in + Pull.duniverse ~global_state ~root ~full ~trim_clone:(not keep_git_dir) + duniverse let info = let open Cmdliner in diff --git a/lib/config.ml b/lib/config.ml index ec95e70af..71a57ae3a 100644 --- a/lib/config.ml +++ b/lib/config.ml @@ -30,6 +30,8 @@ let base_packages = |> List.map ~f:OpamPackage.Name.of_string |> OpamPackage.Name.Set.of_list +let compiler_package_name = OpamPackage.Name.of_string "ocaml-base-compiler" + let duniverse_opam_repo = "git+https://github.com/dune-universe/opam-overlays.git" diff --git a/lib/lockfile.ml b/lib/lockfile.ml index 3fa070a19..92f432cc5 100644 --- a/lib/lockfile.ml +++ b/lib/lockfile.ml @@ -287,6 +287,13 @@ let create ~source_config ~root_packages ~dependency_entries ~root_depexts source_config; } +let ocaml_version { depends; _ } = + List.find_map depends ~f:(fun { Depends.package; vendored = _ } -> + let name = OpamPackage.name package in + match OpamPackage.Name.equal name Config.compiler_package_name with + | true -> Some (OpamPackage.version package) + | false -> None) + let url_to_duniverse_url url = let url_res = Duniverse.Repo.Url.from_opam_url url in Result.map_error url_res ~f:(function `Msg msg -> diff --git a/lib/lockfile.mli b/lib/lockfile.mli index a97a0b569..5c0f44814 100644 --- a/lib/lockfile.mli +++ b/lib/lockfile.mli @@ -11,6 +11,8 @@ val create : val to_duniverse : t -> (Duniverse.t, [ `Msg of string ]) result +val ocaml_version : t -> OpamPackage.Version.t option + val save : opam_monorepo_cwd:Fpath.t -> file:Fpath.t -> diff --git a/stdext/result.ml b/stdext/result.ml index 41f736374..63902b24f 100644 --- a/stdext/result.ml +++ b/stdext/result.ml @@ -4,6 +4,12 @@ let bind ~f r = bind r f let map ~f = map f +(* some useful functions inspired by Jane Street Base *) + +let of_option v ~error = match v with Some v -> Ok v | None -> Error error + +let ok_if_true v ~error = match v with true -> Ok () | false -> Error error + module O = struct let ( >>= ) res f = bind ~f res diff --git a/stdext/result.mli b/stdext/result.mli index 6c889c1cc..2a70e1f85 100644 --- a/stdext/result.mli +++ b/stdext/result.mli @@ -6,6 +6,10 @@ val bind : f:('a -> ('b, 'err) t) -> ('a, 'err) t -> ('b, 'err) t val map : f:('a -> 'b) -> ('a, 'err) t -> ('b, 'err) t +val of_option : 'ok option -> error:'err -> ('ok, 'err) t + +val ok_if_true : bool -> error:'err -> (unit, 'err) t + module O : sig val ( >>= ) : ('a, 'err) t -> ('a -> ('b, 'err) t) -> ('b, 'err) t From b6cf581c06bb969b293f2f6e2cfb16110d42e2f3 Mon Sep 17 00:00:00 2001 From: Marek Kubica Date: Tue, 8 Mar 2022 16:46:48 +0100 Subject: [PATCH 2/4] Make error into a warning --- cli/pull.ml | 21 +++++++++------------ stdext/result.ml | 2 -- stdext/result.mli | 2 -- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/cli/pull.ml b/cli/pull.ml index ec70cce75..8d033b1e8 100644 --- a/cli/pull.ml +++ b/cli/pull.ml @@ -75,18 +75,15 @@ let run (`Yes yes) (`Root root) (`Lockfile explicit_lockfile) OpamSwitchState.get_package switch_state Config.compiler_package_name |> OpamPackage.version in - let* () = - Result.ok_if_true - (OpamPackage.Version.equal locked_ocaml_version switch_ocaml_version) - ~error: - (let msg = - Fmt.str - "OCaml versions do not match: %a in lockfile, but %a in switch" - Opam.Pp.version locked_ocaml_version Opam.Pp.version - switch_ocaml_version - in - `Msg msg) - in + (match + OpamPackage.Version.equal locked_ocaml_version switch_ocaml_version + with + | true -> () + | false -> + Logs.warn (fun l -> + l "OCaml versions do not match: %a in lockfile, but %a in switch" + Opam.Pp.version locked_ocaml_version Opam.Pp.version + switch_ocaml_version)); Pull.duniverse ~global_state ~root ~full ~trim_clone:(not keep_git_dir) duniverse diff --git a/stdext/result.ml b/stdext/result.ml index 63902b24f..a43b994c9 100644 --- a/stdext/result.ml +++ b/stdext/result.ml @@ -8,8 +8,6 @@ let map ~f = map f let of_option v ~error = match v with Some v -> Ok v | None -> Error error -let ok_if_true v ~error = match v with true -> Ok () | false -> Error error - module O = struct let ( >>= ) res f = bind ~f res diff --git a/stdext/result.mli b/stdext/result.mli index 2a70e1f85..04696c1fd 100644 --- a/stdext/result.mli +++ b/stdext/result.mli @@ -8,8 +8,6 @@ val map : f:('a -> 'b) -> ('a, 'err) t -> ('b, 'err) t val of_option : 'ok option -> error:'err -> ('ok, 'err) t -val ok_if_true : bool -> error:'err -> (unit, 'err) t - module O : sig val ( >>= ) : ('a, 'err) t -> ('a -> ('b, 'err) t) -> ('b, 'err) t From 65346d76b6771f78f206ae04f4f817180151e342 Mon Sep 17 00:00:00 2001 From: Marek Kubica Date: Tue, 8 Mar 2022 16:57:24 +0100 Subject: [PATCH 3/4] Don't warn on newer compilers OCaml should be backward compatible so if the user has a newer compiler we most likely don't need to warn. --- cli/pull.ml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cli/pull.ml b/cli/pull.ml index 8d033b1e8..3d0c151d4 100644 --- a/cli/pull.ml +++ b/cli/pull.ml @@ -49,6 +49,11 @@ let check_dune_lang_version ~yes ~root = Logs.debug (fun l -> l "No dune-project found"); Ok ()) +let version_is_at_least locked_version = + let version_constraint = (`Geq, locked_version) in + let version_formula = OpamFormula.Atom version_constraint in + OpamFormula.check_version_formula version_formula + let run (`Yes yes) (`Root root) (`Lockfile explicit_lockfile) (`Keep_git_dir keep_git_dir) (`Duniverse_repos duniverse_repos) () = let open Result.O in @@ -75,9 +80,7 @@ let run (`Yes yes) (`Root root) (`Lockfile explicit_lockfile) OpamSwitchState.get_package switch_state Config.compiler_package_name |> OpamPackage.version in - (match - OpamPackage.Version.equal locked_ocaml_version switch_ocaml_version - with + (match version_is_at_least locked_ocaml_version switch_ocaml_version with | true -> () | false -> Logs.warn (fun l -> From 3ecb3fd3ef77fecf757a72b811e972b1c8a7bbeb Mon Sep 17 00:00:00 2001 From: Marek Kubica Date: Tue, 15 Mar 2022 15:18:10 +0100 Subject: [PATCH 4/4] Improve the warning according to code review --- cli/pull.ml | 29 ++++++++++++++++++----------- lib/opam.ml | 5 +++++ lib/opam.mli | 4 ++++ 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/cli/pull.ml b/cli/pull.ml index 3d0c151d4..4a831fa43 100644 --- a/cli/pull.ml +++ b/cli/pull.ml @@ -49,11 +49,6 @@ let check_dune_lang_version ~yes ~root = Logs.debug (fun l -> l "No dune-project found"); Ok ()) -let version_is_at_least locked_version = - let version_constraint = (`Geq, locked_version) in - let version_formula = OpamFormula.Atom version_constraint in - OpamFormula.check_version_formula version_formula - let run (`Yes yes) (`Root root) (`Lockfile explicit_lockfile) (`Keep_git_dir keep_git_dir) (`Duniverse_repos duniverse_repos) () = let open Result.O in @@ -80,15 +75,27 @@ let run (`Yes yes) (`Root root) (`Lockfile explicit_lockfile) OpamSwitchState.get_package switch_state Config.compiler_package_name |> OpamPackage.version in - (match version_is_at_least locked_ocaml_version switch_ocaml_version with + let* pulled = + Pull.duniverse ~global_state ~root ~full ~trim_clone:(not keep_git_dir) + duniverse + in + (match + Opam.version_is_at_least locked_ocaml_version switch_ocaml_version + with | true -> () | false -> Logs.warn (fun l -> - l "OCaml versions do not match: %a in lockfile, but %a in switch" - Opam.Pp.version locked_ocaml_version Opam.Pp.version - switch_ocaml_version)); - Pull.duniverse ~global_state ~root ~full ~trim_clone:(not keep_git_dir) - duniverse + l + "Pulling duniverse succeeded but the version of the OCaml \ + compiler does not match the lockfile: %a in lockfile, yet %a \ + in switch.\n\ + You might want to change the compiler version of your switch \ + accordingly:\n\ + opam install %a.%a --update-invariant" Opam.Pp.version + locked_ocaml_version Opam.Pp.version switch_ocaml_version + Opam.Pp.package_name Config.compiler_package_name + Opam.Pp.version locked_ocaml_version)); + Ok pulled let info = let open Cmdliner in diff --git a/lib/opam.ml b/lib/opam.ml index 0d32e00a0..b959de86a 100644 --- a/lib/opam.ml +++ b/lib/opam.ml @@ -43,6 +43,11 @@ let depends_on_dune ~allow_jbuilder (formula : OpamTypes.filtered_formula) = in depends_on_any packages formula +let version_is_at_least locked_version = + let version_constraint = (`Geq, locked_version) in + let version_formula = OpamFormula.Atom version_constraint in + OpamFormula.check_version_formula version_formula + let pull_tree ~url ~hashes ~dir global_state = let dir_str = Fpath.to_string dir in let cache_dir = diff --git a/lib/opam.mli b/lib/opam.mli index 10a2a893a..dbb476d2e 100644 --- a/lib/opam.mli +++ b/lib/opam.mli @@ -158,6 +158,10 @@ val depends_on_compiler_variants : OpamTypes.filtered_formula -> bool This is detected by looking direct dependencies on ocaml-variants or dependencies on any relevant ocaml-option-* packages. *) +val version_is_at_least : OpamPackage.Version.t -> OpamPackage.Version.t -> bool +(** [version_is_at_least minimum to_check] returns [true] if the version + [to_check] is greater or equal to [minimum]. *) + val pull_tree : url:OpamUrl.t -> hashes:OpamHash.t list ->