Use gpatch on openBSD

[rjbou]

This PR is a partial fix for #3433, more specifically #3433 (comment).
It conflicts with #3443, need to be rebased before merge.

[AltGr]

LGTM!

[AltGr]

hah, conflicted with your other PR I just merged!

[rjbou]

Rebased. Still need to be tested on openBSD.

[hannesm]

on my FreeBSD, I got a similar message as noted in #3433 (comment) (using opam2 rc2), and was wondering whether opam should require and call out to GNU patch on FreeBSD as well (the port is named devel/patch, the binary installed by it gpatch).

[rjbou]

ok, I'll add it for FreeBSD also then.

[hannesm]

while I've seen this once so far, I'm not entirely sure when this happens... is there (for the OpenBSD patch) a reproducible test case which fails with their BSD patch, which I could test on FreeBSD?

[dra27]

@hannesm - I probably won’t be able to do it today, but I was planning on adding a binary test to #3456 both to ensure that the patch rewriter no longer messes them up and also to act as a test for this issue.

[rjbou]

@hannesm The choice of gpatch on OpenBSD is also from patch not handling binaries. If on FreeBSD the same behavior is seen, it would be better to use gpatch instead. I added a commit for that, pending confirmation if it's needed or no.
A simple test using opam mecanism is to generate a binary patch and try to apply it, using a toplevel, with opam libs loaded:
OpamProcess.Job.run (OpamSystem.patch ~dir:"<directory_to_patch>" "<patch_filename>");;. Enabling verbose mode can be useful here.

[hannesm]

@rjbou thanks! what I did:

• consider an ELF binary foo, I copied it to foo.orig
• strip foo
• diff -ua foo foo.orig > diff (-a is "treat as ASCII text and force output of a diff, otherwise diff only outputs Binary files foo and foo.orig differ)
• mv foo.orig foo.old_orig
• patch -p0 < diff leads to: Hunk #1 succeeded at 1.\nSegmentation fault (core dumped)

now, this is all without opam + utop, how does opam generate a binary diff (and why? - i thought it only every cared about text files)?

[hannesm]

what I forgot to mention, the above works fine when using gpatch -- so I guess: yes, the FreeBSD patch command is not well-suited for opam, and gpatch should be used instead (plus opam requires the port devel/patch as dependency).

[rjbou]

Thanks for your test! I'll clean & keep it then.

how does opam generate a binary diff (and why? - i thought it only every cared about text files)?

For two reasons, on some repo update 1.2 -> 2.0, there is a delete of a remaining index.tar.gz file. As the update process is done using patch, the removal too. And we can't be certain that custom repo doesn't contain binary files.

[rjbou]

ping @jorisgio for commit c661ad7, I'm not sure about the FreeBSD version.

[jorisgio]

@rjbou wow, i'm sorry about that, I was not even aware i was still listed as maintainer of this port. IIRC i tried to notify various person but it's blurry now, it's been years since i last booted a *BSD.
I've just tried to open a PR but apparently i forgot my password and only managed to lock myself out of freebsd bug tracker...
Will retry later, but i'm afraid i cannot help much.

[hannesm]

@jorisgio there is a "lost password" link at https://bugs.freebsd.org/bugzilla/ and the email address you've using is mentioned at https://svnweb.freebsd.org/ports/head/devel/ocaml-opam/Makefile?revision=427548&view=markup#l13 -- I don't know how the governance of FreeBSD ports works (and wasn't able to find much information at https://www.freebsd.org/doc/en/books/porters-handbook/makefile-maintainer.html), but feel free to propose me (hannes AT mehnert dot ORG) as maintainer for devel/ocaml-opam. thanks!

[rjbou]

@jorisgio no worries :) I pinged you because you were still listed as maintainer. Thanks @hannesm to agree to be the next maintainer!

[hannesm]

maybe another approach would be (or is this already possible?), similar to download-command in .opam/config, have a patch-command there?

[rjbou]

We could consider a future better solution (in-built patch? as we have some constraints using installed one). For the opam2 release, we just include the gpatch choice for BSD in order to handle the problem.

One of the worry of having the patch-command in the config file would be that the user given command must have the same constraint as patch -p1 in order to correspond to internal handling.