Resolve the Cygwin bin directory above Windows system32 directory

[dra27]

OpamProcess.cygwin_create_process_env contains some ad hoc logic which ensures that the Cygwin bin directory appears before the Windows system32 directory in Path and adjusts it if necessary. This allows the user to put c:\cygwin64\bin routinely at the end of Path so that Cygwin utilities don't shadow anything (including, importantly, the Microsoft linker link.exe if using the MSVC port of OCaml). However, when invoking a Cygwin executable directly, this results in /usr/bin appearing at the end of PATH, which is confusing - especially if the executable (or script) expects to invoke bash. 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 Cygwin bin directory to the end when executing build and install commands, etc. This environment update interacts surprisingly with OpamProcess.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"]showsC:\Windows\system32\bash.exebeing first but["sh" "-c" "where bash"]shows Cygwin'sbash.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 constructor Cygwin (i.e. it cannot appear in opam files, nor can it be written/read from environment files). It places the directory given as far down the list of directories in Path so that bash.exe, tar.exe and sort.exe (and git.exe, if Cygwin's git is being used) still resolve to the Cygwin copy. In practice, this means that the bin directory will never be placed after C:\Window\system32 (as before) but also means, for example, that it would not be moved after a Scoop bin directory (which may contain bash.exe or git.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 puts C:\cygwin64\bin into Path, they are now responsible for putting it in the right place; when opam puts its own .cygwin\root\bin diectory into Path, it ensures that it puts it into the correct place.)

[dra27]

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:

opam-version: "2.0"
synopsis: "Show the environment"
description: "Yet another environment debug package"
maintainer: "David Allsopp <[email protected]>"
authors: "David Allsopp"
license: "ISC"
homepage: "https://opam.ocaml.org"
bug-reports: "https://github.com/ocaml/opam/issues"
flags: conf
build: [
  ["cmd" "/c" "echo %%PATH%%"] # *** 1 ***
  ["cmd" "/c" "echo ============================================"]
  ["sh" "-c" "env | grep '^PATH='"] # *** 2 ***
  ["cmd" "/c" "echo ============================================"]
  ["cmd" "/c" "env | findstr /R ^^PATH="] # *** 3 ****
  ["cmd" "/c" "echo ============================================"]
  ["where" "bash"] # *** 4 ***
]

but that's not readily convertable to a test script. Before this PR:

1. Has cygbin at the end of PATH
2. Has cygbin just before C:\Windows\system32\ in PATH
3. Is the same as 1 (so cygbin at the end of PATH)
4. Has C:\Windows\system32\bash.exe first if you have WSL installed 💀

With this PR, 1, 2 and 3 all have cygbin just before C:\Windows\system32\ and 4 has bash.exe in cygbin first, regardless of whether you have WSL installed 😇

[kit-ty-kate]

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.

[dra27]

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 test stanza and didn't think just to move it to tests! I'll do that 🙂

[rjbou]

On the idea and the code, LGTM! I've put some comments in the review.
and... what a type gymnastic xD

[rjbou]

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.

[dra27]

Commit removed

[rjbou]

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.

[dra27]

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" tar.exe in PATH in a directory after the Visual Studio bin directories. The MSVC build would then fairly naturally test this - if opam puts Cygwin's bin before the Visual Studio directories, link would fail and if it puts it after the poisoned directory, extraction will fail (so a canary-style test rather than a unit test)

[rjbou]

LGTM! Thanks!

[kit-ty-kate]

Thanks!!