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 +24 -9
1 changed files with 24 additions and 9 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit b9668365cd - Show all commits

33
opamdiff/opamdiff.ml
View file

@ -178,9 +178,10 @@ let pp_opam_diff ppf { pkg ; effectively_equal ; _ } =
(if effectively_equal then "" else " (effectively equal)") (if effectively_equal then "" else " (effectively equal)")
let detailed_opam_diff pkg l r = let detailed_opam_diff pkg l r =
let ( let* ) = Result.bind in
let opaml = OpamFile.OPAM.write_to_string l in let opaml = OpamFile.OPAM.write_to_string l in
let opamr = 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 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 OpamFile.OPAM.to_string_with_preserved_format ~format_from_string:opaml o r
in in
@ -194,16 +195,30 @@ let detailed_opam_diff pkg l r =
(no_build_install_url l) (no_build_install_url r) (no_build_install_url l) (no_build_install_url r)
in in
let diff = 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.
let* tmpl = Bos.OS.File.tmp "opaml_%s" in try
let* () = Bos.OS.File.write tmpl opaml in Bos.OS.File.with_tmp_oc "opaml_%s"
let* tmpr = Bos.OS.File.tmp "opamr_%s" in (fun pl oc () ->
let* () = Bos.OS.File.write tmpr opamr in Out_channel.output_string oc opaml;
let cmd = Bos.Cmd.(v "diff" % "-u" % p tmpl % p tmpr) in Out_channel.close oc;
Bos.OS.Cmd.(run_out cmd |> out_string) 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 in
let diff = match diff with 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 (s, _) -> s | Ok (Ok (Ok (data, _))) -> data
| Error `Msg m -> "error when comparing the opam files: " ^ m | 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 in
{ pkg ; effectively_equal ; diff } { pkg ; effectively_equal ; diff }