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
travis: build with iconv enabled #1872
Conversation
... missed again :-( |
@bagder did you figure the error at getinfo.c:420:17 ? error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior |
@Frenche That's a false positive clang warning which only appears in version 3.9: |
Yes, it seems we need to inhibit that warning on clang 3.9. Otherwise this will annoy us quite a lot... |
5f36fe3
to
9796f5d
Compare
Squashed a little, rebased and now features |
Thanks @MarcelRaad for the explanation, and thanks @bagder for the fix :) |
@bagder Perhaps you could check for version >= 3.9 and < 4.0 instead of only >= 3.9? It's been fixed in 4.0 (present in 3.9.0 as well as 3.9.1). Being able to diagnose undefined behavior at compile time is pretty cool, I think, as long as there are no false positives :-) |
good point, I'll fix that and cherry-pick that fix into master, then rebase this |
4f89591
to
335ff6b
Compare
okay, the current red travis builds are actual test case failures that appear to be either bugs in curl with codeset conversion enabled, or they're bugs in the test cases. I believe each one needs to be researched individually to see where we need to fix or possibly even if the test needs to be disabled for this build setup. |
335ff6b
to
537d68a
Compare
The actual fixes have now been merged into master, while the add-to-travis commit is still left here due to the test failures we've seen. |
Putting this on hold. |
Data mismatch caused test 545 to fail when character encoding conversion is enabled. Bug: #1872
Problem with test 545 resolved by commit d3db7bc. Identified problem with test 555: when character conversion is enabled, data ends in CRLF instead of LF. How would you fix it:
|
I would pick one of your first two alternatives, yes. The one that makes most sense for what is being tested. When it is a libcurl test there's also the option to add more logic to the C code that can be #ifdef'ed or otherwise made conditional. |
If CURL_DOES_CONVERSION is enabled, uploaded LFs are mapped to CRLFs, giving a result that is different from what is expected. This commit avoids using CURLOPT_TRANSFERTEXT and directly encodes data to upload in ascii. Bug: #1872
Test 555 fixed a third way by commit bd5b9e5. I have preferred droping the conversion stuff in this test because it is not its target. The iconv travis job should succeed now. However there is a real mess for the Content-Length calculation when using CURLOPT_POSTFIELDSIZE + CURLOPT_TRANSFERTEXT/CURLOPT_CRLF and I raise a red flag about it. Should probably be the subject of an issue and additional tests. In addition, doc says CURLOPT_TRANSFERTEXT is for FTP only, while it also acts on http data (in transfer.c, so maybe other protocols) when CURL_DOES_CONVERSION is enabled. Please note also that with charconv enabled, a single LF is uploaded as CRCRLF if CURLOPT_TRANSFERTEXT and CURLOPT_CRLF are both enabled. There may be more undiscovered oddities around that... |
537d68a
to
30ac607
Compare
Rebased and pushed again. |
... to verify it builds and works fine. Ref: https://curl.haxx.se/mail/lib-2017-09/0031.html
30ac607
to
1de509c
Compare
... to verify it builds and works fine.
Ref: https://curl.haxx.se/mail/lib-2017-09/0031.html