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

rjbou · 2023-08-17T16:18:54Z

Add environment path rewriting for Windows, see spec for more details.

New x- field x-env-path-rewrite to define rewriting rules, in the format: IDENT <bool> | (<separator-with-filter-formula> <format-with-filter-formula>) list

x-env-path-rewrite: [
  [ VAR1 false ]
  [ VAR2 ":" "host"]
  [ VAR3 (":" | ";" {os=win32}) ("target" | "target-quoted" {os=win32})]
]

host will call cygpath on the added path on windows. *-quoted will quote the added path if it contains the separator. target, if given explicitly will rewrite slashes in bacwardslashes.

It has an effect on environment file, as it needs to store the resolved separator and format, but it remains backward compatible:

VAR1 = path/to/add
VAR2 = path/to/add comment
VAR3 = path/to/add : host comment
VAR4 = path/to/add : host comment

todo:

update changes
update doc
add formula not resolved error case
update effectively equal to check x-env-path-rewrite

Old Version
Implementation of proposal described in #5602
+ add lint to check that the x- field is well defined
+ add lint that check that if x- field is true, an opam-version availability guard is set

todo:

src/format/opamFile.ml

src/client/opamAdminRepoUpgrade.ml

src/state/opamFileTools.ml

rjbou · 2023-08-29T16:58:07Z

Updated

src/format/opamFile.ml

AltGr

LGTM, but... where is the doc ?
In it it may also be appropriate to mention the reason for an x-* field (the manual specifies that it's normally for external tools IIRC)

src/format/opamFile.ml

src/state/opamFileTools.ml

rjbou · 2023-09-07T08:57:58Z

To not change the opam file syntax, opam need to use it's own external tool features ^^

rjbou · 2023-09-07T16:21:44Z

Doc updated!

src/format/opamFile.ml

rjbou · 2023-11-08T20:28:44Z

Final update!

kit-ty-kate

Final review. Looks good otherwise

doc/pages/Manual.md

src/client/opamAction.ml

src/core/opamSystem.ml

src/core/opamSystem.mli

doc/pages/Manual.md

dra27

I'm just testing locally with the various packages that everything continues to work as needed, but I'm not expecting to find a problem, thank you!

Various suggestions, most of them documentation/comments, but it's looking lovely 🙂

doc/pages/Manual.md

dra27 · 2023-11-10T11:57:26Z

src/client/opamAdminRepoUpgrade.ml

+          envu_comment = None;
+          envu_rewrite = empty;
+        }] |>
+      (* XXX Rewrite rules ?? *)


Stray comment?

dra27 · 2023-11-10T12:14:00Z

src/client/opamAction.ml

+         envu_value = OpamFilename.Dir.to_string cygbin;
+         envu_comment = Some "Cygwin path";
+         envu_rewrite = empty;
+       }]


Given that this happens a lot, worth having OpamBaseTypes.env_update:

let env_update ?comment:envu_comment envu_var envu_op envu_value rewrite = let envu_rewrite = match rewrite with | `resolved -> Some (SPF_Resolved None) | `unresolved -> Some (SPF_Unresolved (Empty, Empty)) | `rewrite r -> r in {envu_var; envu_op; envu_value; envu_comment; envu_rewrite}

?

Added in a commit

I don't know which version is more easily readable

[{ envu_var = "PATH"; envu_op = EqPlus; envu_value = OpamFilename.Dir.to_string cygbin; envu_comment = Some "Cygwin path"; envu_rewrite = empty; }]

vs

[ OpamTypesBase.env_update_resolved ~var:"PATH" ~op:EqPlus ~value:(OpamFilename.Dir.to_string cygbin) ~comment:"Cygwin path" () ]

Sorry - looking at it again, I now realise that I'd forgotten that a GADT was involved and that we can't convey that in optional parameters! (yet... it's been on my wish list for quite a few years...)

Wouldn't it be OK not to label those three arguments, though> I think env_update_resolved "PATH" EqPlus (OpamFilename.Dir.to_string cygbin quite clearly conveys what it's doing to give:

[ OpamTypesBase.env_update_resolved ~comment:"Cygwin path" "PATH" EqPlus (OpamFilename.Dir.to_string cygbin) ]

?

It is possible. The labels came at first because of var & values are string, and like that there is no confusion between them.
I'm ok to remove them and have then

val env_update_resolved: ?comment:string -> ?rewrite:spf_resolved separator_path_format option -> string -> env_update_op_kind -> string -> spf_resolved env_update

src/format/opamFile.ml

doc/pages/Manual.md

type env_update = { envu_var : string; envu_op : OpamParserTypes.FullPos.env_update_op_kind; envu_value : string; envu_comment : string option; }

It contains rewriting rules of the environment variables to update: - not a path, no rewrite - formulaes to resolve to retrieve separator & path format - resolved formulaes, directly separator & path format This commit contains only the addition of the field, it does not performs the rewriting.

…<pp1 | pp2>'

…ems'

…ewriting rules for variables defined in `setenv` and `build-env`. Format is x-env-path-rewrite: [ [ VAR false ] # no a path, no rewrite [ VAR ":" "target" ] # always use ":" and target format [ VAR (":" | ";" { os=win32 }) ("target-quoted" | "target" { os = win32}) ] # On windows use ";" separator and don't quote, otherwise ":" and quote the set/added path if needed. ]

…t per variable) to compute environment updates

…ewriting rules

kit-ty-kate · 2023-11-13T13:58:10Z

Thanks!

rjbou added the PR: WIP Not for merge at this stage label Aug 17, 2023

rjbou added this to the 2.2.0~alpha3 milestone Aug 17, 2023

rjbou commented Aug 18, 2023

View reviewed changes

src/format/opamFile.ml Outdated Show resolved Hide resolved

rjbou added this to For alpha3/beta in Opam 2.2.0 Aug 18, 2023

rjbou force-pushed the setenv-slash branch from c49d5bd to 05f6f4a Compare August 25, 2023 15:18

rjbou added PR: WAITING FOR REVIEW and removed PR: WIP Not for merge at this stage labels Aug 25, 2023

rjbou commented Aug 28, 2023

View reviewed changes

src/client/opamAdminRepoUpgrade.ml Outdated Show resolved Hide resolved

rjbou commented Aug 29, 2023

View reviewed changes

src/state/opamFileTools.ml Outdated Show resolved Hide resolved

rjbou commented Aug 29, 2023

View reviewed changes

src/state/opamFileTools.ml Outdated Show resolved Hide resolved

rjbou force-pushed the setenv-slash branch from e148f7d to dc2d8b9 Compare August 29, 2023 16:58

rjbou requested review from kit-ty-kate and AltGr August 29, 2023 16:59

kit-ty-kate reviewed Aug 29, 2023

View reviewed changes

src/format/opamFile.ml Outdated Show resolved Hide resolved

kit-ty-kate approved these changes Sep 4, 2023

View reviewed changes

src/format/opamFile.ml Outdated Show resolved Hide resolved

AltGr approved these changes Sep 7, 2023

View reviewed changes

src/format/opamFile.ml Outdated Show resolved Hide resolved

src/state/opamFileTools.ml Outdated Show resolved Hide resolved

rjbou added PR: NEEDS UPDATE STATE: READY TO MERGE and removed PR: WAITING FOR REVIEW labels Sep 7, 2023

rjbou force-pushed the setenv-slash branch 2 times, most recently from 2028e26 to 7365526 Compare September 7, 2023 16:17

rjbou removed the PR: NEEDS UPDATE label Sep 7, 2023

kit-ty-kate reviewed Sep 7, 2023

View reviewed changes

src/format/opamFile.ml Outdated Show resolved Hide resolved

src/format/opamFile.ml Outdated Show resolved Hide resolved

rjbou force-pushed the setenv-slash branch from 7365526 to 478b53f Compare September 8, 2023 14:24

kit-ty-kate added PR: NEEDS UPDATE IMPORTANT and removed STATE: READY TO MERGE labels Sep 13, 2023

kit-ty-kate reviewed Nov 9, 2023

View reviewed changes

dra27 mentioned this pull request Nov 9, 2023

reftest engine tweaks for Windows #5723

Merged

rjbou commented Nov 9, 2023

View reviewed changes

doc/pages/Manual.md Outdated Show resolved Hide resolved

rjbou force-pushed the setenv-slash branch from 78103d9 to e37474b Compare November 9, 2023 14:48

dra27 reviewed Nov 10, 2023

View reviewed changes

rjbou commented Nov 10, 2023

View reviewed changes

doc/pages/Manual.md Outdated Show resolved Hide resolved

rjbou force-pushed the setenv-slash branch 2 times, most recently from d288ab4 to 611f497 Compare November 13, 2023 11:48

rjbou added 14 commits November 13, 2023 12:49

file: fix x-locked error message

8fff384

types: use a record for env_updates instead of a 4-uple

e32be04

type env_update = { envu_var : string; envu_op : OpamParserTypes.FullPos.env_update_op_kind; envu_value : string; envu_comment : string option; }

format: fix Pp.fallback on printing function, and add fallback name '…

f14a4c3

…<pp1 | pp2>'

format: add 'formula_items', a generic version of 'package_formula_it…

ef4491e

…ems'

typesbase: add to_string functions for separator and path-format

b530e3a

format: add value separator and path_format parser-printer

a67734e

system: cygpath_path_transform option to chosse if path list of no

80eedda

env: use defined rewriting rules (opam file, environment file, defaul…

099727f

…t per variable) to compute environment updates

file: add 'x-env-path-rewrite' to opam file effective part

8e65837

reftest: add 'env.unix' & 'env.win32' to test environment variables r…

a9015ff

…ewriting rules

doc: add 'x-env-path-rewrite' documentation

ec03971

rjbou force-pushed the setenv-slash branch from 611f497 to ec03971 Compare November 13, 2023 11:51

This was linked to issues Nov 13, 2023

Variable expansion in env fields on Windows #5602

Closed

Opam setenv for path lists is broken on Windows/Cygwin #4690

Closed

kit-ty-kate merged commit 0670b46 into ocaml:master Nov 13, 2023
29 checks passed

Opam 2.2.0 automation moved this from For alpha3 to Done Nov 13, 2023

dra27 mentioned this pull request Nov 14, 2023

%<...>% path expansion notation #2927

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rjbou commented Aug 17, 2023 •

edited

rjbou commented Aug 29, 2023

AltGr left a comment

rjbou commented Sep 7, 2023

rjbou commented Sep 7, 2023

rjbou commented Nov 8, 2023

kit-ty-kate left a comment

dra27 left a comment

dra27 Nov 10, 2023

dra27 Nov 10, 2023

rjbou Nov 10, 2023

rjbou Nov 10, 2023 •

edited

dra27 Nov 10, 2023

rjbou Nov 10, 2023

kit-ty-kate commented Nov 13, 2023

Path rewriting for setenv: and build-env: #5636

Path rewriting for setenv: and build-env: #5636

Conversation

rjbou commented Aug 17, 2023 • edited

rjbou commented Aug 29, 2023

AltGr left a comment

Choose a reason for hiding this comment

rjbou commented Sep 7, 2023

rjbou commented Sep 7, 2023

rjbou commented Nov 8, 2023

kit-ty-kate left a comment

Choose a reason for hiding this comment

dra27 left a comment

Choose a reason for hiding this comment

dra27 Nov 10, 2023

Choose a reason for hiding this comment

dra27 Nov 10, 2023

Choose a reason for hiding this comment

rjbou Nov 10, 2023

Choose a reason for hiding this comment

rjbou Nov 10, 2023 • edited

Choose a reason for hiding this comment

dra27 Nov 10, 2023

Choose a reason for hiding this comment

rjbou Nov 10, 2023

Choose a reason for hiding this comment

kit-ty-kate commented Nov 13, 2023

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

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

rjbou commented Aug 17, 2023 •

edited

rjbou Nov 10, 2023 •

edited