-
Notifications
You must be signed in to change notification settings - Fork 350
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
Improvements to opam init
"Git" menu on Windows
#5963
Conversation
a32608c
to
811889f
Compare
LGTM on first read apart from the question above |
1e4490c
to
8c29b1b
Compare
One of the simpler parts of the Win16 API...
Complementing OpamStubs.writeRegistry
On Windows, uses CreateEnvironmentBlock to get the pristine environment which would be applied to a new terminal. Allows `opam init` to determine if `git` _will_ be in Path next time round...
Users of the Cygwin port of opam should be using the Cygwin port of git, with no exceptions.
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.
Little comment, otherwise, lgtm!
@@ -693,6 +767,7 @@ let git_for_windows_check = | |||
`Abort, "Abort initialisation to install recommended Git."; | |||
] | |||
in | |||
OpamStd.Option.iter (OpamConsole.warning "%s\n") gfw_message; |
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.
What about changing the abort message in case git was found ? Something like "Abort initialisation to restart your shell".
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 like it!
DRA@Tau MSYS /c/Devel/opam-3
$ ./opam init
No configuration file found, using built-in defaults.
<><> Git ><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><> 🐫
Cygwin Git is functional but can have credentials issues for private repositories, we recommend using:
- Install via 'winget install Git.Git'
- Git for Windows can be downloaded and installed from https://gitforwindows.org
[WARNING] It looks as though Git for Windows has been installed but the shell needs to be
restarted. You may wish to abort and re-run opam init from a fresh session.
Which Git should opam use?
> 1. Use default Cygwin Git
2. Enter the location of installed Git
3. Abort initialisation to restart your shell.
[1/2/3]
If git is resolved in PATH and the binary's version block indicates that it's a Git-for-Windows binary, skip the menu.
During `opam init` on Windows, if git is not found in PATH, test again using the initial environment and if git is found display an additional warning on the Git configuration menu that the user appears not to have started a fresh shell.
In addition to not restarting the shell, it is possible that a user has installed Git for Windows, but instructed its installer not to update PATH. In this case, the installations will be referred to in the Windows Registry. As a final fallback, opam init searches for these installations and, if found, displays a warning suggesting that the user may wish to reconfigure them (because then git will available to things installed by opam, such as Dune), but also includes them in the list of "gits" which can be manually selected with for git-location.
If opam is configured with --with-private-runtime, then shared linking should be assumed by default.
I pulled your changes from rjbou/gwf-menu as well, thanks! |
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.
thanks!
This PR extends the detection of Git on Windows during
opam init
in several ways (which includes addressing the cyclic logic identified in #5835)The PR is three sections: the first three commits add Windows primitives:
PATH
when a new Command Prompt or PowerShell prompt would be opened (before any user commands are executed) so including things which may have been added to PATH after the current session was started (it's very loosely like runningenv -i $(command -v bash) -lc "env"
but without the terror, as it's an actual OS-supported thing)The next part is easy:
OpamClient.git_for_windows_check
current does stuff when opam is built for Cygwin, which it categorically should not. From our perspective, if a user is using Cygwin's build ofopam
then they should only use Cygwin's build ofgit
(or have to work very very hard themselves to change that); from the Cygwin opam package maintainer's perspective, a Cygwin tool should not suddenly ignore a Cygwin package in preference for an external tool (imagine we were trying to make the Linux build of opam behave this way in WSL and then imagine the response of the Debian maintainer 🙂)There are then three areas where
opam init
's Git for Windows detection is enhanced. We can identify a Git for Windows binary from the Product Version string in the executable which will contain.windows.
(the tag format for Git for Windows versions is covered in their security guide). This instantly differentiates it from much older builds of msysgit (from pre-Git-for-Windows days) and also from Cygwin/MSYS2's Git (which isn't built with resource blocks anyway, and would use Git's tag numbers if it were). I've gone with just checking the.windows.
format for now because its clearly enough - if it were ever to stop being enough, we could examine other fields (like the author name, etc...!).If
opam init
resolves git.exe in PATH, and that git.exe has a Product Version of the format git-major.git-minor.git-revision.windows.gfw-revision, then no menu is displayed - Git for Windows is in use and has been identified, which fixes #5835.The last two commits extend this further to deal with what I think is one very likely user mistake and one possible user misconfiguration.
I think it is highly likely that we will encounter users who do see that they should go and install Git for Windows, but who do not then open a fresh Command Prompt / PowerShell session before running
opam init
again. In this instance, git.exe will still not be found in PATH. To counter this,opam init
now checks the initial environment which, after runningwinget install Git.Git
or using the Git-for-Windows installer, will now include the location of git.exe. If git.exe can be found in PATH in this environment then the menu is still displayed, but with a warning, thus:The final commit extends this one stage further. The Git for Windows installer includes an option to install, but not update PATH. If git.exe is found in neither the current PATH nor the pristine/initial PATH, then opam now searches the Registry for the two entries which may point to Git for Windows installations (it is possible to install both for the entire machine and for the current user). If one or both of these are found, then opam still displays the menu, but with a different warning and it also adds the
\cmd
directory of this installation to the list of possible values to use for git-location: