Merge pull request 'Compute and present the opam file differences as a unified diff' (!7) from opam-diff-as-diff into main

Reviewed-on: #7
Reviewed-by: Reynir Björnsson <reynir@reynir.dk>

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 ->
+      H.h5 [ H.txt "diff" ] ::
-          let l, r = Opamdiff.commands_to_strings a in
+      H.code [ H.txt pd.diff ; H.br () ] :: [])
-          [
-            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 () ])
     diffs
 
 let compare_builds

opamdiff/dune

@@ -1,3 +1,3 @@
 (library
  (name opamdiff)
- (libraries opam-core opam-format yojson))
+ (libraries opam-core opam-format yojson bos))

opamdiff/opamdiff.ml

@@ -168,64 +168,59 @@ let pp_version_diff ppf { name; version_left; version_right } =
 
 type opam_diff = {
   pkg : OpamPackage.t ;
-  build : (OpamTypes.command list * OpamTypes.command list) option ;
+  effectively_equal : bool ;
-  install : (OpamTypes.command list * OpamTypes.command list) option ;
+  diff : string ;
-  url : (OpamFile.URL.t option * OpamFile.URL.t option) option ;
-  otherwise_equal : bool ;
 }
 
-let commands_to_strings (l, r) =
+let pp_opam_diff ppf { pkg ; effectively_equal ; _ } =
-    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 ; otherwise_equal ; _ } =
   Format.fprintf ppf "%a%s"
     pp_opampackage pkg
-    (if otherwise_equal then "" else " (and additional changes)")
+    (if effectively_equal then "" else " (effectively equal)")
-
-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
 
 let detailed_opam_diff pkg l r =
+  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
+  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
-  let otherwise_equal =
     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 =
+    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 (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 }
 
 let detailed_opam_diffs left right pkgs =
   OpamPackage.Set.fold (fun p acc ->
@@ -284,8 +279,7 @@ 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 ->
@@ -293,18 +287,15 @@ let compare left right =
               ("name", `String (OpamPackage.Name.to_string p.OpamPackage.name));
               ("version", `String (OpamPackage.Version.to_string p.OpamPackage.version))
             ] in
-          json :: acc
+          json :: acc) set [])
-        ) set [])
     in
     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)
-        ) opam_diff)
     in
     let duniverse_to_json = function
       | Ok (left, right, detailed_diff) ->
@@ -316,7 +307,6 @@ let compare left right =
                  ("name", `String diff.name);
                ]) detailed_diff))
         ]
-
       | Error (`Msg msg) ->
         `String msg
     in

opamdiff/opamdiff.mli

@@ -1,9 +1,7 @@
 type opam_diff = {
   pkg : OpamPackage.t ;
-  build : (OpamTypes.command list * OpamTypes.command list) option ;
+  effectively_equal : bool ;
-  install : (OpamTypes.command list * OpamTypes.command list) option ;
+  diff : string ;
-  url : (OpamFile.URL.t option * OpamFile.URL.t option) option ;
-  otherwise_equal : bool ;
 }
 
 type version_diff = {
@@ -28,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