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

TODO: Get rid of the double Unix.chdir hack from the libraries #4793

Closed
kit-ty-kate opened this issue Aug 10, 2021 · 1 comment · Fixed by #4961
Closed

TODO: Get rid of the double Unix.chdir hack from the libraries #4793

kit-ty-kate opened this issue Aug 10, 2021 · 1 comment · Fixed by #4961

Comments

@kit-ty-kate
Copy link
Member

While debugging ocurrent/opam-repo-ci#120, @AltGr brought to my attention that some of the opam library functions (mainly OpamProcess.create and OpamSystem.real_path used in OpamFilename.Dir.of_string) use Unix.chdir twice (once to go in the directory and the second to go back to the previous directory) and are thus not thread-safe.

For OpamProcess.create it should be easy to fix: simply use Unix.fork + Unix.execve manually instead of Unix.create_process_env, but OpamSystem.real_path seems more tedious to reimplement.

@dra27
Copy link
Member

dra27 commented Aug 26, 2021

It's not quite as simple as reverting to fork because, alas, Windows - however CreateProcess has a parameter for specifying the working directory for the newly created process, so it's still eminently solvable!

For OpamSystem.real_path I agree - let's move the wrapper to OpamCompat so 4.13+ gets the superior support?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants