Merge pull request ocaml#5718 from rjbou/git-win

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

.github/workflows/main.yml

@@ -207,6 +207,15 @@ jobs:
     - name: Test "static" binaries on Windows
       if: endsWith(matrix.host, '-pc-cygwin') == false
       run: ldd ./opam.exe | test "$(grep -v -F /cygdrive/c/Windows/)" = ''
+    - name: 'Upload opam binaries for Windows'
+      if: endsWith(matrix.host, '-pc-windows')
+      uses: actions/upload-artifact@v3
+      with:
+        name: opam-exe-${{ matrix.host }}-${{ matrix.ocamlv }}-${{ matrix.build }}
+        path: |
+          D:\Local\bin\opam.exe
+          D:\Local\bin\opam-installer.exe
+          D:\Local\bin\opam-putenv.exe
     - name: Test (basic - Cygwin)
       if: endsWith(matrix.host, '-pc-cygwin')
       run: bash -exu .github/scripts/main/test.sh
@@ -219,7 +228,7 @@ jobs:
         set Path=D:\Cache\ocaml-local\bin;%Path%
         if "${{ matrix.host }}" equ "x86_64-pc-windows" call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Auxiliary\Build\vcvars64.bat"
         if "${{ matrix.host }}" equ "i686-pc-windows" call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Auxiliary\Build\vcvars32.bat"
-        opam init --yes --bare default git+file://D:/opam-repository#${{ env.OPAM_TEST_REPO_SHA }} || exit /b 1
+        opam init --yes --bare default git+file://D:/opam-repository#${{ env.OPAM_TEST_REPO_SHA }} --no-git-location || exit /b 1
         opam switch --yes create default ocaml-system || exit /b 1
         opam env || exit /b 1
         opam install --yes lwt || exit /b 1

master_changes.md

@@ -18,6 +18,8 @@ users)
 ## Plugins
 
 ## Init
+  * Check and advertise to use Git for Windows [#5718 @rjbou - fix #5617]
+  * Add the `--git-location` and `--no-git-location` arguments [#5718 @rjbou]
 
 ## Config report
 
@@ -39,6 +41,7 @@ users)
 ## Show
 
 ## Var/Option
+  * Add a new git-location option on Windows [#5718 @rjbou]
 
 ## Update / Upgrade
 

src/client/opamAction.ml

@@ -531,10 +531,15 @@ let compilation_env t opam =
   let cygwin_env =
     match OpamSysInteract.Cygwin.cygbin_opt t.switch_global.config with
     | Some cygbin ->
-      [ OpamTypesBase.env_update_resolved "PATH" EqPlus
-          (OpamFilename.Dir.to_string cygbin)
+      let cygbin = OpamFilename.Dir.to_string cygbin in
+      [ OpamTypesBase.env_update_resolved "PATH" EqPlus cygbin
           ~comment:"Cygwin path"
-      ]
+      ] @ (match OpamCoreConfig.(!r.git_location) with
+          | None -> []
+          | Some git_location ->
+            if String.equal cygbin git_location then [] else
+              [ OpamTypesBase.env_update_resolved "PATH" PlusEq
+                  git_location ~comment:"Git binary path"])
     | None -> []
   in
   let shell_sanitization = "shell env sanitization" in

src/client/opamArg.ml

@@ -575,7 +575,8 @@ let apply_global_options cli o =
       (`A (List.map (fun s -> `String s) (Array.to_list Sys.argv)))
   );
   (* We need to retrieve very early cygwin root path to set up 'cygbin' config
-     field. It is retrieved from config file, and we use a low level reading of
+     field and git binary path.
+     It is retrieved from config file, and we use a low level reading of
      that file instead of OpamStateConfig.safe_load to avoid multiple error
      messages displayed if an error is detected in the config file. If there is
      an error, or best effort notification, it will be highlighted after
@@ -596,6 +597,9 @@ let apply_global_options cli o =
               { pelem = String cygcheck; _}::_  ->
               let cygbin = Filename.dirname cygcheck in
               OpamCoreConfig.update ~cygbin ()
+            | Some { pelem = String "git-location"; _},
+              { pelem = String git_location; _}::_  ->
+              OpamCoreConfig.update ~git_location ()
             | _, element::elements -> aux (Some element) elements
           in
           aux None elements

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 ?git_location:_ () -> None else
+  fun ?git_location () ->
+    let header () = OpamConsole.header_msg "Git" in
+    let contains_git p =
+      OpamSystem.resolve_command ~env:[||] (Filename.concat p "git.exe")
+    in
+    let gits =
+      OpamStd.Env.get "PATH"
+      |> OpamStd.Sys.split_path_variable
+      |> OpamStd.List.filter_map (fun p ->
+          match contains_git p with
+          | Some git ->
+            Some (git, OpamSystem.bin_contains_bash p)
+          | None -> None)
+    in
+    let get_git_location ?git_location () =
+      let bin =
+        match git_location with
+        | Some _ -> git_location
+        | None ->
+          OpamConsole.read "Please enter the path containing git.exe (e.g. C:\\Program Files\\Git\\cmd):"
+      in
+      match bin with
+      | None -> None
+      | Some git_location ->
+        match contains_git git_location, OpamSystem.bin_contains_bash git_location with
+        | Some _, false ->
+          OpamConsole.msg "Using Git from %s" git_location;
+          Some git_location
+        | Some _, true ->
+          OpamConsole.error
+            "A bash executable was found in %s, which will override \
+             Cygwin's bash. Please check your binary path."
+            git_location;
+          None
+        | None, _ ->
+          OpamConsole.error "No Git executable found in %s." git_location;
+          None
+    in
+    let rec loop ?git_location () =
+      match get_git_location ?git_location () with
+      | Some _ as git_location -> git_location
+      | None -> menu ()
+    and menu () =
+      let prompt () =
+        let options =
+          (`Default, "Use default Cygwin Git")
+          :: (List.filter_map (fun (git, bash) ->
+              if bash then None else
+              let bin = Filename.dirname git in
+              Some (`Location bin, "Use found git in "^bin))
+              gits)
+          @ [
+            `Specify, "Enter the location of installed Git";
+            `Abort, "Abort initialisation to install recommended Git.";
+          ]
+        in
+        OpamConsole.menu "Which Git should opam use?"
+          ~default:`Default ~no:`Default ~options
+      in
+      match prompt () with
+      | `Default -> None
+      | `Specify -> loop ()
+      | `Location git_location -> loop ~git_location ()
+      | `Abort ->
+        OpamConsole.note "Once your choosen Git installed, open a new PowerShell or Command Prompt window, and relaunch opam init.";
+        OpamStd.Sys.exit_because `Aborted
+    in
+    let git_location =
+      match git_location with
+      | Some (Right ()) -> None
+      | Some (Left git_location) ->
+        header ();
+        get_git_location ~git_location:(OpamFilename.Dir.to_string git_location) ()
+      | None ->
+        if OpamStd.Sys.tty_out then
+          (header ();
+           OpamConsole.msg
+             "Cygwin Git is functional but can have credentials issues for private repositories, \
+              we recommend using:\n%s\n"
+             (OpamStd.Format.itemize (fun s -> s)
+                [ "Install via 'winget install Git.Git'";
+                  "Git for Windows can be downloaded and installed from https://gitforwindows.org" ]);
+           menu ())
+        else
+          None
+    in
+    OpamStd.Option.iter (fun _ ->
+        OpamConsole.msg
+          "You can change that later with \
+           'opam option \"git-location=C:\\A\\Path\\bin\"'")
+      git_location;
+    git_location
+
+let windows_checks ?cygwin_setup ?git_location config =
   let vars = OpamFile.Config.global_variables config in
   let env =
     List.map (fun (v, c, s) -> v, (lazy (Some c), s)) vars
     |> OpamVariable.Map.of_list
   in
+  (* Git handling *)
+  let git_location : string option = git_for_windows_check ?git_location () in
+  OpamCoreConfig.update ?git_location ();
+  let config =
+    match git_location with
+    | Some git_location ->
+      OpamFile.Config.with_git_location
+        (OpamFilename.Dir.of_string git_location) config
+    | None -> config
+  in
+  (* Cygwin handling *)
   let success cygcheck =
     let config =
       let os_distribution = OpamVariable.of_string "os-distribution" in
@@ -681,6 +787,16 @@ let windows_checks ?cygwin_setup config =
              OpamFilename.(Dir.to_string (dirname_dir (dirname cygcheck))));
       config
   in
+  let install_cygwin_tools () =
+    let packages =
+      match OpamSystem.resolve_command "git" with
+      | None -> OpamInitDefaults.required_packages_for_cygwin
+      | Some _ ->
+        List.filter (fun c -> not OpamSysPkg.(equal (of_string "git") c))
+          OpamInitDefaults.required_packages_for_cygwin
+    in
+    OpamSysInteract.Cygwin.install ~packages
+  in
   let header () = OpamConsole.header_msg "Unix support infrastructure" in
   let get_cygwin = function
     | Some cygcheck
@@ -777,10 +893,7 @@ let windows_checks ?cygwin_setup config =
         match prompt () with
         | `Abort -> OpamStd.Sys.exit_because `Aborted
         | `Internal ->
-          let cygcheck =
-            OpamSysInteract.Cygwin.install
-              ~packages:OpamInitDefaults.required_packages_for_cygwin
-          in
+          let cygcheck = install_cygwin_tools () in
           let config = success cygcheck in
           config
         | `Specify ->
@@ -819,9 +932,7 @@ let windows_checks ?cygwin_setup config =
              header ();
              let cygcheck =
                match setup with
-               | `internal ->
-                 OpamSysInteract.Cygwin.install
-                   ~packages:OpamInitDefaults.required_packages_for_cygwin
+               | `internal -> install_cygwin_tools ()
                | (`default_location | `location _ as setup) ->
                  let cygroot =
                    match setup with
@@ -861,10 +972,11 @@ let windows_checks ?cygwin_setup config =
       else
         config
   in
-  OpamCoreConfig.update
-    ?cygbin:OpamStd.Option.Op.(
-        OpamSysInteract.Cygwin.cygbin_opt config
-        >>| OpamFilename.Dir.to_string) ();
+  let cygbin = OpamStd.Option.Op.(
+      OpamSysInteract.Cygwin.cygbin_opt config
+      >>| OpamFilename.Dir.to_string)
+  in
+  OpamCoreConfig.update ?cygbin ();
   config
 
 let update_with_init_config ?(overwrite=false) config init_config =
@@ -898,11 +1010,12 @@ let update_with_init_config ?(overwrite=false) config init_config =
 
 let reinit ?(init_config=OpamInitDefaults.init_config()) ~interactive
     ?dot_profile ?update_config ?env_hook ?completion ?inplace
-    ?(check_sandbox=true) ?(bypass_checks=false) ?cygwin_setup
+    ?(check_sandbox=true) ?(bypass_checks=false)
+    ?cygwin_setup ?git_location
     config shell =
   let root = OpamStateConfig.(!r.root_dir) in
   let config = update_with_init_config config init_config in
-  let config = windows_checks ?cygwin_setup config in
+  let config = windows_checks ?cygwin_setup ?git_location config in
   let _all_ok =
     if bypass_checks then false else
       init_checks ~hard_fail_exn:false init_config
@@ -943,7 +1056,7 @@ let init
     ?repo ?(bypass_checks=false)
     ?dot_profile ?update_config ?env_hook ?(completion=true)
     ?(check_sandbox=true)
-    ?cygwin_setup
+    ?cygwin_setup ?git_location
     shell =
   log "INIT %a"
     (slog @@ OpamStd.Option.to_string OpamRepositoryBackend.to_string) repo;
@@ -979,7 +1092,7 @@ let init
             init_config |>
           OpamFile.Config.with_repositories (List.map fst repos)
         in
-        let config = windows_checks ?cygwin_setup config in
+        let config = windows_checks ?cygwin_setup ?git_location config in
 
         let dontswitch =
           if bypass_checks then false else

src/client/opamClient.mli

@@ -29,6 +29,7 @@ val init:
   ?completion:bool ->
   ?check_sandbox:bool ->
   ?cygwin_setup: [ `internal | `default_location | `location of dirname | `no ] ->
+  ?git_location:(dirname, unit) either ->
   shell ->
   rw global_state * unlocked repos_state * atom list
 
@@ -46,6 +47,7 @@ val reinit:
   ?update_config:bool -> ?env_hook:bool -> ?completion:bool -> ?inplace:bool ->
   ?check_sandbox:bool -> ?bypass_checks:bool ->
   ?cygwin_setup: [ `internal | `default_location | `location of dirname | `no ] ->
+  ?git_location:(dirname, unit) either ->
   OpamFile.Config.t -> shell -> unit
 
 (** Install the given list of packages. [add_to_roots], if given, specifies that

src/client/opamClientConfig.mli

@@ -165,4 +165,5 @@ val opam_init:
   ?merged_output:bool ->
   ?precise_tracking:bool ->
   ?cygbin:string ->
+  ?git_location:string ->
   unit -> unit

src/client/opamCommands.ml

@@ -327,12 +327,31 @@ let init cli =
     else
       Term.const None
   in
+  let git_location =
+    if Sys.win32 then
+      mk_opt ~cli (cli_from ~experimental:true cli2_2)
+        ["git-location"] "DIR"
+        "Specify git binary directory. \
+         Ensure that it doesn't contains bash in the same directory"
+        Arg.(some dirname) None
+    else
+      Term.const None
+  in
+  let no_git_location =
+    if Sys.win32 then
+      mk_flag ~cli (cli_from ~experimental:true cli2_2)
+        ["no-git-location"]
+        "Don't specify nor ask to specify git binary directory."
+    else
+      Term.const false
+  in
+
   let init global_options
       build_options repo_kind repo_name repo_url
       interactive update_config completion env_hook no_sandboxing shell
       dot_profile_o compiler no_compiler config_file no_config_file reinit
       show_opamrc bypass_checks
-      cygwin_internal cygwin_location
+      cygwin_internal cygwin_location git_location no_git_location
       () =
     apply_global_options cli global_options;
     apply_build_options cli build_options;
@@ -401,6 +420,15 @@ let init cli =
       | (`default_location | `none), Some dir -> Some (`location dir)
       | (`internal | `default_location | `no) as setup, None -> Some setup
     in
+    let git_location =
+      match git_location, no_git_location with
+      | Some _, true ->
+        OpamConsole.error_and_exit `Bad_arguments
+          "Options --no-git-location and --git-location are incompatible";
+      | None, false -> None
+      | Some d, false -> Some (Left d)
+      | None, true -> Some (Right ())
+    in
     if already_init then
       if reinit then
         let init_config =
@@ -410,7 +438,7 @@ let init cli =
         let reinit conf =
           OpamClient.reinit ~init_config ~interactive ?dot_profile
             ?update_config ?env_hook ?completion ~inplace ~bypass_checks
-            ~check_sandbox:(not no_sandboxing) ?cygwin_setup
+            ~check_sandbox:(not no_sandboxing) ?cygwin_setup ?git_location
             conf shell
         in
         let config =
@@ -450,7 +478,7 @@ let init cli =
         ?repo ~bypass_checks ?dot_profile
         ?update_config ?env_hook ?completion
         ~check_sandbox:(not no_sandboxing)
-        ?cygwin_setup
+        ?cygwin_setup ?git_location
         shell
     in
     OpamStd.Exn.finally (fun () -> OpamRepositoryState.drop rt)
@@ -499,7 +527,7 @@ let init cli =
           $setup_completion $env_hook $no_sandboxing $shell_opt cli
           cli_original $dot_profile_flag cli cli_original $compiler
           $no_compiler $config_file $no_config_file $reinit $show_default_opamrc
-          $bypass_checks $cygwin_internal $cygwin_location)
+          $bypass_checks $cygwin_internal $cygwin_location $git_location $no_git_location)
 
 (* LIST *)
 let list_doc = "Display the list of available packages."