Fallback to copying on symlink-less MSYS2

master_changes.md

@@ -223,6 +223,7 @@ users)
   * Remove memoization from `best_effort ()` to allow for multiple different settings during the same session (useful for libaray users) [#4805 @LasseBlaauwbroek]
   * [BUG] Catch `EACCES` in lock function [#4948 @oandrieu - fix #4944]
   * Permissions: chmod+unlink before copy [#4827 @jonahbeckford @dra27]
+  * Support MSYS2: two-phase rsync on MSYS2 to allow MSYS2's behavior of copying rather than symlinking [#4817 @jonahbeckford]
 
 ## Test
   * Update crowbar with compare functions [#4918 @rjbou]
@@ -355,3 +356,5 @@ users)
   * `OpamProcess.wait_one`: display command in verbose mode for finished found process [#5091 @rjbou]
   * `OpamStd.Config.E`: add a `REMOVED` variant to allow removing completely an environment variable handling [#5112 @rjbou]
   * `OpamHash`: add `is_null`
+  * `OpamStd.Sys`: add `get_windows_executable_variant` to use instead of `is_cygwin_variant` [#4817 @jonahbeckford]
+  * `OpamSystem.copy_dir`: two-pass `rsync` copy for `MSYS2`, to handle symlinks [#4817 @jonahbeckford]

src/core/opamStd.ml

@@ -1004,7 +1004,7 @@ module OpamSys = struct
       (fun f -> try f () with _ -> ())
       !registered_at_exit
 
-  let is_cygwin_variant =
+  let get_windows_executable_variant =
     if Sys.win32 then
       let results = Hashtbl.create 17 in
       let requires_cygwin name =
@@ -1013,19 +1013,26 @@ module OpamSys = struct
         let rec f a =
           match input_line c with
           | x ->
-              (* Treat MSYS2's variant of `cygwin1.dll` called `msys-2.0.dll` equivalently.
-                 Confer https://www.msys2.org/wiki/How-does-MSYS2-differ-from-Cygwin/ *)
-              let tx = String.trim x in
-              if (OpamString.ends_with ~suffix:"cygwin1.dll" tx ||
-                  OpamString.ends_with ~suffix:"msys-2.0.dll" tx) then
-                if OpamString.starts_with ~prefix:"  " x then
-                  f `Cygwin
-                else if a <> `Cygwin then
-                  f `CygLinked
-                else
-                  f a
+            (* Treat MSYS2's variant of `cygwin1.dll` called `msys-2.0.dll` equivalently.
+               Confer https://www.msys2.org/wiki/How-does-MSYS2-differ-from-Cygwin/ *)
+            let tx = String.trim x in
+            if (OpamString.ends_with ~suffix:"cygwin1.dll" tx ||
+                OpamString.ends_with ~suffix:"msys-2.0.dll" tx) then
+              if OpamString.starts_with ~prefix:"  " x then
+                f `Cygwin
+              else if a = `Native then
+                f (`Tainted `Cygwin)
               else
                 f a
+            else if OpamString.ends_with ~suffix:"msys-2.0.dll" tx then
+              if OpamString.starts_with ~prefix:"  " x then
+                f `Msys2
+              else if a = `Native then
+                f (`Tainted `Msys2)
+              else
+                f a
+            else
+              f a
           | exception e ->
               Unix.close_process_full process |> ignore;
               fatal e;
@@ -1047,6 +1054,13 @@ module OpamSys = struct
     else
       fun _ -> `Native
 
+  let is_cygwin_variant cmd =
+    match get_windows_executable_variant cmd with
+    | `Native -> `Native
+    | `Cygwin
+    | `Msys2 -> `Cygwin
+    | `Tainted _ -> `CygLinked
+
   exception Exit of int
   exception Exec of string * string array * string array
 

src/core/opamStd.mli

@@ -455,12 +455,26 @@ module Sys : sig
   val split_path_variable: ?clean:bool -> string -> string list
 
   (** For native Windows builds, returns [`Cygwin] if the command is a Cygwin-
-      compiled executable, [`CygLinked] if the command links to a library which is
-      itself Cygwin-compiled or [`Native] otherwise.
+      compiled executable, [`Msys2] if the command is a MSYS2-compiled
+      executable, and [`Tainted of [ `Msys2 | `Cygwin ]] if the command links
+      to a library which is itself Cygwin- or MSYS2-compiled, or [`Native]
+      otherwise.
 
       Note that this returns [`Native] on a Cygwin-build of opam!
 
-      Both cygcheck and an unqualified command will be resolved using the current PATH. *)
+      Both cygcheck and an unqualified command will be resolved using the
+      current PATH. *)
+  val get_windows_executable_variant:
+    string -> [ `Native | `Cygwin | `Tainted of [ `Msys2 | `Cygwin] | `Msys2 ]
+
+  (** For native Windows builds, returns [`Cygwin] if the command is a Cygwin-
+      or Msys2- compiled executable, and [`CygLinked] if the command links to a
+      library which is itself Cygwin/Msys2-compiled, or [`Native] otherwise.
+
+      Note that this returns [`Native] on a Cygwin-build of opam!
+
+      Both cygcheck and an unqualified command will be resolved using the
+      current PATH. *)
   val is_cygwin_variant: string -> [ `Native | `Cygwin | `CygLinked ]
 
   (** {3 Exit handling} *)

src/core/opamSystem.ml

@@ -10,7 +10,8 @@
 (**************************************************************************)
 
 type install_warning =
-  [ `Add_exe | `Install_dll | `Install_script | `Install_unknown | `Cygwin | `Cygwin_libraries ]
+  [ `Add_exe | `Install_dll | `Install_script | `Install_unknown
+  | `Cygwin | `Msys2 | `Tainted of [`Msys2 | `Cygwin] | `Cygwin_libraries ]
 type install_warning_fn = string -> install_warning -> unit
 
 exception Process_error of OpamProcess.result
@@ -627,15 +628,60 @@ let copy_file src dst =
   copy_file_aux ~src ~dst ()
 
 let copy_dir src dst =
-  if Sys.file_exists dst then
+  (* MSYS2 requires special handling because its uses copying rather than
+     symlinks for maximum portability on Windows. However copying a source
+     directory containing symlinks presents a problem.
+
+     As a real example look at https://github.com/OCamlPro/ocp-indent/tree/1.8.2/tests/inplace:
+
+      $ ls -l tests/inplace/
+      total 0
+      -rw-r--r-- 1 user group  0 Aug 12 20:53 executable.ml
+      lrwxrwxrwx 1 user group 12 Aug 12 20:53 link.ml -> otherfile.ml
+      lrwxrwxrwx 1 user group  7 Aug 12 20:53 link2.ml -> link.ml
+      -rw-r--r-- 1 user group  0 Aug 12 20:53 otherfile.ml
+
+    With a regular copy:
+
+      cp -PRp ...\ocp-indent-1.8.1\tests ... \tmp\ocp-indent.1.8.1
+
+    it _can_ fail with:
+
+      # /usr/bin/cp: cannot create symbolic link 'C:\somewhere/tests/inplace/link.ml': No such file or directory
+      # /usr/bin/cp: cannot create symbolic link 'C:\somewhere/tests/inplace/link2.ml': No such file or directory
+
+    What is happening is that _if_ link2.ml is copied before link.ml, then the
+    copy of link2.ml will fail with "No such file or directory". What is worse,
+    it depends on the opaque order in which the files are copied; sometimes it
+    can work and sometimes it won't.
+
+    So we do a two-pass copy. The first pass copies everything except the
+    symlinks, and the second pass copies everything that remained. Rsync is the
+    perfect tool for that.
+   *)
+  if OpamStd.Sys.get_windows_executable_variant "rsync" = `Msys2 then
+    let convert_path = Lazy.force (get_cygpath_function ~command:"rsync") in
+    (* ensure that rsync doesn't recreate a subdir: add trailing '/' even if
+       cygpath may add one *)
+    let trailingslash_cygsrc =
+      (OpamStd.String.remove_suffix ~suffix:"/" (convert_path src)) ^ "/"
+    in
+    let cygdest = convert_path dst in
+    (if Sys.file_exists dst then () else mkdir (Filename.dirname dst);
+     command ~verbose:(verbose_for_base_commands ())
+       ([ "rsync"; "-a"; "--no-links"; trailingslash_cygsrc; cygdest ]);
+     command ~verbose:(verbose_for_base_commands ())
+       ([ "rsync"; "-a"; "--ignore-existing"; trailingslash_cygsrc; cygdest ]))
+  else if Sys.file_exists dst then
     if Sys.is_directory dst then
       match ls src with
       | [] -> ()
       | srcfiles ->
         command ~verbose:(verbose_for_base_commands ())
           ([ "cp"; "-PRp" ] @ srcfiles @ [ dst ])
-    else internal_error "Can not copy dir %s to %s, which is not a directory"
-        src dst
+    else
+      internal_error
+        "Can not copy dir %s to %s, which is not a directory" src dst
   else
     (mkdir (Filename.dirname dst);
      command ~verbose:(verbose_for_base_commands ())
@@ -722,21 +768,33 @@ let classify_executable file =
     `Unknown
 
 let default_install_warning dst = function
-| `Add_exe ->
+  | `Add_exe ->
     OpamConsole.warning "Automatically adding .exe to %s" dst
-| `Install_dll ->
-    (* TODO Installation of .dll to bin is unfortunate, but not sure if it should be a warning *)
+  | `Install_dll ->
+    (* TODO Installation of .dll to bin is unfortunate, but not sure if it
+       should be a warning *)
     ()
-| `Install_script ->
+  | `Install_script ->
     (* TODO Generate a .cmd wrapper (and warn about it - they're not perfect) *)
     OpamConsole.warning "%s is a script; the command won't be available" dst;
-| `Install_unknown ->
-    (* TODO Installation of a non-executable file is unexpected, but not sure if it should be a warning/error *)
+  | `Install_unknown ->
+    (* TODO Installation of a non-executable file is unexpected, but not sure
+       if it should be a warning/error *)
     ()
-| `Cygwin ->
+  | `Cygwin ->
     OpamConsole.warning "%s is a Cygwin-linked executable" dst
-| `Cygwin_libraries ->
-    OpamConsole.warning "%s links with a Cygwin-compiled DLL (almost certainly a packaging or environment error)" dst
+  | `Msys2 ->
+    OpamConsole.warning "%s is a MSYS2-linked executable" dst
+  | `Tainted `Cygwin ->
+    OpamConsole.warning
+      "%s is an executable which links to a Cygwin-linked library" dst
+  | `Tainted `Msys2 ->
+    OpamConsole.warning
+      "%s is an executable which links to a MSYS2-linked library" dst
+  | `Cygwin_libraries ->
+    OpamConsole.warning
+      "%s links with a Cygwin-compiled DLL (almost certainly a packaging \
+       or environment error)" dst
 
 let install ?(warning=default_install_warning) ?exec src dst =
   if Sys.is_directory src then
@@ -771,13 +829,11 @@ let install ?(warning=default_install_warning) ?exec src dst =
       in
       copy_file_aux ~src ~dst ();
       if cygcheck then
-        match OpamStd.Sys.is_cygwin_variant dst with
-          `Native ->
+        match OpamStd.Sys.get_windows_executable_variant dst with
+        | `Native ->
             ()
-        | `Cygwin ->
-            warning dst `Cygwin
-        | `CygLinked ->
-            warning dst `Cygwin_libraries
+        | (`Cygwin | `Msys2 | `Tainted _) as code ->
+            warning dst code
     end else
       copy_file_aux ~src ~dst ()
   else

src/core/opamSystem.mli

@@ -67,6 +67,8 @@ type install_warning = [ `Add_exe          (* [.exe] had to be added *)
                        | `Install_script   (* Installation of script on Windows *)
                        | `Install_unknown  (* Installation of unknown file to bin/libexec *)
                        | `Cygwin           (* Installation of a Cygwin-linked executable *)
+                       | `Msys2            (* Installation of a MSYS2-linked executable *)
+                       | `Tainted of [`Msys2 | `Cygwin] (* Installation of an executable which itself is linked to a Cygwin or MSYS2-linked library *)
                        | `Cygwin_libraries (* Installation of a binary linked to a Cygwin library *)
                        ]
 (** Warnings which come from {!install} *)