Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add curl to the list of required cygwin packages to avoid issues with Windows' curl #6142

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Aug 7, 2024

Fixes #6120
Backported in opam 2.2 by #6143

@dra27
Copy link
Member

dra27 commented Aug 7, 2024

I think this is probably the way it'll have to go - two things which ought to be checked:

  • Does opam init --reinit catch this (e.g. external Cygwin or MSYS2 which doesn't have curl installed; opam init with 2.2.0; does opam init --reinit prompt to add the curl package
  • Is it still possible to use OPAMFETCH to force the use of either wget or the system's curl (it ought to be; I just haven't checked)

@kit-ty-kate
Copy link
Member Author

  • Does opam init --reinit catch this (e.g. external Cygwin or MSYS2 which doesn't have curl installed; opam init with 2.2.0; does opam init --reinit prompt to add the curl package

I've tried this and it's not pulled by opam init --reinit if curl is missing. I'll have a look at a solution

  • Is it still possible to use OPAMFETCH to force the use of either wget or the system's curl (it ought to be; I just haven't checked)

OPAMCURL=C:\Windows\System32\curl.exe and OPAMFETCH=wget both worked as expected

@kit-ty-kate kit-ty-kate requested a review from dra27 August 7, 2024 15:39
@kit-ty-kate
Copy link
Member Author

The last commit makes opam init --reinit re-run setup.exe and after trying it locally i can confirm this will reinstall curl if it is not installed yet.

@kit-ty-kate
Copy link
Member Author

Note for reviewers: the diff is easier to read with the "Hide whitespace" option on.

@dra27
Copy link
Member

dra27 commented Aug 7, 2024

Thanks, @kit-ty-kate! I think the additional code for opam init --reinit will be worth checking and integrating regardless, but I just found curl/curl#13845 ... which makes sense, since Windows 11 has cURL 8.8.0, but my Windows 10 box has 8.0.1.

@kit-ty-kate
Copy link
Member Author

but I just found curl/curl#13845 ... which makes sense, since Windows 11 has cURL 8.8.0, but my Windows 10 box has 8.0.1.

ah interesting, so should we just wait for Microsoft to fix this? I'm not sure how frequent are their updates though

@dra27
Copy link
Member

dra27 commented Aug 8, 2024

I don't know what the policy (or if it's published), but the current version installed is from May 2024, so it's at least happened once recently! The problem with "forcing" Cygwin's curl is the same as with git - it's the wrong SSL stack.

I just had a closer look at the --reinit part - it's not quite what I was thinking... I (think I!) was thinking that this part:

opam/src/client/opamClient.ml

Lines 1504 to 1516 in 6effccc

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_is_tweakable && not OpamStateConfig.(!r.no_depexts) then
OpamInitDefaults.required_packages_for_cygwin
else
[]
in
mechanism, cygwin_packages

would want altering so that even with an internal installation it does the depext check at:

check_for_sys_packages config system_packages;

would automatically add the new packages - i.e. doing it in OpamClient rather than at OpamSysInteract. I'm not sure, but does the present commit mean that opam init --reinit now always re-runs Cygwin setup?

@kit-ty-kate
Copy link
Member Author

I'm not sure, but does the present commit mean that opam init --reinit now always re-runs Cygwin setup?

It does but i personally view it as a feature. As far as i know we currently have no way to update cygwin itself except by running the setup.exe binary itself and give all the correct paths to it, or by installing a depext. I feel like having opam init --reinit do that would be fairly reasonable.
Although looking into it more it seems that it's not the case here either but it should be if --upgrade-also where given to the setup.exe incantation 🪄

@dra27
Copy link
Member

dra27 commented Aug 20, 2024

Yes, indeed - I think re-running setup for the internal Cygwin installation is a good idea .. but, as you note, it needs --upgrade-also! However, the check higher up is also needed for the MSYS2/external-Cygwin case.

@kit-ty-kate kit-ty-kate marked this pull request as draft August 21, 2024 10:50
@kit-ty-kate
Copy link
Member Author

Converted to draft as i think the original issue is better fixed by #6168. However i'm keeping this around as we might need want to think about the upgrade mechanism again at some point

@kit-ty-kate kit-ty-kate removed this from the 2.3.0~alpha milestone Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error : [OPAM INIT] - Platform: Windows 11 HOME - Version: 23h2
2 participants