Git on windows: check and advertise to use Git for Windows #5718

rjbou · 2023-11-03T17:30:53Z

-- outdated, to be updated --

At initialisation, check the origin of resolved git command:

if from Cygwin, ask to install Git for Windows one
if from Git for Windows, check that it is the good directory in path (\cmd)
This is done by checking the existence of a bash executable near git.

no more install git in Cygwin initialisation if already present in path

related to #5617

/cc @jonahbeckford

todo

update changes

kit-ty-kate · 2023-11-03T20:23:35Z

src/client/opamClient.ml

@@ -635,6 +635,57 @@ let init_checks ?(hard_fail_exn=true) init_config =
  if hard_fail && hard_fail_exn then OpamStd.Sys.exit_because `Configuration_error
  else not (soft_fail || hard_fail)

+let git_for_windows_check =
+  if Sys.win32 then


Shouldn’t this code be also ran on Cygwin?

Suggested change

if Sys.win32 then

if Sys.win32 || Sys.cygwin then

I personally agree with @kit-ty-kate but I'm not a Cygwin user. I could see it going either way; Cygwin (not MSYS2) is meant as a full Unix replacement including userland tools.

No - for the same reason as we're advising users aiming for native Windows opam experience to use Git for Windows, users of Cygwin's opam should be using Cygwin's git (or at least, we shouldn't be advising the contrary). Cygwin's opam package is maintained, as well.

jonahbeckford · 2023-11-03T20:50:01Z

src/client/opamClient.ml

+let git_for_windows_check =
+  if Sys.win32 then
+    fun cygbin ->
+      let gitcmd = OpamSystem.resolve_command "git" in


To deal with the common edge case where both Cygwin (or MSYS2) git and Git for Windows git are installed, I've been doing PATH modifications in DkML so that Git for Windows always is in front of the PATH (before /usr/bin). That may be overkill here but the problem remains what to do when they are both installed.

Currently this PR will search the PATH and only check the first git ... which will almost always be Cygwin/MSYS2 /usr/bin/git.

Not sure how, but could OpamSystem.resolve_command be adjusted to find the correct git?

FYI: The PATH hack for shells (PATH="${PATH/:\/usr\/bin:\/bin:/:}") seen in https://learn.microsoft.com/en-us/vcpkg/users/platforms/mingw#how-to-avoid-mixing-different-installations seems like a common way to fix these sorts of Cygwin/MSYS2 problems. So my suggestion would be to have a OpamSystem.resolve_command ~safe:() mode which does the PATH hack in OCaml to find if there is a Git for Windows. And if that fails, then do a normal OpamSystem.resolve_command if needed.

rjbou · 2023-12-07T16:34:44Z

PR update with new mechanism: at init, ask for user to specify its git path (git4win or winget), stores it, and put it at the beginning of build & other paths.

rjbou · 2023-12-11T18:06:11Z

@jonahbeckford could you test the new git selection mechanism, if you have some time?

jonahbeckford · 2023-12-11T20:01:21Z

@jonahbeckford could you test the new git selection mechanism, if you have some time?

Sure. Is there a GitHub artifact with the opam.exe executable? It is time-consuming both setting up and building opam on Windows.

rjbou · 2023-12-12T14:24:34Z

I can try to generate one.

Makes it easy for code reviewers (so they don't have to rebuild the source code). Makes it easy to compare-and-constrast old version. Helps ocaml#5718

Makes it easy for code reviewers (so they don't have to rebuild the source code). Makes it easy to compare-and-constrast old version. + Allow GitHub Actions manual workflow Helps ocaml#5718

Makes it easy for code reviewers (so they don't have to rebuild the source code). Makes it easy to compare-and-constrast old version. Helps ocaml#5718

jonahbeckford · 2023-12-13T00:01:32Z

Don't worry.

I am using the opam.exe associated with Build-Windows (x86_64-pc-windows, x86_64-pc-cygwin, 4.14.1) based off this PR and #5762.

Testing

A. Does it run in Windows Sandbox. ⛔No

Issue A

Looks like it is missing the VC Runtime redistributables (VCRUNTIME140):

with-dkml ldd 'Y:\source\dkml\build\pkg\bump\WindowsSandbox\tools\opam.exe'                                                                                                       
        ntdll.dll => /c/WINDOWS/SYSTEM32/ntdll.dll (0x7ff847410000)
        KERNEL32.DLL => /c/WINDOWS/System32/KERNEL32.DLL (0x7ff8454d0000)
        KERNELBASE.dll => /c/WINDOWS/System32/KERNELBASE.dll (0x7ff8447b0000)
        apphelp.dll => /c/WINDOWS/SYSTEM32/apphelp.dll (0x7ff8415d0000)
        ADVAPI32.dll => /c/WINDOWS/System32/ADVAPI32.dll (0x7ff845e80000)
        msvcrt.dll => /c/WINDOWS/System32/msvcrt.dll (0x7ff846630000)
        sechost.dll => /c/WINDOWS/System32/sechost.dll (0x7ff845160000)
        RPCRT4.dll => /c/WINDOWS/System32/RPCRT4.dll (0x7ff8452e0000)
        GDI32.dll => /c/WINDOWS/System32/GDI32.dll (0x7ff8452b0000)
        win32u.dll => /c/WINDOWS/System32/win32u.dll (0x7ff844780000)
        gdi32full.dll => /c/WINDOWS/System32/gdi32full.dll (0x7ff844f80000)
        msvcp_win.dll => /c/WINDOWS/System32/msvcp_win.dll (0x7ff844b60000)
        ucrtbase.dll => /c/WINDOWS/System32/ucrtbase.dll (0x7ff844e60000)
        USER32.dll => /c/WINDOWS/System32/USER32.dll (0x7ff846dd0000)
        SHELL32.dll => /c/WINDOWS/System32/SHELL32.dll (0x7ff8455a0000)
        WS2_32.dll => /c/WINDOWS/System32/WS2_32.dll (0x7ff845e00000)
        VERSION.dll => /c/WINDOWS/SYSTEM32/VERSION.dll (0x7ff839f20000)
        VCRUNTIME140.dll => /c/WINDOWS/SYSTEM32/VCRUNTIME140.dll (0x7ff8320d0000)
        VCRUNTIME140_1.dll => /c/WINDOWS/SYSTEM32/VCRUNTIME140_1.dll (0x7ff8317e0000)
        MSVCP140.dll => /c/WINDOWS/SYSTEM32/MSVCP140.dll (0x7ff831f50000)

That isn't the end of the world ... I believe you will be building completely static versions. In the meantime, I'll install https://aka.ms/vs/17/release/vc_redist.x64.exe

jonahbeckford · 2023-12-13T00:44:50Z

I have a screen capture at https://vimeo.com/893933043. Most of it is smooth (thx!) but there are a few adjustments needed:

2:27. I think we should recommend winget install Git.Git first, before the URL https://gitforwindows.org, because the Git for Windows installation has ten (10) multiple choice pages! Especially if winget is in the PATH (I should do that too in DkML).
3:45. opam says "You can install it (Git) and relaunch opam initialization". In reality, I have to install Git for Windows, then press Ctrl-C to get out of the opam initialization (not obvious), and then rerun opam init.
4:24. When redoing opam init the Cygwin is re-downloaded and re-installed (although I think this time this goes faster). (Low-priority)
5:34. I was confused what I was supposed to answer. The question "Do you want to specify which git to use? (non cygwin)" didn't give me a clue whether "n" would use the git.exe it found in the PATH. And I don't know what "(non cygwin)" implied.

rjbou · 2023-12-13T11:43:36Z

Thanks a lot for this detailed feedback! The workflow needs to be updated:

see if it is possible for some setup to have the git check before cygwin install
abort properly or propose to pause and give the path ?
rewording, rewording, rewording

src/client/opamClient.ml

jonahbeckford · 2023-12-19T01:06:25Z

I have a small code suggestion, but otherwise LTGM. Thanks!

kit-ty-kate

I’m not convinced gitbinfield is a very descriptive name of the new option. What about git-path ?

rjbou · 2023-12-19T18:34:57Z

I’m not convinced gitbinfield is a very descriptive name of the new option. What about git-path ?

It's normal, it's not meant to stay :) [the names]

Looking for name ideas for:

gitbinfield in config: the binary directory of git
gitbinpath in OpamCoreConfig.t: the binary directory of git
gitbin in OpamClient.init/reinit: the binary directory of git if given or no git setup
--git-location & --no-git-location: options to specify git binary directory git & no git setup
bin_contains_bash in OpamSystem: does the given directory (that should be a /bin one) contains bash.exe?

rjbou · 2023-12-19T18:35:52Z

I have a small code suggestion, but otherwise LTGM. Thanks!

Thanks a lot! Indeed, better advertise to launch another terminal session to have the path updated.

dra27

Some comments on the message, and a few drive-by comments. As ever, I'm sorry to enter these discussions late. I think the general mechanism here is very good, and what I'm suggesting next reuses everything that's already here, just in a slightly different way.

I wonder if there is a possibly conceptually different approach to storing the path to Git's bin (with apologies if this has already been discussed and ruled out, because I know that @rjbou and @kit-ty-kate have discussed this extensively!).

The mechanism for adding Cygwin's bin directory to PATH is a necessary evil for Windows support, but the (admittedly very) long-term trajectory for Windows is that it should one day be unnecessary. Conversely, the "tricks" to get a Git installation's bin directory into PATH is something we'd likely always need to do.

I'd be tempted, therefore, to make it the job of opam init to determine if it can find the exact path to a git.exe which it likes (Git for Windows) and store that - so internally opam stores C:\Program Files\Git\cmd\git.exe (that path can be determined using the Registry). opam should obviously offer the choice, but I think opam init should attempt to find that version of git, regardless of PATH (the menu could of course offer the git.exe found in PATH).

The rest of the check (bash in PATH, etc.) is more general. We should be checking the entire PATH to see if Cygwin's bash is going to be shadowed anyway, and we can also warn that the git found in PATH does not match the one opam wants to use, or has that the user has done something which causes git.exe and bash.exe to be next to each other (non-recommended Git-for-Windows configuration, other package managers, etc.)

So, in summary: I think opam should very precisely determine the git.exe it wants to use (same idea as OPAMFETCH, etc.) and store that - all its git operations should do that. opam shouldn't then do any magic PATH manipulation where git is concerned - it should simply warn the user (possibly at each call - i.e. at opam install, etc.) that the git in PATH may cause problems for package installations. That approach is similar to what Git-for-Windows itself does - it provides the ability to put git-bash in PATH, but strongly recommends not doing so.

dra27 · 2023-12-22T09:12:32Z

src/client/opamClient.ml

@@ -635,12 +635,118 @@ let init_checks ?(hard_fail_exn=true) init_config =
  if hard_fail && hard_fail_exn then OpamStd.Sys.exit_because `Configuration_error
  else not (soft_fail || hard_fail)

-let windows_checks ?cygwin_setup config =
+let git_for_windows_check =
+  if not Sys.win32 && not Sys.cygwin then fun ?gitbin:_ () -> None else


Suggested change

if not Sys.win32 && not Sys.cygwin then fun ?gitbin:_ () -> None else

if Sys.unix then fun ?gitbin:_ () -> None else

src/client/opamClient.ml

Makes it easy for code reviewers (so they don't have to rebuild the source code). Makes it easy to compare-and-constrast old version. Helps ocaml#5718

…Git for Windows if needed

…u that contains everything

Co-authored-by: David Allsopp <[email protected]> Co-authored-by: jonahbeckford <[email protected]>

kit-ty-kate · 2024-01-11T17:19:35Z

Thanks a lot!

rjbou added the AREA: PORTABILITY label Nov 3, 2023

rjbou requested a review from kit-ty-kate November 3, 2023 17:30

kit-ty-kate changed the title ~~Git on windows: check and edvertise to use Git for Windows~~ Git on windows: check and advertise to use Git for Windows Nov 3, 2023

kit-ty-kate reviewed Nov 3, 2023

View reviewed changes

jonahbeckford reviewed Nov 3, 2023

View reviewed changes

rjbou force-pushed the git-win branch 2 times, most recently from b420dea to 19745f2 Compare November 28, 2023 10:45

rjbou added the PR: WAITING FOR REVIEW label Dec 11, 2023

rjbou force-pushed the git-win branch from 19745f2 to 6972f32 Compare December 11, 2023 18:03

jonahbeckford added a commit to jonahbeckford/opam that referenced this pull request Dec 12, 2023

Upload and retain opam.exe artifact

eb9f524

Makes it easy for code reviewers (so they don't have to rebuild the source code). Makes it easy to compare-and-constrast old version. Helps ocaml#5718

jonahbeckford added a commit to jonahbeckford/opam that referenced this pull request Dec 12, 2023

Upload and retain opam.exe artifact

f0b1755

Makes it easy for code reviewers (so they don't have to rebuild the source code). Makes it easy to compare-and-constrast old version. Helps ocaml#5718

jonahbeckford added a commit to jonahbeckford/opam that referenced this pull request Dec 12, 2023

Upload and retain opam.exe artifact

451b66e

Makes it easy for code reviewers (so they don't have to rebuild the source code). Makes it easy to compare-and-constrast old version. Helps ocaml#5718

jonahbeckford mentioned this pull request Dec 12, 2023

Upload and retain opam.exe artifact #5762

Open

rjbou added the CI:BINARIES Enables binaries generation in CI for a given PR label Dec 14, 2023

rjbou force-pushed the git-win branch 2 times, most recently from bb918d4 to c14e3a5 Compare December 14, 2023 20:52

jonahbeckford reviewed Dec 19, 2023

View reviewed changes

src/client/opamClient.ml Outdated Show resolved Hide resolved

kit-ty-kate reviewed Dec 19, 2023

View reviewed changes

dra27 reviewed Dec 22, 2023

View reviewed changes

kit-ty-kate linked an issue Jan 9, 2024 that may be closed by this pull request

opam 2.2+ and Git on Windows #5617

Closed

jonahbeckford and others added 4 commits January 11, 2024 14:20

Upload and retain opam.exe artifact

9ccb2d1

Makes it easy for code reviewers (so they don't have to rebuild the source code). Makes it easy to compare-and-constrast old version. Helps ocaml#5718

init: with Cygwin, don't install git if it is already present in path

93c1153

init: on windows, check origin of resolved git, and advertise to use …

bdfe695

…Git for Windows if needed

retrieve git path and store it in config + opam option checks handling

0b85ef8

kit-ty-kate force-pushed the git-win branch from 3684a87 to b24496b Compare January 11, 2024 14:31

rjbou and others added 9 commits January 11, 2024 14:33

add opam init --git-location option

db38e3e

add --no-git-location option for skipping the configuration

a44ddb8

reftest: update windows test to skip git location configuration

e026ca5

reftest: update var option

24c2da9

rewording + check git availability before cygwin + have a looping men…

d914d2a

…u that contains everything

launch git configuration only if tty

8adc66d

Improve user-facing messages related to the new git-location options

ce6a775

Co-authored-by: David Allsopp <[email protected]> Co-authored-by: jonahbeckford <[email protected]>

Rename gitbin* to git-location for consistency and readability

10fe1a4

Update master_changes.md

4952361

kit-ty-kate force-pushed the git-win branch from b24496b to 4952361 Compare January 11, 2024 14:33

kit-ty-kate approved these changes Jan 11, 2024

View reviewed changes

kit-ty-kate added this to the 2.2.0~beta1 milestone Jan 11, 2024

kit-ty-kate merged commit a358d94 into ocaml:master Jan 11, 2024
29 checks passed

rjbou removed the PR: WAITING FOR REVIEW label Feb 15, 2024

This was referenced Feb 15, 2024

arg: fix loading git location from config file at init #5842

Merged

Windows: git-location in init config file is unused #5845

Closed

dra27 mentioned this pull request Jun 13, 2024

Does OPAM work on windows? #246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Git on windows: check and advertise to use Git for Windows #5718

Git on windows: check and advertise to use Git for Windows #5718

rjbou commented Nov 3, 2023 •

edited by kit-ty-kate

Loading

kit-ty-kate Nov 3, 2023

jonahbeckford Nov 3, 2023

dra27 Dec 22, 2023

jonahbeckford Nov 3, 2023

jonahbeckford Nov 3, 2023

rjbou commented Dec 7, 2023

rjbou commented Dec 11, 2023

jonahbeckford commented Dec 11, 2023

rjbou commented Dec 12, 2023

jonahbeckford commented Dec 13, 2023

jonahbeckford commented Dec 13, 2023 •

edited by rjbou

Loading

rjbou commented Dec 13, 2023 •

edited

Loading

jonahbeckford commented Dec 19, 2023

kit-ty-kate left a comment

rjbou commented Dec 19, 2023 •

edited

Loading

rjbou commented Dec 19, 2023

dra27 left a comment

dra27 Dec 22, 2023

kit-ty-kate commented Jan 11, 2024

	if not Sys.win32 && not Sys.cygwin then fun ?gitbin:_ () -> None else
	if Sys.unix then fun ?gitbin:_ () -> None else

Git on windows: check and advertise to use Git for Windows #5718

Git on windows: check and advertise to use Git for Windows #5718

Conversation

rjbou commented Nov 3, 2023 • edited by kit-ty-kate Loading

kit-ty-kate Nov 3, 2023

Choose a reason for hiding this comment

jonahbeckford Nov 3, 2023

Choose a reason for hiding this comment

dra27 Dec 22, 2023

Choose a reason for hiding this comment

jonahbeckford Nov 3, 2023

Choose a reason for hiding this comment

jonahbeckford Nov 3, 2023

Choose a reason for hiding this comment

rjbou commented Dec 7, 2023

rjbou commented Dec 11, 2023

jonahbeckford commented Dec 11, 2023

rjbou commented Dec 12, 2023

jonahbeckford commented Dec 13, 2023

Testing

Issue A

jonahbeckford commented Dec 13, 2023 • edited by rjbou Loading

rjbou commented Dec 13, 2023 • edited Loading

jonahbeckford commented Dec 19, 2023

kit-ty-kate left a comment

Choose a reason for hiding this comment

rjbou commented Dec 19, 2023 • edited Loading

rjbou commented Dec 19, 2023

dra27 left a comment

Choose a reason for hiding this comment

dra27 Dec 22, 2023

Choose a reason for hiding this comment

kit-ty-kate commented Jan 11, 2024

rjbou commented Nov 3, 2023 •

edited by kit-ty-kate

Loading

jonahbeckford commented Dec 13, 2023 •

edited by rjbou

Loading

rjbou commented Dec 13, 2023 •

edited

Loading

rjbou commented Dec 19, 2023 •

edited

Loading