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

config: remove CURL_SIZEOF_CURL_OFF_T use only SIZEOF_CURL_OFF_T #6702

Closed
wants to merge 2 commits into from

Conversation

@bagder
Copy link
Member

@bagder bagder commented Mar 8, 2021

Make the code consistently use a single name for the size of the curl_off_t type.

@bagder
Copy link
Member Author

@bagder bagder commented Mar 8, 2021

The travis cmake build now fails to set the largefile bit, but it works fine for me locally. 😕

@mback2k
Copy link
Member

@mback2k mback2k commented Mar 8, 2021

The same is true for these AppVeyor builds due to:

test 1014...[Compare curl --version with curl-config --features]
  Mismatch in features lists:
  curl:        alt-svc asynchdns ipv6 ntlm
  curl-config: alt-svc asynchdns ipv6 largefile ntlm
   postcheck FAILED

This is the same issue as I experienced with my attempt in #6277. I don't understand this issue, because the condition should end up being the same as in the adjusted CMake file:

#define CURL_SIZEOF_CURL_OFF_T SIZEOF_CURL_OFF_T

curl/lib/version.c

Lines 402 to 405 in b899cb0

#if (CURL_SIZEOF_CURL_OFF_T > 4) && \
( (SIZEOF_OFF_T > 4) || defined(USE_WIN32_LARGE_FILES) )
| CURL_VERSION_LARGEFILE
#endif

@mback2k
Copy link
Member

@mback2k mback2k commented Mar 8, 2021

I would actually consider this a fix instead of a tidy-up, because previously Largefile support was not detected at all with CMake, because CURL_SIZEOF_CURL_OFF_T is not available in that, just SIZEOF_CURL_OFF_T.

@bagder
Copy link
Member Author

@bagder bagder commented Mar 8, 2021

@snikulov Can you bring any lights to this issue?

@bagder bagder removed the tidy-up label Mar 8, 2021
CMakeLists.txt Outdated Show resolved Hide resolved
@bagder bagder force-pushed the bagder/sizeof-curl_off_t branch from 1212f3a to 036bfb8 Mar 8, 2021
jay added a commit to jay/curl that referenced this issue Mar 8, 2021
- Use preprocessor macros to determine CURL_SIZEOF_CURL_OFF_T,
  CURL_OFF_T_MAX and CURL_OFF_T_MIN and expose them to the user.

- Drop SIZEOF_CURL_OFF_T in favor of CURL_SIZEOF_CURL_OFF_T.

Prior to this change CURL_SIZEOF_CURL_OFF_T was defined depending on the
build system at configure time or build time depending on how it was
built. The macros were not available to the user.

draft, alternative to curl#6702
jay added a commit to jay/curl that referenced this issue Mar 8, 2021
- Use preprocessor macros to determine CURL_SIZEOF_CURL_OFF_T,
  CURL_OFF_T_MAX and CURL_OFF_T_MIN and expose them to the user.

- Drop SIZEOF_CURL_OFF_T in favor of CURL_SIZEOF_CURL_OFF_T.

Prior to this change CURL_SIZEOF_CURL_OFF_T was defined depending on the
build system at configure time or build time depending on how it was
built. The macros were not available to the user.

draft, alternative to curl#6702
@jay
Copy link
Member

@jay jay commented Mar 8, 2021

Alternative take at #6704

@bagder bagder force-pushed the bagder/sizeof-curl_off_t branch from 239a129 to 3e95dc4 Mar 8, 2021
@bagder
Copy link
Member Author

@bagder bagder commented Mar 9, 2021

With the new debug output in the cmake travis CI build showing all the SIZEOF sizes after cmake has run shows they're present in lib/curl_config.h all right...

@mback2k
Copy link
Member

@mback2k mback2k commented Mar 9, 2021

What about USE_WIN32_LARGE_FILES? Is that present? Can you please link the output in the travis jobs? And I looked at the AppVeyor jobs and unfortunately they either failed, because #warn is not understood or CMake build is hiding the output of that.

Looking at the code in version.c it should be working... 😞

@bagder
Copy link
Member Author

@bagder bagder commented Mar 9, 2021

The travis CI cmake output from the grep.

@bagder
Copy link
Member Author

@bagder bagder commented Mar 9, 2021

I wanted to focus on getting the travis cmake build to work first, because that's on Linux and I feel at home there (contrary to anything Windows). But since not even that works even if the two important size defines are set in the include file, it really puzzles me.

Make the code consistently use a single name for the size of the
"curl_off_t" type.
@bagder bagder force-pushed the bagder/sizeof-curl_off_t branch from 3e95dc4 to cbf8622 Mar 9, 2021
@snikulov
Copy link
Member

@snikulov snikulov commented Mar 9, 2021

@bagder
Copy link
Member Author

@bagder bagder commented Mar 9, 2021

Even when I reversed the conditional check in lib/version.c test 1014 fails the same way on both travis and appveyor!

@bagder
Copy link
Member Author

@bagder bagder commented Mar 9, 2021

The irony here is of course that I doubt we ever build on systems these days that don't support "large files"...

@bagder
Copy link
Member Author

@bagder bagder commented Mar 9, 2021

I figured it out and it was sillier than you'd expect:

Largefile is not generated as a feature for curl-config by configure, so therefore, the test 1014 scripts removes the word from the curl -V output before it compares it with the curl-config --features output...

@bagder bagder force-pushed the bagder/sizeof-curl_off_t branch from cbf8622 to 5dd71b7 Mar 9, 2021
@mback2k
Copy link
Member

@mback2k mback2k commented Mar 9, 2021

Wouldn't it be better to fix it the other way around then?

@bagder
Copy link
Member Author

@bagder bagder commented Mar 9, 2021

Yes, but I think I'll work on that separately once this is fixed, as I believe that provides another opportunity for falling down a bottomless pit! 😆

Nah, let's do it right at once.

and make test1014 check for it
@bagder bagder force-pushed the bagder/sizeof-curl_off_t branch from 5dd71b7 to ec81a3b Mar 9, 2021
Copy link
Member

@mback2k mback2k left a comment

So only travis CI is still failing now, but:

both issues are also happening in current master for some reason.

@bagder
Copy link
Member Author

@bagder bagder commented Mar 10, 2021

Yes, they seem unrelated

#6705 works on the torture test fails
The arm64 tests are flaky since before

@bagder
Copy link
Member Author

@bagder bagder commented Mar 10, 2021

@jay filed #6704 in the mean time which partly goes in a contrary direction than this so we need to decide.

I like this PR because it both simplifies and unifies the sizeof defines for curl_off_t.

@bagder bagder closed this in 78f642f Mar 11, 2021
bagder added a commit that referenced this issue Mar 11, 2021
... as cmake now does it correctly, and make test1014 check for it

Closes #6702
@bagder bagder deleted the bagder/sizeof-curl_off_t branch Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants