robur/builder-web
4
0
Fork 0
Code Issues Pull requests 1 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
hannes commented 2025-01-07 12:17:56 +00:00
Owner
Copy link
No description provided.
hannes added 1 commit 2025-01-07 12:17:56 +00:00
hannes added 1 commit 2025-01-07 12:24:50 +00:00
hannes force-pushed opam-diff-as-diff from f20227cfea to 420d049ff0 2025-01-07 12:49:41 +00:00 Compare
reynir reviewed 2025-01-08 09:36:06 +00:00
reynir left a comment
Owner
Copy link

Thanks! This is a nice change. There are some things about temporary files and errors that I can look into myself. Otherwise good to go IMO.

Thanks! This is a nice change. There are some things about temporary files and errors that I can look into myself. Otherwise good to go IMO.
opamdiff/opamdiff.ml
@ -208,0 +182,4 @@
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 commented 2025-01-08 09:12:45 +00:00
Owner
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
Author
Owner
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...
reynir marked this conversation as resolved
opamdiff/opamdiff.ml
@ -227,2 +195,3 @@
in
{ pkg ; build ; install ; url ; otherwise_equal }
let diff =
let* tmpl = Bos.OS.File.tmp "opaml_%s" in
reynir commented 2025-01-08 09:19:22 +00:00
Owner
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
Author
Owner
Copy link

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

Yes, you're right. I'll adapt.
reynir marked this conversation as resolved
opamdiff/opamdiff.ml Outdated
@ -229,0 +203,4 @@
in
let diff = match diff with
| Ok (s, _) -> s
| Error `Msg m -> "error when comparing the opam files: " ^ m
reynir commented 2025-01-08 09:27:12 +00:00
Owner
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
Author
Owner
Copy link

Indeed.

Indeed.
reynir marked this conversation as resolved
hannes added 1 commit 2025-01-08 12:04:47 +00:00
hannes commented 2025-01-08 12:05:03 +00:00
Author
Owner
Copy link

I pushed b966836 -- does that solve your concerns?

I pushed b966836 -- does that solve your concerns?
reynir approved these changes 2025-01-08 14:15:51 +00:00
opamdiff/opamdiff.ml
@ -229,0 +211,4 @@
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
Owner
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.
reynir merged commit 0567f58f05 into main 2025-01-08 14:16:05 +00:00
reynir deleted branch opam-diff-as-diff 2025-01-08 14:16:06 +00:00
reynir commented 2025-01-08 14:16:15 +00:00
Owner
Copy link

Thanks!

Thanks!
Sign in to join this conversation.
Reviewers
No reviewers
reynir
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: robur/builder-web#7
No description provided.
Delete branch "opam-diff-as-diff"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?