From bbe345a5e369836d81a70d241699eca780d6aa40 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sat, 4 May 2024 09:45:12 +0100 Subject: [PATCH 01/31] Harmonise the warning overriding git-location --- src/client/opamClient.ml | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index e768ca879fc..55b4aefeb0b 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -840,16 +840,14 @@ let windows_checks ?cygwin_setup ?git_location config = | Some (Right ()), None -> Some (Right ()) | Some (Right ()), Some _ -> OpamConsole.note - "As '--no-git-location' is given in argument, ignoring field \ - 'git-location' in opamrc"; + "'--no-git-location' specified; field 'git-location' in opamrc \ + has been ignored"; Some (Right ()) | Some (Left gl), None | None, Some gl -> Some (Left gl) - | Some (Left gl_cli), Some gl_config -> + | Some (Left gl_cli), Some _ -> OpamConsole.note - "Git location defined in opamrc '%s' and via CLI \ - ('--git-location' option, %s). Keeping last one." - (OpamFilename.Dir.to_string gl_config) - (OpamFilename.Dir.to_string gl_cli) ; + "'--git-location' specified; field 'git-location' in opamrc \ + has been ignored"; Some (Left gl_cli) in let git_location = From ae4281b9a626f9f6190144d973ef5caef82a7cb9 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sat, 4 May 2024 21:00:21 +0100 Subject: [PATCH 02/31] Expose OpamEnv.cygwin_non_shadowed_programs --- master_changes.md | 1 + src/state/opamEnv.ml | 6 ++++-- src/state/opamEnv.mli | 6 ++++++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/master_changes.md b/master_changes.md index 8829c2cf28b..d4c7e7ba5f9 100644 --- a/master_changes.md +++ b/master_changes.md @@ -175,6 +175,7 @@ users) * `OpamDownload.download_command`: separate output from stdout and stderr [#5984 @kit-ty-kate] ## opam-state + * `OpamEnv.cygwin_non_shadowed_programs`: exposes the list of executables (not including git) which should always come from Cygwin [#6000 @dra27] ## opam-solver diff --git a/src/state/opamEnv.ml b/src/state/opamEnv.ml index 8a407fc9fbf..9b6e0990387 100644 --- a/src/state/opamEnv.ml +++ b/src/state/opamEnv.ml @@ -306,6 +306,9 @@ let rezip ?insert (l1, l2) = let rezip_to_string ?insert z = join_var (rezip ?insert z) +let cygwin_non_shadowed_programs = + ["bash.exe"; "make.exe"; "sort.exe"; "tar.exe"] + let apply_op_zip ~sepfmt var op arg (rl1,l2 as zip) = let arg = transform_format ~sepfmt var arg in let empty_tr = { tr_entry = ""; tr_raw = ""; tr_sep = arg.tr_sep } in @@ -314,8 +317,7 @@ let apply_op_zip ~sepfmt var op arg (rl1,l2 as zip) = Sys.file_exists (Filename.concat dir item) in let shadow_list = - List.filter (contains_in arg) - ["bash.exe"; "make.exe"; "sort.exe"; "tar.exe"; "git.exe"] + List.filter (contains_in arg) ("git.exe" :: cygwin_non_shadowed_programs) in let rec loop acc = function | [] -> acc, [arg] diff --git a/src/state/opamEnv.mli b/src/state/opamEnv.mli index 6ac0bc82986..98e0bcd3ee5 100644 --- a/src/state/opamEnv.mli +++ b/src/state/opamEnv.mli @@ -61,6 +61,12 @@ val hash_env_updates: ('a, euok_writeable) env_update list -> string and optionally the given updates *) val get_pure: ?updates:(spf_resolved, euok_internal) env_update list -> unit -> env +(** The list of executables from Cygwin which must not be allowed to be shadowed + by other directories of PATH. This list must not contain git.exe - it is + added only if Cygwin's git is installed. The list is used to determine the + furthest point in PATH that Cygwin's bin directory can be placed. *) +val cygwin_non_shadowed_programs : string list + (** Update an environment, including reverting opam changes that could have been previously applied (therefore, don't apply to an already updated env as returned by e.g. [get_full]!) *) From 8acd5ac59b3ba942e963a5646cd4a2dfea53fc69 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sun, 19 May 2024 18:20:09 +0100 Subject: [PATCH 03/31] Abort if --git-location doesn't contain Git Previously, --git-location was treated as a default answer for the Git menu, so it would display an error if Git wasn't found, but then simply exit and fallback to adding Git to the Cygwin installation. Fix this and validate either --git-location argument or the git-location opamrc field as referring to a directory which contains Git. --- master_changes.md | 1 + src/client/opamClient.ml | 38 ++++++++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/master_changes.md b/master_changes.md index d4c7e7ba5f9..f8afd7805e8 100644 --- a/master_changes.md +++ b/master_changes.md @@ -28,6 +28,7 @@ users) * Skip Git-for-Windows menu if the Git binary resolved in PATH is Git-for-Windows [#5963 @dra27 - fix #5835] * Enhance the Git menu by warning if the user appears to need to restart the shell to pick up PATH changes [#5963 @dra27] * Include Git for Windows installations in the list of possibilities where the user instructed Git-for-Windows setup not to update PATH [#5963 @dra27] + * [BUG] Fail if `--git-location` points to a directory not containing git [#6000 @dra27] ## Config report diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index 55b4aefeb0b..c23713d14a1 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -817,6 +817,15 @@ let git_for_windows ?git_location () = git_location; git_location +let check_git_location_or_exit git_location source = + let git = + Filename.concat (OpamFilename.Dir.to_string git_location) "git.exe" + in + if OpamSystem.resolve_command ~env:[||] git = None then + OpamConsole.error_and_exit `Not_found + "The location specified with %s does not appear to contain a Git \ + executable!" source + let windows_checks ?cygwin_setup ?git_location config = if (not (Unix.has_symlink ())) then begin OpamConsole.header_msg "Windows Developer Mode"; @@ -837,18 +846,23 @@ let windows_checks ?cygwin_setup ?git_location config = let git_location = match git_location, OpamFile.Config.git_location config with | None, None -> None - | Some (Right ()), None -> Some (Right ()) - | Some (Right ()), Some _ -> - OpamConsole.note - "'--no-git-location' specified; field 'git-location' in opamrc \ - has been ignored"; - Some (Right ()) - | Some (Left gl), None | None, Some gl -> Some (Left gl) - | Some (Left gl_cli), Some _ -> - OpamConsole.note - "'--git-location' specified; field 'git-location' in opamrc \ - has been ignored"; - Some (Left gl_cli) + | Some (Right ()), git_location_opamrc -> + if git_location_opamrc <> None then + OpamConsole.note + "'--no-git-location' specified; field 'git-location' in opamrc has \ + been ignored"; + git_location + | None, Some git_location -> + check_git_location_or_exit git_location + "the 'git-location' field in opamrc"; + Some (Left git_location) + | (Some (Left git_location)) as result, git_location_opamrc -> + if git_location_opamrc <> None then + OpamConsole.note + "'--git-location' specified; field 'git-location' in opamrc has been \ + ignored"; + check_git_location_or_exit git_location "--git-location"; + result in let git_location = if Sys.win32 then From b7d35826fd642e09cf1bc5632a1a386ebde25436 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Wed, 29 May 2024 22:18:14 +0100 Subject: [PATCH 04/31] Add a function to enumerate Registry values --- master_changes.md | 1 + src/core/opamStubs.dummy.ml | 1 + src/core/opamStubs.mli | 6 ++ src/stubs/win32/opamWin32Stubs.ml | 1 + src/stubs/win32/opamWindows.c | 105 ++++++++++++++++++++++++++++++ 5 files changed, 114 insertions(+) diff --git a/master_changes.md b/master_changes.md index f8afd7805e8..15c39a0094a 100644 --- a/master_changes.md +++ b/master_changes.md @@ -196,3 +196,4 @@ users) * `OpamStd.Sys.resolve_command`: extracted the logic from `OpamSystem.resolve_command`, without the default environment handling from OpamProcess. [#5991 @dra27] * `OpamStd.Sys.resolve_in_path`: split the logic of `OpamStd.Sys.resolve_command` to allow searching for an arbitrary file in the search path [#5991 @dra27] * `OpamProcess.run_background`: name the stderr output file have the .err extension when cmd_stdout is given [#5984 @kit-ty-kate] + * `OpamStubs.enumRegistry`: on Windows, retrieves all the values of a given type from a registry key, with their names [#6000 @dra27] diff --git a/src/core/opamStubs.dummy.ml b/src/core/opamStubs.dummy.ml index 44474257d54..a4aff833433 100644 --- a/src/core/opamStubs.dummy.ml +++ b/src/core/opamStubs.dummy.ml @@ -24,6 +24,7 @@ let getWindowsVersion = that's_a_no_no let getArchitecture = that's_a_no_no let waitpids _ = that's_a_no_no let readRegistry _ _ _ = that's_a_no_no +let enumRegistry _ _ = that's_a_no_no let writeRegistry _ _ _ = that's_a_no_no let getConsoleOutputCP = that's_a_no_no let getCurrentConsoleFontEx _ = that's_a_no_no diff --git a/src/core/opamStubs.mli b/src/core/opamStubs.mli index 9f51a528f1c..726baf12226 100644 --- a/src/core/opamStubs.mli +++ b/src/core/opamStubs.mli @@ -70,6 +70,12 @@ val readRegistry : registry_root -> string -> string -> 'a registry_value -> 'a @raise Failure If the value in the registry does not have [value_type] *) +val enumRegistry : registry_root -> string -> 'a registry_value -> (string * 'a) list + (** Windows only. [enumRegistry root key value_type] reads all the values + from registry key [key] of [root] which have type [value_type]. + + Returns [[]] if the key is not found. *) + val writeRegistry : registry_root -> string -> string -> 'a registry_value -> 'a -> unit (** Windows only. [writeRegistry root key name value_type value] sets the diff --git a/src/stubs/win32/opamWin32Stubs.ml b/src/stubs/win32/opamWin32Stubs.ml index d556b1f1063..dc4c2f7ef34 100644 --- a/src/stubs/win32/opamWin32Stubs.ml +++ b/src/stubs/win32/opamWin32Stubs.ml @@ -23,6 +23,7 @@ external getWindowsVersion : unit -> int * int * int * int = "OPAMW_GetWindowsVe external getArchitecture : unit -> 'a = "OPAMW_GetArchitecture" external waitpids : int list -> int -> int * Unix.process_status = "OPAMW_waitpids" external readRegistry : 'a -> string -> string -> 'b -> 'c option = "OPAMW_ReadRegistry" +external enumRegistry : 'a -> string -> 'b -> (string * 'c) list = "OPAMW_RegEnumValue" external writeRegistry : 'a -> string -> string -> 'b -> 'c -> unit = "OPAMW_WriteRegistry" external getConsoleOutputCP : unit -> int = "OPAMW_GetConsoleOutputCP" external getCurrentConsoleFontEx : 'a -> bool -> 'b = "OPAMW_GetCurrentConsoleFontEx" diff --git a/src/stubs/win32/opamWindows.c b/src/stubs/win32/opamWindows.c index e30f2ed8413..2621a11dde5 100644 --- a/src/stubs/win32/opamWindows.c +++ b/src/stubs/win32/opamWindows.c @@ -423,6 +423,111 @@ CAMLprim value OPAMW_ReadRegistry(value hKey, value sub_key, CAMLreturn(result); } +CAMLprim value OPAMW_RegEnumValue(value hKey, value sub_key, value value_type) +{ + CAMLparam0(); + CAMLlocal5(result, tail, v, v_name, v_data); + value cell; + + LPWSTR lpEnvironment; + + result = caml_alloc_small(2, 0); + Field(result, 0) = Val_int(0); /* Unused */ + Field(result, 1) = Val_emptylist; /* The actual result */ + tail = result; + + HKEY key; + DWORD type; + LSTATUS ret; + DWORD index = 0; + LPWSTR lpValueName = NULL; + DWORD cbValueName; + LPBYTE lpData = NULL; + DWORD cbData; + LPWSTR lpSubKey; + + if (!caml_string_is_c_safe(sub_key)) + caml_invalid_argument("OPAMW_RegEnumValue"); + + if (!(lpSubKey = caml_stat_strdup_to_utf16(String_val(sub_key)))) { + caml_raise_out_of_memory(); + } + + ret = RegOpenKey(roots[Int_val(hKey)], lpSubKey, &key); + if (ret == ERROR_SUCCESS) + ret = RegQueryInfoKey(key, NULL, NULL, NULL, NULL, NULL, NULL, NULL, &cbValueName, &cbData, NULL, NULL); + + if (ret == ERROR_SUCCESS) { + caml_stat_free(lpSubKey); + /* Cases match OpamStubsTypes.registry_value */ + switch (Int_val(value_type)) { + case 0: + type = REG_SZ; + break; + default: + RegCloseKey(key); + caml_failwith("OPAMW_RegEnumValue: value not implemented"); + break; + } + + if (ret == ERROR_SUCCESS) { + cbValueName++; + if ((lpData = malloc(cbData)) == NULL) { + caml_raise_out_of_memory(); + } else if ((lpValueName = malloc(cbValueName * sizeof(WCHAR))) == NULL) { + free(lpData); + caml_raise_out_of_memory(); + } + + DWORD valuename_len = cbValueName; + DWORD value_len = cbData; + DWORD value_type; + + while (ret == ERROR_SUCCESS) { + valuename_len = cbValueName; + value_len = cbData; + ret = RegEnumValue(key, index++, lpValueName, &valuename_len, NULL, &value_type, lpData, &value_len); + if (ret == ERROR_SUCCESS) { + if (type == value_type) { + value_len /= 2; /* bytes -> characters */ + if (((wchar_t *)lpData)[value_len - 1] == 0) + value_len--; /* remove NULL terminator */ + int len = win_wide_char_to_multi_byte((wchar_t *)lpData, value_len, NULL, 0); + v_data = caml_alloc_string(len); + win_wide_char_to_multi_byte((wchar_t *)lpData, value_len, (char *)String_val(v_data), len); + len = win_wide_char_to_multi_byte(lpValueName, valuename_len, NULL, 0); + v_name = caml_alloc_string(len); + win_wide_char_to_multi_byte(lpValueName, valuename_len, (char *)String_val(v_name), len); + v = caml_alloc_small(2, 0); + Field(v, 0) = v_name; + Field(v, 1) = v_data; + cell = caml_alloc_small(2, 0); + Field(cell, 0) = v; + Field(cell, 1) = Val_emptylist; + Store_field(tail, 1, cell); + tail = Field(tail, 1); + } + } + } + if (ret == ERROR_NO_MORE_ITEMS) + ret = ERROR_SUCCESS; + + free(lpData); + free(lpValueName); + } + + RegCloseKey(key); + } else { + caml_stat_free(lpSubKey); + } + + if (ret != ERROR_SUCCESS && ret != ERROR_FILE_NOT_FOUND) { + caml_failwith("OPAMW_RegEnumValue"); + } + + CAMLreturn(Field(result, 1)); +} + CAMLprim value OPAMW_WriteRegistry(value hKey, value sub_key, value value_name, From 41b8f73c2e01b6c635e11c668270723c8e8f0049 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Tue, 4 Jun 2024 11:49:37 +0100 Subject: [PATCH 05/31] Simplify cygbin check in OpamAction --- src/client/opamAction.ml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/client/opamAction.ml b/src/client/opamAction.ml index 7b309d29cf4..da544b35502 100644 --- a/src/client/opamAction.ml +++ b/src/client/opamAction.ml @@ -528,9 +528,8 @@ let compilation_env t opam = (OpamFile.OPAM.build_env opam) in let cygwin_env = - match OpamSysInteract.Cygwin.cygbin_opt t.switch_global.config with + match OpamCoreConfig.(!r.cygbin) with | Some cygbin -> - let cygbin = OpamFilename.Dir.to_string cygbin in [ OpamTypesBase.env_update_resolved "PATH" Cygwin cygbin ~comment:"Cygwin path" ] @ (match OpamCoreConfig.(!r.git_location) with From 21dd668e3e1a9c8640cd23d6dcfcdc6fc8ffb3dd Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Tue, 4 Jun 2024 12:01:31 +0100 Subject: [PATCH 06/31] Change the calculation for installing git Rather than filtering git out of the package list, instead add it when required. --- master_changes.md | 1 + src/client/opamClient.ml | 10 +++++----- src/client/opamInitDefaults.ml | 1 - 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/master_changes.md b/master_changes.md index 15c39a0094a..056b835803d 100644 --- a/master_changes.md +++ b/master_changes.md @@ -171,6 +171,7 @@ users) * `OpamClient.init` and `OpamClient.reinit`: now can have additional cygwin packages to install [#5930 @moyodiallo] * Expose `OpamSolution.print_depext_msg` [#5994 @dra27] * Extracted `OpamSolution.install_sys_packages` from `OpamSolution.install_depexts` [#5994 @dra27] + * `OpamInitDefaults.required_packages_for_cygwin`: no longer includes git; as the need to add that is computed in `OpamClient` [#6000 @dra27] ## opam-repository * `OpamDownload.download_command`: separate output from stdout and stderr [#5984 @kit-ty-kate] diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index c23713d14a1..5ed8990d898 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -955,12 +955,12 @@ let windows_checks ?cygwin_setup ?git_location config = config in let install_cygwin_tools packages = + let default_packages = OpamInitDefaults.required_packages_for_cygwin in let default_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 + if OpamSystem.resolve_command "git" = None then + OpamSysPkg.of_string "git" :: default_packages + else + default_packages in (* packages comes last so that the user can override any potential version constraints in default_packages (although, with the current version of diff --git a/src/client/opamInitDefaults.ml b/src/client/opamInitDefaults.ml index 608ac078fb7..6b5a7017fbf 100644 --- a/src/client/opamInitDefaults.ml +++ b/src/client/opamInitDefaults.ml @@ -149,7 +149,6 @@ let required_tools ~sandboxing () = let required_packages_for_cygwin = [ "diffutils"; - "git"; (* XXX hg & mercurial ? *) "make"; "patch"; "tar"; From 9966e1672e5f688844aa0a71dfb5ea211392408f Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Tue, 4 Jun 2024 12:03:53 +0100 Subject: [PATCH 07/31] Add OpamCompat.Seq.find_map --- master_changes.md | 1 + src/core/opamCompat.ml | 18 ++++++++++++++++++ src/core/opamCompat.mli | 5 +++++ 3 files changed, 24 insertions(+) diff --git a/master_changes.md b/master_changes.md index 056b835803d..567026b04ff 100644 --- a/master_changes.md +++ b/master_changes.md @@ -198,3 +198,4 @@ users) * `OpamStd.Sys.resolve_in_path`: split the logic of `OpamStd.Sys.resolve_command` to allow searching for an arbitrary file in the search path [#5991 @dra27] * `OpamProcess.run_background`: name the stderr output file have the .err extension when cmd_stdout is given [#5984 @kit-ty-kate] * `OpamStubs.enumRegistry`: on Windows, retrieves all the values of a given type from a registry key, with their names [#6000 @dra27] + * `OpamCompat`: add `Seq.find_map` from OCaml 4.14 [#6000 @dra27] diff --git a/src/core/opamCompat.ml b/src/core/opamCompat.ml index 216376563aa..70a668691dc 100644 --- a/src/core/opamCompat.ml +++ b/src/core/opamCompat.ml @@ -23,6 +23,24 @@ module String = struct include Stdlib.String end +module Seq = struct + [@@@warning "-32"] + + (** NOTE: OCaml >= 4.14 *) + let rec find_map f xs = + match xs() with + | Seq.Nil -> + None + | Seq.Cons (x, xs) -> + match f x with + | None -> + find_map f xs + | Some _ as result -> + result + + include Seq +end + module Either = struct (** NOTE: OCaml >= 4.12 *) type ('a, 'b) t = diff --git a/src/core/opamCompat.mli b/src/core/opamCompat.mli index 9596aedbb82..e22c4cad784 100644 --- a/src/core/opamCompat.mli +++ b/src/core/opamCompat.mli @@ -13,6 +13,11 @@ module String : sig val exists: (char -> bool) -> string -> bool end +module Seq : sig + (* NOTE: OCaml >= 4.14 *) + val find_map: ('a -> 'b option) -> 'a Seq.t -> 'b option +end + module Either : sig (* NOTE: OCaml >= 4.12 *) type ('a, 'b) t = From 70f4e34178c06e14bbda661a999c78d5a841f795 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Tue, 4 Jun 2024 12:33:54 +0100 Subject: [PATCH 08/31] Overhaul OpamStd.Sys Cygwin functions Make the ~cygbin parameter to the various executable identification functions optional. The parameter is renamed to search_in_first with a slightly revised semantics - if cygcheck.exe is not found in the directory, then PATH is still searched. This has a key benefit early on in opam init, as it allows cygbin to be set for an internal Cygwin installation which has not yet happened, but still permits curl (et al) to be used from PATH (e.g. when running opam init from an MSYS2 shell). Where before ~cygbin implied that cygcheck _must_ be used from the directory, the new parameter effectively prepends an additional directory to PATH. --- master_changes.md | 1 + src/core/opamProcess.ml | 2 +- src/core/opamStd.ml | 52 +++++++++++++++++++++++++++-------------- src/core/opamStd.mli | 29 ++++++++++++----------- src/core/opamSystem.ml | 4 ++-- 5 files changed, 54 insertions(+), 34 deletions(-) diff --git a/master_changes.md b/master_changes.md index 567026b04ff..2927b712b32 100644 --- a/master_changes.md +++ b/master_changes.md @@ -199,3 +199,4 @@ users) * `OpamProcess.run_background`: name the stderr output file have the .err extension when cmd_stdout is given [#5984 @kit-ty-kate] * `OpamStubs.enumRegistry`: on Windows, retrieves all the values of a given type from a registry key, with their names [#6000 @dra27] * `OpamCompat`: add `Seq.find_map` from OCaml 4.14 [#6000 @dra27] + * `OpamStd.Sys.{get_windows_executable_variant,get_cygwin_variant,is_cygwin_variant}`: renamed `~cygbin` to `?search_in_path` with a change in semantics so that it acts as though the directory was simply the first entry in PATH [#6000 @dra27] diff --git a/src/core/opamProcess.ml b/src/core/opamProcess.ml index a13a641c2f0..2d810728ed2 100644 --- a/src/core/opamProcess.ml +++ b/src/core/opamProcess.ml @@ -313,7 +313,7 @@ let create_process_env = fun cmd -> if OpamStd.Option.map_default (OpamStd.Sys.is_cygwin_variant - ~cygbin:(OpamCoreConfig.(!r.cygbin))) + ?search_in_first:(OpamCoreConfig.(!r.cygbin))) false (resolve_command cmd) then cygwin_create_process_env cmd diff --git a/src/core/opamStd.ml b/src/core/opamStd.ml index 97d6e257f11..899f63f876a 100644 --- a/src/core/opamStd.ml +++ b/src/core/opamStd.ml @@ -1358,40 +1358,56 @@ module OpamSys = struct in check_dll `Native in - fun ~cygbin name -> - match cygbin with - | Some cygbin -> - (let cygcheck = Filename.concat cygbin "cygcheck.exe" in - if Filename.is_relative name then - requires_cygwin cygcheck name - else - try Hashtbl.find results (cygcheck, name) - with Not_found -> - let result = requires_cygwin cygcheck name in - Hashtbl.add results (cygcheck, name) result; - result) + fun ?search_in_first name -> + let cygcheck = + let open Option.Op in + let contains_cygcheck dir = + let cygcheck = Filename.concat dir "cygcheck.exe" in + if Sys.file_exists cygcheck then + Some cygcheck + else + None + in + search_in_first >>= contains_cygcheck + >>+ fun () -> + (* ~search_in_first not supplied, or cygcheck.exe not found in it; + now try general PATH-resolution. *) + match resolve_command "cygcheck.exe" with + | `Cmd cmd -> Some cmd + | `Not_found | `Denied -> None + in + match cygcheck with | None -> `Native + | Some cygcheck -> + if Filename.is_relative name then + requires_cygwin cygcheck name + else + try Hashtbl.find results (cygcheck, name) + with Not_found -> + let result = requires_cygwin cygcheck name in + Hashtbl.add results (cygcheck, name) result; + result else - fun ~cygbin:_ _ -> `Native + fun ?search_in_first:_ _ -> `Native - let get_cygwin_variant ~cygbin cmd = + let get_cygwin_variant ?search_in_first cmd = (* Treat MSYS2's variant of `cygwin1.dll` called `msys-2.0.dll` equivalently. Confer https://www.msys2.org/wiki/How-does-MSYS2-differ-from-Cygwin/ *) - match get_windows_executable_variant ~cygbin cmd with + match get_windows_executable_variant ?search_in_first cmd with | `Native -> `Native | `Cygwin | `Msys2 -> `Cygwin | `Tainted _ -> `CygLinked - let is_cygwin_variant ~cygbin cmd = - get_cygwin_variant ~cygbin cmd = `Cygwin + let is_cygwin_variant ?search_in_first cmd = + get_cygwin_variant ?search_in_first cmd = `Cygwin let is_cygwin_cygcheck_t ~variant ~cygbin = match cygbin with | Some cygbin -> let cygpath = Filename.concat cygbin "cygpath.exe" in Sys.file_exists cygpath - && (variant ~cygbin:(Some cygbin) cygpath = `Cygwin) + && (variant ?search_in_first:(Some cygbin) cygpath = `Cygwin) | None -> false let is_cygwin_variant_cygcheck ~cygbin = diff --git a/src/core/opamStd.mli b/src/core/opamStd.mli index 960bd4fb114..c0111260411 100644 --- a/src/core/opamStd.mli +++ b/src/core/opamStd.mli @@ -565,11 +565,15 @@ module Sys : sig to a library which is itself Cygwin- or MSYS2-compiled, or [`Native] otherwise. - Note that this returns [`Native] on a Cygwin-build of opam! + If supplied, [~search_in_first] specifies a directory which should be + searched for cygcheck prior to searching the current PATH. - Both cygcheck and an unqualified command will be resolved if necessary - using the current PATH. *) - val get_windows_executable_variant: cygbin:string option -> + If the command given is not an absolute path, it too is resolved in the + current PATH. + + If cygcheck cannot be resolved in PATH, or when running the Cygwin build + of opam, the function returns `Native. *) + val get_windows_executable_variant: ?search_in_first:string -> string -> [ `Native | `Cygwin | `Tainted of [ `Msys2 | `Cygwin] | `Msys2 ] (** Determines if cygcheck in given cygwin binary directory comes from a @@ -581,18 +585,17 @@ module Sys : sig (Cygwin, Msys2). *) val is_cygwin_variant_cygcheck : cygbin:string option -> bool - (** For native Windows builds, returns [`Cygwin] if the command is a Cygwin- - or Msys2- compiled executable, and [`CygLinked] if the command links to a - library which is itself Cygwin/Msys2-compiled, or [`Native] otherwise. + (** Behaviour is largely as {!get_windows_executable_variant} but where MSYS2 + and Cygwin are seen as equivalent. - Note that this returns [`Native] on a Cygwin-build of opam! - - Both cygcheck and an unqualified command will be resolved using the - current PATH. *) - val get_cygwin_variant: cygbin:string option -> string -> [ `Native | `Cygwin | `CygLinked ] + For native Windows builds, returns [`Cygwin] if the command is a Cygwin- + or Msys2- compiled executable, and [`CygLinked] if the command links to a + library which is itself Cygwin/Msys2-compiled, or [`Native] otherwise. *) + val get_cygwin_variant: + ?search_in_first:string -> string -> [ `Native | `Cygwin | `CygLinked ] (** Returns true if [get_cygwin_variant] is [`Cygwin] *) - val is_cygwin_variant: cygbin:string option -> string -> bool + val is_cygwin_variant: ?search_in_first:string -> string -> bool (** {3 Exit handling} *) diff --git a/src/core/opamSystem.ml b/src/core/opamSystem.ml index 428271cf8c3..5fbd1a2518f 100644 --- a/src/core/opamSystem.ml +++ b/src/core/opamSystem.ml @@ -444,7 +444,7 @@ let get_cygpath_function = lazy ( if OpamStd.Option.map_default (OpamStd.Sys.is_cygwin_variant - ~cygbin:(OpamCoreConfig.(!r.cygbin))) + ?search_in_first:(OpamCoreConfig.(!r.cygbin))) false (resolve_command command) then apply_cygpath @@ -794,7 +794,7 @@ let install ?(warning=default_install_warning) ?exec src dst = copy_file_aux ~src ~dst (); if cygcheck then match OpamStd.Sys.get_windows_executable_variant - ~cygbin:OpamCoreConfig.(!r.cygbin) dst with + ?search_in_first:OpamCoreConfig.(!r.cygbin) dst with | `Native -> () | (`Cygwin | `Msys2 | `Tainted _) as code -> warning dst code end else From 0c40ce7d3370c37a371afae57ca81e57ae0254f4 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Wed, 5 Jun 2024 08:42:29 +0100 Subject: [PATCH 09/31] Detect both Cygwin and MSYS2 os-distribution Previously, MSYS2 required os-distribution to be overridden in global-variables. Now, as with Cygwin, it is inferred in the same way from cygcheck. In this commit, opam init still sets os-distribution, however, if it is manually removed from the root config file, previously `opam var os-distribution` would return `win32` but now returns `msys2`. Additionally, sys_pkg_manager_cmd is handled in the same way for MSYS2 as for Cygwin, allowing MSYS2 to be automatically added in the same way as Cygwin. --- master_changes.md | 1 + src/client/opamClient.ml | 5 ++++- src/state/opamGlobalState.ml | 19 ++++++++----------- src/state/opamSysInteract.ml | 28 ++++++++++++---------------- src/state/opamSysInteract.mli | 5 ++--- src/state/opamSysPoll.ml | 15 ++++++++------- 6 files changed, 35 insertions(+), 38 deletions(-) diff --git a/master_changes.md b/master_changes.md index 2927b712b32..31d3c8407d7 100644 --- a/master_changes.md +++ b/master_changes.md @@ -95,6 +95,7 @@ users) * Pass --no-version-check to Cygwin setup (suppresses a message box if setup needs updating) [#5830 @dra27] * Pass --quiet-mode noinput to stop the user interrupting the setup GUI [#5830 @dra27] * Always pass --no-write-registry to the Cygwin installer, not just on first installation [#5995 @dra27] + * os-distribution is now by default calculated from cygpath for Cygwin and MSYS2, instead of needing to be set by opam init [#6000 @dra27] ## Format upgrade * Handle init OCaml `sys-ocaml-*` eval variables during format upgrade from 2.0 -> 2.1 -> 2.2 [#5829 @dra27] diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index 5ed8990d898..d560abb4cf0 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -1096,7 +1096,10 @@ let windows_checks ?cygwin_setup ?git_location config = because there are commands opam requires which are only provided using it (patch, etc.). MSYS2 avoids this by requiring os-distribution to be set. *) - let cygcheck = OpamSysInteract.Cygwin.cygcheck_opt config in + let cygcheck = + OpamStd.String.Map.find_opt "cygwin" + (OpamFile.Config.sys_pkg_manager_cmd config) + in (match cygwin_setup with | None -> get_cygwin cygcheck | Some setup -> diff --git a/src/state/opamGlobalState.ml b/src/state/opamGlobalState.ml index 02edf6cd924..b647416cdfa 100644 --- a/src/state/opamGlobalState.ml +++ b/src/state/opamGlobalState.ml @@ -41,17 +41,14 @@ let load_config lock_kind global_lock root = (* Update Cygwin variants cygbin *) let cygbin = let config = fst config in - match OpamSysInteract.Cygwin.cygbin_opt config with - | Some cygbin -> Some (OpamFilename.Dir.to_string cygbin) - | None -> - if List.exists (function - | (v, S "msys2", _) -> - String.equal (OpamVariable.to_string v) "os-distribution" - | _ -> false) (OpamFile.Config.global_variables config) - then - OpamStd.Option.map Filename.dirname - (OpamSystem.resolve_command "cygcheck") - else None + let cygwin = OpamSysInteract.Cygwin.cygbin_opt config in + let cygbin = + if cygwin = None then + OpamSysInteract.Cygwin.msys2bin_opt config + else + cygwin + in + Option.map OpamFilename.Dir.to_string cygbin in OpamCoreConfig.update ?cygbin (); config diff --git a/src/state/opamSysInteract.ml b/src/state/opamSysInteract.ml index 0a780db827f..7fcf5c72c47 100644 --- a/src/state/opamSysInteract.ml +++ b/src/state/opamSysInteract.ml @@ -101,22 +101,16 @@ module Commands = struct OpamStd.String.Map.find_opt family (OpamFile.Config.sys_pkg_manager_cmd config) - let get_cmd config family = - match get_cmd_opt config family with - | Some cmd -> cmd - | None -> - let field = "sys-pkg-manager-cmd" in - Printf.ksprintf failwith - "Config field '%s' must be set for '%s'. \ - Use opam option --global '%s+=[\"%s\" \"\"]'" - field family field family family - - let msys2 config = OpamFilename.to_string (get_cmd config "msys2") - let cygwin_t = "cygwin" - let cygcheck_opt config = get_cmd_opt config cygwin_t - let cygcheck config = OpamFilename.to_string (get_cmd config cygwin_t) + let msys2_t = "msys2" + let msys2 config = + let override = get_cmd_opt config msys2_t in + OpamStd.Option.map_default OpamFilename.to_string "pacman.exe" override + + let cygcheck config = + let override = get_cmd_opt config cygwin_t in + OpamStd.Option.map_default OpamFilename.to_string "cygcheck.exe" override end (* Please keep this alphabetically ordered, in the type definition, and in @@ -228,10 +222,12 @@ module Cygwin = struct let setupexe = "setup-x86_64.exe" let cygcheckexe = "cygcheck.exe" - let cygcheck_opt = Commands.cygcheck_opt open OpamStd.Option.Op let cygbin_opt config = - cygcheck_opt config + Commands.(get_cmd_opt config cygwin_t) + >>| OpamFilename.dirname + let msys2bin_opt config = + Commands.(get_cmd_opt config msys2_t) >>| OpamFilename.dirname let cygroot_opt config = cygbin_opt config diff --git a/src/state/opamSysInteract.mli b/src/state/opamSysInteract.mli index 754e975a49e..a697e98f6ff 100644 --- a/src/state/opamSysInteract.mli +++ b/src/state/opamSysInteract.mli @@ -69,10 +69,9 @@ module Cygwin : sig (* Return Cygwin binary path *) val cygbin_opt: OpamFile.Config.t -> OpamFilename.Dir.t option - (* Return Cygwin cygcheck.exe path *) - val cygcheck_opt: OpamFile.Config.t -> OpamFilename.t option - (* Return Cygwin installation prefix *) val cygroot_opt: OpamFile.Config.t -> OpamFilename.Dir.t option + (* Return MSYS2 binary path *) + val msys2bin_opt: OpamFile.Config.t -> OpamFilename.Dir.t option end diff --git a/src/state/opamSysPoll.ml b/src/state/opamSysPoll.ml index 2930d973d77..ebcfb2ffcd5 100644 --- a/src/state/opamSysPoll.ml +++ b/src/state/opamSysPoll.ml @@ -114,14 +114,15 @@ let poll_os_distribution () = try Scanf.sscanf s " %s " norm with Scanf.Scan_failure _ -> linux) | Some "win32" -> - (* If the user provides a Cygwin installation in PATH, by default we'll use - it. Note that this is _not_ done for MSYS2. *) - let cygwin = - OpamSystem.resolve_command "cygcheck" - >>| Filename.dirname - |> (fun cygbin -> OpamStd.Sys.is_cygwin_cygcheck ~cygbin) + let kind = + OpamStd.Sys.get_windows_executable_variant + ?search_in_first:(OpamCoreConfig.(!r.cygbin)) "cygpath.exe" in - if cygwin then Some "cygwin" else os + begin match kind with + | `Msys2 -> Some "msys2" + | `Cygwin -> Some "cygwin" + | `Native | `Tainted _ -> os + end | os -> os let os_distribution = Lazy.from_fun poll_os_distribution From 8b58875a22f4d9fbe9c349c994c0e2edc72eb6da Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Wed, 5 Jun 2024 10:55:28 +0100 Subject: [PATCH 10/31] Ensure Cygwin setup is downloaded and up-to-date --- master_changes.md | 1 + src/client/opamSolution.ml | 4 ++ src/state/opamSysInteract.ml | 76 +++++++++++++++++++++++++++++------- 3 files changed, 66 insertions(+), 15 deletions(-) diff --git a/master_changes.md b/master_changes.md index 31d3c8407d7..d76696a6236 100644 --- a/master_changes.md +++ b/master_changes.md @@ -96,6 +96,7 @@ users) * Pass --quiet-mode noinput to stop the user interrupting the setup GUI [#5830 @dra27] * Always pass --no-write-registry to the Cygwin installer, not just on first installation [#5995 @dra27] * os-distribution is now by default calculated from cygpath for Cygwin and MSYS2, instead of needing to be set by opam init [#6000 @dra27] + * Cygwin setup is now always downloaded and updated both for internal and external Cygwin installations [#6000 @dra27] ## Format upgrade * Handle init OCaml `sys-ocaml-*` eval variables during format upgrade from 2.0 -> 2.1 -> 2.2 [#5829 @dra27] diff --git a/src/client/opamSolution.ml b/src/client/opamSolution.ml index cdbee8b0d66..b0adf8673ea 100644 --- a/src/client/opamSolution.ml +++ b/src/client/opamSolution.ml @@ -1189,6 +1189,8 @@ let install_sys_packages ~map_sysmap ~confirm env config sys_packages t = | `Ignore -> bypass t | `Quit -> give_up_msg (); OpamStd.Sys.exit_because `Aborted and print_command sys_packages = + if OpamSysPoll.os_distribution env = Some "cygwin" then + OpamSysInteract.Cygwin.check_setup None; let commands = OpamSysInteract.install_packages_commands ~env config sys_packages |> List.map (fun ((`AsAdmin c | `AsUser c), a) -> c::a) @@ -1219,6 +1221,8 @@ let install_sys_packages ~map_sysmap ~confirm env config sys_packages t = | `Quit -> give_up () and auto_install t sys_packages = try + if OpamSysPoll.os_distribution env = Some "cygwin" then + OpamSysInteract.Cygwin.check_setup None; OpamSysInteract.install ~env config sys_packages; (* handles dry_run *) map_sysmap (fun _ -> OpamSysPkg.Set.empty) t with Failure msg -> diff --git a/src/state/opamSysInteract.ml b/src/state/opamSysInteract.ml index 7fcf5c72c47..129bec2f4e5 100644 --- a/src/state/opamSysInteract.ml +++ b/src/state/opamSysInteract.ml @@ -252,7 +252,20 @@ module Cygwin = struct let download_setupexe dst = let overwrite = true in + let checksum_file = OpamFilename.add_extension dst "sha512" in + let current_checksum = + if OpamFilename.exists checksum_file then + Some (OpamHash.sha512 (OpamFilename.read checksum_file)) + else + None + in let open OpamProcess.Job.Op in + log "Downloading Cygwin setup checksums"; + if OpamConsole.disp_status_line () then + if OpamFilename.exists dst then + OpamConsole.status_line "Checking if Cygwin setup is up-to-date" + else + OpamConsole.status_line "Downloading Cygwin setup from cygwin.com"; OpamFilename.with_tmp_dir_job @@ fun dir -> OpamDownload.download ~overwrite url_setupexe_sha512 dir @@+ fun file -> let checksum = @@ -272,7 +285,31 @@ module Cygwin = struct try Some (OpamHash.sha512 Re.(Group.get (exec re content) 1)) with Not_found -> None in - OpamDownload.download_as ~overwrite ?checksum url_setupexe dst + let kind = `SHA512 in + if OpamStd.Option.equal OpamHash.equal current_checksum checksum && + OpamFilename.exists dst && + OpamStd.Option.equal OpamHash.equal current_checksum + (Some (OpamHash.compute ~kind (OpamFilename.to_string dst))) then begin + log "Up-to-date"; + OpamConsole.clear_status (); + Done () + end else begin + log "Downloading setup-x86_64.exe"; + if OpamConsole.disp_status_line () then + OpamConsole.status_line "Downloading Cygwin setup from cygwin.com"; + OpamDownload.download_as ~overwrite ?checksum url_setupexe dst @@+ + fun () -> + OpamFilename.remove checksum_file; + let checksum = + match checksum with + | None -> OpamHash.compute ~kind (OpamFilename.to_string dst) + | Some sha512 -> sha512 + in + OpamFilename.with_open_out_bin checksum_file (fun c -> + output_string c (OpamHash.contents checksum)); + OpamConsole.clear_status (); + Done () + end let set_fstab_noacl = let orig = "binary," in @@ -389,16 +426,21 @@ module Cygwin = struct (* Set setup.exe in the good place, ie in .opam/.cygwin/ *) let check_setup setup = let dst = cygsetup () in - if OpamFilename.exists dst then () else - (match setup with - | Some setup -> - log "Copying %s into %s" - (OpamFilename.to_string setup) - (OpamFilename.to_string dst); - OpamFilename.copy ~src:setup ~dst - | None -> - log "Donwloading setup exe"; - OpamProcess.Job.run @@ download_setupexe dst) + match setup with + | Some setup -> + log "Copying %s into %s" + (OpamFilename.to_string setup) + (OpamFilename.to_string dst); + let sha512 = + OpamHash.compute ~kind:`SHA512 (OpamFilename.to_string setup) + in + OpamFilename.copy ~src:setup ~dst; + let checksum_file = OpamFilename.add_extension dst "sha512" in + OpamFilename.remove checksum_file; + OpamFilename.with_open_out_bin checksum_file @@ fun c -> + output_string c (OpamHash.contents sha512) + | None -> + OpamProcess.Job.run @@ download_setupexe dst end let yum_cmd = lazy begin @@ -1066,8 +1108,9 @@ let install ?env config packages = commands let update ?(env=OpamVariable.Map.empty) config = + let family = family ~env () in let cmd = - match family ~env () with + match family with | Alpine -> Some (`AsAdmin "apk", ["update"]) | Arch -> Some (`AsAdmin "pacman", ["-Sy"]) | Centos -> Some (`AsAdmin (Lazy.force yum_cmd), ["makecache"]) @@ -1086,9 +1129,12 @@ let update ?(env=OpamVariable.Map.empty) config = in match cmd with | None -> - OpamConsole.warning - "Unknown update command for %s, skipping system update" - OpamStd.Option.Op.(OpamSysPoll.os_family env +! "unknown") + if family = Cygwin then + Cygwin.check_setup None + else + OpamConsole.warning + "Unknown update command for %s, skipping system update" + OpamStd.Option.Op.(OpamSysPoll.os_family env +! "unknown") | Some (cmd, args) -> try sudo_run_command ~env cmd args with Failure msg -> failwith ("System package update " ^ msg) From 740978c69acab532a1444907b675d83c59b3638e Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Wed, 5 Jun 2024 11:00:07 +0100 Subject: [PATCH 11/31] De-label OpamSysInteract.Cygwin.install Single argument function with a non-base type. --- master_changes.md | 1 + src/client/opamClient.ml | 2 +- src/state/opamSysInteract.ml | 2 +- src/state/opamSysInteract.mli | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/master_changes.md b/master_changes.md index d76696a6236..034eda70d91 100644 --- a/master_changes.md +++ b/master_changes.md @@ -180,6 +180,7 @@ users) ## opam-state * `OpamEnv.cygwin_non_shadowed_programs`: exposes the list of executables (not including git) which should always come from Cygwin [#6000 @dra27] + * `opamSysInteract.Cygwin.install`: de-label `packages` argument [#6000 @dra27] ## opam-solver diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index d560abb4cf0..7e6bd6be08f 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -966,7 +966,7 @@ let windows_checks ?cygwin_setup ?git_location config = constraints in default_packages (although, with the current version of setup, and with the list of default_packages in OpamInitDefaults, this at present doesn't matter too much). *) - OpamSysInteract.Cygwin.install ~packages:(default_packages @ packages) + OpamSysInteract.Cygwin.install (default_packages @ packages) in let header () = OpamConsole.header_msg "Unix support infrastructure" in diff --git a/src/state/opamSysInteract.ml b/src/state/opamSysInteract.ml index 129bec2f4e5..feadb89107c 100644 --- a/src/state/opamSysInteract.ml +++ b/src/state/opamSysInteract.ml @@ -320,7 +320,7 @@ module Cygwin = struct OpamFilename.with_open_out_bin_atomic fstab (fun oc -> Stdlib.output_string oc content) - let install ~packages = + let install packages = let open OpamProcess.Job.Op in let cygwin_root = internal_cygroot () in let cygwin_bin = cygwin_root / "bin" in diff --git a/src/state/opamSysInteract.mli b/src/state/opamSysInteract.mli index a697e98f6ff..239e530aa24 100644 --- a/src/state/opamSysInteract.mli +++ b/src/state/opamSysInteract.mli @@ -47,7 +47,7 @@ module Cygwin : sig val default_cygroot: string (* Install an internal Cygwin install, in /.cygwin *) - val install: packages:OpamSysPkg.t list -> OpamFilename.t + val install: OpamSysPkg.t list -> OpamFilename.t (* [check_install ~variant path] checks a Cygwin install at [path]. It checks that 'path\cygcheck.exe', 'path\bin\cygcheck.exe', or From cc579bce40bd12e938dfec478525ce26e86acd5b Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Wed, 5 Jun 2024 17:23:13 +0100 Subject: [PATCH 12/31] Expand Cygwin.check_install to analyse_install Includes more checks and also returns the kind of installation which was found. --- master_changes.md | 1 + src/client/opamClient.ml | 18 ++--- src/state/opamSysInteract.ml | 139 ++++++++++++++++++++++------------ src/state/opamSysInteract.mli | 23 ++++-- 4 files changed, 114 insertions(+), 67 deletions(-) diff --git a/master_changes.md b/master_changes.md index 034eda70d91..d0ad23988a3 100644 --- a/master_changes.md +++ b/master_changes.md @@ -181,6 +181,7 @@ users) ## opam-state * `OpamEnv.cygwin_non_shadowed_programs`: exposes the list of executables (not including git) which should always come from Cygwin [#6000 @dra27] * `opamSysInteract.Cygwin.install`: de-label `packages` argument [#6000 @dra27] + * `OpamSysInteract.Cygwin.check_install` renamed to `analyse_install` which now also returns whether the installation found was MSYS2 or Cygwin [#6000 @dra27] ## opam-solver diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index 7e6bd6be08f..6159b7ecfd4 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -1003,9 +1003,8 @@ let windows_checks ?cygwin_setup ?git_location config = in (* Check for default cygwin installation path *) let default = - match OpamSysInteract.Cygwin.(check_install - ~variant:true default_cygroot) with - | Ok cygcheck -> + match OpamSysInteract.Cygwin.(analyse_install default_cygroot) with + | Ok (_, cygcheck) -> let prompt_cygroot () = let options = [ `manual, @@ -1036,10 +1035,10 @@ let windows_checks ?cygwin_setup ?git_location config = | None -> None | Some entry -> let cygcheck = - OpamSysInteract.Cygwin.check_install ~variant:true entry + OpamSysInteract.Cygwin.analyse_install entry in match cygcheck with - | Ok cygcheck -> Some cygcheck + | Ok (_, cygcheck) -> Some cygcheck | Error msg -> OpamConsole.error "%s" msg; None in (* And finally ask for setup.exe *) @@ -1113,9 +1112,8 @@ let windows_checks ?cygwin_setup ?git_location config = | `default_location -> OpamSysInteract.Cygwin.default_cygroot | `location dir -> OpamFilename.Dir.to_string dir in - (match OpamSysInteract.Cygwin.check_install ~variant:true - cygroot with - | Ok cygcheck -> cygcheck + (match OpamSysInteract.Cygwin.analyse_install cygroot with + | Ok (_, cygcheck) -> cygcheck | Error msg -> OpamConsole.error_and_exit `Not_found "Error while checking %sCygwin install (%s): %s" @@ -1131,9 +1129,9 @@ let windows_checks ?cygwin_setup ?git_location config = (* We check that current install is good *) (match OpamSysInteract.Cygwin.cygroot_opt config with | Some cygroot -> - (match OpamSysInteract.Cygwin.check_install ~variant:true + (match OpamSysInteract.Cygwin.analyse_install (OpamFilename.Dir.to_string cygroot) with - | Ok cygcheck -> + | Ok (_, cygcheck) -> OpamSysInteract.Cygwin.check_setup None; success cygcheck | Error err -> OpamConsole.error "%s" err; get_cygwin None) diff --git a/src/state/opamSysInteract.ml b/src/state/opamSysInteract.ml index feadb89107c..4fdd739cf95 100644 --- a/src/state/opamSysInteract.ml +++ b/src/state/opamSysInteract.ml @@ -8,8 +8,6 @@ (* *) (**************************************************************************) -open OpamTypes - let log fmt = OpamConsole.log "XSYS" fmt (* Run commands *) @@ -232,10 +230,15 @@ module Cygwin = struct let cygroot_opt config = cygbin_opt config >>| OpamFilename.dirname_dir - let get_opt = function + let cygroot config = + match cygroot_opt config with | Some c -> c - | None -> failwith "Cygwin install not found" - let cygroot config = get_opt (cygroot_opt config) + | None -> + match OpamSystem.resolve_command "cygcheck.exe" with + | Some cygcheck -> + OpamFilename.dirname_dir (OpamFilename.Dir.of_string (Filename.dirname cygcheck)) + | None -> + failwith "Cygwin install not found" let internal_cygwin = let internal = @@ -375,53 +378,89 @@ module Cygwin = struct let default_cygroot = "C:\\cygwin64" - let check_install ~variant path = - let is_cygwin = - if variant then OpamStd.Sys.is_cygwin_variant_cygcheck - else OpamStd.Sys.is_cygwin_cygcheck - in - if not (Sys.file_exists path) then - Error (Printf.sprintf "%s not found!" path) - else if Filename.basename path = "cygcheck.exe" then - (* We have cygcheck.exe path *) - let cygbin = Some (Filename.dirname path) in - if is_cygwin ~cygbin then - Ok (OpamFilename.of_string path) - else - Error - (Printf.sprintf - "%s found, but it is not from a Cygwin installation" - path) - else if not (Sys.is_directory path) then - Error (Printf.sprintf "%s is not a directory" path) - else - (* We have cygroot alike path *) - let bin = Filename.concat path "bin" in - let usr_bin = Filename.concat (Filename.concat path "usr") "bin" in - let check cygbin = - if Sys.file_exists cygbin then - if is_cygwin ~cygbin:(Some cygbin) then - Some (Left (OpamFilename.of_string - (Filename.concat cygbin "cygcheck.exe"))) - else - Some (Right cygbin) + let analysis_cache = Hashtbl.create 17 + + let analyse_install path = + let cygbin = + if not (Sys.file_exists path) then + Error (path ^ " not found!") + else if Filename.remove_extension (Filename.basename path) + = "cygcheck" then + (* path refers to cygcheck directly *) + Ok (Filename.dirname path) + else if not (Sys.is_directory path) then + Error (Printf.sprintf "%s neither a directory nor cygcheck.exe" path) else - None + (* path is a directory - search path, path\bin and path\usr\bin *) + let contains_cygcheck dir = + Sys.file_exists (Filename.concat dir "cygcheck.exe") + in + let tests = [ + path; (* e.g. C:\cygwin64\bin / C:\msys64\usr\bin *) + Filename.concat path "bin"; (* e.g. C:\cygwin64 *) + Filename.concat (Filename.concat path "usr") "bin" (* e.g. C:\msys64 *) + ] in + match List.filter contains_cygcheck tests with + | [] -> + Error (Printf.sprintf + "cygcheck.exe not found in %s, or subdirectories \ + bin and usr\\bin" path) + | _::_::_ -> + Error (Printf.sprintf + "cygcheck.exe found in multiple places in %s which suggests \ + it is not a Cygwin/MSYS2 installation" path) + | [path] -> + Ok path + in + let identify dir = + try Hashtbl.find analysis_cache dir + with Not_found -> + let result = + let cygpath = Filename.concat dir "cygpath.exe" in + if not (Sys.file_exists cygpath) then + Error (Printf.sprintf + "cygcheck.exe found in %s, but cygpath.exe was not" dir) + else + match OpamStd.Sys.get_windows_executable_variant + ~search_in_first:dir cygpath with + | `Native | `Tainted _ -> + Error (Printf.sprintf + "cygcheck.exe found in %s; but it does not appear \ + to be part of a Cygwin or MSYS2 installation" dir) + | (`Msys2 | `Cygwin) as kind -> + (* Check that pacman.exe is present with MSYS2: it is typically + not present with a Git-for-Windows Git Bash session, and as + these are basically unusable (they don't have all the required + tools, and we have no package manager with which to add them), + it's better to exclude them). *) + if kind = `Msys2 + && not (Sys.file_exists (Filename.concat dir "pacman.exe")) then + Error (Printf.sprintf + "cygcheck.exe found in %s, which appears to be from \ + an MSYS2 installation, but pacman.exe was not" dir) + else + let r = + OpamProcess.run + (OpamProcess.command ~name:(OpamSystem.temp_file "command") + ~allow_stdin:false cygpath ["-w"; "--"; "/"]) + in + OpamProcess.cleanup ~force:true r; + if OpamProcess.is_success r then + match r.OpamProcess.r_stdout with + | [] -> + Error ("Unexpected error translating \"/\" with " ^ cygpath) + | _::_ -> + let cygcheck = + OpamFilename.of_string (Filename.concat dir "cygpath.exe") + in + Ok (kind, cygcheck) + else + Error ("Could not determine the root for " ^ cygpath) + in + Hashtbl.add analysis_cache dir result; + result in - (* We need to keep that order, to have a better error message *) - match check bin, check usr_bin with - | Some (Left cygcheck), _ | _, Some (Left cygcheck) -> - Ok cygcheck - | Some (Right cygbin), _ | _, Some (Right cygbin) -> - Error - (Printf.sprintf - "%s found, but it does not appear to be a Cygwin installation" - cygbin) - | _, None -> - Error - (Printf.sprintf - "cygcheck.exe not found in %s subdirectories bin or usr\bin" - path) + Result.bind cygbin identify (* Set setup.exe in the good place, ie in .opam/.cygwin/ *) let check_setup setup = diff --git a/src/state/opamSysInteract.mli b/src/state/opamSysInteract.mli index 239e530aa24..1acd0d32c7f 100644 --- a/src/state/opamSysInteract.mli +++ b/src/state/opamSysInteract.mli @@ -49,13 +49,22 @@ module Cygwin : sig (* Install an internal Cygwin install, in /.cygwin *) val install: OpamSysPkg.t list -> OpamFilename.t - (* [check_install ~variant path] checks a Cygwin install at [path]. It checks - that 'path\cygcheck.exe', 'path\bin\cygcheck.exe', or - 'path\usr\bin\cygcheck.exe' exists. - If [~variant] is false, checks that it is strictly a Cygwin install, - otherwise a Cygwin-like install as MSYS2. *) - val check_install: - variant:bool -> string -> (OpamFilename.t, string) result + (* [analyse_install path] searches for and identifies Cygwin/MSYS2 + installations. [path] may be able the location of cygcheck.exe itself + (with or without the .exe) or just a directory. If [path] is just a + directory, then the function searches for 'path\cygcheck.exe', + 'path\bin\cygcheck.exe', or 'path\usr\bin\cygcheck.exe'. If exactly one + is found, and cygpath.exe is found with it, then cygpath is used both to + identify whether the installation is Cygwin or MSYS2 and to translate the + root directory [/] to its Windows path (i.e. to get the canonical root + directory of the installation). MSYS2 is additionally required to have + pacman.exe in the same directory as cygcheck.exe and cygpath.exe. + + On success, the result is the kind of installation (Cygwin/MSYS2) along + with the full path to cygcheck.exe, otherwise a description of the problem + encountered is returned. *) + val analyse_install: + string -> ([ `Cygwin | `Msys2 ] * OpamFilename.t, string) result (* Returns true if Cygwin install is internal *) val is_internal: OpamFile.Config.t -> bool From ab47e449f348b93eb364ae8d12542ad1949c2027 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sun, 2 Jun 2024 12:17:27 +0100 Subject: [PATCH 13/31] Supercalifragilisticexpialidocious --- master_changes.md | 1 + src/client/opamClient.ml | 1116 ++++++++++++++++++++++----------- src/core/opamStd.ml | 14 - src/core/opamStd.mli | 9 - src/state/opamSysInteract.ml | 12 +- src/state/opamSysInteract.mli | 15 +- 6 files changed, 769 insertions(+), 398 deletions(-) diff --git a/master_changes.md b/master_changes.md index d0ad23988a3..fa44bd7e445 100644 --- a/master_changes.md +++ b/master_changes.md @@ -131,6 +131,7 @@ users) ## Client * Fix rounding error when displaying the timestamp in debug mode [#5912 @kit-ty-kate - fix #5910] + * Overhaul Windows `opam init` to determine Unix and Git configuration simultaneously, and to detect from Cygwin, Git and MSYS2 from all the known package managers and shells [#6000 @dra27] ## Shell diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index 6159b7ecfd4..a88def0c95f 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -635,7 +635,7 @@ 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 is_git_for_windows git = +let rec is_git_for_windows git = (* The resource file compiled for Git for Windows sets the ProductVersion string to M.m.r.windows.b where M.m.r is the git version and b is the revision number of Git for Windows. This differentiates it from very old @@ -652,21 +652,60 @@ let is_git_for_windows git = try Scanf.sscanf version "%u.%u.%u.windows.%u%!" (fun _ _ _ _ -> true) with Scanf.Scan_failure _ | Failure _ | End_of_file -> false end - | _ -> false + | _ -> + (* The Scoop package manager installs a shim git.exe (see + https://github.com/ScoopInstaller/Shim) which will fail our test of the + version information block, while actually being Git for Windows. + If git.shim and scoop.cmd are found with git.exe and we can parse the + path line from git.shim, then we test the executable pointed to instead. + *) + let dir = Filename.dirname git in + let git_shim = Filename.concat dir "git.shim" in + let scoop = Filename.concat dir "scoop.cmd" in + let find_path_value (key, value) = + if String.trim key = "path" then + Some (String.trim value) + else + None + in + let test_scoop_shim s = + let new_git = + let last = String.length s - 1 in + if last > 0 && s.[0] = '"' && s.[last] = '"' then + String.sub s 1 (last - 1) + else s + in + log "%s appears to be a Scoop shim; trying %s" git new_git; + is_git_for_windows new_git + in + if Sys.file_exists git_shim && Sys.file_exists scoop then + OpamSystem.read git_shim + |> String.split_on_char '\n' + |> List.filter_map (Fun.flip OpamStd.String.cut_at '=') + |> OpamStd.List.find_map_opt find_path_value + |> OpamStd.Option.map_default test_scoop_shim false + else + false + +let string_of_kind = function + | `Msys2 -> "MSYS2" + | `Cygwin -> "Cygwin" -let git_for_windows ?git_location () = - let header () = OpamConsole.header_msg "Git" in +let git_for_windows kind mechanism cygwin_is_tweakable = 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 -> + |> OpamStd.List.fold_left_map (fun gits p -> match contains_git p with - | Some git -> + | Some git when not (OpamStd.String.Set.mem git gits) -> + OpamStd.String.Set.add git gits, Some (git, OpamSystem.bin_contains_bash p) - | None -> None) + | _ -> gits, None) OpamStd.String.Set.empty + |> snd + |> List.filter_map Fun.id in let abort_action = "install Git for Windows." in let gits, gfw_message, abort_action = @@ -681,7 +720,7 @@ let git_for_windows ?git_location () = [], Some "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.", - "restart your shell." + "restart your shell." | _ -> (* Git is neither in the current nor the initial PATH. There is one further possibility: the user may have installed Git for Windows @@ -692,7 +731,7 @@ let git_for_windows ?git_location () = in PATH, but also gives the opportunity to use the git-location mechanism to select it for opam's internal use. *) let test_for_installation ((gits, gfw_message, abort_action) as acc) - (hive, key) = + (hive, key) = let process root = let git_location = Filename.concat root "cmd" in let git = Filename.concat git_location "git.exe" in @@ -726,7 +765,8 @@ let git_for_windows ?git_location () = (* User-specific installation *) (OpamStubsTypes.HKEY_CURRENT_USER, "Software"); ] in - List.fold_left test_for_installation (gits, None, abort_action) installations + List.fold_left test_for_installation + (gits, None, abort_action) installations else gits, None, abort_action in @@ -735,12 +775,15 @@ let git_for_windows ?git_location () = match git_location with | Some _ -> git_location | None -> - OpamConsole.read "Please enter the path containing git.exe (e.g. C:\\Program Files\\Git\\cmd):" + 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 + 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 @@ -754,68 +797,114 @@ let git_for_windows ?git_location () = OpamConsole.error "No Git executable found in %s." git_location; None in + let options = + (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 your Git installation"; + `Abort, ("Abort initialisation to " ^ abort_action); + ] + in + let default, options = + match mechanism with + | `Internal -> + assert cygwin_is_tweakable; + let internal = + `Default, Printf.sprintf + "Install Git with along with %s internally" (string_of_kind kind) + in + `Default, internal::options + | `Root root -> + assert cygwin_is_tweakable; + let root = OpamFilename.Dir.to_string root in + let git = Filename.concat root "git.exe" in + let prefix = + if OpamSystem.resolve_command ~env:[||] git = None then + "Add Git to" + else + "Use Git from" + in + let root = + `Default, Printf.sprintf + "%s the %s installation in %s" prefix (string_of_kind kind) root + in + `Default, root::options + | `Path root -> + match OpamSystem.resolve_command "git.exe" with + | Some git -> + let options = + if Filename.dirname git = root then + let option = + `Default, Printf.sprintf + "Use %s Git from the installation at %s in PATH" + (string_of_kind kind) root + in + option::options + else + (`Default, Printf.sprintf "Use Git from PATH")::options + in + `Default, options + | None -> + if cygwin_is_tweakable then + let option = + `Default, Printf.sprintf + "Add Git to %s installation in %s (from PATH)" + (string_of_kind kind) root + in + `Default, option::options + else + (fst (List.hd options)), options + in let rec loop ?git_location () = match get_git_location ?git_location () with - | Some _ as git_location -> git_location + | Some _ as git_location -> git_location, false | 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 " ^ abort_action); - ] - in OpamStd.Option.iter (OpamConsole.warning "%s\n") gfw_message; OpamConsole.menu "Which Git should opam use?" - ~default:`Default ~no:`Default ~options + ~default ~no:default ~options in match prompt () with - | `Default -> None + | `Default -> None, cygwin_is_tweakable | `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."; + OpamConsole.note + "Once your chosen Git is 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 -> - let git_found = - match OpamSystem.resolve_command "git" with - | None -> false - | Some git -> is_git_for_windows git - in - if not git_found && 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 + let git_location, use_cygwin = + let git_found = + match OpamSystem.resolve_command "git" with + | None -> false + | Some git -> is_git_for_windows git + in + if not git_found && OpamStd.Sys.tty_out then + (OpamConsole.header_msg "Git"; + 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, not git_found && cygwin_is_tweakable 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 + Option.map OpamFilename.Dir.of_string git_location, use_cygwin let check_git_location_or_exit git_location source = let git = @@ -826,23 +915,369 @@ let check_git_location_or_exit git_location source = "The location specified with %s does not appear to contain a Git \ executable!" source -let windows_checks ?cygwin_setup ?git_location config = +(* Default search mechanisms for Cygwin/MSYS2 *) +let cygwin_searches = [ + `Cygwins + (OpamStubsTypes.HKEY_LOCAL_MACHINE, "SOFTWARE\\Cygwin\\Installations"); + `Cygwins + (OpamStubsTypes.HKEY_CURRENT_USER, "Software\\Cygwin\\Installations"); + `Test "C:\\cygwin64"; + `Test "C:\\msys64"; + `Msys2_generic; + `ScoopMsys2; +] + +(* cygwin_searches is a sequence of `Path and `Test mechanisms based on the + cygwin_searches list above. If specified, the ~first parameter allows a + different first mechanism to be returned. *) +let cygwin_searches ?first () = + let cygwin_searches = + match first with + | Some first -> first::cygwin_searches + | None -> cygwin_searches + in + let rec seq searches () = + match searches with + | ((`Path | `Test _) as search)::searches -> + (* Return the next mechanism *) + Seq.Cons(search, seq searches) + | (`Cygwins (hive, key))::searches -> + (* Search the given registry hive key for Cygwin locations *) + let possibles = OpamStubs.enumRegistry hive key OpamStubsTypes.REG_SZ in + let map (_, path) = + let path = + if OpamStd.String.starts_with ~prefix:"\\??\\" path then + String.sub path 4 (String.length path - 4) + else + path + in + `Test path + in + seq (List.map map possibles @ searches) () + | `ScoopMsys2::searches -> + (* Scoop installs an msys2.cmd shim in PATH. If this is encountered, parse + it. *) + begin match OpamStd.Sys.resolve_in_path "msys2.cmd" with + | None -> + seq searches () + | Some msys2 -> + let re = + Re.(compile @@ seq [ + bos; + str "@\""; + group @@ rep @@ diff any (char '"'); + char '"'; + rep any; + str " -msys2"; + alt [char ' '; eos] + ]) + in + let parse_line s = + Stdlib.Option.bind (Re.exec_opt re s) (Fun.flip Re.Group.get_opt 1) + in + let msys2_shell = + OpamSystem.read msys2 + |> String.split_on_char '\n' + |> OpamStd.List.find_map_opt parse_line + in + match msys2_shell with + | None -> + seq searches () + | Some msys2_shell -> + Seq.Cons(`Test (Filename.dirname msys2_shell), seq searches) + end + | `Msys2_generic::searches -> + (* Some package managers put the root msys64 directory into PATH, in which + case there will be msys2.exe - if that can be resolved in PATH, try + that. *) + begin match OpamSystem.resolve_command "msys2.exe" with + | None -> + seq searches () + | Some msys2 -> + Seq.Cons(`Test (Filename.dirname msys2), seq searches) + end + | [] -> Seq.Nil + in + seq cygwin_searches + +let rec cygwin_menu header = + let start = Unix.gettimeofday () in + let test_mechanism (roots, count, mechanisms) search = + match test_mechanism header search with + | Some ((kind, `Root root) as mechanism) -> + if OpamFilename.Dir.Set.mem root roots then + roots, count, mechanisms + else + let roots = OpamFilename.Dir.Set.add root roots in + let mechanisms = + (`Chosen mechanism, + Printf.sprintf + "Use %s installation found in %s" + (string_of_kind kind) + (OpamFilename.Dir.to_string root))::mechanisms + in + let count = succ count in + if OpamConsole.disp_status_line () + && Unix.gettimeofday () -. start >= 0.5 then + OpamConsole.status_line + "Searching for Cygwin/MSYS2 installations: %d found so far" count; + roots, count, mechanisms + | _ -> roots, count, mechanisms + in + let detected = + let _, _, mechanisms = + Seq.fold_left test_mechanism + (OpamFilename.Dir.Set.empty, 0, []) (cygwin_searches ()) + in + List.rev mechanisms + in + OpamConsole.clear_status (); + let internal_option = `Chosen (`Cygwin, `Internal) in + let options = + (internal_option, + "Automatically create an internal Cygwin installation that will be \ + managed by opam (recommended)") :: + (detected @ + [`Specify, "Use an" ^ (if detected = [] then "" else "other") ^ + " existing Cygwin/MSYS2 installation"; + `Abort, "Abort initialisation"]) + in + let options, default, warn_path = + (* First of all see if cygcheck can be found in PATH *) + let cygcheck = + OpamSystem.resolve_command "cygcheck.exe" + |> Option.map OpamSysInteract.Cygwin.analyse_install + in + begin match cygcheck with + | Some (Error _) | None -> + (* cygcheck wasn't in PATH, so default to the internal installation *) + options, `Chosen (`Cygwin, `Internal), None + | Some (Ok (kind, root)) -> + let pacman = + OpamFilename.Op.(root / "usr" / "bin" // "pacman.exe") + |> OpamFilename.to_string + in + let root = OpamFilename.Dir.to_string root in + let path_option = `Chosen (kind, `Path root) in + let options = + (path_option, Printf.sprintf + "Use tools found in PATH (%s installation at %s)" + (string_of_kind kind) root)::options + in + (* Check whether cygcheck is still available in the initial environment. + This allows a warning to be displayed reminding the user to continue + running opam from a Cygwin/MSYS2 shell that has been manually started, + but is not displayed if they have permanently configured their PATH to + include Cygwin/MSYS2. *) + let env = OpamStubs.get_initial_environment () in + let cygcheck = + OpamSystem.resolve_command ~env:(Array.of_list env) "cygcheck.exe" + |> Option.map OpamSysInteract.Cygwin.analyse_install + in + begin match cygcheck with + | Some (Ok (kind2, root2)) -> + let root2 = OpamFilename.Dir.to_string root2 in + if (kind : [`Cygwin | `Msys2]) = kind2 && String.equal root root2 then + let default, warning = + if kind = `Msys2 && OpamSystem.resolve_command pacman = None then + internal_option, Some + (Printf.sprintf + "The current PATH gives an installation of MSYS2 at %s, \ + but it does not include the package manager, \ + pacman.exe (this is expected behaviour for the Git \ + Bash shell from Git for Windows). It's recommended \ + you use a full MSYS2 installation, rather than one \ + without its package manager." root) + else + path_option, None + in + options, default, warning + else + let warning = Printf.sprintf + "The current PATH gives an installation of %s at %s, but your \ + system appears to default to an installation of %s at %s for \ + new terminal sessions. You will need to ensure that the \ + correct installation is available in PATH when you run opam \ + in future." + (string_of_kind kind) root (string_of_kind kind2) root2 + in + options, internal_option, Some warning + | Some (Error _) -> + let warning = Printf.sprintf + "The current PATH gives an installation of %s at %s, but it \ + doesn't appear to be correctly available for new terminal \ + sessions. You will need to ensure that the correct \ + installation is available in PATH when you run opam in \ + future." (string_of_kind kind) root + in + options, internal_option, Some warning + | None -> + match OpamStd.Sys.guess_shell_compat () with + | SH_sh | SH_bash | SH_zsh | SH_csh | SH_fish -> + let default, warning = + if kind = `Msys2 && OpamSystem.resolve_command pacman = None then + internal_option, Printf.sprintf + "The current PATH gives an installation of MSYS2 at %s, \ + but it does not include the package manager, pacman.exe \ + (this is expected behaviour for the Git Bash shell from \ + Git for Windows). It's recommended you use a full MSYS2 \ + installation, rather than one without its package \ + manager.\n You will need to run opam from a terminal \ + session in future." + root + else + path_option, Printf.sprintf + "You will need to run opam from a terminal session for %s \ + in future." root + in + options, default, Some warning + | SH_pwsh _ | SH_cmd -> + let warning = Printf.sprintf + "You appear to have added %s to PATH for this session only. \ + You will need to do this again before running opam in future." + root + in + options, internal_option, Some warning + end + end + in + Lazy.force header; + OpamConsole.msg + "\n\ + opam and the OCaml ecosystem in general require various Unix tools in \ + order to operate correctly. At present, this requires the installation \ + of Cygwin to provide these tools.\n\n"; + match OpamConsole.menu "How should opam obtain Unix tools?" + ~default ~no:default ~options with + | `Chosen (kind, `Internal) -> + assert (kind = `Cygwin); + Some (kind, `Internal OpamInitDefaults.required_packages_for_cygwin) + | `Chosen (kind, ((`Root _) as mechanism)) -> + Some (kind, mechanism) + | `Chosen ((_, `Path _) as mechanism) -> + OpamStd.Option.iter (OpamConsole.warning "%s") warn_path; + Some mechanism + | `Specify -> + begin + match OpamConsole.read + "Enter the prefix of an existing Cygwin installation \ + (e.g. C:\\cygwin64)" with + | None -> None + | Some entry -> + match OpamSysInteract.Cygwin.analyse_install entry with + | Ok (kind, root) -> + Some (kind, `Root root) + | Error msg -> + OpamConsole.error "%s" msg; + cygwin_menu header + end + | `Abort -> OpamStd.Sys.exit_because `Aborted + +and test_mechanism header = function + | (`Internal _) as mechanism -> Some (`Cygwin, mechanism) + | `Path -> + let cygcheck = + OpamSystem.resolve_command "cygcheck.exe" + |> Option.map OpamSysInteract.Cygwin.analyse_install + in + begin match cygcheck with + | Some (Ok (kind, root)) -> + Some (kind, `Path (OpamFilename.Dir.to_string root)) + | Some (Error _) | None -> + None + end + | `Test dir -> + begin match OpamSysInteract.Cygwin.analyse_install dir with + | Ok (kind, root) -> Some (kind, `Root root) + | Error _ -> None + end + | `Location dir -> + begin match OpamSysInteract.Cygwin.analyse_install dir with + | Ok (kind, root) -> Some (kind, `Root root) + | Error msg -> + OpamConsole.error_and_exit `Not_found "%s" msg + end + | `Menu -> cygwin_menu header + +let string_of_cygwin_setup = function + | `internal pkgs -> + let pkgs = + if pkgs = [] then "" + else + " with " ^ String.concat ", " (List.map OpamSysPkg.to_string pkgs) + in + "Internal" ^ pkgs + | `default_location -> "Search" + | `location dir -> "External from " ^ OpamFilename.Dir.to_string dir + | `no -> "Path-only (and no tweaking)" + +let string_of_git_location_cli = function + | Left location -> "Using git-location=" ^ OpamFilename.Dir.to_string location + | Right () -> "git-location disabled via CLI" + +let initialise_msys2 root = + let pacman = OpamFilename.Op.(root / "usr" / "bin" // "pacman.exe") in + let gnupg_dir = OpamFilename.Op.(root / "etc" / "pacman.d" / "gnupg") in + if OpamFilename.exists pacman && not (OpamFilename.exists_dir gnupg_dir) then + let cmd = + OpamFilename.Op.(root / "usr" / "bin" // "bash.exe") + |> OpamFilename.to_string + in + let answer = + let cmd = OpamConsole.colorise `yellow (cmd ^ " -lc \"uname -a\"") in + OpamConsole.menu ~unsafe_yes:`Yes ~default:`Yes ~no:`Quit + "MSYS2 appears not to have been initialised. opam can:" + ~options:[ + `Yes, Printf.sprintf + "Run %s to initialise it" cmd; + `No, Printf.sprintf + "Wait while you %s manually (e.g. in another terminal)" cmd; + `Ignore, "Continue anyway (but note that external dependency \ + may not work correctly until MSYS2 is initialised)"; + `Quit, "Abort initialisation"; + ] + in + OpamConsole.msg "\n"; + match answer with + | `Yes -> + if OpamConsole.disp_status_line () then + OpamConsole.status_line "Initialising MSYS2 (this may take a minute)"; + let r = + OpamProcess.run + (OpamProcess.command ~name:(OpamSystem.temp_file "command") + ~allow_stdin:false cmd ["-lc"; "uname -a"]) + in + OpamProcess.cleanup ~force:true r; + OpamConsole.clear_status (); + if not (OpamProcess.is_success r) then + OpamConsole.error_and_exit `Aborted "MSYS2 failed to initialise" + | `No -> + OpamConsole.pause "Standing by, press enter to continue when done."; + OpamConsole.msg "\n" + | `Ignore -> + () + | `Quit -> + OpamStd.Sys.exit_because `Aborted + +let determine_windows_configuration ?cygwin_setup ?git_location config = + OpamStd.Option.iter + (log "Cygwin (from CLI): %a" (slog string_of_cygwin_setup)) cygwin_setup; + (* Check whether symlinks can be created. Developer Mode is not the only way + to do this, but it's the easiest. *) if (not (Unix.has_symlink ())) then begin OpamConsole.header_msg "Windows Developer Mode"; - OpamConsole.msg "opam does not require Developer Mode to be enabled on Windows, but it is\n\ - recommended, in particular because it enables support for symlinks without\n\ - requiring opam to be run elevated (which we do %s recommend doing).\n\ - \n\ - More information on enabling Developer Mode may be obtained from\n\ - https://learn.microsoft.com/en-gb/windows/apps/get-started/enable-your-device-for-development\n" - (OpamConsole.colorise `bold "not") + OpamConsole.msg + "opam does not require Developer Mode to be enabled on Windows, but it is\n\ + recommended, in particular because it enables support for symlinks without\n\ + requiring opam to be run elevated (which we do %s recommend doing).\n\ + \n\ + More information on enabling Developer Mode may be obtained from\n\ + https://learn.microsoft.com/en-gb/windows/apps/get-started/enable-your-device-for-development\n" + (OpamConsole.colorise `bold "not") end; - 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 *) + + (* Augment ~git_location (from the CLI) with information from opamrc and + validate --git-location/git-location *) let git_location = match git_location, OpamFile.Config.git_location config with | None, None -> None @@ -864,304 +1299,235 @@ let windows_checks ?cygwin_setup ?git_location config = check_git_location_or_exit git_location "--git-location"; result in - let git_location = - if Sys.win32 then - git_for_windows ?git_location () - else - None + OpamStd.Option.iter (log "%a" (slog string_of_git_location_cli)) git_location; + + (* Checks and initialisation for both Cygwin/MSYS2 and Git (which is made + mandatory on Windows) + + The aim of this process is to determine four things: + - An optional directory containing git.exe but not shadowing any of + the executables in OpamEnv.cygwin_non_shadowed_programs. This is written + to git-location in ~/.opam/config and the resulting directory appears + as the first entry for Path on opam process calls + (see OpamStd.Env.cyg_env) + - Whether sys-pkg-manager-cmd should contain entries for either "cygwin" + or "msys2". The presence of one of those values also causes opam to add + the directory containing the package manager to Path + (see OpamCoreConfig.cygbin) + - Whether an internal installation of Cygwin is required, and if it needs + the git package + + The process is affected by various CLI options: + - --no-git-location causes git-location in opamrc to be ignored + - --git-location overrides git-location in opamrc and short-circuits + searching PATH for git.exe + - --no-cygwin-setup specifies that Cygwin/MSYS2 should be found in PATH + and no additional handling should be done + - --cygwin-internal-install specifies that opam should maintain its own + internal installation of Cygwin and make that fully available on Path + when building packages and executing commands internally. If + --git-location is not in use, and git.exe is not already installed, this + installation may include Cygwin's git package + - --cygwin-local-install specifies that opam should either search for + Cygwin/MSYS2 installations or, if --cygwin-location is specified, use + the Cygwin/MSYS2 installation specified. + *) + + let apply_git_location config git_location = + let config = OpamFile.Config.with_git_location git_location config in + let git_location = OpamFilename.Dir.to_string git_location in + OpamCoreConfig.update ~git_location (); + config in - OpamCoreConfig.update ?git_location (); - let config = + + (* If --git-location has been specified, apply it now *) + let config, git_location, git_determined, git_required_from_cygwin = 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 is_cygwin cygcheck = - OpamStd.Sys.is_cygwin_cygcheck - ~cygbin:(Some OpamFilename.(Dir.to_string (dirname cygcheck))) - in - let is_variant cygcheck = - OpamStd.Sys.is_cygwin_variant_cygcheck - ~cygbin:(Some OpamFilename.(Dir.to_string (dirname cygcheck))) - in - let is_msys2 cygcheck = is_variant cygcheck && not (is_cygwin cygcheck) in - let success cygcheck = - let cygbin = OpamFilename.dirname cygcheck in - let distrib = if is_cygwin cygcheck then "cygwin" else "msys2" in - let config = - let os_distribution = OpamVariable.of_string "os-distribution" in - let update vars = - OpamFile.Config.with_global_variables - ((os_distribution, S distrib, "Set by opam init")::vars) - config - in - match OpamStd.List.pick (fun (v,_,_) -> - OpamVariable.equal v os_distribution) - vars with - | Some (_, S d, _), _ when String.equal d distrib -> config - | None, vars -> update vars - | Some (_, vc, _), vars -> - OpamConsole.warning - "'os-distribution' already set to another value %s" - (OpamVariable.string_of_variable_contents vc); - if OpamConsole.confirm ~default:false "Override?" then - (OpamConsole.msg - "You can revert this setting using \ - 'opam var --global os-distribution=%s'" - (OpamVariable.string_of_variable_contents vc); - update vars) - else - OpamStd.Sys.exit_because `Aborted + | Some (Left git_location) -> + apply_git_location config git_location, Some git_location, true, false + | Some (Right ()) -> + config, None, true, (OpamSystem.resolve_command "git.exe" = None) + | None -> + config, None, false, false + in + + (* Based on the supplied command line options, determine which mechanisms can + be tried to acquire a Unix environment. + mechanisms - list of things to try from: + `Path - search for cygcheck.exe in PATH and test from there + `Test - search given root directory for cygcheck.exe (either in bin + or usr\bin) + `Location - as `Test, but _must_ succeed (--cygwin-location) + `Internal - create a Cygwin internal with the given packages + `Menu - interactive mode permitted + tweakable - can pacman / Cygwin setup be used to adjust setup + (--no-cygwin-setup disables this) + *) + let mechanisms, cygwin_tweakable = + match cygwin_setup with + | Some (`internal packages) -> + (* git, if needed, will be added later *) + let packages = OpamInitDefaults.required_packages_for_cygwin @ packages in + Seq.return (`Internal packages), true + | Some `no -> + if git_required_from_cygwin then + OpamConsole.error_and_exit `Not_found + "Both --no-cygwin-setup and --no-git-location have been specified, \ + but Git was not found in PATH. opam requires Git - please either \ + install Git for Windows and make it available in PATH or re-run \ + opam init with less restrictive command line options." + else + Seq.return `Path, false + | Some `default_location -> + cygwin_searches ~first:`Path (), true + | Some (`location dir) -> + Seq.return (`Location (OpamFilename.Dir.to_string dir)), true + | None -> + Seq.return `Menu, true + in + + let header = lazy (OpamConsole.header_msg "Unix support infrastructure") in + + (* Reduce mechanisms to a single mechanism (which may therefore display a + menu). *) + let kind, mechanism = + match OpamCompat.Seq.find_map (test_mechanism header) mechanisms with + | Some result -> result + | None -> + Lazy.force header; + OpamConsole.error_and_exit `Not_found + "A solution for Unix infrastructure is required, but the options \ + given to opam have not yielded one!" + in + + (* If --git-location is in use, then there's no further checking required on + the Git executable. If not, then before cygbin is potentially applied + through --cygwin-location, determine if we need to check that Git for + Windows is not going to be shadowed. *) + let have_git_for_windows_in_path, git_in_path_dir = + if git_location = None then + match OpamSystem.resolve_command "git.exe" with + | Some git -> + is_git_for_windows git, Filename.dirname git + | None -> + false, "" + else + false, "" + in + + (* Apply cygbin, if necessary *) + let config, msys2_check_root = + let apply cygcheck = + let cygbin = OpamFilename.Dir.to_string (OpamFilename.dirname cygcheck) in + OpamCoreConfig.update ~cygbin (); + let family = match kind with `Msys2 -> "msys2" | `Cygwin -> "cygwin" in + OpamFile.Config.with_sys_pkg_manager_cmd + (OpamStd.String.Map.add family cygcheck + (OpamFile.Config.sys_pkg_manager_cmd config)) + config in - let config = - if is_msys2 cygcheck then - let env = - OpamStd.Env.cyg_env ~cygbin:(OpamFilename.Dir.to_string cygbin) - ~git_location:None ~env:(OpamStd.Env.raw_env ()) + let open OpamFilename.Op in + let config, msys2_check_root = + match mechanism with + | `Path root -> + let msys2_check_root = + if kind = `Msys2 then + Some (OpamFilename.Dir.of_string root) + else + None in - match OpamSystem.resolve_command ~env "pacman.exe" with - | Some pacman -> - if OpamConsole.confirm - "Found package manager pacman binary at %s.\n\ - Do you want to use it for depexts?" - pacman then - OpamFile.Config.with_sys_pkg_manager_cmd - (OpamStd.String.Map.add distrib (OpamFilename.of_string pacman) - (OpamFile.Config.sys_pkg_manager_cmd config)) - config - else config - | None -> config - else + (* For opam init --reinit, it may be necessary to remove + sys-pkg-manager-path *) OpamFile.Config.with_sys_pkg_manager_cmd - (OpamStd.String.Map.add distrib cygcheck - (OpamFile.Config.sys_pkg_manager_cmd config)) - config + OpamStd.String.Map.empty config, msys2_check_root + | `Internal _ -> + (* The directory gets applied, but obviously it's not yet been + installed *) + let cygcheck = + OpamSysInteract.Cygwin.internal_cygroot () / "bin" // "cygcheck.exe" + in + apply cygcheck, None + | `Root root -> + let bindir = + if kind = `Msys2 then + root / "usr" / "bin" + else + root / "bin" + in + (* If the user has specified --no-git-location and Git for Windows was + in PATH and the given location occludes it, then this is our last + chance to warn about it. *) + if git_determined && have_git_for_windows_in_path && + OpamFilename.exists (bindir // "git.exe") then + OpamConsole.warning + "Git for Windows is in PATH (from %s), but it will be shadowed \ + when opam builds packages and executes commands internally. It is \ + recommended that only Git for Windows is used, and this could be \ + ensured by uninstalling the Git package from %s" + git_in_path_dir (OpamFilename.Dir.to_string bindir); + if kind = `Msys2 then + apply (bindir // "pacman.exe"), Some root + else + apply (bindir // "cygcheck.exe"), None in - OpamConsole.note "Configured with %s for depexts" - (if is_cygwin cygcheck then - if OpamSysInteract.Cygwin.is_internal config then - "internal Cygwin install" - else - (* cygcheck is in CYGWINROOT/bin *) - Printf.sprintf "Cygwin at %s" - OpamFilename.(Dir.to_string (dirname_dir cygbin)) - else - (* cygcheck is in MSYS2ROOT/usr/bin *) - Printf.sprintf "MSYS2 at %s" - OpamFilename.(Dir.to_string (dirname_dir (dirname_dir cygbin)))); - config + config, msys2_check_root in - let install_cygwin_tools packages = - let default_packages = OpamInitDefaults.required_packages_for_cygwin in - let default_packages = - if OpamSystem.resolve_command "git" = None then - OpamSysPkg.of_string "git" :: default_packages - else - default_packages - in - (* packages comes last so that the user can override any potential version - constraints in default_packages (although, with the current version of - setup, and with the list of default_packages in OpamInitDefaults, this at - present doesn't matter too much). *) - OpamSysInteract.Cygwin.install (default_packages @ packages) - in - let header () = OpamConsole.header_msg "Unix support infrastructure" in - - let get_cygwin = function - | Some cygcheck when OpamFilename.exists cygcheck && is_variant cygcheck -> - success cygcheck - | Some _ | None -> - let rec menu () = - let enter_paths () = - let prompt_setup () = - let options = [ - `download, "Let opam downloads it"; - `manual, "Manually enter its location on disk"; - `abort, "Abort initialisation"; - ] - in - OpamConsole.menu - "Opam needs Cygwin setup executable 'setup-x86_64.exe'" - ~default:`download ~no:`download ~options - in - let rec enter_setup () = - match prompt_setup () with - | `abort -> OpamStd.Sys.exit_because `Aborted - | `download -> None - | `manual -> - match OpamConsole.read "Enter path of Cygwin setup executable:" with - | None -> None - | Some setup -> - let setup = OpamFilename.of_string setup in - if OpamFilename.exists setup then Some setup else - (OpamConsole.msg "Cygwin setup executable doesn't exist at %s\n" - (OpamFilename.to_string setup); - enter_setup ()) - in - (* Check for default cygwin installation path *) - let default = - match OpamSysInteract.Cygwin.(analyse_install default_cygroot) with - | Ok (_, cygcheck) -> - let prompt_cygroot () = - let options = [ - `manual, - "Manually enter prefix of an existing Cygwin installation \ - (e.g. D:\\cygwin64)"; - `default, - (Printf.sprintf "Use default Cygwin installation at %s" - OpamSysInteract.Cygwin.default_cygroot); - `abort, "Abort initialisation"; - ] in - OpamConsole.menu "Cygwin location" - ~default:`default ~no:`default ~options - in - (match prompt_cygroot () with - | `abort -> OpamStd.Sys.exit_because `Aborted - | `manual -> None - | `default -> Some cygcheck) - | Error _ -> None - in - (* Otherwise, ask for prefix *) - let cygcheck = - match default with - | Some cygcheck -> Some cygcheck - | None -> - match OpamConsole.read - "Enter the prefix of an existing Cygwin installation \ - (e.g. C:\\cygwin64)" with - | None -> None - | Some entry -> - let cygcheck = - OpamSysInteract.Cygwin.analyse_install entry - in - match cygcheck with - | Ok (_, cygcheck) -> Some cygcheck - | Error msg -> OpamConsole.error "%s" msg; None - in - (* And finally ask for setup.exe *) - match cygcheck with - | Some cygcheck -> - if is_cygwin cygcheck then - OpamSysInteract.Cygwin.check_setup (enter_setup ()); - Some (success cygcheck) - | None -> None - in - let prompt () = - let options = [ - `Internal, - "Automatically create an internal Cygwin installation \ - that will be managed by opam (recommended)"; - `Specify, "Enter the location of an existing Cygwin installation"; - `Abort, "Abort initialisation"; - ] in - OpamConsole.menu "How should opam handle Cygwin?" - ~no:`Internal ~options + + (* Display the menu for Git configuration, if possible and required *) + let config, mechanism, cygwin_packages, git_location = + let mechanism, cygwin_packages = + match mechanism with + | `Internal pkgs -> + `Internal, pkgs + | (`Root _ | `Path _) as mechanism -> + let cygwin_packages = + if cygwin_tweakable && not OpamStateConfig.(!r.no_depexts) then + OpamInitDefaults.required_packages_for_cygwin + else + [] in - match prompt () with - | `Abort -> OpamStd.Sys.exit_because `Aborted - | `Internal -> - let cygcheck = install_cygwin_tools [] in - let config = success cygcheck in - config - | `Specify -> - match enter_paths () with - | Some config -> config - | None -> menu () + mechanism, cygwin_packages + in + if git_location = None && not git_determined + && not have_git_for_windows_in_path then + let git_location, from_cygwin = + git_for_windows kind mechanism cygwin_tweakable in - header (); - OpamConsole.msg - "\n\ - opam and the OCaml ecosystem in general require various Unix tools \ - in order to operate correctly. At present, this requires the \ - installation of Cygwin to provide these tools.\n\n"; - menu () - in - let config = - match cygwin_setup with - | Some `no -> config - | (Some (`internal _ | `default_location | `location _) | None) - as cygwin_setup -> - if OpamSysPoll.os env = Some "win32" then - match OpamSysPoll.os_distribution env with - | Some "win32" -> - (* If there's a "cygwin" entry in sys-pkg-manager-cmd, but - os-distribution hasn't (yet) been set to "cygwin", then that'll be - done here. Otherwise, the user must either allow opam to install - Cygwin or must provide the path to it. - Note that a depext solution is _mandatory_ on Windows for now, - because there are commands opam requires which are only provided - using it (patch, etc.). MSYS2 avoids this by requiring - os-distribution to be set. *) - let cygcheck = - OpamStd.String.Map.find_opt "cygwin" - (OpamFile.Config.sys_pkg_manager_cmd config) - in - (match cygwin_setup with - | None -> get_cygwin cygcheck - | Some setup -> - header (); - let cygcheck = - match setup with - | `internal pkgs -> install_cygwin_tools pkgs - | (`default_location | `location _ as setup) -> - let cygroot = - match setup with - | `default_location -> OpamSysInteract.Cygwin.default_cygroot - | `location dir -> OpamFilename.Dir.to_string dir - in - (match OpamSysInteract.Cygwin.analyse_install cygroot with - | Ok (_, cygcheck) -> cygcheck - | Error msg -> - OpamConsole.error_and_exit `Not_found - "Error while checking %sCygwin install (%s): %s" - (match setup with - | `default_location -> " default" - | `location _ -> "") - (OpamSysInteract.Cygwin.default_cygroot) msg) - in - if is_cygwin cygcheck then - OpamSysInteract.Cygwin.check_setup None; - success cygcheck) - | Some "cygwin" | Some "msys2" -> - (* We check that current install is good *) - (match OpamSysInteract.Cygwin.cygroot_opt config with - | Some cygroot -> - (match OpamSysInteract.Cygwin.analyse_install - (OpamFilename.Dir.to_string cygroot) with - | Ok (_, cygcheck) -> - OpamSysInteract.Cygwin.check_setup None; - success cygcheck - | Error err -> OpamConsole.error "%s" err; get_cygwin None) - | None -> - (* A Cygwin install (Cygwin or MSYS2) is detected from environment - (path), we check the install in that case and stores it in - config *) - OpamSystem.resolve_command "cygcheck" - |> OpamStd.Option.map OpamFilename.of_string - |> get_cygwin - ) - | _ -> config - else - config + let config = + OpamStd.Option.map_default (apply_git_location config) + config git_location + in + let cygwin_packages = + if cygwin_tweakable && from_cygwin then + OpamSysPkg.of_string "git" :: cygwin_packages + else + cygwin_packages + in + config, mechanism, cygwin_packages, git_location + else + config, mechanism, cygwin_packages, git_location in - let cygbin = - match OpamSysInteract.Cygwin.cygbin_opt config with - | Some cygbin -> Some (OpamFilename.Dir.to_string cygbin) - | None -> - if List.exists (function - | (v, S "msys2", _) -> - String.equal (OpamVariable.to_string v) "os-distribution" - | _ -> false) (OpamFile.Config.global_variables config) - then - OpamStd.Option.map Filename.dirname - (OpamSystem.resolve_command "cygcheck") - else None + + log "Unix support mechanism: %s %s" (string_of_kind kind) + (match mechanism with + | `Path root -> Printf.sprintf "from PATH (%s)" root + | `Internal -> "internal installation" + | `Root root -> + "local installation at " ^ OpamFilename.Dir.to_string root); + if cygwin_packages <> [] then + log "Systems packages to check for: %s" + (String.concat ", " (List.map OpamSysPkg.to_string cygwin_packages)); + log "git-location %s" + (OpamStd.Option.map_default + (fun d -> Printf.sprintf "= %s" (OpamFilename.Dir.to_string d)) + "is not in use" git_location); + + let mechanism, cygwin_packages = + match mechanism with + | `Path _ | `Root _ -> None, cygwin_packages + | `Internal -> Some cygwin_packages, [] in - OpamCoreConfig.update ?cygbin (); - config + config, mechanism, cygwin_packages, msys2_check_root let update_with_init_config ?(overwrite=false) config init_config = let module I = OpamFile.InitConfig in @@ -1196,14 +1562,41 @@ let update_with_init_config ?(overwrite=false) config init_config = setifnew C.git_location C.with_git_location_opt (I.git_location init_config) +let check_for_sys_packages config system_packages = + if system_packages <> [] then + let ((missing, _) as set) = + OpamSysInteract.packages_status config + (OpamSysPkg.Set.of_list system_packages) + in + if not (OpamSysPkg.Set.is_empty missing) then + 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 + (*Lazy.force header;*) + OpamSolution.print_depext_msg set; + OpamSolution.install_sys_packages ~confirm:true env config missing () + let reinit ?(init_config=OpamInitDefaults.init_config()) ~interactive ?dot_profile ?update_config ?env_hook ?completion ?inplace ?(check_sandbox=true) ?(bypass_checks=false) ?cygwin_setup ?git_location config shell = + log "RE-INIT"; let root = OpamStateConfig.(!r.root_dir) in let config = update_with_init_config config init_config in - let config = windows_checks ?cygwin_setup ?git_location config in + let config, mechanism, system_packages, msys2_check_root = + if Sys.win32 then + determine_windows_configuration ?cygwin_setup ?git_location config + else + config, None, [], None + in + + OpamStd.Option.iter initialise_msys2 msys2_check_root; + OpamStd.Option.iter OpamSysInteract.Cygwin.install mechanism; + check_for_sys_packages config system_packages; + let _all_ok = if bypass_checks then false else init_checks ~hard_fail_exn:false init_config @@ -1280,7 +1673,16 @@ let init init_config |> OpamFile.Config.with_repositories (List.map fst repos) in - let config = windows_checks ?cygwin_setup ?git_location config in + let config, mechanism, system_packages, msys2_check_root = + if Sys.win32 then + determine_windows_configuration ?cygwin_setup ?git_location config + else + config, None, [], None + in + + OpamStd.Option.iter initialise_msys2 msys2_check_root; + OpamStd.Option.iter OpamSysInteract.Cygwin.install mechanism; + check_for_sys_packages config system_packages; let dontswitch = if bypass_checks then false else diff --git a/src/core/opamStd.ml b/src/core/opamStd.ml index 899f63f876a..ddcf8c3de41 100644 --- a/src/core/opamStd.ml +++ b/src/core/opamStd.ml @@ -1402,20 +1402,6 @@ module OpamSys = struct let is_cygwin_variant ?search_in_first cmd = get_cygwin_variant ?search_in_first cmd = `Cygwin - let is_cygwin_cygcheck_t ~variant ~cygbin = - match cygbin with - | Some cygbin -> - let cygpath = Filename.concat cygbin "cygpath.exe" in - Sys.file_exists cygpath - && (variant ?search_in_first:(Some cygbin) cygpath = `Cygwin) - | None -> false - - let is_cygwin_variant_cygcheck ~cygbin = - is_cygwin_cygcheck_t ~variant:get_cygwin_variant ~cygbin - - let is_cygwin_cygcheck ~cygbin = - is_cygwin_cygcheck_t ~variant:get_windows_executable_variant ~cygbin - exception Exit of int exception Exec of string * string array * string array diff --git a/src/core/opamStd.mli b/src/core/opamStd.mli index c0111260411..3ba4aa261f0 100644 --- a/src/core/opamStd.mli +++ b/src/core/opamStd.mli @@ -576,15 +576,6 @@ module Sys : sig val get_windows_executable_variant: ?search_in_first:string -> string -> [ `Native | `Cygwin | `Tainted of [ `Msys2 | `Cygwin] | `Msys2 ] - (** Determines if cygcheck in given cygwin binary directory comes from a - Cygwin installation. Determined by analysing the cygpath command - found with it. *) - val is_cygwin_cygcheck : cygbin:string option -> bool - - (** As [is_cygwin_cygcheck], but returns true if it is a Cygwin variant - (Cygwin, Msys2). *) - val is_cygwin_variant_cygcheck : cygbin:string option -> bool - (** Behaviour is largely as {!get_windows_executable_variant} but where MSYS2 and Cygwin are seen as equivalent. diff --git a/src/state/opamSysInteract.ml b/src/state/opamSysInteract.ml index 4fdd739cf95..413b6643bc0 100644 --- a/src/state/opamSysInteract.ml +++ b/src/state/opamSysInteract.ml @@ -373,10 +373,7 @@ module Cygwin = struct args @@> fun r -> OpamSystem.raise_on_process_error r; set_fstab_noacl fstab; - Done ()); - cygcheck - - let default_cygroot = "C:\\cygwin64" + Done ()) let analysis_cache = Hashtbl.create 17 @@ -449,11 +446,8 @@ module Cygwin = struct match r.OpamProcess.r_stdout with | [] -> Error ("Unexpected error translating \"/\" with " ^ cygpath) - | _::_ -> - let cygcheck = - OpamFilename.of_string (Filename.concat dir "cygpath.exe") - in - Ok (kind, cygcheck) + | l::_ -> + Ok (kind, OpamFilename.Dir.of_string l) else Error ("Could not determine the root for " ^ cygpath) in diff --git a/src/state/opamSysInteract.mli b/src/state/opamSysInteract.mli index 1acd0d32c7f..ec8c4d9d79c 100644 --- a/src/state/opamSysInteract.mli +++ b/src/state/opamSysInteract.mli @@ -43,11 +43,11 @@ val repo_enablers: ?env:gt_variables -> OpamFile.Config.t -> string option module Cygwin : sig - (* Default Cygwin installation prefix C:\cygwin64 *) - val default_cygroot: string + (* Location of the internal Cygwin installation *) + val internal_cygroot: unit -> OpamFilename.Dir.t (* Install an internal Cygwin install, in /.cygwin *) - val install: OpamSysPkg.t list -> OpamFilename.t + val install: OpamSysPkg.t list -> unit (* [analyse_install path] searches for and identifies Cygwin/MSYS2 installations. [path] may be able the location of cygcheck.exe itself @@ -61,10 +61,10 @@ module Cygwin : sig pacman.exe in the same directory as cygcheck.exe and cygpath.exe. On success, the result is the kind of installation (Cygwin/MSYS2) along - with the full path to cygcheck.exe, otherwise a description of the problem - encountered is returned. *) + with the root directory (e.g. {v C:\cygwin64 v} or {v C:\msys64 v}), + otherwise a description of the problem encountered is returned. *) val analyse_install: - string -> ([ `Cygwin | `Msys2 ] * OpamFilename.t, string) result + string -> ([ `Cygwin | `Msys2 ] * OpamFilename.Dir.t, string) result (* Returns true if Cygwin install is internal *) val is_internal: OpamFile.Config.t -> bool @@ -78,9 +78,6 @@ module Cygwin : sig (* Return Cygwin binary path *) val cygbin_opt: OpamFile.Config.t -> OpamFilename.Dir.t option - (* Return Cygwin installation prefix *) - val cygroot_opt: OpamFile.Config.t -> OpamFilename.Dir.t option - (* Return MSYS2 binary path *) val msys2bin_opt: OpamFile.Config.t -> OpamFilename.Dir.t option end From c47b2953c3856bc8a3d291abc05dbbd5e05a583e Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Fri, 7 Jun 2024 11:36:44 +0100 Subject: [PATCH 14/31] Make --bypass-checks => --no-cygwin-setup --- src/client/opamClient.ml | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index a88def0c95f..efaddd6c533 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -1000,10 +1000,10 @@ let cygwin_searches ?first () = in seq cygwin_searches -let rec cygwin_menu header = +let rec cygwin_menu ~bypass_checks header = let start = Unix.gettimeofday () in let test_mechanism (roots, count, mechanisms) search = - match test_mechanism header search with + match test_mechanism ~bypass_checks header search with | Some ((kind, `Root root) as mechanism) -> if OpamFilename.Dir.Set.mem root roots then roots, count, mechanisms @@ -1064,6 +1064,9 @@ let rec cygwin_menu header = "Use tools found in PATH (%s installation at %s)" (string_of_kind kind) root)::options in + if bypass_checks then + options, path_option, None + else (* Check whether cygcheck is still available in the initial environment. This allows a warning to be displayed reminding the user to continue running opam from a Cygwin/MSYS2 shell that has been manually started, @@ -1169,11 +1172,11 @@ let rec cygwin_menu header = Some (kind, `Root root) | Error msg -> OpamConsole.error "%s" msg; - cygwin_menu header + cygwin_menu ~bypass_checks header end | `Abort -> OpamStd.Sys.exit_because `Aborted -and test_mechanism header = function +and test_mechanism ~bypass_checks header = function | (`Internal _) as mechanism -> Some (`Cygwin, mechanism) | `Path -> let cygcheck = @@ -1197,7 +1200,7 @@ and test_mechanism header = function | Error msg -> OpamConsole.error_and_exit `Not_found "%s" msg end - | `Menu -> cygwin_menu header + | `Menu -> cygwin_menu ~bypass_checks header let string_of_cygwin_setup = function | `internal pkgs -> @@ -1259,7 +1262,8 @@ let initialise_msys2 root = | `Quit -> OpamStd.Sys.exit_because `Aborted -let determine_windows_configuration ?cygwin_setup ?git_location config = +let determine_windows_configuration ?cygwin_setup ?git_location + ~bypass_checks config = OpamStd.Option.iter (log "Cygwin (from CLI): %a" (slog string_of_cygwin_setup)) cygwin_setup; (* Check whether symlinks can be created. Developer Mode is not the only way @@ -1363,6 +1367,13 @@ let determine_windows_configuration ?cygwin_setup ?git_location config = tweakable - can pacman / Cygwin setup be used to adjust setup (--no-cygwin-setup disables this) *) + (* --bypass-checks => --no-cygwin-setup if nothing else was specified *) + let cygwin_setup = + if bypass_checks && cygwin_setup = None then + Some `no + else + cygwin_setup + in let mechanisms, cygwin_tweakable = match cygwin_setup with | Some (`internal packages) -> @@ -1391,7 +1402,8 @@ let determine_windows_configuration ?cygwin_setup ?git_location config = (* Reduce mechanisms to a single mechanism (which may therefore display a menu). *) let kind, mechanism = - match OpamCompat.Seq.find_map (test_mechanism header) mechanisms with + let test_mechanism = test_mechanism ~bypass_checks header in + match OpamCompat.Seq.find_map test_mechanism mechanisms with | Some result -> result | None -> Lazy.force header; @@ -1588,7 +1600,8 @@ let reinit ?(init_config=OpamInitDefaults.init_config()) ~interactive let config = update_with_init_config config init_config in let config, mechanism, system_packages, msys2_check_root = if Sys.win32 then - determine_windows_configuration ?cygwin_setup ?git_location config + determine_windows_configuration ?cygwin_setup ?git_location + ~bypass_checks config else config, None, [], None in @@ -1675,7 +1688,8 @@ let init in let config, mechanism, system_packages, msys2_check_root = if Sys.win32 then - determine_windows_configuration ?cygwin_setup ?git_location config + determine_windows_configuration ?cygwin_setup ?git_location + ~bypass_checks config else config, None, [], None in From ce50004a30f0da73307a28195d3441e92269bb84 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sat, 8 Jun 2024 10:31:51 +0100 Subject: [PATCH 15/31] fixup "Ensure Cygwin setup is downloaded and up-to-date" - don't create setup-x86_64.exe.sha512 I think I must have had in mind originally caching the sha512.sum file from Cygwin.com, but what's ended up is unnecessarily complicated. Instead, if setup-x86_64.exe then we compute its sha512; download sha512.sum from Cygwin (which we assume will be quick) and compare that before continuing to download setup-x86_64.exe if it's changed. --- src/state/opamSysInteract.ml | 15 +++------------ src/state/opamSysInteract.mli | 3 +-- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/src/state/opamSysInteract.ml b/src/state/opamSysInteract.ml index 413b6643bc0..720ddaa92ba 100644 --- a/src/state/opamSysInteract.ml +++ b/src/state/opamSysInteract.ml @@ -255,10 +255,10 @@ module Cygwin = struct let download_setupexe dst = let overwrite = true in - let checksum_file = OpamFilename.add_extension dst "sha512" in + let kind = `SHA512 in let current_checksum = - if OpamFilename.exists checksum_file then - Some (OpamHash.sha512 (OpamFilename.read checksum_file)) + if OpamFilename.exists dst then + Some (OpamHash.compute ~kind (OpamFilename.to_string dst)) else None in @@ -288,7 +288,6 @@ module Cygwin = struct try Some (OpamHash.sha512 Re.(Group.get (exec re content) 1)) with Not_found -> None in - let kind = `SHA512 in if OpamStd.Option.equal OpamHash.equal current_checksum checksum && OpamFilename.exists dst && OpamStd.Option.equal OpamHash.equal current_checksum @@ -302,14 +301,6 @@ module Cygwin = struct OpamConsole.status_line "Downloading Cygwin setup from cygwin.com"; OpamDownload.download_as ~overwrite ?checksum url_setupexe dst @@+ fun () -> - OpamFilename.remove checksum_file; - let checksum = - match checksum with - | None -> OpamHash.compute ~kind (OpamFilename.to_string dst) - | Some sha512 -> sha512 - in - OpamFilename.with_open_out_bin checksum_file (fun c -> - output_string c (OpamHash.contents checksum)); OpamConsole.clear_status (); Done () end diff --git a/src/state/opamSysInteract.mli b/src/state/opamSysInteract.mli index ec8c4d9d79c..90e52e862f6 100644 --- a/src/state/opamSysInteract.mli +++ b/src/state/opamSysInteract.mli @@ -71,8 +71,7 @@ module Cygwin : sig (* [check_setup path] checks and store Cygwin setup executable. Is [path] is [None], it downloads it, otherwise it copies it to - /.cygwin/setup-x86_64.exe. If the file is already existent, it - is a no-op. *) + /.cygwin/setup-x86_64.exe. *) val check_setup: OpamFilename.t option -> unit (* Return Cygwin binary path *) From 696d86b12af9c7014f628c89425220dbb65bcaf8 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sat, 8 Jun 2024 10:36:42 +0100 Subject: [PATCH 16/31] Tweak OpamSysInteract.Cygwin.check_setup more The ability to copy setup-x86_64.exe is no longer needed - but restore the previous functionality which only downloaded setup if it didn't exist and use this when displaying a command to the user. In this mode, we only download setup-x86_64.exe so that the command we give to the user actually works. If we're actually going to run setup-x86_64.exe, then we download the latest version. --- src/client/opamSolution.ml | 6 ++++-- src/state/opamSysInteract.ml | 24 ++++++++---------------- src/state/opamSysInteract.mli | 9 +++++---- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/client/opamSolution.ml b/src/client/opamSolution.ml index b0adf8673ea..905d53cd82d 100644 --- a/src/client/opamSolution.ml +++ b/src/client/opamSolution.ml @@ -1189,8 +1189,10 @@ let install_sys_packages ~map_sysmap ~confirm env config sys_packages t = | `Ignore -> bypass t | `Quit -> give_up_msg (); OpamStd.Sys.exit_because `Aborted and print_command sys_packages = + (* Ensure that setup-x86_64.exe exists, so that an invalid command is not + displayed to the user. *) if OpamSysPoll.os_distribution env = Some "cygwin" then - OpamSysInteract.Cygwin.check_setup None; + OpamSysInteract.Cygwin.check_setup ~update:false; let commands = OpamSysInteract.install_packages_commands ~env config sys_packages |> List.map (fun ((`AsAdmin c | `AsUser c), a) -> c::a) @@ -1222,7 +1224,7 @@ let install_sys_packages ~map_sysmap ~confirm env config sys_packages t = and auto_install t sys_packages = try if OpamSysPoll.os_distribution env = Some "cygwin" then - OpamSysInteract.Cygwin.check_setup None; + OpamSysInteract.Cygwin.check_setup ~update:true; OpamSysInteract.install ~env config sys_packages; (* handles dry_run *) map_sysmap (fun _ -> OpamSysPkg.Set.empty) t with Failure msg -> diff --git a/src/state/opamSysInteract.ml b/src/state/opamSysInteract.ml index 720ddaa92ba..b506174f908 100644 --- a/src/state/opamSysInteract.ml +++ b/src/state/opamSysInteract.ml @@ -448,22 +448,9 @@ module Cygwin = struct Result.bind cygbin identify (* Set setup.exe in the good place, ie in .opam/.cygwin/ *) - let check_setup setup = + let check_setup ~update = let dst = cygsetup () in - match setup with - | Some setup -> - log "Copying %s into %s" - (OpamFilename.to_string setup) - (OpamFilename.to_string dst); - let sha512 = - OpamHash.compute ~kind:`SHA512 (OpamFilename.to_string setup) - in - OpamFilename.copy ~src:setup ~dst; - let checksum_file = OpamFilename.add_extension dst "sha512" in - OpamFilename.remove checksum_file; - OpamFilename.with_open_out_bin checksum_file @@ fun c -> - output_string c (OpamHash.contents sha512) - | None -> + if update || not (OpamFilename.exists dst) then OpamProcess.Job.run @@ download_setupexe dst end @@ -1153,8 +1140,13 @@ let update ?(env=OpamVariable.Map.empty) config = in match cmd with | None -> + (* Cygwin doesn't have an update database per se, but one is supposed to use + the most current setup program when downloading setup.ini (which is the + package database (cf. the --no-version-check option). + Also, when #5839 is addressed, we'll need to cache setup.ini, and that + will want to be updated here too. *) if family = Cygwin then - Cygwin.check_setup None + Cygwin.check_setup ~update:true else OpamConsole.warning "Unknown update command for %s, skipping system update" diff --git a/src/state/opamSysInteract.mli b/src/state/opamSysInteract.mli index 90e52e862f6..c8bdd34d4ba 100644 --- a/src/state/opamSysInteract.mli +++ b/src/state/opamSysInteract.mli @@ -69,10 +69,11 @@ module Cygwin : sig (* Returns true if Cygwin install is internal *) val is_internal: OpamFile.Config.t -> bool - (* [check_setup path] checks and store Cygwin setup executable. Is [path] is - [None], it downloads it, otherwise it copies it to - /.cygwin/setup-x86_64.exe. *) - val check_setup: OpamFilename.t option -> unit + (* [check_setup ~update] downloads and stores a Cygwin setup executable to + /.cygwin/setup-x86_64.exe. If [~update = false], this only + happens if the setup executable does not already exist, otherwise it is. + updated. *) + val check_setup: update:bool -> unit (* Return Cygwin binary path *) val cygbin_opt: OpamFile.Config.t -> OpamFilename.Dir.t option From 0ddcf2b4c60b07d84d12ee4af3242c13e0a0db2b Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sat, 8 Jun 2024 10:59:25 +0100 Subject: [PATCH 17/31] fixup MP commit: Add OpamSysInteract.Cygwin.bindir_for_root Factor out the computation of the expected bindir. --- src/client/opamClient.ml | 14 +++++--------- src/state/opamSysInteract.ml | 6 ++++++ src/state/opamSysInteract.mli | 5 +++++ 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index efaddd6c533..9a8fc454b94 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -1054,7 +1054,7 @@ let rec cygwin_menu ~bypass_checks header = options, `Chosen (`Cygwin, `Internal), None | Some (Ok (kind, root)) -> let pacman = - OpamFilename.Op.(root / "usr" / "bin" // "pacman.exe") + OpamFilename.Op.(OpamSysInteract.Cygwin.bindir_for_root `Msys2 root // "pacman.exe") |> OpamFilename.to_string in let root = OpamFilename.Dir.to_string root in @@ -1219,11 +1219,12 @@ let string_of_git_location_cli = function | Right () -> "git-location disabled via CLI" let initialise_msys2 root = - let pacman = OpamFilename.Op.(root / "usr" / "bin" // "pacman.exe") in + let bindir = OpamSysInteract.Cygwin.bindir_for_root `Msys2 root in + let pacman = OpamFilename.Op.(bindir // "pacman.exe") in let gnupg_dir = OpamFilename.Op.(root / "etc" / "pacman.d" / "gnupg") in if OpamFilename.exists pacman && not (OpamFilename.exists_dir gnupg_dir) then let cmd = - OpamFilename.Op.(root / "usr" / "bin" // "bash.exe") + OpamFilename.Op.(bindir // "bash.exe") |> OpamFilename.to_string in let answer = @@ -1460,12 +1461,7 @@ let determine_windows_configuration ?cygwin_setup ?git_location in apply cygcheck, None | `Root root -> - let bindir = - if kind = `Msys2 then - root / "usr" / "bin" - else - root / "bin" - in + let bindir = OpamSysInteract.Cygwin.bindir_for_root kind root in (* If the user has specified --no-git-location and Git for Windows was in PATH and the given location occludes it, then this is our last chance to warn about it. *) diff --git a/src/state/opamSysInteract.ml b/src/state/opamSysInteract.ml index b506174f908..c14b8f1eb26 100644 --- a/src/state/opamSysInteract.ml +++ b/src/state/opamSysInteract.ml @@ -447,6 +447,12 @@ module Cygwin = struct in Result.bind cygbin identify + let bindir_for_root kind root = + let open OpamFilename.Op in + match kind with + | `Msys2 -> root / "usr" / "bin" + | `Cygwin -> root / "bin" + (* Set setup.exe in the good place, ie in .opam/.cygwin/ *) let check_setup ~update = let dst = cygsetup () in diff --git a/src/state/opamSysInteract.mli b/src/state/opamSysInteract.mli index c8bdd34d4ba..9254c9a00f6 100644 --- a/src/state/opamSysInteract.mli +++ b/src/state/opamSysInteract.mli @@ -66,6 +66,11 @@ module Cygwin : sig val analyse_install: string -> ([ `Cygwin | `Msys2 ] * OpamFilename.Dir.t, string) result + (* [bindir_for_root kind root] returns the bin directory for the given + installation root and [kind], as returned by {!analyse_install}. *) + val bindir_for_root: + [ `Cygwin | `Msys2 ] -> OpamFilename.Dir.t -> OpamFilename.Dir.t + (* Returns true if Cygwin install is internal *) val is_internal: OpamFile.Config.t -> bool From 0ca3440478eb97bca9933d7acda4358adef19b7f Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sat, 8 Jun 2024 11:00:25 +0100 Subject: [PATCH 18/31] fixup MP: improve the message when using an existing Cygwin/MSYS2 Refactored to use "Add Git to" / "Use Git from" for both --cygwin-location and when tools were found in PATH. --- src/client/opamClient.ml | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index 9a8fc454b94..0fa35bc1d17 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -692,14 +692,14 @@ let string_of_kind = function | `Cygwin -> "Cygwin" let git_for_windows kind mechanism cygwin_is_tweakable = - let contains_git p = + let resolve_git_in p = OpamSystem.resolve_command ~env:[||] (Filename.concat p "git.exe") in let gits = OpamStd.Env.get "PATH" |> OpamStd.Sys.split_path_variable |> OpamStd.List.fold_left_map (fun gits p -> - match contains_git p with + match resolve_git_in p with | Some git when not (OpamStd.String.Set.mem git gits) -> OpamStd.String.Set.add git gits, Some (git, OpamSystem.bin_contains_bash p) @@ -782,7 +782,7 @@ let git_for_windows kind mechanism cygwin_is_tweakable = match bin with | None -> None | Some git_location -> - match contains_git git_location, + match resolve_git_in git_location, OpamSystem.bin_contains_bash git_location with | Some _, false -> OpamConsole.msg "Using Git from %s" git_location; @@ -810,6 +810,13 @@ let git_for_windows kind mechanism cygwin_is_tweakable = `Abort, ("Abort initialisation to " ^ abort_action); ] in + let add_or_use_git root = + let bindir = OpamSysInteract.Cygwin.bindir_for_root kind root in + if resolve_git_in (OpamFilename.Dir.to_string bindir) = None then + "Add Git to" + else + "Use Git from" + in let default, options = match mechanism with | `Internal -> @@ -821,17 +828,12 @@ let git_for_windows kind mechanism cygwin_is_tweakable = `Default, internal::options | `Root root -> assert cygwin_is_tweakable; - let root = OpamFilename.Dir.to_string root in - let git = Filename.concat root "git.exe" in - let prefix = - if OpamSystem.resolve_command ~env:[||] git = None then - "Add Git to" - else - "Use Git from" - in let root = `Default, Printf.sprintf - "%s the %s installation in %s" prefix (string_of_kind kind) root + "%s the %s installation in %s" + (add_or_use_git root) + (string_of_kind kind) + (OpamFilename.Dir.to_string root) in `Default, root::options | `Path root -> @@ -853,8 +855,10 @@ let git_for_windows kind mechanism cygwin_is_tweakable = if cygwin_is_tweakable then let option = `Default, Printf.sprintf - "Add Git to %s installation in %s (from PATH)" - (string_of_kind kind) root + "%s %s installation in %s (from PATH)" + (add_or_use_git (OpamFilename.Dir.of_string root)) + (string_of_kind kind) + root in `Default, option::options else From e1b0bb03fa8fb767a95b510c014ed821c22b0c5c Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Fri, 24 Feb 2023 10:46:10 +0100 Subject: [PATCH 19/31] reftest: add init with space in windows test --- tests/reftests/dune.inc | 21 +++++++++++++++++++++ tests/reftests/init.win32.test | 25 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 tests/reftests/init.win32.test diff --git a/tests/reftests/dune.inc b/tests/reftests/dune.inc index e9190d12e7d..42c8627c950 100644 --- a/tests/reftests/dune.inc +++ b/tests/reftests/dune.inc @@ -722,6 +722,27 @@ %{targets} (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:init.test} %{read-lines:testing-env})))) +(rule + (alias reftest-init.win32) + (enabled_if (= %{os_type} "Win32")) + (action + (diff init.win32.test init.win32.out))) + +(alias + (name reftest) + (enabled_if (= %{os_type} "Win32")) + (deps (alias reftest-init.win32))) + +(rule + (targets init.win32.out) + (deps root-N0REP0) + (enabled_if (= %{os_type} "Win32")) + (package opam) + (action + (with-stdout-to + %{targets} + (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:init.win32.test} %{read-lines:testing-env})))) + (rule (alias reftest-inplace) (action diff --git a/tests/reftests/init.win32.test b/tests/reftests/init.win32.test new file mode 100644 index 00000000000..8c25800b8ed --- /dev/null +++ b/tests/reftests/init.win32.test @@ -0,0 +1,25 @@ +N0REP0 +### : Redirection : +### rm -rf $OPAMROOT +### OPAMROOT="roots/with path" +### opam init --no-setup --bare --bypass-checks default REPO/ | grep -v Cygwin +No configuration file found, using built-in defaults. + +<><> Fetching repository information ><><><><><><><><><><><><><><><><><><><><><> +[default] Initialised +### opam var root --debug --debug-level=-1 +CLI Parsing CLI version 2.2 +GSTATE LOAD-GLOBAL-STATE @ ${BASEDIR}/roots/with path +${BASEDIR}/roots/with path +### cat 'roots/with path/redirected-opamroot' +/usr/bin/cat: 'roots/with path/redirected-opamroot': No such file or directory +# Return code 1 # +### opam switch create --empty test +### opam switch --debug --debug-level=-1 +CLI Parsing CLI version 2.2 +GSTATE LOAD-GLOBAL-STATE @ ${BASEDIR}/roots/with path +SWITCH list +# switch compiler description +-> test test +### echo $OPAMROOT +roots/with path From 0d9ecba4534017f49b6b5e8331ff22c064570921 Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Thu, 23 Feb 2023 17:20:37 +0100 Subject: [PATCH 20/31] path: add `redirected` file, used to indicate oam root redirection --- src/format/opamPath.ml | 2 ++ src/format/opamPath.mli | 3 +++ 2 files changed, 5 insertions(+) diff --git a/src/format/opamPath.ml b/src/format/opamPath.ml index 208838dd842..2577dad21fd 100644 --- a/src/format/opamPath.ml +++ b/src/format/opamPath.ml @@ -19,6 +19,8 @@ let ( /- ) dir f = OpamFile.make (dir // f) let config t = t /- "config" +let redirected t = t // "redirected-opamroot" + let init_config_files () = List.map OpamFile.make [ OpamFilename.Dir.of_string (OpamStd.Sys.etc ()) // "opamrc"; diff --git a/src/format/opamPath.mli b/src/format/opamPath.mli index cf3f3752387..43a40e4862c 100644 --- a/src/format/opamPath.mli +++ b/src/format/opamPath.mli @@ -31,6 +31,9 @@ val lock: t -> filename (** Main configuration file: {i $opam/config} *) val config: t -> OpamFile.Config.t OpamFile.t +(** Redirection file for opam root: {i $opam/redirected-opamroot} *) +val redirected: t -> OpamFilename.t + (** The list of configuration files location used by default ({i /etc/opamrc} and {i ~/.opamrc}). More general (lower priority) first. *) val init_config_files: unit -> OpamFile.InitConfig.t OpamFile.t list From 063fd82eaee2112b23dd897c8a45ea5471a44cfb Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Thu, 23 Feb 2023 17:22:18 +0100 Subject: [PATCH 21/31] stateConfig: add `original_root_dir` to store non redirected root dir --- src/client/opamClientConfig.mli | 1 + src/state/opamStateConfig.ml | 6 ++++++ src/state/opamStateConfig.mli | 2 ++ 3 files changed, 9 insertions(+) diff --git a/src/client/opamClientConfig.mli b/src/client/opamClientConfig.mli index be286dfc5ea..a11077c8e13 100644 --- a/src/client/opamClientConfig.mli +++ b/src/client/opamClientConfig.mli @@ -120,6 +120,7 @@ val opam_init: ?assume_depexts:bool -> ?cli:OpamCLIVersion.t -> ?scrubbed_environment_variables:string list -> + ?original_root_dir:OpamTypes.dirname -> ?current_switch:OpamSwitch.t -> ?switch_from:OpamStateTypes.provenance -> ?jobs:int Lazy.t -> diff --git a/src/state/opamStateConfig.ml b/src/state/opamStateConfig.ml index b579f7acf42..b1fa359c07e 100644 --- a/src/state/opamStateConfig.ml +++ b/src/state/opamStateConfig.ml @@ -53,6 +53,7 @@ end type t = { root_dir: OpamFilename.Dir.t; + original_root_dir: OpamFilename.Dir.t; current_switch: OpamSwitch.t option; switch_from: provenance; jobs: int Lazy.t; @@ -86,6 +87,7 @@ let default = { in concat_and_resolve local_appdata "opam" ); + original_root_dir = default_root (); current_switch = None; switch_from = `Default; jobs = lazy (max 1 (OpamSysPoll.cores () - 1)); @@ -108,6 +110,7 @@ let default = { type 'a options_fun = ?root_dir:OpamFilename.Dir.t -> + ?original_root_dir:OpamFilename.Dir.t -> ?current_switch:OpamSwitch.t -> ?switch_from:provenance -> ?jobs:(int Lazy.t) -> @@ -126,6 +129,7 @@ type 'a options_fun = let setk k t ?root_dir + ?original_root_dir ?current_switch ?switch_from ?jobs @@ -144,6 +148,7 @@ let setk k t let (+) x opt = match opt with Some x -> x | None -> x in k { root_dir = t.root_dir + root_dir; + original_root_dir = t.original_root_dir + original_root_dir; current_switch = (match current_switch with None -> t.current_switch | s -> s); switch_from = t.switch_from + switch_from; @@ -176,6 +181,7 @@ let initk k = in setk (setk (fun c -> r := c; k)) !r ?root_dir:(E.root () >>| OpamFilename.Dir.of_string) + ?original_root_dir:(E.root () >>| OpamFilename.Dir.of_string) ?current_switch ?switch_from ?jobs:(E.jobs () >>| fun s -> lazy s) diff --git a/src/state/opamStateConfig.mli b/src/state/opamStateConfig.mli index 85498a23eff..e5221e827ed 100644 --- a/src/state/opamStateConfig.mli +++ b/src/state/opamStateConfig.mli @@ -38,6 +38,7 @@ end type t = private { root_dir: OpamFilename.Dir.t; + original_root_dir: OpamFilename.Dir.t; current_switch: OpamSwitch.t option; switch_from: provenance; jobs: int Lazy.t; @@ -56,6 +57,7 @@ type t = private { type 'a options_fun = ?root_dir:OpamFilename.Dir.t -> + ?original_root_dir:OpamFilename.Dir.t -> ?current_switch:OpamSwitch.t -> ?switch_from:provenance -> ?jobs:(int Lazy.t) -> From c317e229f1b9311c1d3c307f3587328d09deb5de Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Thu, 23 Feb 2023 17:29:17 +0100 Subject: [PATCH 22/31] Redirect opam on Windows if path contains a space. It is needed for Cygwin installation, that doesn't handle paths with space. At init, detection and redirection are done, afterwards opam always load redirected opam root. Original root directory is stored in `OpamStateConfig.!r.original_root_dir`. --- master_changes.md | 1 + src/client/opamClient.ml | 37 ++++++++++++++++++++++- src/client/opamClientConfig.ml | 9 ++++++ src/state/opamStateConfig.ml | 44 ++++++++++++++++----------- tests/reftests/env.test | 54 ---------------------------------- tests/reftests/env.unix.test | 26 ++++++++++++++++ tests/reftests/init.win32.test | 13 ++++---- 7 files changed, 107 insertions(+), 77 deletions(-) diff --git a/master_changes.md b/master_changes.md index fa44bd7e445..956e5371781 100644 --- a/master_changes.md +++ b/master_changes.md @@ -29,6 +29,7 @@ users) * Enhance the Git menu by warning if the user appears to need to restart the shell to pick up PATH changes [#5963 @dra27] * Include Git for Windows installations in the list of possibilities where the user instructed Git-for-Windows setup not to update PATH [#5963 @dra27] * [BUG] Fail if `--git-location` points to a directory not containing git [#6000 @dra27] + * Redirect the opam root to C:\opamroot when the opam root contains spaces on Windows [#5457 @rjbou] ## Config report diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index 0fa35bc1d17..3cd05b391e6 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -1654,7 +1654,42 @@ let init shell = log "INIT %a" (slog @@ OpamStd.Option.to_string OpamRepositoryBackend.to_string) repo; - let root = OpamStateConfig.(!r.root_dir) in + let root = + let root = OpamStateConfig.(!r.root_dir) in + let has_space s = OpamStd.String.contains_char s ' ' in + if Sys.win32 && + has_space (OpamFilename.Dir.to_string root) then + (let default = "C:\\opamroot" in + let rec ask () = + match OpamConsole.read "Opam root: " with + | Some r -> + if has_space r then + (OpamConsole.msg + "Given path '%s' contains space, please choose another one.\n" + (OpamConsole.colorise `bold r); + ask ()) + else r + | None -> default + in + let new_root_f = + if OpamConsole.confirm ~default:false + "Your opam root path '%s' contains a space, we'll redirect to \ + '%s'.\nDo you want to choose and enter another spaceless folder?" + (OpamFilename.Dir.to_string root) default then + ask () + else default + in + let new_root = OpamFilename.Dir.of_string new_root_f in + OpamFilename.write (OpamPath.redirected root) new_root_f; + (* Add the readme file in C:\opamroot as redirected *) + OpamFilename.write + OpamFilename.Op.(root // "readme.txt") + (Printf.sprintf "Opam root redirected from %s" + (OpamFilename.Dir.to_string OpamStateConfig.(!r.root_dir))); + OpamStateConfig.update ~root_dir:new_root (); + new_root) + else root + in let config_f = OpamPath.config root in let root_empty = not (OpamFilename.exists_dir root) || OpamFilename.dir_is_empty root in diff --git a/src/client/opamClientConfig.ml b/src/client/opamClientConfig.ml index 9f1571cde86..ee1bebb6743 100644 --- a/src/client/opamClientConfig.ml +++ b/src/client/opamClientConfig.ml @@ -210,6 +210,15 @@ let opam_init ?root_dir ?strict ?solver = (* (i) get root dir *) let root = OpamStateConfig.opamroot ?root_dir () in + if Sys.win32 + (* if default, redirection will be handled by opam init, or should have + been handled *) + && (root_dir <> None || OpamStateConfig.E.root () <> None) + && OpamStd.String.contains_char (OpamFilename.Dir.to_string root) ' ' then + OpamConsole.error "You opam root directory contains a space, this may lead \ + to several malfunction... bzzz.... nooo%s" + (* NOTE: UTF-8 Collision emoji *) + (if OpamConsole.color () then "\xF0\x9F\x92\xA5" else ""); (* (ii) load conf file and set defaults *) (* the init for OpamFormat is done in advance since (a) it has an effect on diff --git a/src/state/opamStateConfig.ml b/src/state/opamStateConfig.ml index b1fa359c07e..9c42c6ac48d 100644 --- a/src/state/opamStateConfig.ml +++ b/src/state/opamStateConfig.ml @@ -70,23 +70,30 @@ type t = { no_depexts: bool; } +let win_space_redirection root = + let redirected = OpamPath.redirected root in + if OpamFilename.exists redirected then + OpamFilename.Dir.of_string (OpamFilename.read redirected) + else root + +let default_root () = + (* On Windows, if a .opam directory is found in %HOME% or %USERPROFILE% then + then we'll use it. Otherwise, we use %LOCALAPPDATA%. *) + let home_location = + let open OpamFilename in + concat_and_resolve (Dir.of_string (OpamStd.Sys.home ())) ".opam" + in + if not Sys.win32 || OpamFilename.exists_dir home_location then + home_location + else + let open OpamFilename in + let local_appdata = + Dir.of_string (OpamStubs.getPathToLocalAppData ()) + in + concat_and_resolve local_appdata "opam" + let default = { - root_dir = ( - (* On Windows, if a .opam directory is found in %HOME% or %USERPROFILE% then - then we'll use it. Otherwise, we use %LOCALAPPDATA%. *) - let home_location = - let open OpamFilename in - concat_and_resolve (Dir.of_string (OpamStd.Sys.home ())) ".opam" - in - if not Sys.win32 || OpamFilename.exists_dir home_location then - home_location - else - let open OpamFilename in - let local_appdata = - Dir.of_string (OpamStubs.getPathToLocalAppData ()) - in - concat_and_resolve local_appdata "opam" - ); + root_dir = default_root () |> win_space_redirection; original_root_dir = default_root (); current_switch = None; switch_from = `Default; @@ -180,7 +187,9 @@ let initk k = | Some s -> Some (OpamSwitch.of_string s), Some `Env in setk (setk (fun c -> r := c; k)) !r - ?root_dir:(E.root () >>| OpamFilename.Dir.of_string) + ?root_dir:(E.root () + >>| OpamFilename.Dir.of_string + >>| win_space_redirection) ?original_root_dir:(E.root () >>| OpamFilename.Dir.of_string) ?current_switch ?switch_from @@ -208,6 +217,7 @@ let opamroot ?root_dir () = (root_dir >>+ fun () -> OpamStd.Env.getopt "OPAMROOT" >>| OpamFilename.Dir.of_string) +! default.root_dir + |> win_space_redirection let is_newer_raw = function | Some v -> diff --git a/tests/reftests/env.test b/tests/reftests/env.test index f4ac8d1dafa..484703ec25b 100644 --- a/tests/reftests/env.test +++ b/tests/reftests/env.test @@ -439,60 +439,6 @@ The following actions will be performed: Done. ### opam exec -- sh -c "eval $(opam env | tr -d '\\r'); opam remove foo; opam env; eval $(opam env | tr -d '\\r'); opam env" | grep "FOO" FOO=''; export FOO; -### : root and switch with spaces : -### RT="$BASEDIR/root 2" -### SW="switch w spaces" -### OPAMNOENVNOTICE=0 -### opam init -na --bare --bypass-check --disable-sandbox --root "$RT" defaut ./REPO | grep -v Cygwin -No configuration file found, using built-in defaults. - -<><> Fetching repository information ><><><><><><><><><><><><><><><><><><><><><> -[defaut] Initialised -### opam switch create "./$SW" nv --root "$RT" - -<><> Installing new switch packages <><><><><><><><><><><><><><><><><><><><><><> -Switch invariant: ["nv"] - -<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><> --> installed nv.1 -Done. -# Run eval $(opam env '--root=${BASEDIR}/root 2' '--switch=${BASEDIR}/switch w spaces') to update the current shell environment -### opam env --root "$RT" --switch "./$SW" | grep "NV_VARS" | ';' -> ':' -NV_VARS3='foo:/yet/another/different/path': export NV_VARS3: -NV_VARS4='': export NV_VARS4: -NV_VARS_5925_1='foo': export NV_VARS_5925_1: -NV_VARS_5925_2='foo': export NV_VARS_5925_2: -NV_VARS_5925_3='foo': export NV_VARS_5925_3: -NV_VARS_5925_4='foo': export NV_VARS_5925_4: -NV_VARS_5925_5='foo:': export NV_VARS_5925_5: -NV_VARS_5925_6='foo:': export NV_VARS_5925_6: -NV_VARS_5925_7=':foo': export NV_VARS_5925_7: -NV_VARS_5925_8=':foo': export NV_VARS_5925_8: -NV_VARS_5926_L_1='b::a': export NV_VARS_5926_L_1: -NV_VARS_5926_L_2='b::a': export NV_VARS_5926_L_2: -NV_VARS_5926_L_3=':a:b': export NV_VARS_5926_L_3: -NV_VARS_5926_L_4=':a:b': export NV_VARS_5926_L_4: -NV_VARS_5926_L_5='b::a': export NV_VARS_5926_L_5: -NV_VARS_5926_L_6='b::a': export NV_VARS_5926_L_6: -NV_VARS_5926_L_7=':a:b': export NV_VARS_5926_L_7: -NV_VARS_5926_L_8=':a:b': export NV_VARS_5926_L_8: -NV_VARS_5926_M_1='b:a1::a2': export NV_VARS_5926_M_1: -NV_VARS_5926_M_2='a1::a2:b': export NV_VARS_5926_M_2: -NV_VARS_5926_M_3='b:a1::a2': export NV_VARS_5926_M_3: -NV_VARS_5926_M_4='a1::a2:b': export NV_VARS_5926_M_4: -NV_VARS_5926_S_1='a:': export NV_VARS_5926_S_1: -NV_VARS_5926_S_2=':a': export NV_VARS_5926_S_2: -NV_VARS_5926_S_3='a:': export NV_VARS_5926_S_3: -NV_VARS_5926_S_4=':a': export NV_VARS_5926_S_4: -NV_VARS_5926_T_1='b:a:': export NV_VARS_5926_T_1: -NV_VARS_5926_T_2='b:a:': export NV_VARS_5926_T_2: -NV_VARS_5926_T_3='a::b': export NV_VARS_5926_T_3: -NV_VARS_5926_T_4='a::b': export NV_VARS_5926_T_4: -NV_VARS_5926_T_5='b:a:': export NV_VARS_5926_T_5: -NV_VARS_5926_T_6='b:a:': export NV_VARS_5926_T_6: -NV_VARS_5926_T_7='a::b': export NV_VARS_5926_T_7: -NV_VARS_5926_T_8='a::b': export NV_VARS_5926_T_8: -### OPAMNOENVNOTICE=1 ### : Env hooks : ### opam-version: "2.0" diff --git a/tests/reftests/env.unix.test b/tests/reftests/env.unix.test index eaa9395e32c..be7fdf29e00 100644 --- a/tests/reftests/env.unix.test +++ b/tests/reftests/env.unix.test @@ -1,4 +1,30 @@ N0REP0 +### : root and switches with spaces : +### +opam-version: "2.0" +flags: compiler +### RT="$BASEDIR/root 2" +### SW="switch w spaces" +### OPAMNOENVNOTICE=0 +### opam init -na --bare --bypass-check --disable-sandbox --root "$RT" defaut ./REPO +No configuration file found, using built-in defaults. + +<><> Fetching repository information ><><><><><><><><><><><><><><><><><><><><><> +[defaut] Initialised +### opam switch create "./$SW" nv --root "$RT" + +<><> Installing new switch packages <><><><><><><><><><><><><><><><><><><><><><> +Switch invariant: ["nv"] + +<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><> +-> installed nv.1 +Done. +# Run eval $(opam env '--root=${BASEDIR}/root 2' '--switch=${BASEDIR}/switch w spaces') to update the current shell environment +### opam env --root "$RT" --switch "./$SW" | grep "PREFIX" | ';' -> ':' +OPAM_SWITCH_PREFIX='${BASEDIR}/switch w spaces/_opam': export OPAM_SWITCH_PREFIX: +### opam var root --root "$RT" +${BASEDIR}/root 2 +### OPAMNOENVNOTICE=1 ### : setenv & build env rewriting : ### opam switch create rewriting --empty ### :::::::::::::::::: diff --git a/tests/reftests/init.win32.test b/tests/reftests/init.win32.test index 8c25800b8ed..17c519fb7d7 100644 --- a/tests/reftests/init.win32.test +++ b/tests/reftests/init.win32.test @@ -3,23 +3,26 @@ N0REP0 ### rm -rf $OPAMROOT ### OPAMROOT="roots/with path" ### opam init --no-setup --bare --bypass-checks default REPO/ | grep -v Cygwin +[ERROR] You opam root directory contains a space, this may lead to several malfunction... bzzz.... nooo No configuration file found, using built-in defaults. +Your opam root path '${BASEDIR}/roots/with path' contains a space, we'll redirect to 'C:\opamroot'. +Do you want to choose and enter another spaceless folder? [y/n] n <><> Fetching repository information ><><><><><><><><><><><><><><><><><><><><><> [default] Initialised ### opam var root --debug --debug-level=-1 CLI Parsing CLI version 2.2 -GSTATE LOAD-GLOBAL-STATE @ ${BASEDIR}/roots/with path -${BASEDIR}/roots/with path +GSTATE LOAD-GLOBAL-STATE @ C:\opamroot +C:\opamroot ### cat 'roots/with path/redirected-opamroot' -/usr/bin/cat: 'roots/with path/redirected-opamroot': No such file or directory -# Return code 1 # +C:\opamroot ### opam switch create --empty test ### opam switch --debug --debug-level=-1 CLI Parsing CLI version 2.2 -GSTATE LOAD-GLOBAL-STATE @ ${BASEDIR}/roots/with path +GSTATE LOAD-GLOBAL-STATE @ C:\opamroot SWITCH list # switch compiler description -> test test ### echo $OPAMROOT roots/with path +### rm -rf 'C:\opamroot' From 5e77dfef657b600345dfae6bcbde40dabc253ac8 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sun, 9 Jun 2024 07:53:28 +0100 Subject: [PATCH 23/31] Add provenance of opam root, as for current switch --- src/client/opamArg.ml | 1 + src/client/opamCliMain.ml | 4 ++-- src/client/opamClientConfig.ml | 4 ++-- src/client/opamCommands.ml | 2 +- src/state/opamStateConfig.ml | 32 +++++++++++++++++++++++--------- src/state/opamStateConfig.mli | 4 +++- 6 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/client/opamArg.ml b/src/client/opamArg.ml index c43ff1f6382..29d75df455d 100644 --- a/src/client/opamArg.ml +++ b/src/client/opamArg.ml @@ -543,6 +543,7 @@ let apply_global_options cli o = (* ?solver_preferences_best_effort_prefix: *) (* - state options - *) ?root_dir:o.opt_root + ?original_root_dir:o.opt_root ?current_switch:(o.opt_switch >>| OpamSwitch.of_string) ?switch_from:(o.opt_switch >>| fun _ -> `Command_line) (* ?jobs: int *) diff --git a/src/client/opamCliMain.ml b/src/client/opamCliMain.ml index 2ddf5ff7b3e..75e953335a1 100644 --- a/src/client/opamCliMain.ml +++ b/src/client/opamCliMain.ml @@ -181,7 +181,7 @@ let check_and_run_external_commands () = let yes = if yes then Some (Some true) else None in OpamCoreConfig.init ?yes ?confirm_level (); OpamFormatConfig.init (); - let root_dir = OpamStateConfig.opamroot () in + let root_from, root_dir = OpamStateConfig.opamroot () in let has_init, root_upgraded = match OpamStateConfig.load_defaults ~lock_kind:`Lock_read root_dir with | None -> (false, false) @@ -210,7 +210,7 @@ let check_and_run_external_commands () = env_update_resolved "PATH" PlusEq (OpamFilename.Dir.to_string plugins_bin) ] in - OpamStateConfig.init ~root_dir (); + OpamStateConfig.init ~root_from ~root_dir (); match OpamStateConfig.get_switch_opt () with | None -> env_array (OpamEnv.get_pure ~updates ()) | Some sw -> diff --git a/src/client/opamClientConfig.ml b/src/client/opamClientConfig.ml index ee1bebb6743..619897a60b9 100644 --- a/src/client/opamClientConfig.ml +++ b/src/client/opamClientConfig.ml @@ -209,7 +209,7 @@ let opam_init ?root_dir ?strict ?solver = let open OpamStd.Option.Op in (* (i) get root dir *) - let root = OpamStateConfig.opamroot ?root_dir () in + let root_from, root = OpamStateConfig.opamroot ?root_dir () in if Sys.win32 (* if default, redirection will be handled by opam init, or should have been handled *) @@ -270,5 +270,5 @@ let opam_init ?root_dir ?strict ?solver = OpamCoreConfig.initk ?log_dir |> OpamRepositoryConfig.initk |> OpamSolverConfig.initk ?solver |> - OpamStateConfig.initk ~root_dir:root |> + OpamStateConfig.initk ~root_dir:root ~root_from |> initk diff --git a/src/client/opamCommands.ml b/src/client/opamCommands.ml index dc72f544ff3..3e55a67628f 100644 --- a/src/client/opamCommands.ml +++ b/src/client/opamCommands.ml @@ -94,7 +94,7 @@ let global_options cli = switch_to_updated_self OpamStd.Option.Op.(options.debug_level ++ OpamCoreConfig.E.debug () +! 0 |> abs > 0) - (OpamStateConfig.opamroot ?root_dir:options.opt_root ()); + (snd (OpamStateConfig.opamroot ?root_dir:options.opt_root ())); let root_is_ok = OpamStd.Option.default false (OpamClientConfig.E.rootisok ()) in diff --git a/src/state/opamStateConfig.ml b/src/state/opamStateConfig.ml index 9c42c6ac48d..be143e2dc78 100644 --- a/src/state/opamStateConfig.ml +++ b/src/state/opamStateConfig.ml @@ -54,6 +54,7 @@ end type t = { root_dir: OpamFilename.Dir.t; original_root_dir: OpamFilename.Dir.t; + root_from: provenance; current_switch: OpamSwitch.t option; switch_from: provenance; jobs: int Lazy.t; @@ -95,6 +96,7 @@ let default_root () = let default = { root_dir = default_root () |> win_space_redirection; original_root_dir = default_root (); + root_from = `Default; current_switch = None; switch_from = `Default; jobs = lazy (max 1 (OpamSysPoll.cores () - 1)); @@ -118,6 +120,7 @@ let default = { type 'a options_fun = ?root_dir:OpamFilename.Dir.t -> ?original_root_dir:OpamFilename.Dir.t -> + ?root_from:provenance -> ?current_switch:OpamSwitch.t -> ?switch_from:provenance -> ?jobs:(int Lazy.t) -> @@ -137,6 +140,7 @@ type 'a options_fun = let setk k t ?root_dir ?original_root_dir + ?root_from ?current_switch ?switch_from ?jobs @@ -156,6 +160,7 @@ let setk k t k { root_dir = t.root_dir + root_dir; original_root_dir = t.original_root_dir + original_root_dir; + root_from = t.root_from + root_from; current_switch = (match current_switch with None -> t.current_switch | s -> s); switch_from = t.switch_from + switch_from; @@ -181,16 +186,22 @@ let update ?noop:_ = setk (fun cfg () -> r := cfg) !r let initk k = let open OpamStd.Option.Op in + let root_dir, original_root_dir, root_from = + match E.root () with + | None -> None, None, None + | Some root -> + let root = OpamFilename.Dir.of_string root in + Some (win_space_redirection root), Some root, Some `Env + in let current_switch, switch_from = match E.switch () with | Some "" | None -> None, None | Some s -> Some (OpamSwitch.of_string s), Some `Env in setk (setk (fun c -> r := c; k)) !r - ?root_dir:(E.root () - >>| OpamFilename.Dir.of_string - >>| win_space_redirection) - ?original_root_dir:(E.root () >>| OpamFilename.Dir.of_string) + ?root_dir + ?original_root_dir + ?root_from ?current_switch ?switch_from ?jobs:(E.jobs () >>| fun s -> lazy s) @@ -213,11 +224,14 @@ let initk k = let init ?noop:_ = initk (fun () -> ()) let opamroot ?root_dir () = - let open OpamStd.Option.Op in - (root_dir >>+ fun () -> - OpamStd.Env.getopt "OPAMROOT" >>| OpamFilename.Dir.of_string) - +! default.root_dir - |> win_space_redirection + match root_dir with + | Some root -> `Command_line, win_space_redirection root + | None -> + match OpamStd.Env.getopt "OPAMROOT" with + | Some root -> + `Env, win_space_redirection (OpamFilename.Dir.of_string root) + | None -> + `Default, default.root_dir let is_newer_raw = function | Some v -> diff --git a/src/state/opamStateConfig.mli b/src/state/opamStateConfig.mli index e5221e827ed..4531767b049 100644 --- a/src/state/opamStateConfig.mli +++ b/src/state/opamStateConfig.mli @@ -39,6 +39,7 @@ end type t = private { root_dir: OpamFilename.Dir.t; original_root_dir: OpamFilename.Dir.t; + root_from: provenance; current_switch: OpamSwitch.t option; switch_from: provenance; jobs: int Lazy.t; @@ -58,6 +59,7 @@ type t = private { type 'a options_fun = ?root_dir:OpamFilename.Dir.t -> ?original_root_dir:OpamFilename.Dir.t -> + ?root_from:provenance -> ?current_switch:OpamSwitch.t -> ?switch_from:provenance -> ?jobs:(int Lazy.t) -> @@ -84,7 +86,7 @@ val default : t (** Get the initial opam root value (from default, env or optional argument). This allows one to get it before doing the init, which is useful to get the configuration file used to fill some options to init() *) -val opamroot: ?root_dir:dirname -> unit -> dirname +val opamroot: ?root_dir:dirname -> unit -> provenance * dirname (** Loads the global configuration file, protecting against concurrent writes *) val load: ?lock_kind: 'a lock -> dirname -> OpamFile.Config.t option From f49217738acce27a2a0f405d7be474ddd569d735 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sun, 9 Jun 2024 07:56:38 +0100 Subject: [PATCH 24/31] Refactor out the root redirection function (no change) --- src/client/opamClient.ml | 64 +++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index 3cd05b391e6..db4fa26c355 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -1645,6 +1645,39 @@ let reinit ?(init_config=OpamInitDefaults.init_config()) ~interactive in OpamRepositoryState.drop rt +let has_space s = OpamStd.String.contains_char s ' ' + +let get_redirected_root root = + let default = "C:\\opamroot" in + let rec ask () = + match OpamConsole.read "Opam root: " with + | Some r -> + if has_space r then + (OpamConsole.msg + "Given path '%s' contains space, please choose another one.\n" + (OpamConsole.colorise `bold r); + ask ()) + else r + | None -> default + in + let new_root_f = + if OpamConsole.confirm ~default:false + "Your opam root path '%s' contains a space, we'll redirect to \ + '%s'.\nDo you want to choose and enter another spaceless folder?" + (OpamFilename.Dir.to_string root) default then + ask () + else default + in + let new_root = OpamFilename.Dir.of_string new_root_f in + OpamFilename.write (OpamPath.redirected root) new_root_f; + (* Add the readme file in C:\opamroot as redirected *) + OpamFilename.write + OpamFilename.Op.(root // "readme.txt") + (Printf.sprintf "Opam root redirected from %s" + (OpamFilename.Dir.to_string OpamStateConfig.(!r.root_dir))); + OpamStateConfig.update ~root_dir:new_root (); + new_root + let init ~init_config ~interactive ?repo ?(bypass_checks=false) @@ -1656,38 +1689,9 @@ let init (slog @@ OpamStd.Option.to_string OpamRepositoryBackend.to_string) repo; let root = let root = OpamStateConfig.(!r.root_dir) in - let has_space s = OpamStd.String.contains_char s ' ' in if Sys.win32 && has_space (OpamFilename.Dir.to_string root) then - (let default = "C:\\opamroot" in - let rec ask () = - match OpamConsole.read "Opam root: " with - | Some r -> - if has_space r then - (OpamConsole.msg - "Given path '%s' contains space, please choose another one.\n" - (OpamConsole.colorise `bold r); - ask ()) - else r - | None -> default - in - let new_root_f = - if OpamConsole.confirm ~default:false - "Your opam root path '%s' contains a space, we'll redirect to \ - '%s'.\nDo you want to choose and enter another spaceless folder?" - (OpamFilename.Dir.to_string root) default then - ask () - else default - in - let new_root = OpamFilename.Dir.of_string new_root_f in - OpamFilename.write (OpamPath.redirected root) new_root_f; - (* Add the readme file in C:\opamroot as redirected *) - OpamFilename.write - OpamFilename.Op.(root // "readme.txt") - (Printf.sprintf "Opam root redirected from %s" - (OpamFilename.Dir.to_string OpamStateConfig.(!r.root_dir))); - OpamStateConfig.update ~root_dir:new_root (); - new_root) + get_redirected_root root else root in let config_f = OpamPath.config root in From c67fd2eac0c42977723fee6682b6a5449a29280a Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sun, 9 Jun 2024 08:13:34 +0100 Subject: [PATCH 25/31] Empty warning should be on original_root Previously, if the redirection was left behind but the redirected root had been deleted, opam proceeded without any indication at all (easy if either C:\opamroot has been deleted or, as in current behaviour, when aborted opam init doesn't clean the redirection) --- src/client/opamClient.ml | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index db4fa26c355..3e21e25fa4d 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -1687,16 +1687,19 @@ let init shell = log "INIT %a" (slog @@ OpamStd.Option.to_string OpamRepositoryBackend.to_string) repo; + let original_root = OpamStateConfig.(!r.original_root_dir) in + let root_empty = + not (OpamFilename.exists_dir original_root) + || OpamFilename.dir_is_empty original_root in + let root = OpamStateConfig.(!r.root_dir) in let root = - let root = OpamStateConfig.(!r.root_dir) in - if Sys.win32 && + if root_empty && + Sys.win32 && has_space (OpamFilename.Dir.to_string root) then get_redirected_root root else root in let config_f = OpamPath.config root in - let root_empty = - not (OpamFilename.exists_dir root) || OpamFilename.dir_is_empty root in let gt, rt, default_compiler = if OpamFile.exists config_f then ( @@ -1710,7 +1713,7 @@ let init ) else ( if not root_empty then ( OpamConsole.warning "%s exists and is not empty" - (OpamFilename.Dir.to_string root); + (OpamFilename.Dir.to_string original_root); if not (OpamConsole.confirm "Proceed?") then OpamStd.Sys.exit_because `Aborted); try From 47dc02cfd1aa6bd3456aaed6b1514bce8a6a3db4 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sun, 9 Jun 2024 08:23:56 +0100 Subject: [PATCH 26/31] Clean-up original_root_dir on error, etc. --- src/client/opamClient.ml | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index 3e21e25fa4d..5fd00b70d6e 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -1692,12 +1692,21 @@ let init not (OpamFilename.exists_dir original_root) || OpamFilename.dir_is_empty original_root in let root = OpamStateConfig.(!r.root_dir) in - let root = + let root, remove_root = + let ignore_non_fatal f x = + try f x + with e -> OpamStd.Exn.fatal e + in if root_empty && Sys.win32 && has_space (OpamFilename.Dir.to_string root) then - get_redirected_root root - else root + let root = get_redirected_root root in + root, (fun () -> + ignore_non_fatal OpamFilename.rmdir root; + ignore_non_fatal OpamFilename.rmdir original_root + ) + else + root, (fun () -> ignore_non_fatal OpamFilename.rmdir root) in let config_f = OpamPath.config root in @@ -1785,7 +1794,7 @@ let init in if failed <> [] then (if root_empty then - (try OpamFilename.rmdir root with _ -> ()); + remove_root (); OpamConsole.error_and_exit `Sync_error "Initial download of repository failed."); let default_compiler = @@ -1820,7 +1829,7 @@ let init OpamStd.Exn.finalise e @@ fun () -> if not (OpamConsole.debug ()) && root_empty then begin OpamSystem.release_all_locks (); - OpamFilename.rmdir root + remove_root () end) in OpamEnv.setup root ~interactive From a338c32ae77a7c00f7ff250014a2a141ede3b32e Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sun, 9 Jun 2024 09:57:47 +0100 Subject: [PATCH 27/31] Convert the root redirection into a menu --- src/client/opamClient.ml | 148 +++++++++++++++++++++++++-------- src/client/opamClientConfig.ml | 9 -- src/core/opamSystem.ml | 7 ++ src/core/opamSystem.mli | 4 + 4 files changed, 124 insertions(+), 44 deletions(-) diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index 5fd00b70d6e..65ed5e1d0dc 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -1647,36 +1647,108 @@ let reinit ?(init_config=OpamInitDefaults.init_config()) ~interactive let has_space s = OpamStd.String.contains_char s ' ' -let get_redirected_root root = - let default = "C:\\opamroot" in +let default_redirect_root = OpamFilename.Dir.of_string "C:\\opamroot" + +let setup_redirection target = + let {contents = {OpamStateConfig.original_root_dir = root; _}} = + OpamStateConfig.r + in + let target = + match target with + | Some target -> target + | None -> + OpamFilename.mkdir default_redirect_root; + let readme = OpamFilename.Op.(default_redirect_root // "ReadMe.txt") in + if not (OpamFilename.exists readme) then + OpamFilename.write readme + "This directory is used to contain redirected opam roots.\n\n\ + The contents may be shared with other users on this system."; + OpamSystem.mk_unique_dir ~dir:(OpamFilename.Dir.to_string default_redirect_root) () + in + let root_dir = OpamFilename.Dir.of_string target in + OpamFilename.write (OpamPath.redirected root) target; + OpamStateConfig.update ~root_dir (); + root_dir + +let get_redirected_root () = + let {contents = {OpamStateConfig.original_root_dir = root; root_from; _}} = + OpamStateConfig.r + in + let r = OpamConsole.colorise `bold (OpamFilename.Dir.to_string root) in + let collision = + (* UTF-8 (Collision emoji) *) + if OpamConsole.color () then " \xF0\x9F\x92\xA5" else "" + in + let options = [ + `Redirect, Printf.sprintf + "Redirect files to a directory in %s" + (OpamConsole.colorise `bold (OpamFilename.Dir.to_string default_redirect_root)); + `Ask, "Redirect files to an alternate directory"; + `Endure, Printf.sprintf + "Do not redirect anything and stick with %s%s" r collision; + `Quit, "Abort initialisation" + ] in + let default, explanation = + match root_from with + | `Command_line -> + (* The user has been explicit with --root; nemo salvet modo... *) + `Endure, + "You have specified a root directory for opam containing a space." + | `Env -> + (* The user has perhaps carelessly set an environment variable *) + `Redirect, + "Your OPAMROOT environment variable contains a space." + | `Default -> + (* The user has fallen victim to the defaults of Windows Setup and has a + space in their user name *) + `Redirect, + Printf.sprintf + "By default, opam would store its data in:\n\ + %s\n\ + however, this directory contains a space." r + in let rec ask () = - match OpamConsole.read "Opam root: " with - | Some r -> - if has_space r then - (OpamConsole.msg - "Given path '%s' contains space, please choose another one.\n" - (OpamConsole.colorise `bold r); - ask ()) - else r - | None -> default - in - let new_root_f = - if OpamConsole.confirm ~default:false - "Your opam root path '%s' contains a space, we'll redirect to \ - '%s'.\nDo you want to choose and enter another spaceless folder?" - (OpamFilename.Dir.to_string root) default then - ask () - else default - in - let new_root = OpamFilename.Dir.of_string new_root_f in - OpamFilename.write (OpamPath.redirected root) new_root_f; - (* Add the readme file in C:\opamroot as redirected *) - OpamFilename.write - OpamFilename.Op.(root // "readme.txt") - (Printf.sprintf "Opam root redirected from %s" - (OpamFilename.Dir.to_string OpamStateConfig.(!r.root_dir))); - OpamStateConfig.update ~root_dir:new_root (); - new_root + let check r = + if Filename.is_relative r then begin + OpamConsole.msg + "That path is relative!\n\ + Please enter an absolute path without spaces.\n"; + ask () + end else if has_space r then begin + OpamConsole.msg + "That path contains contains a space!\n\ + Please enter an absolute path without spaces.\n"; + ask () + end else + Some (Some r) + in + OpamStd.Option.replace check (OpamConsole.read "Root directory for opam: ") + in + let rec menu () = + match OpamConsole.menu "Where should opam store files?" ~default ~options + ~no:default with + | `Redirect -> + Some None + | `Endure -> + None + | `Ask -> + let r = ask () in + if r = None then + menu () + else + r + | `Quit -> + OpamStd.Sys.exit_because `Aborted + in + OpamConsole.header_msg "opam root file store"; + OpamConsole.msg + "\n\ + %s\n\ + \n\ + Many parts of the OCaml ecosystem do not presently work correctly\n\ + when installed to directories containing spaces. You have been warned!%s\n\ + \n" explanation collision; + Option.map setup_redirection (menu ()) let init ~init_config ~interactive @@ -1697,16 +1769,22 @@ let init try f x with e -> OpamStd.Exn.fatal e in - if root_empty && - Sys.win32 && - has_space (OpamFilename.Dir.to_string root) then - let root = get_redirected_root root in + let new_root = + if root_empty && + Sys.win32 && + has_space (OpamFilename.Dir.to_string root) then + get_redirected_root () + else + None + in + match new_root with + | None -> + root, (fun () -> ignore_non_fatal OpamFilename.rmdir root) + | Some root -> root, (fun () -> ignore_non_fatal OpamFilename.rmdir root; ignore_non_fatal OpamFilename.rmdir original_root ) - else - root, (fun () -> ignore_non_fatal OpamFilename.rmdir root) in let config_f = OpamPath.config root in diff --git a/src/client/opamClientConfig.ml b/src/client/opamClientConfig.ml index 619897a60b9..5e4c6a09e74 100644 --- a/src/client/opamClientConfig.ml +++ b/src/client/opamClientConfig.ml @@ -210,15 +210,6 @@ let opam_init ?root_dir ?strict ?solver = (* (i) get root dir *) let root_from, root = OpamStateConfig.opamroot ?root_dir () in - if Sys.win32 - (* if default, redirection will be handled by opam init, or should have - been handled *) - && (root_dir <> None || OpamStateConfig.E.root () <> None) - && OpamStd.String.contains_char (OpamFilename.Dir.to_string root) ' ' then - OpamConsole.error "You opam root directory contains a space, this may lead \ - to several malfunction... bzzz.... nooo%s" - (* NOTE: UTF-8 Collision emoji *) - (if OpamConsole.color () then "\xF0\x9F\x92\xA5" else ""); (* (ii) load conf file and set defaults *) (* the init for OpamFormat is done in advance since (a) it has an effect on diff --git a/src/core/opamSystem.ml b/src/core/opamSystem.ml index 5fbd1a2518f..c326bb10a1c 100644 --- a/src/core/opamSystem.ml +++ b/src/core/opamSystem.ml @@ -88,6 +88,13 @@ let rec mk_temp_dir ?(prefix="opam") () = else real_path s +let rec mk_unique_dir ~dir ?(prefix="opam") () = + let s = dir / Printf.sprintf "%s-%06x" prefix (Random.int 0xFFFFFF) in + if Sys.file_exists s then + mk_unique_dir ~dir ~prefix () + else + real_path s + let safe_mkdir dir = try log "mkdir %s" dir; diff --git a/src/core/opamSystem.mli b/src/core/opamSystem.mli index 25dc6c1e043..f736089dff8 100644 --- a/src/core/opamSystem.mli +++ b/src/core/opamSystem.mli @@ -51,6 +51,10 @@ val verbose_for_base_commands: unit -> bool (if [prefix] is not set), pid, and random number. *) val mk_temp_dir: ?prefix:string -> unit -> string +(** Returns a directory name, in the [~dir], composed by {i opam} + (if [prefix] is not set), and a random number. *) +val mk_unique_dir: dir:string -> ?prefix:string -> unit -> string + (** [copy_file src dst] copies [src] to [dst]. Remove [dst] before the copy if it is a link. *) val copy_file: string -> string -> unit From f3c0b69abd2d53e8599f3c0e76548575d8aed696 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sun, 9 Jun 2024 10:56:06 +0100 Subject: [PATCH 28/31] WIP: update test The rm -rf C:\opamroot can't stay! --- tests/reftests/init.win32.test | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/reftests/init.win32.test b/tests/reftests/init.win32.test index 17c519fb7d7..daee99a94ef 100644 --- a/tests/reftests/init.win32.test +++ b/tests/reftests/init.win32.test @@ -3,21 +3,33 @@ N0REP0 ### rm -rf $OPAMROOT ### OPAMROOT="roots/with path" ### opam init --no-setup --bare --bypass-checks default REPO/ | grep -v Cygwin -[ERROR] You opam root directory contains a space, this may lead to several malfunction... bzzz.... nooo No configuration file found, using built-in defaults. -Your opam root path '${BASEDIR}/roots/with path' contains a space, we'll redirect to 'C:\opamroot'. -Do you want to choose and enter another spaceless folder? [y/n] n + +<><> opam root file store <><><><><><><><><><><><><><><><><><><><><><><><><><><> + +Your OPAMROOT environment variable contains a space. + +Many parts of the OCaml ecosystem do not presently work correctly +when installed to directories containing spaces. You have been warned! + +Where should opam store files? +> 1. Redirect files to a directory in C:\opamroot + 2. Redirect files to an alternate directory + 3. Do not redirect anything and stick with ${BASEDIR}/roots/with path + 4. Abort initialisation + +[1/2/3/4] 1 <><> Fetching repository information ><><><><><><><><><><><><><><><><><><><><><> [default] Initialised -### opam var root --debug --debug-level=-1 +### opam var root --debug --debug-level=-1 | '\\opam-.*' -> '' CLI Parsing CLI version 2.2 GSTATE LOAD-GLOBAL-STATE @ C:\opamroot C:\opamroot -### cat 'roots/with path/redirected-opamroot' +### cat 'roots/with path/redirected-opamroot' | '\\opam-.*' -> '' C:\opamroot ### opam switch create --empty test -### opam switch --debug --debug-level=-1 +### opam switch --debug --debug-level=-1 | '\\opam-.*' -> '' CLI Parsing CLI version 2.2 GSTATE LOAD-GLOBAL-STATE @ C:\opamroot SWITCH list From 67af380185b7949c9c29db69238bbfd5a21591f8 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sun, 17 Mar 2024 21:55:53 +0100 Subject: [PATCH 29/31] Hack: Displaying extractions in the status bar --- master_changes.md | 1 + src/repository/opamRepository.ml | 15 ++++++++++++++- tests/reftests/download.test | 8 ++++---- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/master_changes.md b/master_changes.md index 956e5371781..cad62daaa71 100644 --- a/master_changes.md +++ b/master_changes.md @@ -56,6 +56,7 @@ users) ## Update / Upgrade * [BUG] Stop triggering "Undefined filter variable variable" warning for `?variable` [#5983 @dra27] + * Display extractions in the status bar [#5977 @dra27] ## Tree * [BUG] Fix `opam tree --with-*` assigning the `with-*` variables to unrequested packages [#5919 @kit-ty-kate @rjbou - fix #5755] diff --git a/src/repository/opamRepository.ml b/src/repository/opamRepository.ml index 39531ace163..826257a14bc 100644 --- a/src/repository/opamRepository.ml +++ b/src/repository/opamRepository.ml @@ -263,9 +263,11 @@ let pull_tree_t | Some e -> Done (Not_available (None, Printexc.to_string e)) in match dirnames with - | [ _label, local_dirname, _subpath ] -> + | [ label, local_dirname, _subpath ] -> (fun archive msg -> OpamFilename.cleandir local_dirname; + let text = OpamProcess.make_command_text label "extract" in + OpamProcess.Job.with_text text @@ OpamFilename.extract_job archive local_dirname @@+ fallback (fun () -> Done (Up_to_date msg))) | _ -> @@ -285,6 +287,17 @@ let pull_tree_t (Some label, OpamProcess.result_summary r)))) dirnames in + let text = + let label = + match dirnames with + | [(label1, _, _); (label2, _, _)] -> + label1 ^ ", " ^ label2 + | (label, _, _)::rest -> + Printf.sprintf "%s + %d others" label (List.length rest) + | [] -> assert false in + OpamProcess.make_command_text label "extract" + in + OpamProcess.Job.with_text text @@ OpamFilename.extract_job archive tmpdir @@+ fallback (fun () -> let failing = diff --git a/tests/reftests/download.test b/tests/reftests/download.test index fd7c9de4e21..23172f3ff03 100644 --- a/tests/reftests/download.test +++ b/tests/reftests/download.test @@ -19,7 +19,7 @@ The following actions will be performed: <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><> Processing 1/1: [foo.1: http] curl-or-wget -- "https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz" -Processing 1/1: +Processing 1/1: [foo.1: extract] + tar "xfz" "${OPAMTMP}/v1.0.0.tar.gz" "-C" "${OPAMTMP}" Done. ### opam clean -c @@ -34,7 +34,7 @@ The following actions will be performed: <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><> Processing 1/1: [foo.1: http] + wget "--content-disposition" "-t" "3" "-O" "${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part" "-U" "opam/current" "--" "https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz" -Processing 1/1: +Processing 1/1: [foo.1: extract] + tar "xfz" "${OPAMTMP}/v1.0.0.tar.gz" "-C" "${OPAMTMP}" Done. ### opam clean -c @@ -49,7 +49,7 @@ The following actions will be performed: <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><> Processing 1/1: [foo.1: http] + curl "--write-out" "%{http_code}\n" "--retry" "3" "--retry-delay" "2" "--user-agent" "opam/current" "-L" "-o" "${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part" "--" "https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz" -Processing 1/1: +Processing 1/1: [foo.1: extract] + tar "xfz" "${OPAMTMP}/v1.0.0.tar.gz" "-C" "${OPAMTMP}" Done. ### opam clean -c @@ -148,7 +148,7 @@ The following actions will be performed: <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><> Processing 1/1: [foo.1: http] + wget "--content-disposition" "-t" "3" "-O" "${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part" "-U" "opam/current" "--" "https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz" -Processing 1/1: +Processing 1/1: [foo.1: extract] + tar "xfz" "${OPAMTMP}/v1.0.0.tar.gz" "-C" "${OPAMTMP}" Done. ### opam clean -c From fe46d0c98bfced28dd54c8922dff7affa0d6a56f Mon Sep 17 00:00:00 2001 From: Kate Date: Tue, 28 May 2024 12:32:23 +0100 Subject: [PATCH 30/31] reftest: Add a test showing the output of opam update in verbose mode --- tests/reftests/dune.inc | 18 ++++++++++++++++++ tests/reftests/update.test | 9 +++++++++ 2 files changed, 27 insertions(+) create mode 100644 tests/reftests/update.test diff --git a/tests/reftests/dune.inc b/tests/reftests/dune.inc index 42c8627c950..350c6ab0167 100644 --- a/tests/reftests/dune.inc +++ b/tests/reftests/dune.inc @@ -1436,6 +1436,24 @@ %{targets} (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:update-upgrade.test} %{read-lines:testing-env})))) +(rule + (alias reftest-update) + (action + (diff update.test update.out))) + +(alias + (name reftest) + (deps (alias reftest-update))) + +(rule + (targets update.out) + (deps root-N0REP0) + (package opam) + (action + (with-stdout-to + %{targets} + (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:update.test} %{read-lines:testing-env})))) + (rule (alias reftest-upgrade-format) (action diff --git a/tests/reftests/update.test b/tests/reftests/update.test new file mode 100644 index 00000000000..40c4cee8ce9 --- /dev/null +++ b/tests/reftests/update.test @@ -0,0 +1,9 @@ +N0REP0 +### +opam-version: "2.0" +### opam update --verbose + +<><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><> +Processing 1/1: [default: rsync] +[default] synchronised from file://${BASEDIR}/REPO +Now run 'opam upgrade' to apply any package updates. From eb49b3e242d7e6935677a4b6d706779b76348ff7 Mon Sep 17 00:00:00 2001 From: Kate Date: Mon, 27 May 2024 22:40:48 +0100 Subject: [PATCH 31/31] Display a note when reloading a repository so that users are aware it may take some time --- master_changes.md | 1 + src/state/opamRepositoryState.ml | 7 ++++++- tests/reftests/update.test | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/master_changes.md b/master_changes.md index cad62daaa71..5c1b37f9a0c 100644 --- a/master_changes.md +++ b/master_changes.md @@ -57,6 +57,7 @@ users) ## Update / Upgrade * [BUG] Stop triggering "Undefined filter variable variable" warning for `?variable` [#5983 @dra27] * Display extractions in the status bar [#5977 @dra27] + * Display a note when reloading a repository [#5977 @kit-ty-kate] ## Tree * [BUG] Fix `opam tree --with-*` assigning the `with-*` variables to unrequested packages [#5919 @kit-ty-kate @rjbou - fix #5755] diff --git a/src/state/opamRepositoryState.ml b/src/state/opamRepositoryState.ml index f3f84343bb5..bebc7d4e905 100644 --- a/src/state/opamRepositoryState.ml +++ b/src/state/opamRepositoryState.ml @@ -78,6 +78,9 @@ module Cache = struct end let load_opams_from_dir repo_name repo_root = + if OpamConsole.disp_status_line () || OpamConsole.verbose () then + OpamConsole.status_line "Processing: [%s: loading data]" + (OpamConsole.colorise `blue (OpamRepositoryName.to_string repo_name)); (* FIXME: why is this different from OpamPackage.list ? *) let rec aux r dir = if OpamFilename.exists_dir dir then @@ -104,7 +107,9 @@ let load_opams_from_dir repo_name repo_root = r fnames else r in - aux OpamPackage.Map.empty (OpamRepositoryPath.packages_dir repo_root) + Fun.protect + (fun () -> aux OpamPackage.Map.empty (OpamRepositoryPath.packages_dir repo_root)) + ~finally:OpamConsole.clear_status let load_repo repo repo_root = let t = OpamConsole.timer () in diff --git a/tests/reftests/update.test b/tests/reftests/update.test index 40c4cee8ce9..568829b1c1c 100644 --- a/tests/reftests/update.test +++ b/tests/reftests/update.test @@ -6,4 +6,5 @@ opam-version: "2.0" <><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><> Processing 1/1: [default: rsync] [default] synchronised from file://${BASEDIR}/REPO +Processing: [default: loading data] Now run 'opam upgrade' to apply any package updates.