build: delete checks for C89 standard headers #11940

vszakats · 2023-09-25T20:11:11Z

Delete checks and guards for standard C89 headers and assume these are
available: stdio.h, string.h, time.h, setjmp.h, stdlib.h,
stddef.h, signal.h

Some of these we already used unconditionally, some others we only used
for feature checks.

Follow-up to 9c7165e #11918 (for stdio.h in CMake)

Closes #11940

ncopa · 2023-09-25T20:55:56Z

stdio.h is needed to detect fseeko, both on MacOS and Linux.

-- Looking for fseeko
-- Looking for fseeko - not found

vszakats · 2023-09-25T21:25:16Z

@ncop: Do you think this solution would also work?:

check_symbol_exists(fseeko         "${CURL_INCLUDES};stdio.h" HAVE_FSEEKO)
check_symbol_exists(_fseeki64      "${CURL_INCLUDES};stdio.h" HAVE__FSEEKI64)

This also self-documents that stdio.h is necessary for detection, without adding it for every other check by default.

vszakats · 2023-09-25T21:28:31Z

@bagder: I extended this PR with other C89 headers that we're checking for, most likely unnecessarily. I'm looking into signal.h and plan to add it shorly.

vszakats · 2023-09-25T21:43:37Z

ea8fbb5 (from 2008) suggests that a system called "Cell" might not actually come with a C89 signal.h. According to the original report this is CellOS for PlayStation 3. Could this be still relevant (or on other systems) to keep checking for signal.h?

jay · 2023-09-25T22:27:19Z

c89 4.1.2 standard headers:

         <assert.h>                 <locale.h>                 <stddef.h>
         <ctype.h>                  <math.h>                   <stdio.h>
         <errno.h>                  <setjmp.h>                 <stdlib.h>
         <float.h>                  <signal.h>                 <string.h>
         <limits.h>                 <stdarg.h>                 <time.h>

regarding HAVE__FSEEKI64 see #11944

`HAVE_STDIO_H` is not used in the source. `stdio.h` is a standard C89 header and we use it everywhere unconditionally. Ref: https://github.com/curl/curl/pull/11918/files#r1336337304 Regression from 9c7165e curl#11918 Closes #xxxxx

vszakats · 2023-09-25T23:13:25Z

I plan to follow this up with a patch to move CMake global standard headers (string.h, stdlib.h, stddef.h) to the specific detections where they are actually needed (if they are) based on autotools logic. Also found an incomplete/broken detection in CMake and one missing compared to autotools.

ncopa · 2023-09-26T06:37:49Z

@ncop: Do you think this solution would also work?:
check_symbol_exists(fseeko         "${CURL_INCLUDES};stdio.h" HAVE_FSEEKO)
check_symbol_exists(_fseeki64      "${CURL_INCLUDES};stdio.h" HAVE__FSEEKI64)
This also self-documents that stdio.h is necessary for detection, without adding it for every other check by default.

Yes, I would expect that work work, and according the logs it does:

-- Looking for fseeko
-- Looking for fseeko - found

I agree that it is better this way.

ncopa · 2023-09-26T06:41:35Z

Also found an incomplete/broken detection in CMake and one missing compared to autotools.

I only discovered the broken test of fseeko, because I was looking at the logs. It is fragile because the fallback code also works, so a broken test can easily go undetected.

Maybe it would be an idea to add a test to CI to run both configure and cmake and compare the generated curl_config.h?

bagder · 2023-09-26T07:16:12Z

Maybe it would be an idea to add a test to CI to run both configure and cmake and compare the generated curl_config.h?

That's a good idea. It will of course only test the exact options used in the test and we can run a few different ones, but it should still help us.

vszakats · 2023-09-26T09:31:08Z

Such detection comparison test would be very useful indeed.

I missed it in 96c2990 curl#11940

Before this patch we added standard headers unconditionally to the global list of headers used for feature checks. This is unnecessary and also doesn't help CMake 'Generate' performance. This patch moves these headers to each feature check where they are actually needed. Stop using `stddef.h`, as it seems unnecessary. I've used autotools' `m4/curl-functions.m4` to figure out these dependencies. Also delete checking for the C89 standard header `time.h`, that I missed in the earlier commit. Ref: 96c2990 #11940 Closes #11951

MarcelRaad · 2023-09-27T08:19:30Z

m4/curl-functions.m4

 #ifdef HAVE_STRINGS_H
 #  include <strings.h>
 #endif
 /* includes end */"
  AC_CHECK_HEADERS(
-    sys/types.h string.h strings.h,


This seems to break strerror_r detection on Solaris, visible as warnings in the autobuilds since yesterday:
https://curl.se/dev/log.cgi?id=20230927074238-1773871#prob1

configure: WARNING: cannot determine strerror_r() style: edit lib/curl_config.h manually.

Is it possible Solaris doesn't have a string.h? Or it something else? Is there a way to pull or view config.log of a failed build?

According to this, strerror_r is declared in string.h: https://docs.oracle.com/cd/E86824_01/html/E54766/strerror-r-3c.html

Possibly related (caused by my bad edit): d14089d

This made the getaddrinfo detection fail, but we did not spot it in the CI because it graciously falled back to using legacy functions instead! Follow-up to 96c2990 (#11940)

This made the getaddrinfo detection fail, but we did not spot it in the CI because it graciously falled back to using legacy functions instead! Follow-up to 96c2990 (#11940) Closes #11965

Follow-up to 96c2990 (curl#11940) Closes #xxxxx

Follow-up to 96c2990 #11940 Closes #11966

We removed C89 `setjmp.h` and `signal.h` detections and excluded them from the global header list we use for detecting functions [1]. Then missed to re-add these headers to the specific functions which need them to be detected [2]. Fix this omission in this patch. `sigsetjmp` has a second detection attempt documented as 'special' in the comment. However this detection uses the same method as the first attempt, with the difference of using `setjmp.h` header only, without including the rest of non-standard headers detected. It detects both function and macro like the first attempt. This second attempt seems redundant. It's been there since the initial commit of CMake support. [1] Follow-up to 3795fcd curl#11951 [2] Follow-up to 96c2990 curl#11940 Closes #xxxxx

We removed C89 `setjmp.h` and `signal.h` detections and excluded them from the global header list we use when detecting functions [1]. Then missed to re-add these headers to the specific functions which need them to be detected [2]. Fix this omission in this patch. [1] Follow-up to 3795fcd #11951 [2] Follow-up to 96c2990 #11940 Closes #12043

vszakats added the tidy-up label Sep 25, 2023

vszakats changed the title ~~cmake: delete check for stdio.h~~ cmake: delete check for stdio.h Sep 25, 2023

github-actions bot added build cmake labels Sep 25, 2023

vszakats marked this pull request as draft September 25, 2023 20:17

bagder approved these changes Sep 25, 2023

View reviewed changes

vszakats force-pushed the cmake-stdioh branch from 7f2af51 to 59cfdbe Compare September 25, 2023 21:19

github-actions bot added the tests label Sep 25, 2023

vszakats changed the title ~~cmake: delete check for stdio.h~~ build: delete checks for C89 standard headers Sep 25, 2023

vszakats force-pushed the cmake-stdioh branch from 59cfdbe to 0e07821 Compare September 25, 2023 22:05

github-actions bot added the cmdline tool label Sep 25, 2023

vszakats marked this pull request as ready for review September 25, 2023 22:15

vszakats added 7 commits September 25, 2023 22:44

cmake: delete check for stdio.h

474a5f9

`HAVE_STDIO_H` is not used in the source. `stdio.h` is a standard C89 header and we use it everywhere unconditionally. Ref: https://github.com/curl/curl/pull/11918/files#r1336337304 Regression from 9c7165e curl#11918 Closes #xxxxx

assume C89 string.h

9586590

cmake: assume C89 time.h

4918ba0

assume C89 setjmp.h

8603cbb

assume C89 stdlib.h

621a98a

cmake: assume C89 stddef.h

ce2ca28

assume C89 signal.h

fe35197

vszakats force-pushed the cmake-stdioh branch from 0e07821 to fe35197 Compare September 25, 2023 23:09

vszakats closed this in 96c2990 Sep 26, 2023

vszakats deleted the cmake-stdioh branch September 26, 2023 14:27

vszakats mentioned this pull request Sep 26, 2023

cmake: move global headers to specific checks #11951

Closed

vszakats added a commit to vszakats/curl that referenced this pull request Sep 26, 2023

also fixup time.h

91e0ac6

I missed it in 96c2990 curl#11940

MarcelRaad reviewed Sep 27, 2023

View reviewed changes

bagder added a commit that referenced this pull request Sep 27, 2023

curl-functions.m4: fix include line

267bb78

This made the getaddrinfo detection fail, but we did not spot it in the CI because it graciously falled back to using legacy functions instead! Follow-up to 96c2990 (#11940)

bagder mentioned this pull request Sep 27, 2023

curl-functions.m4: fix include line #11965

Closed

vszakats added a commit to vszakats/curl that referenced this pull request Sep 27, 2023

autotools: fixup bad edits

13a4ab1

Follow-up to 96c2990 (curl#11940) Closes #xxxxx

vszakats mentioned this pull request Sep 27, 2023

curl-functions.m4: fixup recent bad edits #11966

Closed

vszakats added a commit that referenced this pull request Sep 27, 2023

curl-functions.m4: fixup recent bad edits

a7f8d04

Follow-up to 96c2990 #11940 Closes #11966

vszakats mentioned this pull request Oct 5, 2023

cmake: re-add missed C89 headers for specific detections #12043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: delete checks for C89 standard headers #11940

build: delete checks for C89 standard headers #11940

vszakats commented Sep 25, 2023 •

edited

ncopa commented Sep 25, 2023

vszakats commented Sep 25, 2023 •

edited

vszakats commented Sep 25, 2023 •

edited

vszakats commented Sep 25, 2023 •

edited

jay commented Sep 25, 2023

vszakats commented Sep 25, 2023 •

edited

ncopa commented Sep 26, 2023

ncopa commented Sep 26, 2023

bagder commented Sep 26, 2023

vszakats commented Sep 26, 2023

MarcelRaad Sep 27, 2023

vszakats Sep 27, 2023

vszakats Sep 27, 2023

vszakats Sep 27, 2023

build: delete checks for C89 standard headers #11940

build: delete checks for C89 standard headers #11940

Conversation

vszakats commented Sep 25, 2023 • edited

ncopa commented Sep 25, 2023

vszakats commented Sep 25, 2023 • edited

vszakats commented Sep 25, 2023 • edited

vszakats commented Sep 25, 2023 • edited

jay commented Sep 25, 2023

vszakats commented Sep 25, 2023 • edited

ncopa commented Sep 26, 2023

ncopa commented Sep 26, 2023

bagder commented Sep 26, 2023

vszakats commented Sep 26, 2023

MarcelRaad Sep 27, 2023

Choose a reason for hiding this comment

vszakats Sep 27, 2023

Choose a reason for hiding this comment

vszakats Sep 27, 2023

Choose a reason for hiding this comment

vszakats Sep 27, 2023

Choose a reason for hiding this comment

vszakats commented Sep 25, 2023 •

edited

vszakats commented Sep 25, 2023 •

edited

vszakats commented Sep 25, 2023 •

edited

vszakats commented Sep 25, 2023 •

edited

vszakats commented Sep 25, 2023 •

edited