Fix #10252: cabal upload fails with curl chunked-encoding error - #12211
Conversation
|
Hello and thanks for your contribution. This PR is missing a template (here). |
|
wow, if this really fixes it, it's a big one. Maybe we should even consider a lightning 3.18.1.1. release.. And it only touches the tool |
|
Yay! BTW, a changelog snippet would also be welcome. |
|
Hello,
https://github.com/tusharad/sql2er/actions/runs/31173210576/job/92849383471 @Mikolaj is this changelog enough? https://github.com/haskell/cabal/pull/12211/changes#diff-60db094ee10f0a5f3f9cd8a8a15959da265886a020fd4f55d6028f80846a5dbe @ffaf1 @ulysses4ever can you review and approve this PR? |
Looks good, thank you. |
|
Will do. Hopefully on the weekend |
Is three any info on which curl do and don't work? Does this patch support both of these cases or only newer curl? |
ffaf1
left a comment
There was a problem hiding this comment.
I left some comments. I am aware not all code is yours, some is just shuffled around, but if we are refactoring stuff (good!) it might be a good opportunity to clean some code.
ulysses4ever
left a comment
There was a problem hiding this comment.
We could have merged it in an instant if you had just kicked out the \n. Instead, you introduced a subtle refactoring that'a completely unrelated, and we'll spend much more time reviewing it. Please, don't introduce unrelated changes in one PR especially when you're fixing high-priority bugs like this one.
The best way to proceed would be to reset this patch to a state when it only removes the \n. And leave all rest for another PR. I'm not saying you must do it, it's just the easiest and most effective. But if you prefer keeping it as one PR, we can work with it.
|
Hello @ulysses4ever and @ffaf1 https://github.com/tusharad/sql2er/actions/runs/31257218055 - Seems like curl 7.6 to 8.5 are breaking while passing on latest cabal. my fixed is passing on all - https://github.com/tusharad/sql2er/actions/runs/31257224015 Btw on my mac, the issue is not appearing at all. Linux specific? |
03a4df4 to
2e9536d
Compare
There are no tests in this PR. |
723e949 to
e2d6813
Compare
49ef270 to
24f4aa8
Compare
My bad, the commit revert had it removed. Added them now. @ffaf1 |
|
@ffaf1 I think it can be merged? |
|
Sure, I have put the |
Merge Queue Status
This pull request spent 1 hour 55 minutes 11 seconds in the queue, including 1 hour 40 minutes 55 seconds running CI. Required conditions to merge
|
|
@mergify backport 3.18 |
✅ Backports have been createdDetails
|
haskell#12211) * fix: remove newline from curl --write-out option in posthttpfile and puthttpfile * test: add unit tests for HttpUtils and document fix for curl chunked-encoding upload failures * chore: update synopsis for cabal upload error fix in changelog * refactor: update HTTP status code extraction to parse the last three characters of the response body * docs: clarify status code extraction logic in HttpUtils curl response parsing (cherry picked from commit ca805f1)


Fixes #10252.
Description
Newer versions of
curlmisinterpret the literal\ncharacter embedded within the--write-out "\n%{http_code}"argument during chunked-encoding transfers (such asPOSTandPUTrequests used incabal upload). This results incurl: (56) Illegal or missing hexadecimal sequence in chunked-encodingerrors.This PR:
--write-outparameter for bothposthttpfileandputhttpfile.parseResponseincurlTransport(and extracts it asparseCurlResponse) to extract the 3-digit HTTP status code directly from the end of the response string, avoiding the need to split the string by lines.parseCurlResponseinUnitTests.Distribution.Client.HttpUtils.QA Notes
Calling
cabal upload <package>.tar.gzon a system with a newer version ofcurlinstalled should succeed without throwing acurl: (56)chunked-encoding error, correctly parsing the upload status code.Template Α: This PR modifies behaviour or interface
significance: significantin the changelog file. (Note: Not checked as this is a bug fix, but you may check this if you feel it's a highly visible bug fix)