From 29af2fd406faf7ab845374d1d0b95c6e529d73d0 Mon Sep 17 00:00:00 2001 From: Kate Date: Wed, 26 Mar 2025 16:26:48 +0000 Subject: [PATCH 1/4] Replace the use of GNU Patch by the OCaml patch library --- .github/workflows/ci.ml | 4 +- .github/workflows/main.yml | 4 +- configure | 26 +++++++ configure.ac | 2 + master_changes.md | 10 +++ opam-core.opam | 1 + opam-repository.opam | 1 + src/client/opamAction.ml | 116 +++++++++++++++---------------- src/client/opamAction.mli | 2 +- src/client/opamInitDefaults.ml | 8 --- src/core/dune | 2 +- src/core/opamFilename.ml | 5 +- src/core/opamFilename.mli | 8 ++- src/core/opamSystem.ml | 106 ++++++++++++++++++---------- src/core/opamSystem.mli | 8 ++- src/repository/opamRepository.ml | 10 +-- src_ext/Makefile | 5 +- src_ext/Makefile.sources | 3 + tests/reftests/repository.test | 16 ++--- 19 files changed, 202 insertions(+), 135 deletions(-) diff --git a/.github/workflows/ci.ml b/.github/workflows/ci.ml index 2fc71589177..c2e22d86095 100644 --- a/.github/workflows/ci.ml +++ b/.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/"); diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 4b4631d2d2a..ce1c2187db7 100644 --- a/.github/workflows/main.yml +++ b/.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 diff --git a/configure b/configure index 10a0cd26ce9..5765227408c 100755 --- a/configure +++ b/configure @@ -622,6 +622,7 @@ ac_ct_CXX CXXFLAGS CXX OCAML_PKG_mccs +OCAML_PKG_patch OCAML_PKG_swhid_core OCAML_PKG_sha OCAML_PKG_uutf @@ -6521,6 +6522,30 @@ printf "%s\n" "not found" >&6; } + { printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for OCaml findlib package patch" >&5 +printf %s "checking for OCaml findlib package patch... " >&6; } + + unset found + unset pkg + found=no + for pkg in patch ; do + if $OCAMLFIND query $pkg >/dev/null 2>/dev/null; then + { printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: found" >&5 +printf "%s\n" "found" >&6; } + OCAML_PKG_patch=$pkg + found=yes + break + fi + done + if test "$found" = "no" ; then + { printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: not found" >&5 +printf "%s\n" "not found" >&6; } + OCAML_PKG_patch=no + fi + + + + # Optional dependencies { printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for OCaml findlib package mccs 1.1+17 or later" >&5 @@ -7047,6 +7072,7 @@ 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";} then : diff --git a/configure.ac b/configure.ac index 919416bd320..22d13216573 100644 --- a/configure.ac +++ b/configure.ac @@ -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]) diff --git a/master_changes.md b/master_changes.md index ea555e4fa3e..60a7c3388eb 100644 --- a/master_changes.md +++ b/master_changes.md @@ -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 ` upgrading unrelated packages [#6373 @AltGr] * [BUG] Fix a regression on `opam upgrade --all ` 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] diff --git a/opam-core.opam b/opam-core.opam index e9ab6da90b8..d42bc942f91 100644 --- a/opam-core.opam +++ b/opam-core.opam @@ -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"} & diff --git a/opam-repository.opam b/opam-repository.opam index 96505f6887c..3264dd39f0b 100644 --- a/opam-repository.opam +++ b/opam-repository.opam @@ -30,5 +30,6 @@ build: [ depends: [ "ocaml" {>= "4.08.0"} "opam-format" {= version} + "patch" {>= "3.0.0~alpha1"} "dune" {>= "2.8.0"} ] diff --git a/src/client/opamAction.ml b/src/client/opamAction.ml index 7781e4aebc5..81a83ee0e75 100644 --- a/src/client/opamAction.ml +++ b/src/client/opamAction.ml @@ -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 = diff --git a/src/client/opamAction.mli b/src/client/opamAction.mli index 029f1b203b4..419f0bc833a 100644 --- a/src/client/opamAction.mli +++ b/src/client/opamAction.mli @@ -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. diff --git a/src/client/opamInitDefaults.ml b/src/client/opamInitDefaults.ml index c077b787766..e92711ad1db 100644 --- a/src/client/opamInitDefaults.ml +++ b/src/client/opamInitDefaults.ml @@ -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"; diff --git a/src/core/dune b/src/core/dune index 89beb7e47ea..df971773c48 100644 --- a/src/core/dune +++ b/src/core/dune @@ -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) diff --git a/src/core/opamFilename.ml b/src/core/opamFilename.ml index 1806b9d3323..bd438f89cc8 100644 --- a/src/core/opamFilename.ml +++ b/src/core/opamFilename.ml @@ -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) diff --git a/src/core/opamFilename.mli b/src/core/opamFilename.mli index c9b2589ff4f..1b571223967 100644 --- a/src/core/opamFilename.mli +++ b/src/core/opamFilename.mli @@ -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 diff --git a/src/core/opamSystem.ml b/src/core/opamSystem.ml index deb8515142e..23779496133 100644 --- a/src/core/opamSystem.ml +++ b/src/core/opamSystem.ml @@ -1588,41 +1588,73 @@ let translate_patch ~dir orig corrected = end; close_in ch -let gpatch = lazy begin - let rec search_gpatch = function - | [] -> None - | patch_cmd::patch_cmds -> - match OpamProcess.run (make_command ~name:"patch" patch_cmd ["--version"]) with - | r -> - (match OpamProcess.is_success r, r.OpamProcess.r_stdout with - | true, full::_ when - OpamStd.String.is_prefix_of ~from:0 ~full "GNU patch " -> - Some patch_cmd - | _ -> - search_gpatch patch_cmds) - | exception _ -> search_gpatch patch_cmds +exception Internal_patch_error of string + +let internal_patch ~allow_unclean ~patch_filename ~dir diffs = + let internal_patch_error fmt = + Printf.ksprintf (fun str -> raise (Internal_patch_error str)) fmt in - let default_cmd, other_cmds = - match OpamStd.Sys.os () with - | Darwin - | DragonFly - | FreeBSD - | NetBSD - | OpenBSD -> ("gpatch", ["patch"]) - | Cygwin - | Linux - | Unix - | Win32 - | Other _ -> ("patch", ["gpatch"]) + (* NOTE: It is important to keep this `concat dir ""` to ensure the + is_prefix_of below doesn't match another similarly named directory *) + let dir = Filename.concat (real_path dir) "" in + let get_path file = + let file = real_path (Filename.concat dir file) in + if not (OpamStd.String.is_prefix_of ~from:0 ~full:file dir) then + internal_patch_error "Patch %S tried to escape its scope." + patch_filename; + file in - match search_gpatch (default_cmd :: other_cmds) with - | Some gpatch -> gpatch - | None -> - OpamConsole.warning "Invalid patch utility. Please install GNU patch"; - default_cmd -end + let patch ~file content diff = + (* NOTE: The None case returned by [Patch.patch] is only returned + if [diff = Patch.Delete _]. This sub-function is not called in + this case so we [assert false] instead. *) + match Patch.patch ~cleanly:true content diff with + | Some x -> x + | None -> assert false (* See NOTE above *) + | exception _ when not allow_unclean -> + internal_patch_error "Patch %S does not apply cleanly." + patch_filename + | exception _ -> + match Patch.patch ~cleanly:false content diff with + | Some x -> + OpamStd.Option.iter (write (file^".orig")) content; + x + | None -> assert false (* See NOTE above *) + | exception _ -> + OpamStd.Option.iter (write (file^".orig")) content; + write (file^".rej") (Format.asprintf "%a" Patch.pp diff); + internal_patch_error "Patch %S does not apply cleanly." + patch_filename + in + let apply diff = match diff.Patch.operation with + | Patch.Edit (file1, file2) -> + (* That seems to be the GNU patch behaviour *) + let file = + let file1 = get_path file1 in + if Sys.file_exists file1 then + file1 + else + get_path file2 + in + let content = read file in + let content = patch ~file:file (Some content) diff in + write file content; + | Patch.Delete file -> + let file = get_path file in + remove_file_t ~with_log:false file + | Patch.Create file -> + let file = get_path file in + let content = patch ~file None diff in + write file content + | Patch.Rename_only (src, dst) -> + let src = get_path src in + let dst = get_path dst in + (* we use rename as we have all guarantee *) + Unix.rename src dst + in + List.iter apply diffs -let patch ?(preprocess=true) ~dir p = +let patch ?(preprocess=true) ~allow_unclean ~dir p = if not (Sys.file_exists p) then (OpamConsole.error "Patch file %S not found." p; raise Not_found); @@ -1634,11 +1666,13 @@ let patch ?(preprocess=true) ~dir p = else p in - let patch_cmd = Lazy.force gpatch in - make_command ~name:"patch" ~dir patch_cmd ["-p1"; "-i"; p'] @@> fun r -> + let content = read p' in + try + let diffs = Patch.parse ~p:1 content in + internal_patch ~allow_unclean ~patch_filename:p ~dir diffs; if not (OpamConsole.debug ()) then Sys.remove p'; - if OpamProcess.is_success r then Done None - else Done (Some (Process_error r)) + None + with exn -> Some exn let register_printer () = Printexc.register_printer (function diff --git a/src/core/opamSystem.mli b/src/core/opamSystem.mli index 9f8e3de2a9d..52bff39b393 100644 --- a/src/core/opamSystem.mli +++ b/src/core/opamSystem.mli @@ -327,8 +327,12 @@ val get_lock_fd: lock -> Unix.file_descr (** Apply a patch file in the current directory. If [preprocess] is set to false, there is no CRLF translation. Returns the error if the patch didn't - apply. *) -val patch: ?preprocess:bool -> dir:string -> string -> exn option OpamProcess.job + apply. + + @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 -> dir:string -> string -> exn option (** Returns the end-of-line encoding style for the given file. [None] means that either the encoding of line endings is mixed, or the file contains no line diff --git a/src/repository/opamRepository.ml b/src/repository/opamRepository.ml index 1ff3eddcca4..d18d9131041 100644 --- a/src/repository/opamRepository.ml +++ b/src/repository/opamRepository.ml @@ -538,11 +538,11 @@ let apply_repo_update repo repo_root = function | `http | `rsync -> false | _ -> true in - (OpamFilename.patch ~preprocess f repo_root @@+ function - | Some e -> - if not (OpamConsole.debug ()) then OpamFilename.remove f; - raise e - | None -> OpamFilename.remove f; Done ()) + (match OpamFilename.patch ~preprocess ~allow_unclean:false f repo_root with + | Some e -> + if not (OpamConsole.debug ()) then OpamFilename.remove f; + raise e + | None -> OpamFilename.remove f; Done ()) | Update_empty -> OpamConsole.msg "[%s] no changes from %s\n" (OpamConsole.colorise `green diff --git a/src_ext/Makefile b/src_ext/Makefile index 6df8f66cce9..30b5b446852 100644 --- a/src_ext/Makefile +++ b/src_ext/Makefile @@ -22,7 +22,10 @@ ifndef FETCH endif endif -SRC_EXTS = cppo base64 extlib re cmdliner ocamlgraph cudf dose3 opam-file-format seq stdlib-shims spdx_licenses opam-0install-cudf 0install-solver uutf jsonm sha swhid_core menhir +SRC_EXTS = \ + cppo base64 extlib re cmdliner ocamlgraph cudf dose3 opam-file-format seq \ + stdlib-shims spdx_licenses opam-0install-cudf 0install-solver uutf \ + jsonm sha swhid_core menhir patch ifeq ($(MCCS_ENABLED),true) SRC_EXTS := $(SRC_EXTS) mccs diff --git a/src_ext/Makefile.sources b/src_ext/Makefile.sources index 01edf1a2989..6bf1bf8fc51 100644 --- a/src_ext/Makefile.sources +++ b/src_ext/Makefile.sources @@ -59,3 +59,6 @@ MD5_swhid_core = 77d88d4b1d96261c866f140c64d89af8 URL_menhir = https://gitlab.inria.fr/fpottier/menhir/-/archive/20240715/archive.tar.gz MD5_menhir = d39a8943fe1be28199e5ec1f4133504c + +URL_patch = https://github.com/hannesm/patch/releases/download/v3.0.0-alpha1/patch-3.0.0-alpha1.tar.gz +MD5_patch = 03aa87f8500c9caf4a73b2299c19b514 diff --git a/tests/reftests/repository.test b/tests/reftests/repository.test index fa07335b318..af182d775e7 100644 --- a/tests/reftests/repository.test +++ b/tests/reftests/repository.test @@ -18,10 +18,8 @@ some content ### sh hash.sh REPO foo.1 ### : Internal repository storage as archive or plain directory : ### opam switch create tarring --empty -### opam update -vv | grep '^\+' | sed-cmd diff | sed-cmd patch | sed-cmd gpatch | '\+ gpatch ' -> '+ patch ' | 'patch-[^"]+' -> 'patch' +### opam update -vv | grep '^\+' | sed-cmd diff + diff "-ruaN" "default" "default.new" (CWD=${BASEDIR}/OPAM/repo) -+ patch "--version" -+ patch "-p1" "-i" "${BASEDIR}/OPAM/log/patch" (CWD=${BASEDIR}/OPAM/repo/default) ### ls $OPAMROOT/repo | grep -v "cache" default lock @@ -38,10 +36,8 @@ build: ["test" "-f" "baz"] ### some content ### sh hash.sh REPO foo.2 -### opam update default -vv | grep '^\+' | sed-cmd tar | sed-cmd diff | sed-cmd patch | sed-cmd gpatch | '\+ gpatch ' -> '+ patch ' | 'patch-[^"]+' -> 'patch' +### opam update default -vv | grep '^\+' | sed-cmd tar | sed-cmd diff + diff "-ruaN" "default" "default.new" (CWD=${BASEDIR}/OPAM/repo) -+ patch "--version" -+ patch "-p1" "-i" "${BASEDIR}/OPAM/log/patch" (CWD=${BASEDIR}/OPAM/repo/default) + tar "cfz" "${BASEDIR}/OPAM/repo/default.tar.gz.tmp" "-C" "${BASEDIR}/OPAM/repo" "default" ### ls $OPAMROOT/repo | grep -v "cache" default.tar.gz @@ -78,11 +74,9 @@ build: ["test" "-f" "baz"] ### some content ### sh hash.sh REPO foo.4 -### opam update -vv | grep '^\+' | sed-cmd tar | sed-cmd diff | sed-cmd patch | sed-cmd gpatch | '\+ gpatch ' -> '+ patch ' | 'patch-[^"]+' -> 'patch' +### opam update -vv | grep '^\+' | sed-cmd tar | sed-cmd diff + tar "xfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz" "-C" "${OPAMTMP}" + diff "-ruaN" "tarred" "tarred.new" (CWD=${OPAMTMP}) -+ patch "--version" -+ patch "-p1" "-i" "${BASEDIR}/OPAM/log/patch" (CWD=${OPAMTMP}/tarred) + tar "cfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz.tmp" "-C" "${OPAMTMP}" "tarred" ### opam install foo.4 -vv | grep '^\+' | sed-cmd test | sed-cmd tar + tar "xfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz" "-C" "${OPAMTMP}" @@ -102,11 +96,9 @@ build: ["test" "-f" "quux"] ### some content ### sh hash.sh REPO foo.5 -### opam update -vv | grep '^\+' | sed-cmd tar | sed-cmd diff | sed-cmd patch | sed-cmd gpatch | '\+ gpatch ' -> '+ patch ' | 'patch-[^"]+' -> 'patch' +### opam update -vv | grep '^\+' | sed-cmd tar | sed-cmd diff + tar "xfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz" "-C" "${OPAMTMP}" + diff "-ruaN" "tarred" "tarred.new" (CWD=${OPAMTMP}) -+ patch "--version" -+ patch "-p1" "-i" "${BASEDIR}/OPAM/log/patch" (CWD=${OPAMTMP}/tarred) ### opam install foo.5 -vv | grep '^\+' | sed-cmd test + test "-f" "quux" (CWD=${BASEDIR}/OPAM/tarring/.opam-switch/build/foo.5) ### ls $OPAMROOT/repo | grep -v "cache" From 5a79d19e2a0d880d1d57bd9d8e65d360fdfe5efb Mon Sep 17 00:00:00 2001 From: Kate Date: Wed, 26 Mar 2025 17:44:00 +0000 Subject: [PATCH 2/4] OpamSystem.patch: do not remove the original patch file if called with ~preprocess:false --- master_changes.md | 1 + src/core/opamSystem.ml | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/master_changes.md b/master_changes.md index 60a7c3388eb..9dae11f3897 100644 --- a/master_changes.md +++ b/master_changes.md @@ -275,5 +275,6 @@ users) * `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] + * `OpamSystem.patch`: do not remove the original patch file if called with `~preprocess:false` [#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] diff --git a/src/core/opamSystem.ml b/src/core/opamSystem.ml index 23779496133..dfc4cf497dd 100644 --- a/src/core/opamSystem.ml +++ b/src/core/opamSystem.ml @@ -1670,7 +1670,7 @@ let patch ?(preprocess=true) ~allow_unclean ~dir p = try let diffs = Patch.parse ~p:1 content in internal_patch ~allow_unclean ~patch_filename:p ~dir diffs; - if not (OpamConsole.debug ()) then Sys.remove p'; + if preprocess && not (OpamConsole.debug ()) then Sys.remove p'; None with exn -> Some exn From 4efcfb59a898df2c67cb32d37ff75ec89028eb28 Mon Sep 17 00:00:00 2001 From: Kate Date: Wed, 26 Mar 2025 16:31:47 +0000 Subject: [PATCH 3/4] Replace the use of the system diff utility by the OCaml patch library --- master_changes.md | 8 +- src/client/opamInitDefaults.ml | 2 - src/core/opamSystem.mli | 4 + src/repository/dune | 2 +- src/repository/opamHTTP.ml | 17 ++- src/repository/opamLocal.ml | 9 +- src/repository/opamRepositoryBackend.ml | 127 +++++++++++++++++++---- src/repository/opamRepositoryBackend.mli | 11 +- tests/reftests/repository.test | 12 +-- tests/reftests/update.test | 1 - 10 files changed, 140 insertions(+), 53 deletions(-) diff --git a/master_changes.md b/master_changes.md index 9dae11f3897..ea723d852c4 100644 --- a/master_changes.md +++ b/master_changes.md @@ -68,7 +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 ` upgrading unrelated packages [#6373 @AltGr] * [BUG] Fix a regression on `opam upgrade --all ` 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] + * Updates are now applied using the `patch` OCaml library instead of the system GNU Patch and diff utilities [#5892 @kit-ty-kate - fix ocaml/setup-ocaml#933 #6052] ## Tree @@ -87,6 +87,7 @@ users) * Update SWH API request [#6036 @rjbou] * Rework SWH fallback to have a more correct archive retrieval and more fine grained error handling [#6036 @rjbou - fix #5721] * Check that the repositories given to `opam repository remove` actually exist [#5014 @kit-ty-kate - fixes #5012] + * ✘ Symlinks in repositories are no longer supported [#5892 @kit-ty-kate] ## Lock * [BUG] Fix `pin-depends` for `with-*` dependencies [#5471 @rjbou - fix #5428] @@ -104,7 +105,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] + * The system GNU Patch and diff are no longer runtime dependencies of opam [#5892 @kit-ty-kate - fix #6052] ## Format upgrade @@ -245,6 +246,8 @@ users) * `OpamDownload.download`: more fine grained HTTP request error code detection for curl [#6036 @rjbou] * `OpamRepository.revision`: now returns a `string` instead of a `version` [#6409 @kit-ty-kate] * `OpamRepositoryBackend.S.revision`: now returns a `string` instead of a `version` [#6409 @kit-ty-kate] + * `OpamRepositoryBackend.get_diff`: now returns `exn option` instead of `exn option OpamProcess.job` and no longer calls the system `diff` utility [#5892 @kit-ty-kate] + * `OpamRepositoryBackend.get_diff`: now raises `Stdlib.Failure` if an unsupported file type or comparison is detected [#5892 @kit-ty-kate] ## opam-state * `OpamStateConfig`: Make the `?lock_kind` parameters non-optional to avoid breaking the library users after they upgrade their opam root [#5488 @kit-ty-kate] @@ -272,6 +275,7 @@ users) * `OpamStd.Sys.get_freebsd_version`: was added, which returns the output of the `uname -U` command [#6217 @kit-ty-kate] * `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.get_files`: was exposed which returns the list of files (without prefix) inside the given directory [#5892 @kit-ty-kate] * `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] diff --git a/src/client/opamInitDefaults.ml b/src/client/opamInitDefaults.ml index e92711ad1db..5e6456023c5 100644 --- a/src/client/opamInitDefaults.ml +++ b/src/client/opamInitDefaults.ml @@ -126,7 +126,6 @@ let recommended_tools () = let required_tools ~sandboxing () = req_dl_tools () @ [ - ["diff"], None, None; ["tar"], None, Some tar_filter; ["gtar"], None, Some gtar_filter; ["unzip"], None, None; @@ -139,7 +138,6 @@ let required_tools ~sandboxing () = let required_packages_for_cygwin = [ - "diffutils"; "make"; "tar"; "unzip"; diff --git a/src/core/opamSystem.mli b/src/core/opamSystem.mli index 52bff39b393..4150ce13fda 100644 --- a/src/core/opamSystem.mli +++ b/src/core/opamSystem.mli @@ -125,6 +125,10 @@ val read: string -> string advisory write lock to prevent concurrent reads or writes) *) val write: string -> string -> unit +(** [get_files dir] returns the list of files (without prefix) inside the + directory [dir]. *) +val get_files : string -> string list + (** [remove filename] removes [filename]. Works whether [filename] is a file or a directory *) val remove: string -> unit diff --git a/src/repository/dune b/src/repository/dune index cb622b218b4..0815ce885ea 100644 --- a/src/repository/dune +++ b/src/repository/dune @@ -3,7 +3,7 @@ (public_name opam-repository) (synopsis "OCaml Package Manager remote repository handling library") ; TODO: Remove (re_export ...) when CI uses the OCaml version that includes https://github.com/ocaml/ocaml/pull/11989 - (libraries (re_export opam-format)) + (libraries (re_export opam-format) patch) (flags (:standard (:include ../ocaml-flags-standard.sexp) (:include ../ocaml-flags-configure.sexp) diff --git a/src/repository/opamHTTP.ml b/src/repository/opamHTTP.ml index 9c62d0ea231..fd4c4ac44e3 100644 --- a/src/repository/opamHTTP.ml +++ b/src/repository/opamHTTP.ml @@ -56,15 +56,14 @@ module B = struct OpamFilename.dir_is_empty repo_root then Done (OpamRepositoryBackend.Update_full quarantine) else - OpamProcess.Job.finally finalise @@ fun () -> - OpamRepositoryBackend.job_text repo_name "diff" - (OpamRepositoryBackend.get_diff - (OpamFilename.dirname_dir repo_root) - (OpamFilename.basename_dir repo_root) - (OpamFilename.basename_dir quarantine)) - @@| function - | None -> OpamRepositoryBackend.Update_empty - | Some patch -> OpamRepositoryBackend.Update_patch patch + OpamStd.Exn.finally finalise @@ fun () -> + OpamRepositoryBackend.get_diff + (OpamFilename.dirname_dir repo_root) + (OpamFilename.basename_dir repo_root) + (OpamFilename.basename_dir quarantine) + |> function + | None -> Done OpamRepositoryBackend.Update_empty + | Some patch -> Done (OpamRepositoryBackend.Update_patch patch) let repo_update_complete _ _ = Done () diff --git a/src/repository/opamLocal.ml b/src/repository/opamLocal.ml index 4b1f77d97da..5b08ed1be33 100644 --- a/src/repository/opamLocal.ml +++ b/src/repository/opamLocal.ml @@ -178,15 +178,14 @@ module B = struct OpamFilename.dir_is_empty repo_root then Done (OpamRepositoryBackend.Update_full quarantine) else - OpamProcess.Job.finally finalise @@ fun () -> - OpamRepositoryBackend.job_text repo_name "diff" @@ + OpamStd.Exn.finally finalise @@ fun () -> OpamRepositoryBackend.get_diff (OpamFilename.dirname_dir repo_root) (OpamFilename.basename_dir repo_root) (OpamFilename.basename_dir quarantine) - @@| function - | None -> OpamRepositoryBackend.Update_empty - | Some p -> OpamRepositoryBackend.Update_patch p + |> function + | None -> Done OpamRepositoryBackend.Update_empty + | Some p -> Done (OpamRepositoryBackend.Update_patch p) let repo_update_complete _ _ = Done () diff --git a/src/repository/opamRepositoryBackend.ml b/src/repository/opamRepositoryBackend.ml index fbc9305f2a1..ec3e4d8461b 100644 --- a/src/repository/opamRepositoryBackend.ml +++ b/src/repository/opamRepositoryBackend.ml @@ -10,7 +10,7 @@ open OpamTypes -let log = OpamConsole.log "REPO_BACKEND" +let log ?level fmt = OpamConsole.log "REPO_BACKEND" ?level fmt let slog = OpamConsole.slog type update = @@ -65,33 +65,120 @@ let check_digest filename = function false) | _ -> true -open OpamProcess.Job.Op - let job_text name label = OpamProcess.Job.with_text (Printf.sprintf "[%s: %s]" (OpamConsole.colorise `green (OpamRepositoryName.to_string name)) label) +let get_files_for_diff parent_dir dir1 dir2 = + let getfiles parent_dir dir = + let dir = Filename.concat (OpamFilename.Dir.to_string parent_dir) dir in + OpamSystem.get_files dir + in + match dir1, dir2 with + | None, None -> assert false + | Some dir, None -> + List.map (fun file -> Patch.Delete (dir^"/"^file)) + (getfiles parent_dir dir) + | None, Some dir -> + List.map (fun file -> Patch.Create (dir^"/"^file)) + (getfiles parent_dir dir) + | Some dir1, Some dir2 -> + let files1 = List.fast_sort String.compare (getfiles parent_dir dir1) in + let files2 = List.fast_sort String.compare (getfiles parent_dir dir2) in + let rec aux acc files1 files2 = match files1, files2 with + | (file1::files1 as orig1), (file2::files2 as orig2) -> + let cmp = String.compare file1 file2 in + if cmp = 0 then + aux (Patch.Edit + (dir1^"/"^file1, dir2^"/"^file2) + :: acc) + files1 files2 + else if cmp < 0 then + aux (Patch.Delete (dir1^"/"^file1) :: acc) files1 orig2 + else + aux (Patch.Create (dir2^"/"^file2) :: acc) orig1 files2 + | file1::files1, [] -> + aux (Patch.Delete (dir1^"/"^file1) :: acc) files1 [] + | [], file2::files2 -> + aux (Patch.Create (dir2^"/"^file2) :: acc) [] files2 + | [], [] -> + acc + in + aux [] files1 files2 + let get_diff parent_dir dir1 dir2 = + let chrono = OpamConsole.timer () in log "diff: %a/{%a,%a}" (slog OpamFilename.Dir.to_string) parent_dir (slog OpamFilename.Base.to_string) dir1 (slog OpamFilename.Base.to_string) dir2; - let patch = OpamSystem.temp_file ~auto_clean: false "patch" in - let patch_file = OpamFilename.of_string patch in - let finalise () = OpamFilename.remove patch_file in - OpamProcess.Job.catch (fun e -> finalise (); raise e) @@ fun () -> - OpamSystem.make_command - ~verbose:OpamCoreConfig.(!r.verbose_level >= 2) - ~dir:(OpamFilename.Dir.to_string parent_dir) ~stdout:patch - "diff" - [ "-ruaN"; - OpamFilename.Base.to_string dir1; - OpamFilename.Base.to_string dir2; ] - @@> function - | { OpamProcess.r_code = 0; _ } -> finalise(); Done None - | { OpamProcess.r_code = 1; _ } as r -> - OpamProcess.cleanup ~force:true r; - Done (Some patch_file) - | r -> OpamSystem.process_error r + let readfile parent_dir file = + let file = Filename.concat (OpamFilename.Dir.to_string parent_dir) file in + OpamSystem.read file + in + let lstat_opt parent_dir = function + | None -> None + | Some file -> + let file = Filename.concat (OpamFilename.Dir.to_string parent_dir) file in + Some (Unix.lstat file) + in + let rec aux diffs dir1 dir2 = + let files = get_files_for_diff parent_dir dir1 dir2 in + let diffs = + List.fold_left (fun diffs operation -> + let file1, file2 = match operation with + | Patch.Delete filename -> (Some filename, None) + | Patch.Create filename -> (None, Some filename) + | Patch.Edit (file1, file2) + | Patch.Rename_only (file1, file2) -> (Some file1, Some file2) + in + let add_to_diffs content1 content2 diffs = + match Patch.diff operation content1 content2 with + | None -> diffs + | Some diff -> diff :: diffs + in + match lstat_opt parent_dir file1, lstat_opt parent_dir file2 with + | Some {st_kind = S_REG; _}, None + | None, Some {st_kind = S_REG; _} + | Some {st_kind = S_REG; _}, Some {st_kind = S_REG; _} -> + let content1 = Option.map (readfile parent_dir) file1 in + let content2 = Option.map (readfile parent_dir) file2 in + add_to_diffs content1 content2 diffs + | Some {st_kind = S_DIR; _}, None | None, Some {st_kind = S_DIR; _} + | Some {st_kind = S_DIR; _}, Some {st_kind = S_DIR; _} -> + aux diffs file1 file2 + | Some {st_kind = S_DIR; _}, Some {st_kind = S_REG; _} -> + failwith "Change from a directory to a regular file is unsupported" + | Some {st_kind = S_REG; _}, Some {st_kind = S_DIR; _} -> + failwith "Change from a regular file to a directory is unsupported" + | Some {st_kind = S_LNK; _}, _ | _, Some {st_kind = S_LNK; _} -> + failwith "Symlinks are unsupported" + | Some {st_kind = S_CHR; _}, _ | _, Some {st_kind = S_CHR; _} -> + failwith "Character devices are unsupported" + | Some {st_kind = S_BLK; _}, _ | _, Some {st_kind = S_BLK; _} -> + failwith "Block devices are unsupported" + | Some {st_kind = S_FIFO; _}, _ | _, Some {st_kind = S_FIFO; _} -> + failwith "Named pipes are unsupported" + | Some {st_kind = S_SOCK; _}, _ | _, Some {st_kind = S_SOCK; _} -> + failwith "Sockets are unsupported" + | None, None -> assert false) + diffs files + in + diffs + in + match + aux [] + (Some (OpamFilename.Base.to_string dir1)) + (Some (OpamFilename.Base.to_string dir2)) + with + | [] -> + log "Internal diff (empty) done in %.2fs." (chrono ()); + None + | diffs -> + log "Internal diff (non-empty) done in %.2fs." (chrono ()); + let patch = OpamSystem.temp_file ~auto_clean:false "patch" in + let patch_file = OpamFilename.of_string patch in + OpamFilename.write patch_file (Format.asprintf "%a" Patch.pp_list diffs); + Some patch_file diff --git a/src/repository/opamRepositoryBackend.mli b/src/repository/opamRepositoryBackend.mli index 1c03b885413..107911b794f 100644 --- a/src/repository/opamRepositoryBackend.mli +++ b/src/repository/opamRepositoryBackend.mli @@ -111,8 +111,9 @@ val job_text: subdirs of [parent_dir], returns None if they are equal, and the corresponding patch otherwise. - Note: this relies on the [diff -ruN] command, a built-in diff may be more - portable -- in particular, [-u], [-N] are not POSIX, and recursive diffs - might not be completely reliable. It also assumes text files only, and fails - otherwise. *) -val get_diff: dirname -> basename -> basename -> filename option OpamProcess.job + @raise Stdlib.Failure if an unsupported file type or comparison is + detected in any of [subdir1] or [subdir2]. + Unsupported file types: symlinks, character devices, block devices, + named pipes, sockets. + Unsupported comparison: comparison between regular files and directories. *) +val get_diff: dirname -> basename -> basename -> filename option diff --git a/tests/reftests/repository.test b/tests/reftests/repository.test index af182d775e7..cbc0890dba0 100644 --- a/tests/reftests/repository.test +++ b/tests/reftests/repository.test @@ -18,8 +18,7 @@ some content ### sh hash.sh REPO foo.1 ### : Internal repository storage as archive or plain directory : ### opam switch create tarring --empty -### opam update -vv | grep '^\+' | sed-cmd diff -+ diff "-ruaN" "default" "default.new" (CWD=${BASEDIR}/OPAM/repo) +### opam update -vv | grep '^\+' ### ls $OPAMROOT/repo | grep -v "cache" default lock @@ -36,8 +35,7 @@ build: ["test" "-f" "baz"] ### some content ### sh hash.sh REPO foo.2 -### opam update default -vv | grep '^\+' | sed-cmd tar | sed-cmd diff -+ diff "-ruaN" "default" "default.new" (CWD=${BASEDIR}/OPAM/repo) +### opam update default -vv | grep '^\+' | sed-cmd tar + tar "cfz" "${BASEDIR}/OPAM/repo/default.tar.gz.tmp" "-C" "${BASEDIR}/OPAM/repo" "default" ### ls $OPAMROOT/repo | grep -v "cache" default.tar.gz @@ -74,9 +72,8 @@ build: ["test" "-f" "baz"] ### some content ### sh hash.sh REPO foo.4 -### opam update -vv | grep '^\+' | sed-cmd tar | sed-cmd diff +### opam update -vv | grep '^\+' | sed-cmd tar + tar "xfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz" "-C" "${OPAMTMP}" -+ diff "-ruaN" "tarred" "tarred.new" (CWD=${OPAMTMP}) + tar "cfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz.tmp" "-C" "${OPAMTMP}" "tarred" ### opam install foo.4 -vv | grep '^\+' | sed-cmd test | sed-cmd tar + tar "xfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz" "-C" "${OPAMTMP}" @@ -96,9 +93,8 @@ build: ["test" "-f" "quux"] ### some content ### sh hash.sh REPO foo.5 -### opam update -vv | grep '^\+' | sed-cmd tar | sed-cmd diff +### opam update -vv | grep '^\+' | sed-cmd tar + tar "xfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz" "-C" "${OPAMTMP}" -+ diff "-ruaN" "tarred" "tarred.new" (CWD=${OPAMTMP}) ### opam install foo.5 -vv | grep '^\+' | sed-cmd test + test "-f" "quux" (CWD=${BASEDIR}/OPAM/tarring/.opam-switch/build/foo.5) ### ls $OPAMROOT/repo | grep -v "cache" diff --git a/tests/reftests/update.test b/tests/reftests/update.test index 568829b1c1c..69c62178226 100644 --- a/tests/reftests/update.test +++ b/tests/reftests/update.test @@ -4,7 +4,6 @@ opam-version: "2.0" ### opam update --verbose <><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><> -Processing 1/1: [default: rsync] [default] synchronised from file://${BASEDIR}/REPO Processing: [default: loading data] Now run 'opam upgrade' to apply any package updates. From 9ad9ef5801cf0994c351e10c180485b91da7a619 Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Thu, 20 Mar 2025 15:42:40 +0100 Subject: [PATCH 4/4] Add a library test for the new OCaml implementation of patch and diff --- master_changes.md | 1 + tests/lib/dune | 5 + tests/lib/patchDiff.expected | 339 +++++++++++++++++++++++++++++++++++ tests/lib/patchDiff.ml | 312 ++++++++++++++++++++++++++++++++ 4 files changed, 657 insertions(+) create mode 100644 tests/lib/patchDiff.expected create mode 100644 tests/lib/patchDiff.ml diff --git a/master_changes.md b/master_changes.md index ea723d852c4..86d35dc6e5f 100644 --- a/master_changes.md +++ b/master_changes.md @@ -174,6 +174,7 @@ users) ## Internal: Windows ## Test + * Add a library test for the new OCaml implementation of patch and diff [#5892 @rjbou] ## Benchmarks * Add benchmarks for `opam show` [#6212 @kit-ty-kate] diff --git a/tests/lib/dune b/tests/lib/dune index 311c379869e..56108d82818 100644 --- a/tests/lib/dune +++ b/tests/lib/dune @@ -8,3 +8,8 @@ (libraries opam-format) (modules TypeGymnastics) (action (run %{test}))) + +(test + (name patchDiff) + (modules patchDiff) + (libraries opam-repository)) diff --git a/tests/lib/patchDiff.expected b/tests/lib/patchDiff.expected new file mode 100644 index 00000000000..f24f4355d3a --- /dev/null +++ b/tests/lib/patchDiff.expected @@ -0,0 +1,339 @@ + +---------------------- + Test 1: normal +---------------------- + +*** SETUP *** ++ first/ ++ first/diff-dir-plus-fst ++ first/diff-dir-plus-fst/fst + > foo ++ first/diff-dir-plus-snd ++ first/diff-dir-plus-snd/fst + > foo + > bar ++ first/diff-file + > foo ++ first/diff-file-plus-fst + > foo + > bar ++ first/diff-file-plus-snd + > foo ++ first/dir-only-fst ++ first/dir-only-fst/fst + > foo ++ first/file-only-fst + > foo ++ first/same-file + > foo ++ second/ ++ second/diff-dir-plus-fst ++ second/diff-dir-plus-fst/fst + > foo + > bar ++ second/diff-dir-plus-snd ++ second/diff-dir-plus-snd/fst + > foo ++ second/diff-file + > bar ++ second/diff-file-plus-fst + > foo ++ second/diff-file-plus-snd + > foo + > bar ++ second/file-only-snd + > foo ++ second/same-file + > foo + +*** DIFF *** +--- first/diff-dir-plus-fst/fst ++++ second/diff-dir-plus-fst/fst +@@ -2,0 +2,1 @@ ++bar +--- first/diff-dir-plus-snd/fst ++++ second/diff-dir-plus-snd/fst +@@ -2,1 +2,0 @@ +-bar +--- first/diff-file ++++ second/diff-file +@@ -1,1 +1,1 @@ +-foo ++bar +--- first/diff-file-plus-fst ++++ second/diff-file-plus-fst +@@ -2,1 +2,0 @@ +-bar +--- first/diff-file-plus-snd ++++ second/diff-file-plus-snd +@@ -2,0 +2,1 @@ ++bar +--- first/dir-only-fst/fst ++++ /dev/null +@@ -1,1 +0,0 @@ +-foo +--- first/file-only-fst ++++ /dev/null +@@ -1,1 +0,0 @@ +-foo +--- /dev/null ++++ second/file-only-snd +@@ -0,0 +1,1 @@ ++foo + +*** PATCHED *** ++ first/ ++ first/diff-dir-plus-fst ++ first/diff-dir-plus-fst/fst + > foo + > bar ++ first/diff-dir-plus-snd ++ first/diff-dir-plus-snd/fst + > foo ++ first/diff-file + > bar ++ first/diff-file-plus-fst + > foo ++ first/diff-file-plus-snd + > foo + > bar ++ first/dir-only-fst ++ first/file-only-snd + > foo ++ first/same-file + > foo ++ second/ ++ second/diff-dir-plus-fst ++ second/diff-dir-plus-fst/fst + > foo + > bar ++ second/diff-dir-plus-snd ++ second/diff-dir-plus-snd/fst + > foo ++ second/diff-file + > bar ++ second/diff-file-plus-fst + > foo ++ second/diff-file-plus-snd + > foo + > bar ++ second/file-only-snd + > foo ++ second/same-file + > foo + + +---------------------- + Test 2: diff file/dir error +---------------------- + +*** SETUP *** ++ first/ ++ first/file-fst-dir-snd + > foo ++ first/same-file + > foo ++ second/ ++ second/file-fst-dir-snd ++ second/file-fst-dir-snd/fst + > foo ++ second/same-file + > foo + +*** DIFF *** +ERROR: Change from a regular file to a directory is unsupported + +---------------------- + Test 3: diff dir/file error +---------------------- + +*** SETUP *** ++ first/ ++ first/dir-fst-file-snd ++ first/dir-fst-file-snd/fst + > foo ++ first/same-file + > foo ++ second/ ++ second/dir-fst-file-snd + > foo ++ second/same-file + > foo + +*** DIFF *** +ERROR: Change from a directory to a regular file is unsupported + +---------------------- + Test 4: symlink fst +---------------------- + +*** SETUP *** ++ first/ ++ first/linked-file-fst + > bar ++ first/same-file + > foo ++ second/ ++ second/linked-file-fst + > foo ++ second/same-file + > foo + +*** DIFF *** +ERROR: Symlinks are unsupported + +---------------------- + Test 5: symlink snd +---------------------- + +*** SETUP *** ++ first/ ++ first/linked-file-snd + > foo ++ first/same-file + > foo ++ second/ ++ second/linked-file-snd + > bar ++ second/same-file + > foo + +*** DIFF *** +ERROR: Symlinks are unsupported + +---------------------- + Test 6: hardlink fst +---------------------- + +*** SETUP *** ++ first/ ++ first/hardlinked-file-fst + > bar ++ first/same-file + > foo ++ second/ ++ second/hardlinked-file-fst + > foo ++ second/same-file + > foo + +*** DIFF *** +--- first/hardlinked-file-fst ++++ second/hardlinked-file-fst +@@ -1,1 +1,1 @@ +-bar ++foo + +*** PATCHED *** ++ first/ ++ first/hardlinked-file-fst + > foo ++ first/same-file + > foo ++ second/ ++ second/hardlinked-file-fst + > foo ++ second/same-file + > foo + + +---------------------- + Test 7: hardlink snd +---------------------- + +*** SETUP *** ++ first/ ++ first/hardlinked-file-snd + > foo ++ first/same-file + > foo ++ second/ ++ second/hardlinked-file-snd + > foo ++ second/same-file + > foo + +*** DIFF *** +No diff + +---------------------- + Test 8: patch error garbage +---------------------- + +*** SETUP *** ++ first/ ++ first/diff-file + > foo ++ first/same-file + > foo ++ second/ ++ second/diff-file + > bar ++ second/same-file + > foo + +*** GIVEN DIFF *** +something in +the file +that is not +patch format + +*** PATCHED *** ++ first/ ++ first/diff-file + > foo ++ first/same-file + > foo ++ second/ ++ second/diff-file + > bar ++ second/same-file + > foo + + +---------------------- + Test 9: patch truncated +---------------------- + +*** SETUP *** ++ first/ ++ first/diff-file + > foo ++ first/diff-file-plus-fst + > foo + > bar ++ first/same-file + > foo ++ second/ ++ second/diff-file + > bar ++ second/diff-file-plus-fst + > foo ++ second/same-file + > foo + +*** GIVEN DIFF *** +--- first/diff-file ++++ second/diff-file +@@ -1,1 +1,1 @@ +-foo ++bar +--- first/diff-fi + +*** PATCHED *** ++ first/ ++ first/diff-file + > bar ++ first/diff-file-plus-fst + > foo + > bar ++ first/same-file + > foo ++ second/ ++ second/diff-file + > bar ++ second/diff-file-plus-fst + > foo ++ second/same-file + > foo + diff --git a/tests/lib/patchDiff.ml b/tests/lib/patchDiff.ml new file mode 100644 index 00000000000..28fccb7d9d1 --- /dev/null +++ b/tests/lib/patchDiff.ml @@ -0,0 +1,312 @@ +type content = + | File of string (* file *) + | Dir of (string * string) list (* directory with list filename * content *) + | Symlink (* Soft Link *) + | Hardlink (* Hard link *) + | V (* void *) + +type arborescence = { + name: string; + first : content; + second : content; +} + +(** Contents *) + +let foo = "foo\n" +let bar = "bar\n" +let foobar = "foo\nbar\n" +let same_file = { + name = "same-file"; + first = File foo; + second = File foo; +} +let diff_file = { + name = "diff-file"; + first = File foo; + second = File bar; +} +let diff_file_plus_fst = { + name = "diff-file-plus-fst"; + first = File foobar; + second = File foo; +} +let diff_file_plus_snd = { + name = "diff-file-plus-snd"; + first = File foo; + second = File foobar; +} + +let content_working_diff = [ + same_file; + diff_file; + diff_file_plus_fst; + diff_file_plus_snd; + { name = "diff-file"; + first = File foo; + second = File bar; + }; + { name = "diff-file-plus-fst"; + first = File foobar; + second = File foo; + }; + { name = "diff-file-plus-snd"; + first = File foo; + second = File foobar; + }; + { name = "file-only-fst"; + first = File foo; + second = V; + }; + { name = "file-only-snd"; + first = V; + second = File foo; + }; + { name = "same-dir"; + first = Dir []; + second = Dir []; + }; + { name = "diff-dir-plus-fst"; + first = Dir [ "fst", foo ]; + second = Dir [ "fst", foobar ] ; + }; + { name = "diff-dir-plus-snd"; + first = Dir [ "fst", foobar ]; + second = Dir [ "fst", foo ]; + }; + { name = "dir-only-fst"; + first = Dir [ "fst", foo ]; + second = V; + }; +] + +let content_dir_file = [ + same_file; + { name = "file-fst-dir-snd"; + first = File foo; + second = Dir [ "fst", foo]; + }; +] + +let content_file_dir = [ + same_file; + { name = "dir-fst-file-snd"; + first = Dir [ "fst", foo ]; + second = File foo; + }; +] + +let content_symlink_fst = [ + same_file; + { name = "linked-file-fst"; + first = Symlink; + second = File foo; + }; +] + +let content_symlink_snd = [ + same_file; + { name = "linked-file-snd"; + first = File foo; + second = Symlink; + }; +] + +let content_hardlink_fst = [ + same_file; + { name = "hardlinked-file-fst"; + first = Hardlink; + second = File foo; + }; +] + +let content_hardlink_snd = [ + same_file; + { name = "hardlinked-file-snd"; + first = File foo; + second = Hardlink; + }; +] + + +let content_patch_failure_garbage = [ + same_file; + diff_file; +] +let diff_patch_failure_garbage = + "something in\n" ^ + "the file\n" ^ + "that is not\n" ^ + "patch format\n" + +let content_patch_failure_truncated = [ + same_file; + diff_file; + diff_file_plus_fst; +] +let diff_patch_failure_truncated = + "--- first/diff-file\n" ^ + "+++ second/diff-file\n" ^ + "@@ -1,1 +1,1 @@\n" ^ + "-foo\n" ^ + "+bar\n" ^ + "--- first/diff-fi\n" + +let _good_diff = + "\n" ^ + "--- first/diff-file\n" ^ + "+++ second/diff-file\n" ^ + "@@ -1,1 +1,1 @@\n" ^ + "-foo\n" ^ + "+bar\n" ^ + "--- first/diff-file-plus-fst\n" ^ + "+++ second/diff-file-plus-fst\n" ^ + "@@ -2,1 +2,0 @@\n" ^ + "-bar\n" + +(** Utils *) + +let print = Printf.printf + +open OpamFilename.Op +let read_dir root names = + let lst = + List.map (fun name -> + let dir = root / name in + (name^"/", []) :: + List.map (fun f -> + OpamFilename.remove_prefix root f, + OpamStd.String.split (OpamFilename.read f) '\n') + (OpamFilename.rec_files dir) + @ + List.map (fun d -> OpamFilename.remove_prefix_dir root d, []) + (OpamFilename.rec_dirs dir)) + names + |> List.flatten + |> List.map (fun (file, content) -> + (OpamSystem.back_to_forward file, content)) + in + + let lst = List.sort (fun (f,_) (f',_) -> String.compare f f') lst in + OpamStd.Format.itemize ~bullet:"+ " + (function + | d, [] -> d + | d, c -> + Printf.sprintf "%s\n%s" + d + (OpamStd.List.concat_map ~left:"" ~right:"" "\n" + (Printf.sprintf "> %s") c)) + lst + +let first = "first" +let second = "second" + +let write_setup dir content = + let first_root = dir / first in + let second_root = dir / second in + List.iter (fun d -> + OpamFilename.cleandir d; + OpamFilename.mkdir d) + [ first_root; second_root; ] ; + let link_f = + let link = lazy ( + let f = dir // "linked_file" in + if not (OpamFilename.exists f) then + OpamFilename.write f bar; + f + ) in + fun () -> Lazy.force link + in + let create inner_dir name = function + | File content -> + OpamFilename.write (inner_dir // name) content + | Dir lst -> + let inner_dir = inner_dir / name in + List.iter (fun (n,c) -> OpamFilename.write (inner_dir // n) c) lst + | Symlink -> + OpamFilename.link ~relative:false ~target:(link_f ()) + ~link:(inner_dir // name) + | Hardlink -> + let target = OpamFilename.to_string (link_f ()) in + let link = OpamFilename.to_string (inner_dir // name) in + Unix.link target link + | V -> () + in + List.iter (fun {name; first; second} -> + create first_root name first; + create second_root name second) + content + +type diff_patch = + | DiffPatch + | Patch of string + +let print_dirs dir = + print "%s\n" (read_dir dir [ first; second ]) + +let diff_patch dir content kind = + write_setup dir content; + print "*** SETUP ***\n"; + print_dirs dir; + let diff = + match kind with + | Patch patch -> + print "*** GIVEN DIFF ***\n"; + let fpatch = dir // "patch" in + OpamFilename.write fpatch patch; + Some fpatch + | DiffPatch -> + print "*** DIFF ***\n"; + match + OpamRepositoryBackend.get_diff dir + (OpamFilename.Base.of_string first) + (OpamFilename.Base.of_string second) + with + | exception Failure s -> print "ERROR: %s\n" s; None + | None -> print "No diff\n"; None + | some -> some + in + match diff with + | None -> () + | Some diff -> + print "%s\n" (OpamFilename.read diff); + let result = + OpamFilename.patch ~allow_unclean:false diff + (dir / first) + in + match result with + | None -> + print "*** PATCHED ***\n"; + print_dirs dir + | Some exn -> + print "*** PATCH ERROR ***\n"; + print "ERROR: %s\n" (Printexc.to_string exn) + +(** The tests *) + +let tests = [ + "normal", content_working_diff, DiffPatch; + "diff file/dir error", content_dir_file, DiffPatch; + "diff dir/file error", content_file_dir, DiffPatch; + "symlink fst", content_symlink_fst, DiffPatch; + "symlink snd", content_symlink_snd, DiffPatch; + "hardlink fst", content_hardlink_fst, DiffPatch; + "hardlink snd", content_hardlink_snd, DiffPatch; + "patch error garbage", content_patch_failure_garbage, + Patch diff_patch_failure_garbage; + "patch truncated", content_patch_failure_truncated, + Patch diff_patch_failure_truncated; +] + +let () = + (* This causes Windows to use LF endings instead of CRLF, which simplifies the comparison with the reference file *) + Unix.putenv "LC_ALL" "C"; + set_binary_mode_out stdout true; + Unix.dup2 Unix.stdout Unix.stderr; + OpamFilename.with_tmp_dir @@ fun dir -> + List.iteri (fun i (label, content, kind) -> + print "\n----------------------\n"; + print " Test %d: %s\n" (i+1) label; + print "----------------------\n\n"; + diff_patch dir content kind) + tests