-
Notifications
You must be signed in to change notification settings - Fork 364
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
Mitigate problems with curl on Windows 11 #6168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I added a commit (feel free to remove it if you disagree) to remove the EDIT: oops sorry i'm tired, nevermind ^^"with_curl_mitigation
argument which seems to me like something we probably want to do everytime. The other places that use it do not seem to use the error code at all.
To me it looks good to go once rebased (I would also suggest squashing everything into one commit as the first commit makes more sense combined with the second)
Thanks, @kit-ty-kate! FWIW, the point in two commits is entirely because the first commit makes sense on its own (and simplifies the diff of the actual diff in the second): if foo then
raise Bar;
baz () is functionally inferior to: if foo then
raise Bar
else
baz () especially when considering an alteration to the first branch of the |
Thanks, @rjbou - changes applied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Mitigates a known issue in curl 8.8.0 especially affecting Windows hosts. cf curl/curl#13845
Fixes #6120 by altering the call to curl. curl/curl#13845 can be mitigated by avoiding the use of
--write-out
so, having detected exit code 43 (which should never happen using the curl binary, ascurl
itself should normally know how to call its own functions!), a second call is made to call using--fail
instead of--write-out
.--fail
was previously considered in #467 (comment). I've done some digging, and the semantics of--fail
are good enough for what we're after, though using--write-out
as in @samoht's f2ee0ec is superior in general because it conveys more information to the user when an error occurs (which was the point).The bug in cURL is causing enough issues elsewhere that I imagine Microsoft will update the
curl.exe
binary shipped with Windows before long - if they don't, it's easy to extend the mitigation further to extract the actual http response code from the error message, as the format of that message has never altered, from a look at cURL's sources.There are three reasons to prefer this slightly more complicated fix to just adding the requirement for cURL from Cygwin/MSYS2 as in #6142:
curl
because Cygwin's SSL certificate doesn't contain the offending OIDS. If that were to change, the workaround would fail (because we have to download Cygwin's setup usingC:\Windows\System32\curl.exe
)Running
opam init --debug -vv
on my Windows 11 laptop with this binary we can now see:downloading Cygwin with no issues at all, but then mitigation kicking in for opam.ocaml.org: