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

Add winsymlinks:native to the CYGWIN environment variable when running a Cygwin executable on Windows #5793

Merged
merged 1 commit into from Feb 19, 2024

Conversation

kit-ty-kate
Copy link
Member

Fixes #5782

Actually no, at least not yet. Upon creating the PR I realized another point in the code also modifies CYGWIN (in a destructive manner)

@dra27 I’m reading the comments for OpamProcess.cygwin_create_process_env and the use of win_create_process (which i’m also realizing does not exist in OCaml >= 5.0, which means opam can’t be compiled on Windows if you’re using ocaml 5) and I’m not sure if it’s just a relic of the past that can now be replaced by Unix.create_process_env or not? Was the issue encountered with Unix.create_process_env ever got fixed or was it only an issue in opam and thus not something that could actually be fixed upstream?

If it is a relic of the past I’ll go ahead and remove it and together with the current code, it should work.

@dra27
Copy link
Member

dra27 commented Jan 24, 2024

In somewhat haste, nothing's been fixed in OCaml (nor is ever likely to be), so I believe the code is still needed. The stubs are still there, they are called caml_unix_create_process and caml_unix_create_process_native in OCaml 5.0+

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this! The part in OpamProcess is completely orthogonal - that's all to do with getting the escaping of arguments correct. It doesn't destructively change CYGWIN though - it's editing it (in what's looking to me 9 years later to be a possibly over-pedantic fashion, but perhaps the rules for specifying options in the CYGWIN variable used to be more strict).

Regardless, it's definitely not interfering because my local workaround has been to set CYGWIN=winsymlinks:strict in my shell prior to invoking opam and it's not overriding it 🙂

src/client/opamAction.ml Outdated Show resolved Hide resolved
@kit-ty-kate
Copy link
Member Author

@dra27 done

@dra27
Copy link
Member

dra27 commented Feb 16, 2024

A thought which didn't occur to me before - does this also apply when we're running commands from within opam (e.g. quite critically, tar)?

doc/pages/Manual.md Outdated Show resolved Hide resolved
@kit-ty-kate
Copy link
Member Author

ah i think you're right. That code should be in OpamProcess.cygwin_create_process_env instead

…ling a package on Windows

Co-authored-by: David Allsopp <david.allsopp@metastack.com>
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@kit-ty-kate kit-ty-kate marked this pull request as draft February 19, 2024 17:37
@kit-ty-kate kit-ty-kate changed the title Add winsymlinks:native to the CYGWIN environment variable when installing a package on Windows Add winsymlinks:native to the CYGWIN environment variable when running a Cygwin executable on Windows Feb 19, 2024
@kit-ty-kate kit-ty-kate marked this pull request as ready for review February 19, 2024 19:35
@kit-ty-kate kit-ty-kate merged commit adb6911 into ocaml:master Feb 19, 2024
28 of 29 checks passed
Opam 2.2.0 automation moved this from For beta2 to Done Feb 19, 2024
@kit-ty-kate kit-ty-kate deleted the winsymlinks-native branch February 19, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

Cygwin internal installation should add winsymlinks:native to CYGWIN
3 participants