From 8c29161144e768c66332a48e4fede18aa9694d93 Mon Sep 17 00:00:00 2001 From: Hannes Mehnert Date: Tue, 7 Jan 2025 13:17:10 +0100 Subject: [PATCH 1/3] Compute and present the opam file differences as a unified diff --- lib/views.ml | 29 ++---------------- opamdiff/dune | 2 +- opamdiff/opamdiff.ml | 68 +++++++++++++++++++------------------------ opamdiff/opamdiff.mli | 6 ++-- 4 files changed, 35 insertions(+), 70 deletions(-) diff --git a/lib/views.ml b/lib/views.ml index eac0e80..fcc6758 100644 --- a/lib/views.ml +++ b/lib/views.ml @@ -828,33 +828,8 @@ let duniverse_diffs diffs = let opam_diffs diffs = List.concat_map (fun pd -> H.h4 [ txtf "%a" Opamdiff.pp_opam_diff pd ] :: - (match pd.Opamdiff.build with None -> [] | Some a -> - let l, r = Opamdiff.commands_to_strings a in - [ - H.h5 [ H.txt "build instruction (without common prefix) \ - modifications, old:" ] ; - H.code (List.concat_map (fun s -> [ H.txt s ; H.br () ]) l) ; - H.h5 [ H.txt "new" ] ; - H.code (List.concat_map (fun s -> [ H.txt s ; H.br () ]) r) - ]) @ - (match pd.Opamdiff.install with None -> [] | Some a -> - let l, r = Opamdiff.commands_to_strings a in - [ - H.h5 [ H.txt "install instruction (without common prefix) \ - modifications, old:" ] ; - H.code (List.concat_map (fun s -> [ H.txt s ; H.br () ]) l) ; - H.h5 [ H.txt "new" ] ; - H.code (List.concat_map (fun s -> [ H.txt s ; H.br () ]) r) - ]) @ - (match pd.Opamdiff.url with None -> [] | Some a -> - let l, r = Opamdiff.opt_url_to_string a in - [ - H.h5 [ H.txt "URL" ] ; - txtf "old: %s" l; - H.br (); - txtf "new: %s" r - ]) @ - [ H.br () ]) + H.h5 [ H.txt "diff" ] :: + H.code [ H.txt pd.diff ; H.br () ] :: []) diffs let compare_builds diff --git a/opamdiff/dune b/opamdiff/dune index fc5561d..0cb8475 100644 --- a/opamdiff/dune +++ b/opamdiff/dune @@ -1,3 +1,3 @@ (library (name opamdiff) - (libraries opam-core opam-format yojson)) + (libraries opam-core opam-format yojson bos)) diff --git a/opamdiff/opamdiff.ml b/opamdiff/opamdiff.ml index 059af95..0befbd3 100644 --- a/opamdiff/opamdiff.ml +++ b/opamdiff/opamdiff.ml @@ -168,10 +168,8 @@ let pp_version_diff ppf { name; version_left; version_right } = type opam_diff = { pkg : OpamPackage.t ; - build : (OpamTypes.command list * OpamTypes.command list) option ; - install : (OpamTypes.command list * OpamTypes.command list) option ; - url : (OpamFile.URL.t option * OpamFile.URL.t option) option ; - otherwise_equal : bool ; + effectively_equal : bool ; + diff : string ; } let commands_to_strings (l, r) = @@ -186,46 +184,40 @@ let opt_url_to_string (l, r) = in url_to_s l, url_to_s r -let pp_opam_diff ppf { pkg ; otherwise_equal ; _ } = +let pp_opam_diff ppf { pkg ; effectively_equal ; _ } = Format.fprintf ppf "%a%s" pp_opampackage pkg - (if otherwise_equal then "" else " (and additional changes)") - -let rec strip_common_prefix a b = - match a, b with - | hd::tl, hd'::tl' -> - if hd = hd' then - strip_common_prefix tl tl' - else - a, b - | a, b -> a, b + (if effectively_equal then "" else " (effectively equal)") let detailed_opam_diff pkg l r = - let no_build_install_url p = - OpamFile.OPAM.with_url_opt None - (OpamFile.OPAM.with_install [] - (OpamFile.OPAM.with_build [] p)) + let ( let* ) = Result.bind in + let opaml = OpamFile.OPAM.write_to_string l in + let opamr = + let o = OpamFile.make (OpamFilename.raw "opam") in + OpamFile.OPAM.to_string_with_preserved_format ~format_from_string:opaml o r in - let otherwise_equal = + let effectively_equal = + let no_build_install_url p = + OpamFile.OPAM.with_url_opt None + (OpamFile.OPAM.with_install [] + (OpamFile.OPAM.with_build [] p)) + in OpamFile.OPAM.effectively_equal (no_build_install_url l) (no_build_install_url r) - and build = - if OpamFile.OPAM.build l = OpamFile.OPAM.build r then - None - else - Some (strip_common_prefix (OpamFile.OPAM.build l) (OpamFile.OPAM.build r)) - and install = - if OpamFile.OPAM.install l = OpamFile.OPAM.install r then - None - else - Some (strip_common_prefix (OpamFile.OPAM.install l) (OpamFile.OPAM.install r)) - and url = - if OpamFile.OPAM.url l = OpamFile.OPAM.url r then - None - else - Some (OpamFile.OPAM.url l, OpamFile.OPAM.url r) in - { pkg ; build ; install ; url ; otherwise_equal } + let diff = + let* tmpl = Bos.OS.File.tmp "opaml_%s" in + let* () = Bos.OS.File.write tmpl opaml in + let* tmpr = Bos.OS.File.tmp "opamr_%s" in + let* () = Bos.OS.File.write tmpr opamr in + let cmd = Bos.Cmd.(v "diff" % "-u" % p tmpl % p tmpr) in + Bos.OS.Cmd.(run_out cmd |> out_string) + in + let diff = match diff with + | Ok (s, _) -> s + | Error `Msg m -> "error when comparing the opam files: " ^ m + in + { pkg ; effectively_equal ; diff } let detailed_opam_diffs left right pkgs = OpamPackage.Set.fold (fun p acc -> @@ -299,9 +291,9 @@ let compare left right = let opam_diff_to_json opam_diff = `List (List.map (fun (diff : opam_diff) -> `Assoc [ - ("package_version", `String (OpamPackage.to_string diff.pkg)); - ("otherwise_equal", `Bool diff.otherwise_equal) + ("effectively_equal", `Bool diff.effectively_equal); + ("diff", `String diff.diff); ] ) opam_diff) diff --git a/opamdiff/opamdiff.mli b/opamdiff/opamdiff.mli index fa1bf3d..54a4bec 100644 --- a/opamdiff/opamdiff.mli +++ b/opamdiff/opamdiff.mli @@ -1,9 +1,7 @@ type opam_diff = { pkg : OpamPackage.t ; - build : (OpamTypes.command list * OpamTypes.command list) option ; - install : (OpamTypes.command list * OpamTypes.command list) option ; - url : (OpamFile.URL.t option * OpamFile.URL.t option) option ; - otherwise_equal : bool ; + effectively_equal : bool ; + diff : string ; } type version_diff = { -- 2.47.1 From 420d049ff055ed5760ff672bc1ea7f0fc378665d Mon Sep 17 00:00:00 2001 From: Hannes Mehnert Date: Tue, 7 Jan 2025 13:24:39 +0100 Subject: [PATCH 2/3] remove more unneeded code --- opamdiff/opamdiff.ml | 49 ++++++++++++++----------------------------- opamdiff/opamdiff.mli | 6 ------ 2 files changed, 16 insertions(+), 39 deletions(-) diff --git a/opamdiff/opamdiff.ml b/opamdiff/opamdiff.ml index 0befbd3..bd4da6d 100644 --- a/opamdiff/opamdiff.ml +++ b/opamdiff/opamdiff.ml @@ -172,18 +172,6 @@ type opam_diff = { diff : string ; } -let commands_to_strings (l, r) = - let v a = - OpamPrinter.FullPos.value (OpamPp.print OpamFormat.V.command a) - in - List.map v l, List.map v r - -let opt_url_to_string (l, r) = - let url_to_s = function - | None -> "" | Some u -> OpamFile.URL.write_to_string u - in - url_to_s l, url_to_s r - let pp_opam_diff ppf { pkg ; effectively_equal ; _ } = Format.fprintf ppf "%a%s" pp_opampackage pkg @@ -268,7 +256,7 @@ let compare left right = in (opam_diff, version_diff, left_pkgs, right_pkgs, duniverse_ret) - let compare_to_json +let compare_to_json (opam_diff, version_diff, left_pkgs, right_pkgs, duniverse_diff) : Yojson.Basic.t = let version_diff_to_json lst = `List (List.map (fun { name; version_left; version_right } -> @@ -276,17 +264,15 @@ let compare left right = ("name", `String (OpamPackage.Name.to_string name)); ("version_left", `String (OpamPackage.Version.to_string version_left)); ("version_right", `String (OpamPackage.Version.to_string version_right)) - ] - ) lst) + ]) lst) in let package_set_to_json set = `List (Set.fold (fun p acc -> let json = `Assoc [ - ("name", `String (OpamPackage.Name.to_string p.OpamPackage.name)); - ("version", `String (OpamPackage.Version.to_string p.OpamPackage.version)) - ] in - json :: acc - ) set []) + ("name", `String (OpamPackage.Name.to_string p.OpamPackage.name)); + ("version", `String (OpamPackage.Version.to_string p.OpamPackage.version)) + ] in + json :: acc) set []) in let opam_diff_to_json opam_diff = `List (List.map (fun (diff : opam_diff) -> @@ -294,23 +280,20 @@ let compare left right = ("package_version", `String (OpamPackage.to_string diff.pkg)); ("effectively_equal", `Bool diff.effectively_equal); ("diff", `String diff.diff); - ] - - ) opam_diff) + ]) opam_diff) in let duniverse_to_json = function | Ok (left, right, detailed_diff) -> `Assoc [ - ("left", `List (List.map (fun (k, v) -> `Assoc [("name", `String k); ("value", `String v)]) left)); - ("right", `List (List.map (fun (k, v) -> `Assoc [("name", `String k); ("value", `String v)]) right)); - ("detailed_diff",`List (List.map (fun (diff : duniverse_diff) -> - `Assoc [ - ("name", `String diff.name); - ]) detailed_diff)) - ] - - | Error (`Msg msg) -> - `String msg + ("left", `List (List.map (fun (k, v) -> `Assoc [("name", `String k); ("value", `String v)]) left)); + ("right", `List (List.map (fun (k, v) -> `Assoc [("name", `String k); ("value", `String v)]) right)); + ("detailed_diff",`List (List.map (fun (diff : duniverse_diff) -> + `Assoc [ + ("name", `String diff.name); + ]) detailed_diff)) + ] + | Error (`Msg msg) -> + `String msg in `Assoc [ ("opam_diff", opam_diff_to_json opam_diff); diff --git a/opamdiff/opamdiff.mli b/opamdiff/opamdiff.mli index 54a4bec..20aff73 100644 --- a/opamdiff/opamdiff.mli +++ b/opamdiff/opamdiff.mli @@ -26,15 +26,9 @@ val pp_duniverse_dir : Format.formatter -> string * string -> unit val pp_opam_diff : Format.formatter -> opam_diff -> unit -val commands_to_strings : OpamTypes.command list * OpamTypes.command list -> string list * string list - -val opt_url_to_string : OpamFile.URL.t option * OpamFile.URL.t option -> string * string - - val compare : OpamFile.SwitchExport.t -> OpamFile.SwitchExport.t -> opam_diff list * version_diff list * OpamPackage.Set.t * OpamPackage.Set.t * ((string * string) list * (string * string) list * duniverse_diff list, [> `Msg of string ]) result - val compare_to_json : opam_diff list * version_diff list * OpamPackage.Set.t * OpamPackage.Set.t * ((string * string) list * (string * string) list * duniverse_diff list, [< `Msg of string ]) result -> Yojson.Basic.t -- 2.47.1 From b9668365cda25ddd2e10218dfae3294a8b216b45 Mon Sep 17 00:00:00 2001 From: Hannes Mehnert Date: Wed, 8 Jan 2025 13:03:54 +0100 Subject: [PATCH 3/3] use Bos.OS.File.with_tmp_oc instead to cleanup files immediately --- opamdiff/opamdiff.ml | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/opamdiff/opamdiff.ml b/opamdiff/opamdiff.ml index bd4da6d..bc6a818 100644 --- a/opamdiff/opamdiff.ml +++ b/opamdiff/opamdiff.ml @@ -178,9 +178,10 @@ let pp_opam_diff ppf { pkg ; effectively_equal ; _ } = (if effectively_equal then "" else " (effectively equal)") let detailed_opam_diff pkg l r = - let ( let* ) = Result.bind in let opaml = OpamFile.OPAM.write_to_string l in let opamr = + (* Let's minimize the difference between opaml and opamr by taking opaml + as template for opamr. *) let o = OpamFile.make (OpamFilename.raw "opam") in OpamFile.OPAM.to_string_with_preserved_format ~format_from_string:opaml o r in @@ -194,16 +195,30 @@ let detailed_opam_diff pkg l r = (no_build_install_url l) (no_build_install_url r) in let diff = - let* tmpl = Bos.OS.File.tmp "opaml_%s" in - let* () = Bos.OS.File.write tmpl opaml in - let* tmpr = Bos.OS.File.tmp "opamr_%s" in - let* () = Bos.OS.File.write tmpr opamr in - let cmd = Bos.Cmd.(v "diff" % "-u" % p tmpl % p tmpr) in - Bos.OS.Cmd.(run_out cmd |> out_string) + try + Bos.OS.File.with_tmp_oc "opaml_%s" + (fun pl oc () -> + Out_channel.output_string oc opaml; + Out_channel.close oc; + Bos.OS.File.with_tmp_oc "opamr_%s" + (fun pr oc () -> + Out_channel.output_string oc opamr; + Out_channel.close oc; + let cmd = Bos.Cmd.(v "diff" % "-u" % p pl % p pr) in + Bos.OS.Cmd.(run_out cmd |> out_string)) + ()) + () + with e -> + Error (`Msg ("exception " ^ Printexc.to_string e)) in let diff = match diff with - | Ok (s, _) -> s - | Error `Msg m -> "error when comparing the opam files: " ^ m + | Ok (Ok (Ok (data, _))) -> data + | Ok (Ok (Error `Msg msg)) + | Ok (Error `Msg msg) + | Error `Msg msg -> + Logs.err (fun m -> m "Error %s while running diff on opam files@.@.%s@.@.%s@.@." + msg opaml opamr); + "Error comparing opam files" in { pkg ; effectively_equal ; diff } -- 2.47.1