Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Variable expansion in env fields on Windows #5602

Closed
rjbou opened this issue Jul 13, 2023 · 8 comments · Fixed by #5636
Closed

Variable expansion in env fields on Windows #5602

rjbou opened this issue Jul 13, 2023 · 8 comments · Fixed by #5636

Comments

@rjbou
Copy link
Collaborator

rjbou commented Jul 13, 2023

If we take the example of pkg-config, there is

setenv: PKG_CONFIG_PATH += "%{lib}%/pkgconfig"

On cmd, opam env gives

set PKG_CONFIG_PATH=C:\opamroot\switch\lib/pkgconfig

opam should handle present separators in opam files?

@MisterDA
Copy link
Contributor

Duplicate of #4690?

@kit-ty-kate
Copy link
Member

@AltGr and I discussed that at the meeting and we can’t really find a totally satisfying solution that doesn’t involve changing the opam syntax. The best we could find would be to automatically change / to \ on Windows if:

  • the string is in build-env or setenv
  • the string is not a triple-double-quote type string (requires a small change in the API of opam-file-format but not to the syntax)

Another solution i just thought of after the meeting, would be to use a builtin extra x-* field. For example:

setenv: PKG_CONFIG_PATH += "%{lib}%/pkgconfig"
x-allow-env-path-rewrite-on-windows: true

This way the rewriting is made explicit (x-allow-env-path-rewrite-on-windows would be false by default). What do you think @AltGr?

Overall discussion also related to #2927

@AltGr
Copy link
Member

AltGr commented Aug 7, 2023

Hm the additional field feels slightly clunky, but has the good point of being completely explicit and allowing a reasonable workaround to unblock this.

We might consider making it true by default though ? While less satisfactory in that it allows some magic "by default", that remains limited to the Windows scope where we can assume the package wouldn't have worked anyway...

@avsm
Copy link
Member

avsm commented Aug 7, 2023

This seems like a good reason to bump opam-version and adopt the new semantics if the package adopts the higher version number.

@rjbou
Copy link
Collaborator Author

rjbou commented Aug 14, 2023

We should also add a lint that raise a warning if an env variable in build-env or setenv is defined with / and no x- field is defined.
This will help to set the default to false, and make package manager aware of some windows constraints.

@rjbou
Copy link
Collaborator Author

rjbou commented Aug 14, 2023

related #2927

@kit-ty-kate
Copy link
Member

This seems like a good reason to bump opam-version and adopt the new semantics if the package adopts the higher version number.

We can’t do that because opam 2.0 and 2.1 don’t support having opam files with unsupported versions (#5631), and doing a full repository transition like it was done for the transition from opam 1.2 to opam 2.0 requires tools, resources and time we do not have.

@rjbou
Copy link
Collaborator Author

rjbou commented Aug 17, 2023

setenv is also defined for switch-config files.

@rjbou rjbou added this to the 2.2.0~alpha3 milestone Oct 25, 2023
@kit-ty-kate kit-ty-kate linked a pull request Nov 13, 2023 that will close this issue
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants