travis: build with iconv enabled #1872

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@bagder
Member

bagder commented Sep 7, 2017

... to verify it builds and works fine.

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

@monnerat

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat Sep 7, 2017

Collaborator

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

... missed again :-(

Collaborator

monnerat commented Sep 7, 2017

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

... missed again :-(

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Sep 8, 2017

Contributor

@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

Contributor

frenche 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

This comment has been minimized.

Show comment
Hide comment
@MarcelRaad

MarcelRaad Sep 8, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 8, 2017

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 8, 2017

Member

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

Member

bagder commented Sep 8, 2017

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

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Sep 8, 2017

Contributor

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

Contributor

frenche commented Sep 8, 2017

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

@MarcelRaad

This comment has been minimized.

Show comment
Hide comment
@MarcelRaad

MarcelRaad Sep 8, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 8, 2017

Member

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

Member

bagder commented Sep 8, 2017

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

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 12, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 15, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 25, 2018

Member

Putting this on hold.

Member

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

lib544: sync ascii code data with textual data
Data mismatch caused test 545 to fail when character encoding
conversion is enabled.

Bug: #1872
@monnerat

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat Jan 26, 2018

Collaborator

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 ?
Collaborator

monnerat commented Jan 26, 2018

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 26, 2018

Member

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.

Member

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

lib555: drop text conversion and encode data as ascii codes
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

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat Jan 26, 2018

Collaborator

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

Collaborator

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 bagder reopened this Jan 26, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 26, 2018

Member

Rebased and pushed again.

Member

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 Feb 15, 2018

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