config: remove CURL_SIZEOF_CURL_OFF_T use only SIZEOF_CURL_OFF_T #6702

bagder · 2021-03-08T11:32:38Z

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

bagder · 2021-03-08T13:11:50Z

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

mback2k · 2021-03-08T14:15:09Z

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:

curl/lib/curl_setup.h

Line 154 in 246399a

#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 · 2021-03-08T14:17:23Z

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 · 2021-03-08T14:49:05Z

@snikulov Can you bring any lights to this issue?

CMakeLists.txt

- 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 · 2021-03-08T22:15:41Z

Alternative take at #6704

bagder · 2021-03-09T07:24:16Z

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 · 2021-03-09T07:42:27Z

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 · 2021-03-09T07:52:17Z

The travis CI cmake output from the grep.

bagder · 2021-03-09T08:02:38Z

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.

snikulov · 2021-03-09T11:43:47Z

To be honest, I unable to help with this issue, because I'm not familiar with this code. Perhaps I did something for Windows, but definitely not touch this code for Linux. And looks like it either was broken or not worked since the beginning. But you're right feature Largefile is not enabled by default on x86_64 on current HEAD.

…

On Mon, Mar 8, 2021 at 5:49 PM Daniel Stenberg ***@***.***> wrote: @snikulov <https://github.com/snikulov> Can you bring any lights to this issue? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6702 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABCWKZCPM66H3C47IA6L3TTCTPXFANCNFSM4YZHP44A> .

-- Best Regards, Sergei Nikulov

bagder · 2021-03-09T16:30:31Z

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

bagder · 2021-03-09T16:31:36Z

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

bagder · 2021-03-09T16:51:30Z

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

mback2k · 2021-03-09T16:57:46Z

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

bagder · 2021-03-09T22:00:03Z

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

mback2k

So only travis CI is still failing now, but:

https://travis-ci.org/github/curl/curl/jobs/762229525 timeout seems unrelated
https://travis-ci.org/github/curl/curl/jobs/762229531 tests 1561 and 1562 failing seems unrelated

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

bagder · 2021-03-10T08:32:27Z

Yes, they seem unrelated

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

bagder · 2021-03-10T10:01:11Z

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

... as cmake now does it correctly, and make test1014 check for it Closes #6702

bagder added the tidy-up label Mar 8, 2021

bagder mentioned this pull request Mar 8, 2021

config: fix building SMB with configure using Win32 Crypto #6277

Closed

bagder removed the tidy-up label Mar 8, 2021

mback2k reviewed Mar 8, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

bagder force-pushed the bagder/sizeof-curl_off_t branch from 1212f3a to 036bfb8 Compare March 8, 2021 16:01

jay mentioned this pull request Mar 8, 2021

build: define CURL_SIZEOF_CURL_OFF_T using preprocessor #6704

Closed

bagder force-pushed the bagder/sizeof-curl_off_t branch from 239a129 to 3e95dc4 Compare March 8, 2021 23:46

config: remove CURL_SIZEOF_CURL_OFF_T use only SIZEOF_CURL_OFF_T

8979632

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

bagder force-pushed the bagder/sizeof-curl_off_t branch from 3e95dc4 to cbf8622 Compare March 9, 2021 10:39

bagder force-pushed the bagder/sizeof-curl_off_t branch from cbf8622 to 5dd71b7 Compare March 9, 2021 16:53

configure: provide Largefile feature

ec81a3b

and make test1014 check for it

bagder force-pushed the bagder/sizeof-curl_off_t branch from 5dd71b7 to ec81a3b Compare March 9, 2021 23:39

mback2k approved these changes Mar 10, 2021

View reviewed changes

bagder closed this in 78f642f Mar 11, 2021

bagder added a commit that referenced this pull request Mar 11, 2021

configure: provide Largefile feature for curl-config

dcccd8d

... as cmake now does it correctly, and make test1014 check for it Closes #6702

bagder deleted the bagder/sizeof-curl_off_t branch March 11, 2021 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: remove CURL_SIZEOF_CURL_OFF_T use only SIZEOF_CURL_OFF_T #6702

config: remove CURL_SIZEOF_CURL_OFF_T use only SIZEOF_CURL_OFF_T #6702

bagder commented Mar 8, 2021

bagder commented Mar 8, 2021

mback2k commented Mar 8, 2021 •

edited

mback2k commented Mar 8, 2021

bagder commented Mar 8, 2021

jay commented Mar 8, 2021

bagder commented Mar 9, 2021

mback2k commented Mar 9, 2021

bagder commented Mar 9, 2021

bagder commented Mar 9, 2021

snikulov commented Mar 9, 2021 via email •

edited

bagder commented Mar 9, 2021

bagder commented Mar 9, 2021

bagder commented Mar 9, 2021

mback2k commented Mar 9, 2021

bagder commented Mar 9, 2021 •

edited

mback2k left a comment

bagder commented Mar 10, 2021

bagder commented Mar 10, 2021

config: remove CURL_SIZEOF_CURL_OFF_T use only SIZEOF_CURL_OFF_T #6702

config: remove CURL_SIZEOF_CURL_OFF_T use only SIZEOF_CURL_OFF_T #6702

Conversation

bagder commented Mar 8, 2021

bagder commented Mar 8, 2021

mback2k commented Mar 8, 2021 • edited

mback2k commented Mar 8, 2021

bagder commented Mar 8, 2021

jay commented Mar 8, 2021

bagder commented Mar 9, 2021

mback2k commented Mar 9, 2021

bagder commented Mar 9, 2021

bagder commented Mar 9, 2021

snikulov commented Mar 9, 2021 via email • edited

bagder commented Mar 9, 2021

bagder commented Mar 9, 2021

bagder commented Mar 9, 2021

mback2k commented Mar 9, 2021

bagder commented Mar 9, 2021 • edited

mback2k left a comment

Choose a reason for hiding this comment

bagder commented Mar 10, 2021

bagder commented Mar 10, 2021

mback2k commented Mar 8, 2021 •

edited

snikulov commented Mar 9, 2021 via email •

edited

bagder commented Mar 9, 2021 •

edited