Merge pull request #6276 from btjorge/remove-linked-switches

Remove linked switches without failing

master_changes.md

@@ -37,6 +37,7 @@ users)
   * Use a non-underline uppercase character to denotate the default when asking a question [#6289 @hannesm @kit-ty-kate - fix #6288]
 
 ## Switch
+  * [BUG] Fix `opam switch remove <dir>` failure when it is a linked switch [#6276 @btjorge - fix #6275]
 
 ## Config
 
@@ -121,6 +122,7 @@ users)
 
 ## Reftests
 ### Tests
+  * Add switch removal test: failure on removal linked switch [#6276 @btjorge]
 
 ### Engine
 
@@ -161,3 +163,4 @@ users)
   * `OpamStd.Sys.{uname,getconf}`: now accepts only one argument as parameter, as per their documentation [#6230 @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.remove_dir`: do not fail with an exception when directory is a symbolic link [#6276 @btjorge @rjbou - fix #6275]

src/core/opamSystem.ml

@@ -146,26 +146,32 @@ let remove_file_t ?(with_log=true) file =
   with Unix.Unix_error _ as e ->
     internal_error "Cannot remove %s (%s)." file (Printexc.to_string e)
 
+let is_reg_dir dir =
+  match Unix.lstat dir with
+  | {Unix.st_kind = Unix.S_DIR; _} -> true
+  | {Unix.st_kind =
+       Unix.(S_REG | S_LNK | S_CHR | S_BLK | S_FIFO | S_SOCK); _} ->
+    false
+  | exception Unix.(Unix_error (ENOENT, _, _)) when Sys.win32 ->
+    (* This is usually caused by invalid symlinks extracted by a
+       Cygwin/MSYS2 tar. *)
+    false
+
 let rec remove_dir_t dir =
   let files = get_files dir in
   List.iter (fun file ->
       let file = Filename.concat dir file in
-      match Unix.lstat file with
-      | {Unix.st_kind = Unix.S_DIR; _} ->
+      if is_reg_dir file then
         remove_dir_t file
-      | {Unix.st_kind = Unix.(S_REG | S_LNK | S_CHR | S_BLK | S_FIFO | S_SOCK); _} ->
-        remove_file_t ~with_log:false file
-      | exception Unix.(Unix_error (ENOENT, _, _)) when Sys.win32 ->
-        (* This is usually caused by invalid symlinks extracted by a
-           Cygwin/MSYS2 tar. *)
+      else
         remove_file_t ~with_log:false file
     ) files;
   Unix.rmdir dir
 
 let remove_dir dir =
   log "rmdir %s" dir;
   if Sys.file_exists dir then begin
-    if Sys.is_directory dir then
+    if is_reg_dir dir then
       remove_dir_t dir
     else
       remove_file_t ~with_log:true dir

tests/reftests/dune.inc

@@ -1763,6 +1763,27 @@
    %{targets}
    (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:switch-list.test} %{read-lines:testing-env}))))
 
+(rule
+ (alias reftest-switch-remove)
+ (enabled_if (and  (or (<> %{env:TESTALL=1} 0) (= %{env:TESTN0REP0=0} 1))))
+ (action
+  (diff switch-remove.test switch-remove.out)))
+
+(alias
+ (name reftest)
+ (enabled_if (and  (or (<> %{env:TESTALL=1} 0) (= %{env:TESTN0REP0=0} 1))))
+ (deps (alias reftest-switch-remove)))
+
+(rule
+ (targets switch-remove.out)
+ (deps root-N0REP0)
+ (enabled_if (and  (or (<> %{env:TESTALL=1} 0) (= %{env:TESTN0REP0=0} 1))))
+ (package opam)
+ (action
+  (with-stdout-to
+   %{targets}
+   (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:switch-remove.test} %{read-lines:testing-env}))))
+
 (rule
  (alias reftest-switch-set)
  (enabled_if (and  (or (<> %{env:TESTALL=1} 0) (= %{env:TESTN0REP0=0} 1))))

tests/reftests/switch-remove.test

@@ -0,0 +1,7 @@
+N0REP0
+### :: Remove linked local switch
+### opam switch create foo --empty
+### opam switch link foo
+Directory ${BASEDIR} set to use switch foo.
+Just remove ${BASEDIR}/_opam to unlink.
+### opam switch remove .