Warn if GNU patch is not detected when using patch

[kit-ty-kate]

Lightweight alternative to #5892
See #5400

In this PR, once per process if a patch is going to be applied (either a patch application in a package or a repository update) we first check which patch executable to use. If no GNU patch is detected we display a warning.

On some platforms the name of the patch command can change depending on the context (e.g. macOS where Homebrew provides patch but Macports provides gpatch), so to handle that we simply check both patch and gpatch and detect which one is GNU patch. On the platforms more likely to have patch as GNU patch we check patch first and default to it if none were found, and inversely on the platforms that are more likely to have gpatch as GNU patch.

[rjbou]

On the idea and the code, lgtm!
Can you explain in the main comment why this is applied only on updating opam repositories ?
Also, a precision in the changes/PR/commits, it is not specific to opam update, but more generally opam repositories updating. These updates can be launched by opam update, opam switch, and opam init.

[hannesm]

I do understand the logic to look for "gpatch" .. "patch" -- but I'd do it differently:

• not look for gpatch on GNU/Linux (i.e. only look for it on BSD and macOS)
• unify the whole lookup to uniformly look for gpatch first and if not found (or not GNU patch) go for patch

Of course your PR is fine, but I wonder in terms of lines of code and code understandability whether it is worth it (my experience is if you have the least platform-specific conditionals, the easiest it is to debug).

But these are only my 2 cents... thanks for your work on this.

[kit-ty-kate]

@hannesm like this?

Suggested change

	| DragonFly
	| FreeBSD
	| NetBSD
	| OpenBSD -> ("gpatch", ["patch"])
	| Cygwin
	| Darwin
	| Linux
	| Unix
	| Win32
	| Other _ -> ("patch", ["gpatch"])
	| Darwin
	| DragonFly
	| FreeBSD
	| NetBSD
	| OpenBSD -> ("gpatch", ["patch"])
	| Cygwin
	| Linux
	| Unix
	| Win32
	| Other _ -> ("patch", [])

[kit-ty-kate]

the problem with that is that it changes the behaviour compared to 2.1.5

[hannesm]

yes, well, or even:

let default_cmd, other_cmds = "gpatch", [ "patch" ]

avoiding the match on the os type at all. But I hear your "changes the behaviour". With that in mind, I like your PR very much.

The only suggestion left is:

  let default_cmd, other_cmds =
    match OpamStd.Sys.os () with
    | DragonFly
    | FreeBSD
    | NetBSD
    | OpenBSD
    | Darwin -> ("gpatch", ["patch"])
    | Cygwin
    | Linux
    | Unix
    | Win32
    | Other _ -> ("patch", [])

so, put Darwin into the pot where gpatch is tried first, and don't try gpatch on Linux & Windows.

But as said, maybe the current state is best for avoiding too much trouble / regressions.

[hannesm]

Actually considering the code and platforms, maybe

let default_cmd, other_cmds = "patch", [ "gpatch" ]

is suitable? This would on OpenBSD and FreeBSD call patch --version, and then go to using gpatch...

[rjbou]

LGTM! tiny formatting remark that you can ignore :)

[rjbou]

related to #3639