-
Notifications
You must be signed in to change notification settings - Fork 351
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
Resolve the Cygwin bin directory above Windows system32 directory #5832
Conversation
First two commits are optional (and could be moved elsewhere). Could do with some more comments, possibly a test. I've been testing with this:
but that's not readily convertable to a test script. Before this PR:
With this PR, 1, 2 and 3 all have cygbin just before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the question of where the TypeGymnastics
module/test should live (I would personally prefer for it to be in a new tests/libtests
directory or something like that), LGTM.
Yes, I think in my mind the "type gymnastics" test was going to be part of the (dev) build to be sure at all times. Then I used a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the idea and the code, LGTM! I've put some comments in the review.
and... what a type gymnastic xD
|
||
(** Like [get_opam_raw], but returns the list of updates instead of the new | ||
environment. *) | ||
(** Returns the list of updates from the switch's cache file. *) | ||
val get_opam_raw_updates: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about removing these ones, they are part of the library (and exported) since a long time.
We can at least add a comment in the ml file that they are not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit removed
Tested locally, it works well. For a CI test, it needs its proper jobs, to have an environment 100% controlled (git, bash, cygwin, etc.), it can't be on reftests. |
Makes it slightly easier to run the tests separately.
For the testing, my feeling was indeed that to do it via reftests would be very contrived. What we might be able to do in CI in general is to put a "poisoned" |
Allow the possibility to partition the constructors for environment updates to have some operations which cannot come from opam files nor be synthesised to environment files.
The Cygwin environment update moves the directory as far down PATH as possible without allowing bash, tar, sort or, if installed, git to become shadowed.
Assume instead that the user will have configured PATH correctly, knowing that if opam has added Cygwin's bin to PATH, it will have put it in the correct place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
Thanks!! |
Formed part of work on ocaml#5832, but they were not critical, so were dropped. Possibly worth considering: - Removal of unused API functions (because supporting them is effort) - Use of private records is generally more hygienic (and done elsewhere already)
OpamProcess.cygwin_create_process_env
contains some ad hoc logic which ensures that the Cygwinbin
directory appears before the Windowssystem32
directory inPath
and adjusts it if necessary. This allows the user to putc:\cygwin64\bin
routinely at the end ofPath
so that Cygwin utilities don't shadow anything (including, importantly, the Microsoft linkerlink.exe
if using the MSVC port of OCaml). However, when invoking a Cygwin executable directly, this results in/usr/bin
appearing at the end ofPATH
, which is confusing - especially if the executable (or script) expects to invokebash
. Hence the workaround.There are, however, more things than just
bash.exe
where Cygwin executables may want their own version to apply when executing and it's possible for other directories to shadow Cygwin as well.Enter #5545, and opam's internally managed Cygwin. This inserts a
PATH =+
update to append the Cygwinbin
directory to the end when executingbuild
andinstall
commands, etc. This environment update interacts surprisingly withOpamProcess.cygwin_create_process_env
.["bash" "-c" "..."]
"executes" using WSL yet["sh" "-c" "bash -c \"foo\""] executes using Cygwin's
bash, for example. The resolution problems can be clearly seen using
where-
["where" "bash"]shows
C:\Windows\system32\bash.exebeing first but
["sh" "-c" "where bash"]shows Cygwin's
bash.exe` being first.This PR generalises the adjustment being made to
Path
by introducing a new kind of environment update. This update is only available internally via the constructorCygwin
(i.e. it cannot appear inopam
files, nor can it be written/read fromenvironment
files). It places the directory given as far down the list of directories inPath
so thatbash.exe
,tar.exe
andsort.exe
(andgit.exe
, if Cygwin'sgit
is being used) still resolve to the Cygwin copy. In practice, this means that thebin
directory will never be placed afterC:\Window\system32
(as before) but also means, for example, that it would not be moved after a Scoopbin
directory (which may containbash.exe
orgit.exe
), etc.With this slightly more principled mechanism in place, the ad hoc adjustment in
OpamProcess.cygwin_create_process_env
is removed (i.e. if the user putsC:\cygwin64\bin
intoPath
, they are now responsible for putting it in the right place; when opam puts its own.cygwin\root\bin
diectory intoPath
, it ensures that it puts it into the correct place.)