Merge pull request #5832 from dra27/bubbling-cygwin

Resolve the Cygwin bin directory above Windows system32 directory

.gitattributes

@@ -12,9 +12,6 @@ changelog_checker text eol=lf
 *.cmd text eol=crlf
 shell/autogen text eol=lf
 
-# For diffing simplicity, the patch re-write test uses LF endings on Windows
-tests/patcher-test.reference text eol=lf
-
 # Don't normalise patch files
 *.patch -text
 

master_changes.md

@@ -78,6 +78,8 @@ users)
   * Fix incorrect deduplication of environment variables on update. Effect was that FOO += "" would occlude the value of FOO in the environment [#5837 @dra27]
   * Fix regression from #5356 on the detection of out-of-date environment variables. As part of a refactoring, a filter predicate got inverted [#5837 @dra27]
   * Unixify Windows paths in init shells scripts (sh, bash, zsh, fish & tsh) [#5797 @rjbou]
+  * `OpamProcess.cygwin_create_process_env` no longer adjusts PATH [#5832 @dra27]
+  * Internal Cygwin installation's bin directory is placed as far down PATH as is necessary not to shadow `bash`, `tar`, `sort` or `git` [#5832 @dra27]
 
 ## Opamfile
   * Hijack the `%{?val_if_true:val_if_false}%` syntax to support extending the variables of packages with + in their name [#5840 @kit-ty-kate]
@@ -187,6 +189,7 @@ users)
   * `OpamSysInteract.Cygwin.check_install`: look for `cygcheck.exe` in `usr/bin` also as MSYS2 doesn't have "bin" [#5843 @rjbou]
   * `OpamGlobalState.load_config`: load MSYS2 Cygwin binary path too at config file loading [#5843 @rjbou]
   * `OpamEnv`: add `sys_ocaml_eval_variables` value, moved `OpamInitDefaults` as it is also needed in `OpamFormatUpgrade` too [#5829 @rjbou @kit-ty-kate]
+  * `OpamEnv` supports an internal `Cygwin` environment operation which pushes the given directory as far down the list as can be done without shadowing. This mechanism replaces the opposite which was done in OpamProcess [#5832 @dra27]
 
 ## opam-solver
 
@@ -195,6 +198,7 @@ users)
   * `OpamTypesBase`: add `filter_ident_of_string_interp` that is used for parsing variables in string interpolation like `filter_ident_of_string` but permits the parsing of '%{?pkg+:var:}%' syntax [#5840 @rjbou]
   * `OpamTypesBase.filter_ident_of_string_interp`: add `accept` optional argument to be able to raise an error when several pluses are in the package name without using the new syntax, like `%{pkg+++:var}%`
   * `OpamFilter`: add `extract_variables_from_string` to retrieve string of variables, and exposes it [#5840 @rjbou]
+  * `OpamTypes.env_update` now has an additional type parameter indicating whether the update is internal or writeable [#5832 @dra27]
 
 ## opam-core
   * `OpamStd.Sys`: add `is_cygwin_variant_cygcheck` that returns true if in path `cygcheck` is from a Cygwin or MSYS2 installation [#5843 @rjbou]

src/client/opamAction.ml

@@ -520,7 +520,6 @@ let prepare_package_source st nv dir =
       prepare_package_build (OpamPackageVar.resolve ~opam st) opam nv dir
 
 let compilation_env t opam =
-  let open OpamParserTypes in
   let build_env =
     List.map
       (fun env ->
@@ -532,7 +531,7 @@ let compilation_env t opam =
     match OpamSysInteract.Cygwin.cygbin_opt t.switch_global.config with
     | Some cygbin ->
       let cygbin = OpamFilename.Dir.to_string cygbin in
-      [ OpamTypesBase.env_update_resolved "PATH" EqPlus cygbin
+      [ OpamTypesBase.env_update_resolved "PATH" Cygwin cygbin
           ~comment:"Cygwin path"
       ] @ (match OpamCoreConfig.(!r.git_location) with
           | None -> []

src/client/opamConfigCommand.ml

@@ -205,6 +205,17 @@ let print_eval_env ~csh ~sexp ~fish ~pwsh ~cmd env =
   else
     print_env never_with_cr env
 
+let check_writeable l =
+  let map_writeable ({OpamTypes.envu_op; _} as update) =
+    match envu_op with
+    | Eq | PlusEq | EqPlus | ColonEq | EqColon | EqPlusEq as envu_op ->
+      {update with envu_op}
+    | Cygwin ->
+      OpamSystem.internal_error
+        "Attempt to write an internal environment change"
+  in
+  List.map map_writeable l
+
 let regenerate_env ~set_opamroot ~set_opamswitch ~force_path
     gt switch env_file =
   OpamSwitchState.with_ `Lock_none ~switch gt @@ fun st ->
@@ -214,7 +225,7 @@ let regenerate_env ~set_opamroot ~set_opamswitch ~force_path
   if not (OpamCoreConfig.(!r.safe_mode)) then
     (let _, st =
        OpamSwitchState.with_write_lock st @@ fun st ->
-       (OpamFile.Environment.write env_file upd), st
+       (OpamFile.Environment.write env_file (check_writeable upd)), st
      in OpamSwitchState.drop st);
   upd
 
@@ -227,7 +238,7 @@ let load_and_verify_env ~set_opamroot ~set_opamswitch ~force_path
   let environment_opam_switch_prefix =
     OpamStd.List.find_map_opt (function
         | OpamTypes.{ envu_var = "OPAM_SWITCH_PREFIX";
-                      envu_op = OpamParserTypes.Eq;
+                      envu_op = Eq;
                       envu_value; _} ->
           Some envu_value
         | _ -> None)
@@ -249,6 +260,7 @@ let load_and_verify_env ~set_opamroot ~set_opamswitch ~force_path
    [OpamEnv.hash_env_updates updates] and [n] should initially be [0]. If for
    whatever reason the file cannot be created, returns [None]. *)
 let  write_last_env_file gt switch updates =
+  let updates = check_writeable updates in
   let temp_dir = OpamPath.Switch.last_env gt.root switch in
   let hash = OpamEnv.hash_env_updates updates in
   let rec aux  n =
@@ -683,7 +695,7 @@ let switch_allowed_fields, switch_allowed_sections =
                                 envu_value = value'; envu_comment = _;
                                 envu_rewrite = _ } ->
                            String.equal var var'
-                           && (op : OpamParserTypes.env_update_op) = op'
+                           && (op : [> euok_writeable ] env_update_op_kind) = op'
                            && String.equal value value')
                          nc.env) c.env
                  in

src/core/opamProcess.ml

@@ -179,29 +179,6 @@ let cygwin_create_process_env prog args env fd1 fd2 fd3 =
           None
         end else
           Some (key ^ "=" ^ String.concat " " settings)
-    | "path" ->
-        let path_dirs = OpamStd.Sys.split_path_variable value in
-        let winsys = Filename.concat (OpamStd.Sys.system ()) "." |> String.lowercase_ascii in
-        let rec f prefix suffix = function
-        | dir::dirs ->
-            let contains_cygpath = Sys.file_exists (Filename.concat dir "cygpath.exe") in
-            if suffix = [] then
-              if String.lowercase_ascii (Filename.concat dir ".") = winsys then
-                f prefix [dir] dirs
-              else
-                if contains_cygpath then
-                  path_dirs
-                else
-                  f (dir::prefix) [] dirs
-            else
-              if contains_cygpath then begin
-                log ~level:2 "Moving %s to after %s in PATH" dir (List.hd prefix);
-                List.rev_append prefix (dir::(List.rev_append suffix dirs))
-              end else
-                f prefix (dir::suffix) dirs
-        | [] -> []
-        in
-        Some (key ^ "=" ^ String.concat ";" (f [] [] path_dirs))
     | _ ->
         Some item in
   let env = OpamStd.List.filter_map f env in

src/format/opamFile.ml

@@ -542,16 +542,20 @@ module Pinned_legacy = struct
     end)
 end
 
+external open_env_updates:
+  ('a, euok_writeable) env_update list
+    -> ('a, [> euok_writeable]) env_update list = "%identity"
+(* cf. tests/lib/typeGymnastics.ml *)
 
 (** Cached environment updates (<switch>/.opam-switch/environment
     <switch>/.opam-switch/last-env/env-* last env files) *)
 
-module Environment = LineFile(struct
+module Environment = struct include LineFile(struct
 
     let internal = "environment"
     let atomic = true
 
-    type t = spf_resolved env_update list
+    type t = (spf_resolved, euok_writeable) env_update list
 
     let empty = []
 
@@ -582,7 +586,7 @@ module Environment = LineFile(struct
               | _ -> Pp.unexpected ()),
            (fun sep -> String.make 1 (char_of_separator sep)))
       in
-      let env : (string, env_update_op_kind) Pp.t =
+      let env : (string, OpamParserTypes.FullPos.env_update_op_kind) Pp.t =
         (Pp.of_pair "env_update_op"
            (OpamLexer.FullPos.env_update_op, OpamPrinter.env_update_op_kind))
       in
@@ -657,7 +661,7 @@ module Environment = LineFile(struct
         (Pp.pp
            (fun ~pos:_ (envu_var, (envu_op, (envu_value, optional_parts))) ->
               let envu = {
-                envu_var; envu_op; envu_value; envu_comment = None;
+                envu_var; envu_op = op_of_raw envu_op; envu_value; envu_comment = None;
                 envu_rewrite = Some (SPF_Resolved None);
               } in
               match optional_parts with
@@ -701,10 +705,22 @@ module Environment = LineFile(struct
                | Some comment, None ->
                  Some (`comment_norewrite comment)
              in
-             (envu_var, (envu_op, (envu_value, optional_parts)))))
+             (envu_var, (raw_of_op envu_op, (envu_value, optional_parts)))))
 
   end)
 
+  let read file =
+    open_env_updates (read file)
+  let read_opt file =
+    Option.map open_env_updates (read_opt file)
+  let safe_read file =
+    open_env_updates (safe_read file)
+  let read_from_channel ?filename ch =
+    open_env_updates (read_from_channel ?filename ch)
+  let read_from_string ?filename s =
+    open_env_updates (read_from_string ?filename s)
+end
+
 (** (2) Part of the public repository format *)
 
 (** repository index files ("urls.txt"): table
@@ -2014,7 +2030,7 @@ module Switch_configSyntax = struct
     variables: (variable * variable_contents) list;
     opam_root: dirname option;
     wrappers: Wrappers.t;
-    env: spf_resolved env_update list;
+    env: (spf_resolved, euok_writeable) env_update list;
     invariant: OpamFormula.t option;
     depext_bypass: OpamSysPkg.Set.t;
   }
@@ -2032,6 +2048,8 @@ module Switch_configSyntax = struct
     depext_bypass = OpamSysPkg.Set.empty;
   }
 
+  let env t = open_env_updates t.env
+
   (* When adding a field or section, make sure to add it in
      [OpamConfigCommand.switch_allowed_fields] and
      [OpamConfigCommand.switch_allowed_sections] if it is a user modifiable
@@ -2566,7 +2584,7 @@ module OPAMSyntax = struct
     conflict_class : name list;
     available  : filter;
     flags      : package_flag list;
-    env        : spf_unresolved env_update list;
+    env        : (spf_unresolved, euok_writeable) env_update list;
 
     (* Build instructions *)
     build      : command list;
@@ -2577,7 +2595,7 @@ module OPAMSyntax = struct
     (* Auxiliary data affecting the build *)
     substs     : basename list;
     patches    : (basename * filter option) list;
-    build_env  : spf_unresolved env_update list;
+    build_env  : (spf_unresolved, euok_writeable) env_update list;
     features   : (OpamVariable.t * filtered_formula * string) list;
     extra_sources: (basename * URL.t) list;
 
@@ -2733,7 +2751,7 @@ module OPAMSyntax = struct
              envu_comment =
                Some ("Updated by package " ^ OpamPackage.Name.to_string name) }
          | _, b -> b)
-      t.env
+      (open_env_updates t.env)
 
   let build t = t.build
   let run_test t = t.deprecated_build_test @ t.run_test
@@ -2744,7 +2762,7 @@ module OPAMSyntax = struct
 
   let substs t = t.substs
   let patches t = t.patches
-  let build_env t = t.build_env
+  let build_env t = open_env_updates t.build_env
   let features t = t.features
   let extra_sources t = t.extra_sources
 
@@ -4017,7 +4035,7 @@ module CompSyntax = struct
     make         : string list ;
     build        : command list ;
     packages     : formula ;
-    env          : spf_unresolved env_update list;
+    env          : (spf_unresolved, euok_writeable) env_update list;
     tags         : string list;
   }
 

src/format/opamFile.mli

@@ -365,7 +365,7 @@ module OPAM: sig
     conflict_class : name list;
     available  : filter;
     flags      : package_flag list;
-    env        : spf_unresolved env_update list;
+    env        : (spf_unresolved, euok_writeable) env_update list;
 
     (* Build instructions *)
     build      : command list;
@@ -376,7 +376,7 @@ module OPAM: sig
     (* Auxiliary data affecting the build *)
     substs     : basename list;
     patches    : (basename * filter option) list;
-    build_env  : spf_unresolved env_update list;
+    build_env  : (spf_unresolved, euok_writeable) env_update list;
     features   : (OpamVariable.t * filtered_formula * string) list;
     extra_sources: (basename * URL.t) list;
 
@@ -477,7 +477,7 @@ module OPAM: sig
   val substs: t -> basename list
 
   (** List of environment variables to set-up for the build *)
-  val build_env: t -> spf_unresolved env_update list
+  val build_env: t -> (spf_unresolved, [> euok_writeable]) env_update list
 
   (** List of command to run for building the package *)
   val build: t -> command list
@@ -567,7 +567,7 @@ module OPAM: sig
   val has_flag: package_flag -> t -> bool
 
   (** The environment variables that this package exports *)
-  val env: t -> spf_unresolved env_update list
+  val env: t -> (spf_unresolved, [> euok_writeable]) env_update list
 
   val descr: t -> Descr.t option
 
@@ -652,7 +652,7 @@ module OPAM: sig
   (** Construct as [substs] *)
   val with_substs: basename list -> t -> t
 
-  val with_build_env: spf_unresolved env_update list -> t -> t
+  val with_build_env: (spf_unresolved, euok_writeable) env_update list -> t -> t
 
   val with_available: filter -> t -> t
 
@@ -680,7 +680,7 @@ module OPAM: sig
 
   val with_tags: string list -> t -> t
 
-  val with_env: spf_unresolved env_update list -> t -> t
+  val with_env: (spf_unresolved, euok_writeable) env_update list -> t -> t
 
   val with_dev_repo: url -> t -> t
 
@@ -801,7 +801,15 @@ end
 module PkgList: IO_FILE with type t = package_set
 
 (** Cached environment updates (<switch>/environment) *)
-module Environment: IO_FILE with type t = spf_resolved env_update list
+module Environment : sig
+  include IO_FILE with type t = (spf_resolved, euok_writeable) env_update list
+
+  val read: t typed_file -> (spf_resolved, [> euok_writeable ]) env_update list
+  val read_opt: t typed_file -> (spf_resolved, [> euok_writeable ]) env_update list option
+  val safe_read: t typed_file -> (spf_resolved, [> euok_writeable ]) env_update list
+  val read_from_channel: ?filename:t typed_file -> in_channel -> (spf_resolved, [> euok_writeable ]) env_update list
+  val read_from_string: ?filename:t typed_file -> string -> (spf_resolved, [> euok_writeable ]) env_update list
+end
 
 (** Compiler version [$opam/compilers/]. Deprecated, only used to upgrade old
     data *)
@@ -814,7 +822,7 @@ module Comp: sig
 
   (** Create a pre-installed compiler description file *)
   val create_preinstalled:
-    compiler -> compiler_version -> name list -> spf_unresolved env_update list -> t
+    compiler -> compiler_version -> name list -> (spf_unresolved, euok_writeable) env_update list -> t
 
   (** Is it a pre-installed compiler description file *)
   val preinstalled: t -> bool
@@ -849,7 +857,7 @@ module Comp: sig
 
   (** Environment variable to set-up before running commands in the
       subtree *)
-  val env: t -> spf_unresolved env_update list
+  val env: t -> (spf_unresolved, euok_writeable) env_update list
 
   val tags: t -> string list
 
@@ -1034,10 +1042,13 @@ module Switch_config: sig
     variables: (variable * variable_contents) list;
     opam_root: dirname option;
     wrappers: Wrappers.t;
-    env: spf_resolved env_update list;
+    env: (spf_resolved, euok_writeable) env_update list;
     invariant: OpamFormula.t option;
     depext_bypass: OpamSysPkg.Set.t;
   }
+
+  val env: t -> (spf_resolved, [> euok_writeable ]) env_update list
+
   val file_format_version: OpamVersion.t
   val variable: t -> variable -> variable_contents option
   val path: t -> std_path -> string option

src/format/opamFormat.ml

@@ -592,12 +592,12 @@ module V = struct
   let env_binding_t empty =
     let parse ~pos:_ v = match v.pelem with
       | Relop ({ pelem = `Eq;_}, { pelem = Ident i;_}, { pelem = String s;_}) ->
-        { envu_var = i; envu_op = OpamParserTypes.Eq;
+        { envu_var = i; envu_op = Eq;
           envu_value = s; envu_comment = None;
           envu_rewrite = Some empty;
         }
       | Env_binding ({ pelem = Ident i; _}, op, { pelem = String s; _}) ->
-        { envu_var = i; envu_op = op.pelem;
+        { envu_var = i; envu_op = op_of_raw op.pelem;
           envu_value = s; envu_comment = None;
           envu_rewrite = Some empty;
         }
@@ -606,7 +606,7 @@ module V = struct
     let print { envu_var; envu_op; envu_value; envu_comment = _ ;
                 envu_rewrite = _} =
       nullify_pos @@
-      Env_binding (print ident envu_var, nullify_pos envu_op,
+      Env_binding (print ident envu_var, nullify_pos (raw_of_op envu_op),
                    print string envu_value)
     in
     list -| singleton -| pp ~name:"env-binding" parse print