Merge pull request #5636 from rjbou/setenv-slash

Path rewriting for `setenv:` and `build-env:`

doc/pages/Manual.md

@@ -639,8 +639,7 @@ Some fields define updates to environment variables in the form:
 
 The allowed update operators `update-op` denote how the string is applied to the
 environment variable. Prepend and append operators assume list of elements
-separated by an OS-specific character (`;` on Windows, `:` on Cygwin or any
-other system).
+separated by an OS-specific character.
 - `=` override (or set if undefined)
 - `+=` or `:=` prepend. They differ when the variable is unset of empty, where `:=` adds a trailing separator.
 - `=+` or `=:` append. They differ when the variable is unset of empty, where `=:` adds a leading separator.
@@ -652,6 +651,59 @@ other system).
 
 `FOO += ""`, `FOO := ""`, etc. are all ignored - i.e. opam never adds empty segments to an existing variable.
 
+#### Environment update portability
+<a id="env-update-rewrite"></a>
+
+Some fields define an environment update portability specification. In the opam
+file it is the [`x-env-path-rewrite:`](#opamfield-x-env-path-rewrite) field, of the
+form:
+
+```BNF
+<environment-rewrites> ::= { <environment-rewrite> }*
+<environment-rewrite>  ::= <ident> <bool>
+                         | <ident> <separator-formula> <path-format-formula>
+
+<separator-formula>    ::= <separator-formula> "|" <separator-formula>
+                         | ( <separator-formula> )
+                         | <separator> { { <distrib-formula> }* }
+<path-format-formula>  ::= <path-format-formula> "|" <path-format-formula>
+                         | ( <path-format-formula> )
+                         | <path-format> { { <distrib-formula> }* }
+
+<separator>            ::= (") ":" (") | (") ";" (")
+<path-format>          ::= (") "host" (") | (") "host-quoted" (") | (") "target" (") | (") "target-quoted" (")
+
+<distrib-formula>      ::= <distrib-formula> <logop> <distrib-formula>
+                         | "!" <distrib-formula>
+                         | "(" <distrib-formula> ")"
+<logop>                ::= "&" | "|"
+```
+
+The `<separator>` defines the path separator to use for the variable.
+The `<path-format>` defines the way to handle variables path formatting:
+- host: use the *host* interpretation of PATHs (i.e. convert via `cygpath` on
+  Windows).
+- host-quoted: use the *host* interpretation of entries and double-quote any
+  entries which include the separator character.
+- target: use the *target* interpretation of entries (i.e. rewrite slashes to
+  backslashes on Windows).
+- target-quoted: use the *target* interpretation of entries and double-quote
+  any entries which include the separator character.
+
+If a variable is not mentioned in `x-env-path-rewrite`, the separator is assumed to be `;` on Windows and `:` on all other systems; no slash or quoting transformations are performed. There are two special default cases:
+* `PKG_CONFIG_PATH` uses `:` separator and is `target-quoted`
+* `PATH` on Windows uses `;` separator and is `target-quoted`
+
+For example, on Windows:
+- `[FOO false]`: `FOO` won't be translated nor rewritten, and default separator is used if needed
+- `[FOO true]`: `FOO` is rewritten using defaults `;` and `target` with slash rewriting
+  - `FOO = "a:/path/to"` -> FOO=a:\path\to
+- `[FOO ":" "target-quoted"]: `FOO` will be appended using `:` separator, if the added path contains '/' they are transformed into `\`, and if the added path contains a `:`, the added path will be quoted
+  - `FOO += "a/path/to"` -> FOO=a\path\to:R:\previous\path
+  - `FOO += "a:path/to"` -> FOO="a:path\to":R:\previous\path
+- `[FOO ":" "host"]: `FOO` will be appended using `:`, and its path will be translated according to the host translator, i.e. `cygpath <path>`:
+  - `FOO += "A:\path\to"` -> FOO=/cygdrive/a/path/to:/previous/path
+
 ### URLs
 
 URLs are provided as strings. They can refer to:
@@ -1124,6 +1176,9 @@ files.
     - `OPAMCLI=2.0` (since opam 2.1)
     - `TMP` and `TMPDIR` are set by the sandbox script (bubblewrap), but should not be relied on since the sandbox is not used on all platforms and can be disabled by the user.
 
+  See [`x-env-path-rewrite:`](#opamfield-x-env-path-rewrite)
+  for path portability of environment variables on Windows.
+
 - <a id="opamsection-extra-sources">`extra-source <string> "{" <url-file> "}"`</a>:
   allows the definition of extra files that need downloading into the source
   tree before the package can be patched (if necessary) and built. The format is
@@ -1132,6 +1187,9 @@ files.
   `<string>` indicates the name the file should be saved to, and is relative to
   the root of the source tree.
 
+  See [`x-env-path-rewrite:`](#opamfield-x-env-path-rewrite)
+  for path portability of environment variables on Windows.
+
 - <a id="opamfield-extra-files">`extra-files: [ [ <string> <checksum> ] ... ]`</a>:
   optionally lists the files below `files/` with their checksums. Used
   internally for integrity verifications.
@@ -1152,6 +1210,11 @@ files.
        has changed. Even then, this won't unpin any packages that would have
        been removed from `pin-depends:`.
 
+- <a id="opamfield-x-env-path-rewrite">`x-env-path-rewrite: [ <environment-update-rewrite> ... ]`</a>:
+  a specific extra field, used to specify rewrite rules for environment variables
+  defined in [`setenv:`](#opamfield-setenv) and [`build-env`](#opamfield-build-env).
+  See [`environment update portability`](#env-update-rewrite) for syntax.
+
 - <a id="opamfield-extra-fields">`x-*: <value>`</a>:
   extra fields prefixed with `x-` can be defined for use by external tools. opam
   will ignore them except for some search operations.

master_changes.md

@@ -28,6 +28,7 @@ users)
   * [BUG] On install driven by `.install` file, track intermediate directories too, in order to have them suppressed at package removal [#5691 @rjbou - fix #5688]
   * [BUG] With `--assume-built`, resolve variables in depends filter according switch & global environment, not only depends predefined variables [#570 @rjbou - fix #5698]
   * [BUG] Handle undefined variables defaults to false in dependencies formula resolution for assume built [#5701 rjbou]
+  * Reinstall if `x-env-path-rewrite` is updated [#5636 @rjbou]
 
 ## Remove
 
@@ -64,8 +65,13 @@ users)
 
 ## Clean
 
+## Env
+  * When computing environment variables updates, use rewriting rules (defined in opam file, or default per variable) to split, join, quote, etc. [#5636 @rjbou - fix #5602 #4690 #2927]
+
 ## Opamfile
   * Update populating extra-files fields log [#5640 @rjbou]
+  * Fix `x-locked` type error message [#5636 @rjbou]
+  * Add `x-env-path-rewrite` extensions field to permit specification of rewriting rules for variables defined in `setenv` and `build-env`: no rewrite; separator and path format formulae [#5636 @rjbou - fix #5602 #4690 #2927]
 
 ## External dependencies
 
@@ -104,6 +110,7 @@ users)
 ## Shell
 
 ## Internal
+  * `environment` file now stores environmnet variable rewriting rules [#5636 @rjbou]
 
 ## Internal: Windows
   * Fix sporadic crash and segfault in shell detection (seen in native containers) [#5714 @dra27]
@@ -123,6 +130,7 @@ users)
   * dot-install: add a test for removal of non specified in .install empty directories [#5701 @rjbou]
   * Add test in assume-built for depends with switch variable filters [#5700 @rjbou]
   * Add undefined variable handling in assume built test [#5701 @rjbou]
+  * Add `env.unix` & `env.win32` to test environment variables rewriting rules [#5636 @rjbou]
 
 ### Engine
   * With real path resolved for all opam temp dir, remove `/private` from mac temp dir regexp [#5654 @rjbou]
@@ -138,6 +146,7 @@ users)
 ## Doc
   * Fix typos in readme [#5706 @MisterDA]
   * Fix formatting in the Manual [#5708 @kit-ty-kate]
+  * Add `x-env-path-rewriting` documentation [#5636 @rjbou]
 
 ## Security fixes
 
@@ -151,13 +160,31 @@ users)
 
 ## opam-solver
 
-## opam-format
 ## opam-format
   * `OpamFilter`: add `expand_interpolations_in_file_full` which allows setting the output file along with the input file [#5629 @rgrinberg]
   * `OpamFilter`: expose `string_interp_regex` which allows clients to identify variable interpolations in strings [#5633 @gridbugs]
+  * `OpamTypes.env_update`: change from tuple to a record [#5636 @rjbou]
+  * `OpamTypesBase`: add `env_update`, `env_update_resolved`, and `env_update_unresolved` builders [#5636 @rjbou]
+  * `OpamTypes.env_update`: add a `rewrite` field, that contains environment variable rewriting rules (formula to resolved, or already resolved, or no rewriting) [#5636 @rjbou]
+  * `OpamPp.fallback`: add name concatenation and printing fallback too [#5636 @rjbou]
+  * `OpamFormat`: add `formula_items` to permit definition of formulae pp not only of the type `package-formula` [#5636 @rjbou]
+  * `OpamTypesBase`: add to_string function for `path_format` & `separator` [#5636 @rjbou]
+  * `OpamFormat.V`: add `path_format` & `separator` value parser printer [#5636 @rjbou]
+  * `OpamFile.OPAM`: add handling of `x-env-path-rewrite` extensions field, that specifies rewrite rules [#5636 @rjbou]
+  * `OpamFile.Environment`: add parsing-printing of rewriting rules, keeping backward compatibility [#5636 @rjbou]
+  * `OpamFile.OPAM`: `effective_part` keeps `x-env-path-rewrite`, affects also `effectively_equal` [#5636 @rjbou]
+  * `OpamTypesBase`: add `env_update_resolved` and `env_update_unresolved` builders [#5636 @rjbou]
+  * `OpamPp.fallback`: add name concatenation and printing fallback too [#5636 @rjbou]
+  * `OpamFormat`: add `formula_items` to permit definition of formulae pp not only of the type `package-formula` [#5636 @rjbou]
+  * `OpamTypesBase`: add to_string function for `path_format` & `separator` [#5636 @rjbou]
+  * `OpamFormat.V`: add `path_format` & `separator` value parser printer [#5636 @rjbou]
+  * `OpamFile.OPAM`: add handling of `x-env-path-rewrite` extensions field, that specifies rewrite rules [#5636 @rjbou]
+  * `OpamFile.Environment`: add parsing-printing of rewriting rules, keeping backward compatibility [#5636 @rjbou]
+  * `OpamFile.OPAM`: `effective_part` keeps `x-env-path-rewrite`, affects also `effectively_equal` [#5636 @rjbou]
 
 ## opam-core
   * `OpamSystem.mk_temp_dir`: resolve real path with `OpamSystem.real_path` before returning it [#5654 @rjbou]
   * `OpamSystem.resolve_command`: in command resolution path, check that the file is not a directory and that it is a regular file [#5606 @rjbou - fix #5585 #5597 #5650 #5626]
   * `OpamStd.Config.env_level`: fix level parsing, it was inverted (eg, "no" gives level 1, and "yes" level 0) [#5686 @smorimoto]
   * `OpamStd.Sys.chop_exe_suffix`: removes `.exe` from the end of a path, if present
+  * `OpamSystem.get_cygpath_path_transform`: add labeled argument to specify if path is a pathlist [#5636 @rjbou]

src/client/opamAction.ml

@@ -522,30 +522,61 @@ let prepare_package_source st nv dir =
 let compilation_env t opam =
   let open OpamParserTypes in
   let build_env =
-    List.map (OpamEnv.env_expansion ~opam t) (OpamFile.OPAM.build_env opam)
+    List.map
+      (fun env ->
+         OpamEnv.resolve_separator_and_format
+           (OpamEnv.env_expansion ~opam t env))
+      (OpamFile.OPAM.build_env opam)
   in
   let cygwin_env =
     match OpamSysInteract.Cygwin.cygbin_opt t.switch_global.config with
     | Some cygbin ->
-      [ "PATH", EqPlus, OpamFilename.Dir.to_string cygbin, Some "Cygwin path" ]
+      [ OpamTypesBase.env_update_resolved "PATH" EqPlus
+          (OpamFilename.Dir.to_string cygbin)
+          ~comment:"Cygwin path"
+      ]
     | None -> []
   in
+  let shell_sanitization = "shell env sanitization" in
+  let build_env_def = "build environment definition" in
+  let cdpath =
+    OpamTypesBase.env_update_resolved "CDPATH" Eq ""
+      ~comment:shell_sanitization
+  in
+  let makeflags =
+    OpamTypesBase.env_update_resolved "MAKEFLAGS" Eq ""
+      ~comment:shell_sanitization
+  in
+  let makelevel =
+    OpamTypesBase.env_update_resolved "MAKELEVEL" Eq ""
+      ~comment:"make env sanitization"
+  in
+  let pkg_name =
+    OpamTypesBase.env_update_resolved "OPAM_PACKAGE_NAME" Eq
+      (OpamPackage.Name.to_string (OpamFile.OPAM.name opam))
+      ~comment:build_env_def
+  in
+  let pkg_version =
+    OpamTypesBase.env_update_resolved "OPAM_PACKAGE_VERSION" Eq
+      (OpamPackage.Version.to_string (OpamFile.OPAM.version opam))
+      ~comment:build_env_def
+  in
+  let cli =
+    OpamTypesBase.env_update_resolved "OPAMCLI" Eq "2.0"
+      ~comment:"opam CLI version"
+  in
   let scrub = OpamClientConfig.(!r.scrubbed_environment_variables) in
   OpamEnv.get_full ~scrub ~set_opamroot:true ~set_opamswitch:true
     ~force_path:true t ~updates:([
-      "CDPATH", Eq, "", Some "shell env sanitization";
-      "MAKEFLAGS", Eq, "", Some "make env sanitization";
-      "MAKELEVEL", Eq, "", Some "make env sanitization";
-      "OPAM_PACKAGE_NAME", Eq,
-      OpamPackage.Name.to_string (OpamFile.OPAM.name opam),
-      Some "build environment definition";
-      "OPAM_PACKAGE_VERSION", Eq,
-      OpamPackage.Version.to_string (OpamFile.OPAM.version opam),
-      Some "build environment definition";
-      "OPAMCLI", Eq, "2.0", Some "opam CLI version";
-    ] @
-      build_env
-      @ cygwin_env)
+        cdpath;
+        makeflags;
+        makelevel;
+        pkg_name;
+        pkg_version;
+        cli
+      ] @
+        build_env
+        @ cygwin_env)
 
 let installed_opam_opt st nv =
   OpamStd.Option.Op.(

src/client/opamAdminRepoUpgrade.ml

@@ -366,12 +366,16 @@ let do_upgrade repo_root =
         None
       ] |>
       O.with_maintainer [ "[email protected]" ] |>
-      O.with_build_env ["CAML_LD_LIBRARY_PATH", Eq, "", None] |>
-      O.with_env [
-        "CAML_LD_LIBRARY_PATH", Eq, "%{_:stubsdir}%", None;
-        "CAML_LD_LIBRARY_PATH", PlusEq, "%{lib}%/stublibs", None;
-        "OCAML_TOPLEVEL_PATH", Eq, "%{toplevel}%", None;
+      O.with_build_env [
+        OpamTypesBase.env_update_unresolved "CAML_LD_LIBRARY_PATH" Eq ""
       ] |>
+      O.with_env [
+        OpamTypesBase.env_update_unresolved "CAML_LD_LIBRARY_PATH" Eq
+          "%{_:stubsdir}%";
+        OpamTypesBase.env_update_unresolved "CAML_LD_LIBRARY_PATH" PlusEq
+          "%{lib}%/stublibs";
+        OpamTypesBase.env_update_unresolved "OCAML_TOPLEVEL_PATH" Eq
+          "%{toplevel}%" ] |>
       (* leave the Compiler flag to the implementations (since the user
          needs to select one)
          O.with_flags [Pkgflag_Compiler] |> *)

src/client/opamCliMain.ml

@@ -206,10 +206,10 @@ let check_and_run_external_commands () =
     in
     let env =
       if has_init then
-        let updates =
-          ["PATH", OpamParserTypes.PlusEq,
-           OpamFilename.Dir.to_string plugins_bin, None]
-        in
+        let updates = [
+          env_update_resolved "PATH" PlusEq
+            (OpamFilename.Dir.to_string plugins_bin)
+        ] in
         OpamStateConfig.init ~root_dir ();
         match OpamStateConfig.get_switch_opt () with
         | None -> env_array (OpamEnv.get_pure ~updates ())

src/client/opamConfigCommand.ml

@@ -63,7 +63,8 @@ let list t ns =
   OpamConsole.print_table stdout ~sep:" "
 
 let possibly_unix_path_env_value k v =
-  if k = "PATH" then (Lazy.force OpamSystem.get_cygpath_path_transform) v
+  if k = "PATH" then
+    (Lazy.force OpamSystem.get_cygpath_path_transform) ~pathlist:true v
   else v
 
 let rec print_env = function
@@ -209,11 +210,14 @@ let load_and_verify_env ~set_opamroot ~set_opamswitch ~force_path
       gt.root switch
   in
   let environment_opam_switch_prefix =
-    List.find_opt (function
-        | "OPAM_SWITCH_PREFIX", OpamParserTypes.Eq, _, _ -> true
-        | _ -> false)
+    OpamStd.List.find_map_opt (function
+        | OpamTypes.{ envu_var = "OPAM_SWITCH_PREFIX";
+                      envu_op = OpamParserTypes.Eq;
+                      envu_value; _} ->
+          Some envu_value
+        | _ -> None)
       upd
-    |> OpamStd.Option.map_default (fun (_, _, v, _) -> v) ""
+    |> OpamStd.Option.default ""
   in
   let actual_opam_switch_prefix =
     OpamFilename.Dir.to_string (OpamPath.Switch.root gt.root switch)
@@ -272,14 +276,17 @@ let ensure_env_aux ?(base=[]) ?(set_opamroot=false) ?(set_opamswitch=false)
         gt switch env_file
     end
   in
+  let open OpamTypes in
   let updates =
-    List.filter (function ("OPAM_LAST_ENV", _, _, _) -> false | _ -> true)
+    List.filter (fun upd ->
+        not (String.equal upd.envu_var "OPAM_LAST_ENV"))
       updates
   in
   let last_env_file = write_last_env_file gt switch updates in
   let updates =
     OpamStd.Option.map_default (fun target ->
-        ("OPAM_LAST_ENV", OpamParserTypes.Eq, OpamFilename.to_string target, None)
+        (env_update_resolved "OPAM_LAST_ENV" Eq
+           (OpamFilename.to_string target))
         ::updates)
       updates last_env_file
   in
@@ -638,10 +645,21 @@ let switch_allowed_fields, switch_allowed_sections =
           ("setenv", Modifiable (
               (fun nc c -> { c with env = nc.env @ c.env }),
               (fun nc c ->
+                 let open OpamTypes in
                  let env =
-                   List.filter (fun (vr,op,vl,_) ->
-                       None = OpamStd.List.find_opt (fun (vr',op',vl',_) ->
-                           vr = vr' && op = op' && vl = vl') nc.env) c.env
+                   List.filter
+                     (fun { envu_var = var; envu_op = op;
+                            envu_value = value; envu_comment = _;
+                            envu_rewrite = _ } ->
+                       None =
+                       OpamStd.List.find_opt
+                         (fun { envu_var = var'; envu_op = op';
+                                envu_value = value'; envu_comment = _;
+                                envu_rewrite = _ } ->
+                           String.equal var var'
+                           && (op : OpamParserTypes.env_update_op) = op'
+                           && String.equal value value')
+                         nc.env) c.env
                  in
                  { c with env })),
            fun t -> { t with env = empty.env });

src/core/opamSystem.ml

@@ -548,10 +548,12 @@ let get_cygpath_function =
     let f = Lazy.from_val (fun x -> x) in
     fun ~command:_ -> f
 
-let apply_cygpath_path_transform path =
+let apply_cygpath_path_transform ~pathlist path =
+  let args = if pathlist then [ "--path" ] else [] in
   let r =
     OpamProcess.run
-      (OpamProcess.command ~name:(temp_file "command") ~verbose:false "cygpath" ["--path"; "--"; path])
+      (OpamProcess.command ~name:(temp_file "command")
+         ~verbose:false "cygpath" (args @ ["--"; path]))
   in
   OpamProcess.cleanup ~force:true r;
   if OpamProcess.is_success r then
@@ -566,9 +568,9 @@ let get_cygpath_path_transform =
     lazy (
       match resolve_command "cygpath" with
       | Some _ -> apply_cygpath_path_transform
-      | None -> fun x -> x)
+      | None -> fun ~pathlist:_ x -> x)
   else
-    Lazy.from_val (fun x -> x)
+    Lazy.from_val (fun ~pathlist:_ x -> x)
 
 let runs = ref []
 let print_stats () =