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 +35 -70
4 changed files with 35 additions and 70 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 8c29161144 - Show all commits

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

68
opamdiff/opamdiff.ml
View file

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

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