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

travis: build with iconv enabled #1872

Closed
wants to merge 1 commit into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Sep 7, 2017

... to verify it builds and works fine.

Ref: https://curl.haxx.se/mail/lib-2017-09/0031.html

@monnerat
Copy link
Contributor

monnerat commented Sep 7, 2017

$ export CPPFLAGS=\
$ export DCURL_ICONV_CODESET_OF_HOST='\"ISO8859-1\"'

... missed again :-(

@iboukris
Copy link
Contributor

iboukris commented Sep 8, 2017

@bagder did you figure the error at getinfo.c:420:17 ?
I got the same on my PR and I can't figure what triggered it.

error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior

@MarcelRaad
Copy link
Member

MarcelRaad commented Sep 8, 2017

@Frenche That's a false positive clang warning which only appears in version 3.9:
https://bugs.llvm.org/show_bug.cgi?id=29140
Seems like Travis changed the default clang version from 3.5 to 3.9. Maybe we should disable that warning for version 3.9.

@bagder
Copy link
Member Author

bagder commented Sep 8, 2017

Yes, it seems we need to inhibit that warning on clang 3.9. Otherwise this will annoy us quite a lot...

@bagder
Copy link
Member Author

bagder commented Sep 8, 2017

Squashed a little, rebased and now features -Wno-varargs on clang debug builds...

@iboukris
Copy link
Contributor

iboukris commented Sep 8, 2017

Thanks @MarcelRaad for the explanation, and thanks @bagder for the fix :)

@MarcelRaad
Copy link
Member

MarcelRaad commented Sep 8, 2017

@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 :-)

@bagder
Copy link
Member Author

bagder commented Sep 8, 2017

good point, I'll fix that and cherry-pick that fix into master, then rebase this

@bagder bagder force-pushed the bagder/travis-iconv branch 3 times, most recently from 4f89591 to 335ff6b Compare September 11, 2017 08:24
@bagder
Copy link
Member Author

bagder commented Sep 12, 2017

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.

@bagder
Copy link
Member Author

bagder commented Sep 15, 2017

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.

@bagder
Copy link
Member Author

bagder commented Jan 25, 2018

Putting this on hold.

@bagder bagder closed this Jan 25, 2018
@jay jay added the on-hold label Jan 25, 2018
monnerat added a commit that referenced this pull request Jan 26, 2018
Data mismatch caused test 545 to fail when character encoding
conversion is enabled.

Bug: #1872
@monnerat
Copy link
Contributor

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.
This is already fixed by a strippart, but the content length mismatches.

How would you fix it:

  • Always send data ending with CRLF.
  • Ignore Content-Length (multiple occurrences) with a strippart.
  • Something else ?

@bagder
Copy link
Member Author

bagder commented Jan 26, 2018

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.

monnerat added a commit that referenced this pull request Jan 26, 2018
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
@monnerat
Copy link
Contributor

monnerat commented Jan 26, 2018

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...

@bagder
Copy link
Member Author

bagder commented Jan 26, 2018

Rebased and pushed again.

@bagder bagder closed this in bb50177 Feb 15, 2018
@bagder bagder deleted the bagder/travis-iconv branch February 15, 2018 13:18
@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants