Merge pull request #6910 from NathanReb/remove-chdir

Remove unnecessary use of chdir

Author: kit-ty-kate

master_changes.md

@@ -15,6 +15,7 @@ users)
 
 ## Global CLI
   * Update Kate's email address [#6808 @kit-ty-kate]
+  * Remove unnecessary uses of `chdir` [#6910 @NathanReb]
 
 ## Plugins
 
@@ -247,6 +248,7 @@ users)
   * `OpamPathName` was added [#6917 @rjbou]
   * `OpamRepositoryPathName` was added [#6917 @rjbou]
   * `OpamRepositoryPath` was moved from `opam-repository` [#6917 @rjbou]
+  * `OpamFilter.expand_interpolations_in_file`: changed argument type from `basename` to `filename` [#6910 @NathanReb]
 
 ## opam-core
   * `OpamCmdliner` was added. It is accessible through a new `opam-core.cmdliner` sub-library [#6755 @kit-ty-kate]
@@ -263,3 +265,8 @@ users)
   * `OpamFilename`: add `is_dir_read_only` [#6489 @rjbou]
   * `OpamFilename.might_escape`: ensure / is detected as a file separator when called with `~sep:Unspecified` on Windows [#6897 @kit-ty-kate]
   * `OpamFilename.Unix` was added abstracting over `/` separated paths regardless of the current system [#6914 @rjbou @kit-ty-kate]
+  * `OpamFilename.in_dir`: removed [#6910 @NathanReb]
+  * `OpamSystem.in_tmp_dir`: removed [#6910 @NathanReb]
+  * `OpamSystem.in_dir`: removed [#6910 @NathanReb]
+  * `OpamSystem.chdir`: removed [#6910 @NathanReb]
+  * `OpamSystem.{command,commands,read_command_output}`: add a `?dir: dirname` optional arg to launch the command in a specific directory [#6910 @NathanReb]

src/client/opamAction.ml

@@ -168,7 +168,7 @@ let preprocess_dot_install_t st nv build_dir =
     List.map (fun (src, dst) ->
         let file = file_wo_prefix dst in
         let inst warning =
-          let src_file = OpamFilename.create (OpamFilename.cwd ()) src.c in
+          let src_file = OpamFilename.create build_dir src.c in
           if OpamFilename.exists dst
           && OpamConsole.confirm "Overwriting %s?" (OpamFilename.to_string dst) then
             OpamFilename.install ~warning ~src:src_file ~dst ()
@@ -211,7 +211,6 @@ let preprocess_dot_install st nv build_dir =
         else
           (OpamSystem.default_install_warning, (fun () -> false))
       in
-      OpamFilename.in_dir build_dir @@ fun () ->
       log "Installing %s.\n" (OpamPackage.to_string nv);
       let warnings =
         List.filter_map (fun install -> install warning) installs
@@ -396,26 +395,22 @@ let prepare_package_build env opam nv dir =
      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;
+            let file = OpamFilename.create dir f in
+            OpamFilter.expand_interpolations_in_file env file;
             errs
           with e -> (f, e)::errs)
         [] subst_patches
     in
     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;
+            let file = OpamFilename.create dir f in
+            OpamFilter.expand_interpolations_in_file env file;
             errs
           with e -> (f, e)::errs)
         subst_errs subst_others

src/client/opamAuxCommands.ml

@@ -588,9 +588,8 @@ let check_and_revert_sandboxing root config =
         Array.append [| "OPAM_SWITCH_PREFIX=/dev/null" |] (Unix.environment ())
       in
       try
-        (* Don't assume that we can mount the CWD *)
-        OpamSystem.in_tmp_dir @@ fun () ->
-          OpamSystem.read_command_output ~env ~allow_stdin:false (cmd @ test_cmd)
+        OpamSystem.with_tmp_dir @@ fun dir ->
+          OpamSystem.read_command_output ~env ~dir ~allow_stdin:false (cmd @ test_cmd)
         = ["SUCCESS"]
       with e ->
         (OpamConsole.error "Sandboxing is not working on your platform%s:\n%s"

src/client/opamConfigCommand.ml

@@ -377,8 +377,11 @@ let env gt switch ?(set_opamroot=false) ?(set_opamswitch=false)
 let subst gt fs =
   log "config-substitute";
   OpamSwitchState.with_ `Lock_none gt @@ fun st ->
+  let env = OpamPackageVar.resolve st in
   List.iter
-    (OpamFilter.expand_interpolations_in_file (OpamPackageVar.resolve st))
+    (fun b ->
+       let file = OpamFilename.of_basename b in
+       OpamFilter.expand_interpolations_in_file env file)
     fs
 
 let expand gt str =

src/core/opamFilename.ml

@@ -106,19 +106,16 @@ let dirs d =
 let dir_is_empty d =
   OpamSystem.dir_is_empty (Dir.to_string d)
 
-let in_dir dirname fn = OpamSystem.in_dir dirname fn
-
 let is_dir_read_only dirname =
   OpamSystem.is_dir_read_only (Dir.to_string dirname)
 
 let env_of_list l = Array.of_list (List.rev_map (fun (k,v) -> k^"="^v) l)
 
-let exec dirname ?env ?name ?metadata ?keep_going cmds =
+let exec dir ?env ?name ?metadata ?keep_going cmds =
   let env = match env with
     | None   -> None
     | Some l -> Some (env_of_list l) in
-  in_dir dirname
-    (fun () -> OpamSystem.commands ?env ?name ?metadata ?keep_going cmds)
+  OpamSystem.commands ~dir ?env ?name ?metadata ?keep_going cmds
 
 let move_dir ~src ~dst =
   OpamSystem.mv (Dir.to_string src) (Dir.to_string dst)

src/core/opamFilename.mli

@@ -60,9 +60,6 @@ val is_dir_read_only: Dir.t -> bool
 (** List the sub-directory (do not recurse) *)
 val dirs: Dir.t -> Dir.t list
 
-(** Evaluate a function in a given directory *)
-val in_dir: Dir.t -> (unit -> 'a) -> 'a
-
 (** Turns an assoc list into an array suitable to be provided as environment *)
 val env_of_list: (string * string) list -> string array
 

src/core/opamSystem.ml

@@ -337,36 +337,17 @@ let copy_file_aux ?chmod ~src ~dst () =
     (try Unix.unlink dst with Unix.Unix_error _ -> ());
     internal_error "Cannot copy %s to %s (%s)." src dst (Printexc.to_string e)
 
-let chdir dir =
-  try Unix.chdir dir
-  with Unix.Unix_error _ -> raise (File_not_found dir)
-
-let in_dir dir fn =
-  let reset_cwd =
-    let cwd =
-      try Some (Sys.getcwd ())
-      with Sys_error _ -> None in
-    fun () ->
-      match cwd with
-      | None     -> ()
-      | Some cwd -> try chdir cwd with File_not_found _ -> () in
-  chdir dir;
-  try
-    let r = fn () in
-    reset_cwd ();
-    r
-  with e ->
-    OpamStd.Exn.finalise e reset_cwd
-
 let list kind dir =
-  try
-    in_dir dir (fun () ->
-      let d = Sys.readdir (Sys.getcwd ()) in
-      let d = Array.to_list d in
-      let l = List.filter kind d in
-      List.map (Filename.concat dir) (List.sort compare l)
-    )
-  with File_not_found _ -> []
+  let d = try Sys.readdir dir with Sys_error _ -> [||] in
+  let filtered =
+    Array.fold_left
+      (fun acc base ->
+         let path = Filename.concat dir base in
+         if kind path then path::acc else acc)
+      []
+      d
+  in
+  List.sort String.compare filtered
 
 let ls dir = list (fun _ -> true) dir
 
@@ -433,10 +414,6 @@ let with_tmp_dir fn =
     OpamStd.Exn.finalise e @@ fun () ->
     remove_dir dir
 
-let in_tmp_dir fn =
-  with_tmp_dir @@ fun dir ->
-    in_dir dir fn
-
 let with_tmp_dir_job fjob =
   let dir = mk_temp_dir () in
   mkdir dir;
@@ -573,7 +550,7 @@ let make_command
   | `Denied -> permission_denied cmd
 
 let run_process
-    ?verbose ?env ~name ?metadata ?stdout ?allow_stdin command =
+    ?verbose ?env ~name ?metadata ?dir ?stdout ?allow_stdin command =
   let env = match env with None -> OpamProcess.default_env () | Some e -> e in
   let chrono = OpamConsole.timer () in
   match command with
@@ -588,7 +565,7 @@ let run_process
       let r =
         OpamProcess.run
           (OpamProcess.command
-             ~env ~name ~verbose ?metadata ?allow_stdin ?stdout
+             ~env ~name ~verbose ?metadata ?dir ?allow_stdin ?stdout
              full_cmd args)
       in
       let str = String.concat " " (cmd :: args) in
@@ -599,15 +576,15 @@ let run_process
     | `Not_found -> command_not_found cmd
     | `Denied -> permission_denied cmd
 
-let command ?verbose ?env ?name ?metadata ?allow_stdin cmd =
+let command ?verbose ?env ?name ?metadata ?dir ?allow_stdin cmd =
   let name = log_file name in
-  let r = run_process ?verbose ?env ~name ?metadata ?allow_stdin cmd in
+  let r = run_process ?verbose ?env ~name ?metadata ?dir ?allow_stdin cmd in
   OpamProcess.cleanup r;
   raise_on_process_error r
 
-let commands ?verbose ?env ?name ?metadata ?(keep_going=false) commands =
+let commands ?verbose ?env ?name ?metadata ?dir ?(keep_going=false) commands =
   let name = log_file name in
-  let run = run_process ?verbose ?env ~name ?metadata in
+  let run = run_process ?verbose ?env ~name ?metadata ?dir in
   let command r0 c =
     match r0, keep_going with
     | (`Error _ | `Exception _), false -> r0
@@ -625,12 +602,12 @@ let commands ?verbose ?env ?name ?metadata ?(keep_going=false) commands =
   | `Error e -> process_error e
   | `Exception e -> raise e
 
-let read_command_output ?verbose ?env ?metadata ?allow_stdin
+let read_command_output ?verbose ?env ?metadata ?dir ?allow_stdin
     ?(ignore_stderr=false) cmd =
   let name = log_file None in
   let stdout = name ^ (if ignore_stderr then ".stdout" else ".out") in
   let r =
-    run_process ?verbose ?env ~name ?metadata ?allow_stdin
+    run_process ?verbose ?env ~name ?metadata ?dir ?allow_stdin
       ~stdout
       cmd
   in

src/core/opamSystem.mli

@@ -36,9 +36,6 @@ val internal_error: ('a, unit, string, 'b) format4 -> 'a
     passes its name to [fn]. The directory is always removed on completion. *)
 val with_tmp_dir: (string -> 'a) -> 'a
 
-(** [in_tmp_dir fn] executes [fn] in a temporary directory. *)
-val in_tmp_dir: (unit -> 'a) -> 'a
-
 (** Runs a job with a temp dir that is cleaned up afterwards *)
 val with_tmp_dir_job: (string -> 'a OpamProcess.job) -> 'a OpamProcess.job
 
@@ -164,12 +161,6 @@ val rmdir_cleanup : string -> unit
     exists and is not a symlink. Returns [false] otherwise. *)
 val is_reg_dir: string -> bool
 
-(** Change the current working directory *)
-val chdir: string -> unit
-
-(** [in_dir dir fn] evaluates [fn] in the directory [dir] *)
-val in_dir: string -> (unit -> 'a) -> 'a
-
 (** Returns the list of files and directories in the given directory (full
     names) *)
 val ls: string -> string list
@@ -251,14 +242,15 @@ val apply_cygpath: string -> string
 (** [command cmd] executes the command [cmd] in the correct OPAM
     environment. *)
 val command: ?verbose:bool -> ?env:string array -> ?name:string ->
-  ?metadata:(string * string) list -> ?allow_stdin:bool ->
+  ?metadata:(string * string) list -> ?dir: string -> ?allow_stdin:bool ->
   command -> unit
 
 (** [commands cmds] executes the commands [cmds] in the correct OPAM
     environment. It stops whenever one command fails unless [keep_going] is set
     to [true]. In this case, the first error is re-raised at the end. *)
 val commands: ?verbose:bool -> ?env:string array -> ?name:string ->
-  ?metadata:(string * string) list -> ?keep_going:bool -> command list -> unit
+  ?metadata:(string * string) list -> ?dir: string -> ?keep_going:bool ->
+  command list -> unit
 
 (** [read_command_output cmd] executes the command [cmd] in the
     correct OPAM environment and return the lines from output if the command
@@ -267,7 +259,7 @@ val commands: ?verbose:bool -> ?env:string array -> ?name:string ->
     It returns stdout and stder combiend, unless [ignore_stderr] is st to true.
     *)
 val read_command_output: ?verbose:bool -> ?env:string array ->
-  ?metadata:(string * string) list ->  ?allow_stdin:bool ->
+  ?metadata:(string * string) list -> ?dir: string -> ?allow_stdin:bool ->
   ?ignore_stderr:bool -> command -> string list
 
 (** END *)

src/format/opamFilter.ml

@@ -461,10 +461,9 @@ let expand_interpolations_in_file_full env ~src ~dst =
   close_out oc
 
 (* Substitute the file contents *)
-let expand_interpolations_in_file env file =
-  let file = OpamFilename.of_basename file in
-  let src = OpamFilename.add_extension file OpamPathName.subst_ext in
-  expand_interpolations_in_file_full env ~src ~dst:file
+let expand_interpolations_in_file env dst =
+  let src = OpamFilename.add_extension dst OpamPathName.subst_ext in
+  expand_interpolations_in_file_full env ~src ~dst
 
 (* Apply filters and interpolations to package commands *)
 

src/format/opamFilter.mli

@@ -143,12 +143,12 @@ val expand_interpolations_in_file_full: env -> src:filename -> dst:filename -> u
 (** Same as {!expand_interpolations_in_file} but allows to set the source [src] and
     destination [dst] files independently instead of implying [src] = [dst].in *)
 
-(** Rewrites [basename.in] to [basename], expanding interpolations.
+(** Rewrites [filename.in] to [filename], expanding interpolations.
     If the first line begins ["opam-version:"], assumes that expansion of
     variables within strings should be properly escaped. In particular, this
     means that Windows paths should expand correctly when generating .config
     files. *)
-val expand_interpolations_in_file: env -> basename -> unit
+val expand_interpolations_in_file: env -> filename -> unit
 
 
 (** Processes filters evaluation in a command list: parameter expansion and