robur/builder-web
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Compute and present the opam file differences as a unified diff #7

Merged
reynir merged 3 commits from opam-diff-as-diff into main 2025-01-08 14:16:05 +00:00
Conversation 2 Commits 3 Files changed 4 +66 -109
4 changed files with 66 additions and 109 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

29
lib/views.ml
View file

@ -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

2
opamdiff/dune
View file

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

98
opamdiff/opamdiff.ml
View file

@ -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 ;
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) =
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 ; _ } =
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 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
reynir marked this conversation as resolved
reynir commented 2025-01-08 09:12:45 +00:00
Review
Copy link

So this dance here is to try to get the same formatting as for l/opaml? I think that makes sense. Maybe worth attaching a comment?

I am curious what the behavior is if the two packages differ too much. I guess since OpamFile.OPAM.write_to_string is likely to use a fixed formatting style the style will be consistent even with a larger difference in packages? (this doesn't have to be answered IMO)

So this dance here is to try to get the same formatting as for `l`/`opaml`? I think that makes sense. Maybe worth attaching a comment? I am curious what the behavior is if the two packages differ too much. I guess since `OpamFile.OPAM.write_to_string` is likely to use a fixed formatting style the style will be consistent even with a larger difference in packages? (this doesn't have to be answered IMO)
hannes commented 2025-01-08 11:38:56 +00:00
Review
Copy link

in my experience, using the "with_preserved_format" is good -- apart from potentially dangling comments (in the last line). I also think a comment is worth it. Of course we can leave the opam files as is and do a direct diff, i.e. do not care about the "minimal" diff...

in my experience, using the "with_preserved_format" is good -- apart from potentially dangling comments (in the last line). I also think a comment is worth it. Of course we can leave the opam files as is and do a direct diff, i.e. do not care about the "minimal" diff...
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 =
reynir marked this conversation as resolved
reynir commented 2025-01-08 09:19:22 +00:00
Review
Copy link

I think the files are only deleted at_exit. There are functions Bos.OS.File.with_tmp_output / Bos.OS.File.with_tmp_oc that ensure the file is deleted when the callback returns - maybe worth using one of them?

The files are usually quite small so I imagine it would usually take years before the dangling temporary files become a problem.

I think the files are only deleted `at_exit`. There are functions `Bos.OS.File.with_tmp_output` / `Bos.OS.File.with_tmp_oc` that ensure the file is deleted when the callback returns - maybe worth using one of them? The files are usually quite small so I imagine it would usually take years before the dangling temporary files become a problem.
hannes commented 2025-01-08 11:39:13 +00:00
Review
Copy link

Yes, you're right. I'll adapt.

Yes, you're right. I'll adapt.
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;
reynir marked this conversation as resolved Outdated
reynir commented 2025-01-08 09:27:12 +00:00
Outdated
Review
Copy link

Maybe we should log the error message instead? I think from a security point of view there could be error messages that we don't want to display to the client (e.g. information about the filesystem).

Maybe we should log the error message instead? I think from a security point of view there could be error messages that we don't want to display to the client (e.g. information about the filesystem).
hannes commented 2025-01-08 11:39:34 +00:00
Outdated
Review
Copy link

Indeed.

Indeed.
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
reynir commented 2025-01-08 14:15:45 +00:00
Review
Copy link

I think we can write match Result.join (Result.join diff) with to avoid the nested matches, but this is fine, too.

I think we can write `match Result.join (Result.join diff) with` to avoid the nested matches, but this is fine, too.
| 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
) set [])
json :: acc) 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)
]
) opam_diff)
("effectively_equal", `Bool diff.effectively_equal);
("diff", `String diff.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

12
opamdiff/opamdiff.mli
View file

@ -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 = {
@ -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