Replace the use of GNU Patch by the OCaml patch library

Author: kit-ty-kate

.github/workflows/ci.ml

@@ -527,8 +527,8 @@ let main oc : unit =
     ("OPAM12CACHE", "~/.cache/opam1.2/cache");
     (* These should be identical to the values in appveyor.yml *)
     ("OPAM_REPO", "https://github.com/ocaml/opam-repository.git");
-    ("OPAM_TEST_REPO_SHA", "67e940587b8aca227f511e1943bcd31eabe6b1db");
-    ("OPAM_REPO_SHA", "67e940587b8aca227f511e1943bcd31eabe6b1db");
+    ("OPAM_TEST_REPO_SHA", "0c42e982f4cf97fc698132fb2a16b49524a26ab3");
+    ("OPAM_REPO_SHA", "0c42e982f4cf97fc698132fb2a16b49524a26ab3");
     ("SOLVER", "");
     (* Cygwin configuration *)
     ("CYGWIN_MIRROR", "http://mirrors.kernel.org/sourceware/cygwin/");

.github/workflows/main.yml

@@ -26,8 +26,8 @@ env:
   OPAMBSROOT: ~/.cache/.opam.cached
   OPAM12CACHE: ~/.cache/opam1.2/cache
   OPAM_REPO: https://github.com/ocaml/opam-repository.git
-  OPAM_TEST_REPO_SHA: 67e940587b8aca227f511e1943bcd31eabe6b1db
-  OPAM_REPO_SHA: 67e940587b8aca227f511e1943bcd31eabe6b1db
+  OPAM_TEST_REPO_SHA: 0c42e982f4cf97fc698132fb2a16b49524a26ab3
+  OPAM_REPO_SHA: 0c42e982f4cf97fc698132fb2a16b49524a26ab3
   SOLVER:
   CYGWIN_MIRROR: http://mirrors.kernel.org/sourceware/cygwin/
   CYGWIN_ROOT: D:\cygwin

configure

@@ -366,6 +366,7 @@ AC_CHECK_OCAML_PKG([jsonm])
 AC_CHECK_OCAML_PKG([uutf])
 AC_CHECK_OCAML_PKG([sha])
 AC_CHECK_OCAML_PKG([swhid_core])
+AC_CHECK_OCAML_PKG([patch])
 
 # Optional dependencies
 AC_CHECK_OCAML_PKG_AT_LEAST([mccs],[1.1+17])
@@ -414,6 +415,7 @@ AS_IF([test "x${enable_checks}" != "xno" && {
        test "x$OCAML_PKG_uutf" = "xno" ||
        test "x$OCAML_PKG_sha" = "xno" ||
        test "x$OCAML_PKG_swhid_core" = "xno" ||
+       test "x$OCAML_PKG_patch" = "xno" ||
        test "x$OCAML_PKG_mccs$MCCS_ENABLED" = "xnotrue";}],[
   AS_IF([test "x${with_vendored_deps}" != "xyes"],[
     AC_MSG_ERROR([Dependencies missing. Use --with-vendored-deps or --disable-checks])

configure.ac

@@ -29,6 +29,9 @@ users)
 ## Install
 
 ## Build (package)
+  * Patches are now applied using the `patch` OCaml library instead of GNU Patch [#5892 @kit-ty-kate - fix #6019 #6052]
+  * ✘ Patches: Context diffs are not supported anymore, only Unified diffs are (including its git extensions) [#5892 @kit-ty-kate]
+  * ✘ Patches: Stop support of file permission changes via git extension to the unified diff specification [#5892 @kit-ty-kate - fix #3782]
 
 ## Remove
 
@@ -65,6 +68,7 @@ users)
   * [BUG] Do not show the not-up-to-date message with packages tagged with avoid-version [#6273 @kit-ty-kate - fix #6271]
   * [BUG] Fix a regression on `opam upgrade <package>` upgrading unrelated packages [#6373 @AltGr]
   * [BUG] Fix a regression on `opam upgrade --all <uninstalled-pkg>` not upgrading the whole switch [#6373 @kit-ty-kate]
+  * Updates are now applied using the `patch` OCaml library instead of the system GNU Patch [#5892 @kit-ty-kate - fix ocaml/setup-ocaml#933 #6052]
 
 ## Tree
 
@@ -100,6 +104,7 @@ users)
   * Lookup at `gpatch` before `patch` on macOS now that both homebrew and macports expose `gpatch` as `gpatch` since Homebrew/homebrew-core#174687 [#6255 @kit-ty-kate]
   * Relax lookup on OpenBSD to consider all installed packages [#6362 @semarie]
   * Speedup the detection of available system packages with pacman and brew [#6324 @kit-ty-kate]
+  * The system GNU Patch is no longer runtime dependency of opam [#5892 @kit-ty-kate - fix #6052]
 
 ## Format upgrade
 
@@ -226,6 +231,7 @@ users)
 
 # API updates
 ## opam-client
+  * `OpamAction.prepare_package_build`: now returns `exn option` instead of `exn option OpamProcess.job` and no longer calls the system GNU Patch [#5892 @kit-ty-kate]
   * `OpamArg.InvalidCLI`: export exception [#6150 @rjbou]
   * `OpamArg`: export `require_checksums` and `no_checksums`, that are shared with `build_options` [#5563 @rjbou]
   * `OpamArg.hash_kinds`: was added [#5960 @kit-ty-kate]
@@ -254,6 +260,8 @@ users)
 ## opam-core
   * `OpamConsole`: Replace `black` text style (unused and not very readable) by `gray` [#6358 @kit-ty-kate]
   * `OpamConsole.pause`: Ensure the function always prints a newline character at the end [#6376 @kit-ty-kate]
+  * `OpamFilename.patch`: now returns `exn option` instead of `exn option OpamProcess.job` and no longer calls the system GNU Patch [#5892 @kit-ty-kate]
+  * `OpamFilename.patch`: a named-parameter `~allow_unclean` was added [#5892 @kit-ty-kate]
   * `OpamHash.all_kinds`: was added, which returns the list of all possible values of `OpamHash.kind` [#5960 @kit-ty-kate]
   * `OpamStd.List.split`: Improve performance [#6210 @kit-ty-kate]
   * `OpamStd.Option.equal_some`: was added, which tests equality of an option with a value [#6381 @kit-ty-kate]
@@ -265,5 +273,7 @@ users)
   * `OpamStubs.get_stdout_ws_col`: new Unix-only function returning the number of columns of the current terminal window [#6244 @kit-ty-kate]
   * `OpamSystem`: add `is_archive_from_string` that does the same than `is_archive` but without looking at the file, only analysing the string (extension) [#6219 @rjbou]
   * `OpamSystem.remove_dir`: do not fail with an exception when directory is a symbolic link [#6276 @btjorge @rjbou - fix #6275]
+  * `OpamSystem.patch`: now returns `exn option` instead of `exn option OpamProcess.job` and no longer calls the system GNU Patch [#5892 @kit-ty-kate]
+  * `OpamSystem.patch`: a named-parameter `~allow_unclean` was added [#5892 @kit-ty-kate]
   * `OpamParallel.*.{map,reduce,iter}`: Run `Gc.compact` when the main process is waiting for the children processes for the first time [#5396 @kkeundotnet]
   * `OpamSystem`, `OpamFilename`: add `with_tmp_file` and `with_tmp_file_job` function, that create a file name in temporary directory and removes it at the end of the call [#6036 @rjbou]

master_changes.md

@@ -30,6 +30,7 @@ depends: [
   "sha" {>= "1.13"}
   "jsonm"
   "swhid_core"
+  "patch" {>= "3.0.0~alpha1"}
   "uutf"
   (("host-system-mingw" {os = "win32" & os-distribution != "cygwinports"} &
     "conf-mingw-w64-gcc-i686" {os = "win32" & os-distribution != "cygwinports"} &

opam-core.opam

@@ -30,5 +30,6 @@ build: [
 depends: [
   "ocaml" {>= "4.08.0"}
   "opam-format" {= version}
+  "patch" {>= "3.0.0~alpha1"}
   "dune" {>= "2.8.0"}
 ]

opam-repository.opam

@@ -367,18 +367,18 @@ let prepare_package_build env opam nv dir =
 
   let apply_patches ?(dryrun=false) () =
     let patch base =
-      if dryrun then Done None else
-        OpamFilename.patch
+      if dryrun then None else
+        OpamFilename.patch ~allow_unclean:true
           (dir // OpamFilename.Base.to_string base) dir
     in
     let rec aux = function
-      | [] -> Done []
+      | [] -> []
       | (patchname,filter)::rest ->
         if OpamFilter.opt_eval_to_bool env filter then
           (print_apply patchname;
-           patch patchname @@+ function
+           match patch patchname with
            | None -> aux rest
-           | Some err -> aux rest @@| fun e -> (patchname, err) :: e)
+           | Some err -> (patchname, err) :: aux rest)
         else aux rest
     in
     aux patches
@@ -390,63 +390,57 @@ let prepare_package_build env opam nv dir =
   in
   if OpamStateConfig.(!r.dryrun) || OpamClientConfig.(!r.fake) then
     (List.iter print_subst (OpamFile.OPAM.substs opam);
-     apply_patches ~dryrun:true ()) @@| fun _ -> None
+     let _ : _ list = apply_patches ~dryrun:true () in
+     None)
   else
-  let subst_errs =
-    OpamFilename.in_dir dir  @@ fun () ->
-    List.fold_left (fun errs f ->
-        try
-          print_subst f;
-          OpamFilter.expand_interpolations_in_file env f;
-          errs
-        with e -> (f, e)::errs)
-      [] subst_patches
-  in
-
-  (* Apply the patches *)
-  let text =
-    OpamProcess.make_command_text (OpamPackage.Name.to_string nv.name) "patch"
-  in
-  OpamProcess.Job.with_text text (apply_patches ())
-  @@+ fun patching_errors ->
-
-  (* Substitute the configuration files. We should be in the right
-     directory to get the correct absolute path for the
-     substitution files (see [OpamFilter.expand_interpolations_in_file] and
-     [OpamFilename.of_basename]. *)
-  let subst_errs =
-    OpamFilename.in_dir dir @@ fun () ->
-    List.fold_left (fun errs f ->
-        try
-          print_subst f;
-          OpamFilter.expand_interpolations_in_file env f;
-          errs
-        with e -> (f, e)::errs)
-      subst_errs subst_others
-  in
-  if patching_errors <> [] || subst_errs <> [] then
-    let msg =
-      (if patching_errors <> [] then
-         Printf.sprintf "These patches didn't apply at %s:\n%s"
-           (OpamFilename.Dir.to_string dir)
-           (OpamStd.Format.itemize
-              (fun (f,err) ->
-                 Printf.sprintf "%s: %s"
-                   (OpamFilename.Base.to_string f) (Printexc.to_string err))
-              patching_errors)
-       else "") ^
-      (if subst_errs <> [] then
-         Printf.sprintf "String expansion failed for these files:\n%s"
-           (OpamStd.Format.itemize
-              (fun (b,err) ->
-                 Printf.sprintf "%s.in: %s" (OpamFilename.Base.to_string b)
-                   (Printexc.to_string err))
-           subst_errs)
-       else "")
+    let subst_errs =
+      OpamFilename.in_dir dir  @@ fun () ->
+      List.fold_left (fun errs f ->
+          try
+            print_subst f;
+            OpamFilter.expand_interpolations_in_file env f;
+            errs
+          with e -> (f, e)::errs)
+        [] subst_patches
     in
-    Done (Some (Failure msg))
-  else
-    Done None
+    let patching_errors = apply_patches () in
+    (* Substitute the configuration files. We should be in the right
+       directory to get the correct absolute path for the
+       substitution files (see [OpamFilter.expand_interpolations_in_file] and
+       [OpamFilename.of_basename]. *)
+    let subst_errs =
+      OpamFilename.in_dir dir @@ fun () ->
+      List.fold_left (fun errs f ->
+          try
+            print_subst f;
+            OpamFilter.expand_interpolations_in_file env f;
+            errs
+          with e -> (f, e)::errs)
+        subst_errs subst_others
+    in
+    if patching_errors <> [] || subst_errs <> [] then
+      let msg =
+        (if patching_errors <> [] then
+           Printf.sprintf "These patches didn't apply at %s:\n%s"
+             (OpamFilename.Dir.to_string dir)
+             (OpamStd.Format.itemize
+                (fun (f,err) ->
+                   Printf.sprintf "%s: %s"
+                     (OpamFilename.Base.to_string f) (Printexc.to_string err))
+                patching_errors)
+         else "") ^
+        (if subst_errs <> [] then
+           Printf.sprintf "String expansion failed for these files:\n%s"
+             (OpamStd.Format.itemize
+                (fun (b,err) ->
+                   Printf.sprintf "%s.in: %s" (OpamFilename.Base.to_string b)
+                     (Printexc.to_string err))
+                subst_errs)
+         else "")
+      in
+      Some (Failure msg)
+    else
+      None
 
 let prepare_package_source st nv dir =
   log "prepare_package_source: %a at %a"
@@ -517,7 +511,7 @@ let prepare_package_source st nv dir =
   get_extra_sources_job @@+ function Some _ as err -> Done err | None ->
     check_extra_files |> function Some _ as err -> Done err | None ->
       let opam = OpamSwitchState.opam st nv in
-      prepare_package_build (OpamPackageVar.resolve ~opam st) opam nv dir
+      Done (prepare_package_build (OpamPackageVar.resolve ~opam st) opam nv dir)
 
 let compilation_env t opam =
   let build_env =

src/client/opamAction.ml

@@ -40,7 +40,7 @@ val prepare_package_source:
     of `prepare_package_source`, without requiring a switch and
     without handling extra downloads. *)
 val prepare_package_build:
-  OpamFilter.env -> OpamFile.OPAM.t -> package -> dirname -> exn option OpamProcess.job
+  OpamFilter.env -> OpamFile.OPAM.t -> package -> dirname -> exn option
 
 (** [build_package t build_dir pkg] builds the package [pkg] within [build_dir].
     Returns [None] on success, [Some exn] on error.

src/client/opamAction.mli

@@ -52,11 +52,6 @@ let not_win32_filter =
   FOp (FIdent ([], OpamVariable.of_string "os", None), `Neq, FString "win32")
 let sandbox_filter = FOr (linux_filter, macos_filter)
 
-let gpatch_filter =
-  FOr (FOr (openbsd_filter, netbsd_filter),
-       FOr (freebsd_filter, FOr (dragonflybsd_filter, macos_filter)))
-let patch_filter = FNot gpatch_filter
-
 let gtar_filter = openbsd_filter
 let tar_filter = FNot gtar_filter
 
@@ -132,8 +127,6 @@ let required_tools ~sandboxing () =
   req_dl_tools () @
   [
     ["diff"], None, None;
-    ["patch"], None, Some patch_filter;
-    ["gpatch"], None, Some gpatch_filter;
     ["tar"], None, Some tar_filter;
     ["gtar"], None, Some gtar_filter;
     ["unzip"], None, None;
@@ -148,7 +141,6 @@ let required_packages_for_cygwin =
   [
     "diffutils";
     "make";
-    "patch";
     "tar";
     "unzip";
     "rsync";

src/client/opamInitDefaults.ml

@@ -3,7 +3,7 @@
   (public_name opam-core)
   (synopsis    "OCaml Package Manager core internal stdlib")
   ; TODO: Remove (re_export ...) when CI uses the OCaml version that includes https://github.com/ocaml/ocaml/pull/11989
-  (libraries   re (re_export ocamlgraph) unix sha jsonm swhid_core uutf)
+  (libraries   re (re_export ocamlgraph) unix sha jsonm swhid_core uutf patch)
   (flags       (:standard
                (:include ../ocaml-flags-standard.sexp)
                (:include ../ocaml-flags-configure.sexp)

src/core/dune

@@ -445,8 +445,9 @@ let link ?(relative=false) ~target ~link =
   OpamSystem.link target (to_string link)
 [@@ocaml.warning "-16"]
 
-let patch ?preprocess filename dirname =
-  OpamSystem.patch ?preprocess ~dir:(Dir.to_string dirname) (to_string filename)
+let patch ?preprocess ~allow_unclean filename dirname =
+  OpamSystem.patch ?preprocess ~allow_unclean ~dir:(Dir.to_string dirname)
+    (to_string filename)
 
 let flock flag ?dontblock file = OpamSystem.flock flag ?dontblock (to_string file)
 

src/core/opamFilename.ml

@@ -271,8 +271,12 @@ val remove_prefix_dir: Dir.t -> Dir.t -> string
 val remove_suffix: Base.t -> t -> string
 
 (** Apply a patch in a directory. If [preprocess] is set to false, there is no
-    CRLF translation. Returns [None] on success, the process error otherwise *)
-val patch: ?preprocess:bool -> t -> Dir.t -> exn option OpamProcess.job
+    CRLF translation. Returns [None] on success, the process error otherwise.
+
+    @param allow_unclean decides if applying a patch on a directory which
+    differs slightly from the one described in the patch file is allowed.
+    Allowing unclean applications imitates the default behaviour of GNU Patch. *)
+val patch: ?preprocess:bool -> allow_unclean:bool -> t -> Dir.t -> exn option
 
 (** Create an empty file *)
 val touch: t -> unit