cmake: untangle feature detection interdependencies - #15164
Closed
vszakats wants to merge 15 commits into
Closed
Conversation
vszakats
marked this pull request as draft
October 6, 2024 01:20
vszakats
force-pushed
the
cm-detection-untangle
branch
5 times, most recently
from
October 10, 2024 22:54
58cd1f3 to
13f6a8b
Compare
vszakats
force-pushed
the
cm-detection-untangle
branch
from
October 10, 2024 23:46
13f6a8b to
a3900c5
Compare
vszakats
marked this pull request as ready for review
October 10, 2024 23:50
Member
Author
|
Script used to extract detection results from GHA logs: #!/bin/sh
grep -R -E '(\#define HAVE_| checking |-- Checking |-- Check |-- Performing |-- Looking )' \
| grep -v -F ' | ' | grep -v -F 'configure:' \
| sed -E \
-e 's/:[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}.[0-9]{7}Z /: /g' \
-e 's|./logs_[0-9]{11}/([0-9]{1,2}_)?||g' \
-e 's/ [0-9]+ include files [a-zA-Z0-9._/-]+, \.\.\., / /g' \
-e 's/ for include files [a-zA-Z0-9._/-]+, / for /g' \
-e 's/ for include files? / for /g' \
| sortThis will become simpler after #15266. |
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Nov 12, 2024
It's not offered by Apple operating systems, but in some cases it gets mis-detected when using `check_function_exists()`. Then making the build fail later. A better fix would be to figure out why are these mis-detections. One common pattern seen is this option, but it may or not be the cause: ``` -DCMAKE_TOOLCHAIN_FILE=[...]/toolchains/ios.toolchain.cmake ``` Ref: curl#15525 Regression from 8e34505 curl#15164 Reported-by: Dan Rosser Fixes curl#15557 Closes curl#15558
vszakats
added a commit
that referenced
this pull request
Nov 13, 2024
Fix builds with CMake configured to falsely return successful detection when using `check_function_exists()` (and `check_library_exists()`, and anything based on `try_compile()` that's relying on the linker). After such mis-detection the build fails when trying to use the feature that doesn't in fact exist. The mis-detection is caused by this CMake setting: ``` set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) ``` It is set by default (or on conditions) when using 3rd-party toolchain: https://github.com/leetal/ios-cmake/blob/master/ios.toolchain.cmake After this patch the curl build overrides this setting for the duration of feature tests, and logs a message about it. Also preset and skip feature tests for known mis-detections: - `connect()` in `libsocket` - `getpass_r()` - `eventfd()` (did not cause an issue due to a separate bug) - `sendmmsg()` (did not cause an issue because it's Linux-only) If mis-detections are still seen, the workaround is to force-set the specific feature by passing `-DHAVE_*=OFF` to cmake. Also consider passing `-DENABLE_STRICT_TRY_COMPILE=ON` for `ios.toolchain.cmake` to fix the root cause. Interestingly curl itself uses this setting to speed up compile-only detections: be17f29 #3744 Also: - OtherTests.cmake: restore original value of `CMAKE_TRY_COMPILE_TARGET_TYPE`. Before this patch it reset it to empty. - OtherTests.cmake: unset a local variable after use, quote a string. Follow-up to 8e34505 #15164 Follow-up to 8b76a8a #15525 Ref: leetal/ios-cmake#47 Ref: https://gitlab.kitware.com/cmake/cmake/-/issues/18121 Ref: https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_TARGET_TYPE.html Reported-by: Dan Rosser Fixes #15557 Closes #15559
vszakats
added a commit
that referenced
this pull request
Dec 28, 2024
vszakats
added a commit
that referenced
this pull request
Jan 1, 2025
To make it build again with CMake + Android 20 and earlier. 8e34505 synced `getpwuid_r()` detection in cmake with autotools. It means cmake started detecting it with Android <21 just like autotools, and thus cmake builds also need to tackle the missing declaration with old Android SDK versions. Use a PP solution, allowing to drop the autotools-specific on used before this patch. Follow-up to 8e34505 #15164 Follow-up to 9c33813 #2609 Ref: #2058 Closes #15871
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Feb 8, 2025
Regression from 8e34505 curl#15164
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Feb 8, 2025
Regression from 8e34505 curl#15164
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Feb 9, 2025
Regression from 8e34505 curl#15164
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Feb 9, 2025
Regression from 8e34505 curl#15164
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Feb 10, 2025
Almost all feature detection results are pre-filled on Windows for performance, so none of the issues fixed here affected builds. But, for good measure, this patch add missing detections and fixes others to make sure they work even when omitting the pre-fill. - fix `HAVE_STRUCT_TIMEVAL` detection for MSVC. Follow-up to c1bc090 curl#12495 - add `HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID` detection for Windows. - fix `HAVE_STRDUP` detection for MSVC. - fix `HAVE_SNPRINTF` detection for Windows. Regression from 8e34505 curl#15164 - fix `HAVE_IOCTLSOCKET` detection for non-UWP MSVC. - exclude `if_nametoindex` detection for Windows. Although it exists on Windows, detection, usage and availability is complicated, and curl doesn't use it on this platform. Regression from 8e34505 curl#15164 Also: - move IPv6-related detections so that pre-filling applies to them. - add debug option to test without pre-filling. - fix compiler warnings happening in feature detections to reduce log noise. (uninitialized/unused variables, missing static, missing const, constant conditional, macro redef, OpenSSL 3 deprecation warning.) Closes curl#16278
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Feb 10, 2025
Regression from 8e34505 curl#15164
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Feb 11, 2025
Almost all feature detection results are pre-filled on Windows for performance, so none of the issues fixed here affected builds. But, for good measure, this patch add missing detections and fixes others to make sure they work even when omitting the pre-fill. - fix `HAVE_STRUCT_TIMEVAL` detection for MSVC. Follow-up to c1bc090 curl#12495 - add `HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID` detection for Windows. - fix `HAVE_STRDUP` detection for MSVC. - fix `HAVE_SNPRINTF` detection for Windows. Regression from 8e34505 curl#15164 - fix `HAVE_IOCTLSOCKET` detection for non-UWP MSVC. - exclude `if_nametoindex` detection for Windows. Although it exists on Windows, detection, usage and availability is complicated, and curl doesn't use it on this platform. Regression from 8e34505 curl#15164 Also: - move IPv6 detections so that pre-filling applies to them. - add debug option to test without pre-filling. - replace `NOT LESS` with `GREATER_EQUAL` Closes curl#16278
vszakats
added a commit
that referenced
this pull request
Feb 11, 2025
Almost all feature detection results are pre-filled on Windows for performance, so none of the issues fixed here affected builds. For good measure, this patch add missing detections and fixes others to make sure they work even when omitting the pre-fill. It also fixes detecting IPv6 for MS-DOS. - fix `HAVE_STRUCT_TIMEVAL` detection for MSVC. Follow-up to c1bc090 #12495 - add `HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID` detection for Windows. - fix `HAVE_STRDUP` detection for MSVC. - fix `HAVE_SNPRINTF` detection for Windows. Regression from 8e34505 #15164 - fix `HAVE_IOCTLSOCKET` detection for non-UWP MSVC. - exclude `if_nametoindex` detection for Windows. Although it exists on Windows, detection, usage and availability is complicated, and curl doesn't use it on this platform. Regression from 8e34505 #15164 - move IPv6 detections so that pre-filling and MS-DOS Watt-32 configuration applies to them. This fixes `HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID` detection with MS-DOS. Ref: https://github.com/curl/curl/actions/runs/13260511764/job/37015877585#step:7:306 Follow-up to a3585c9 #15543 Also: - add debug option to test without pre-filling. - replace `NOT LESS` with `GREATER_EQUAL` Closes #16278
pps83
pushed a commit
to pps83/curl
that referenced
this pull request
Apr 26, 2025
- reduce `check_include_file_concat()` use to those headers that either depend on a previously detected header, or another header or symbol detection depend on it. - replace `check_symbol_exists()` with `check_function_exists()` for functions that are detected with `AC_CHECK_FUNCS()` in `./configure`. This makes `setmode()` no longer be detected with MSYS, syncing this with `./configure`. Instead `_setmode()` is used now also in CMake MSYS builds. This is consistent with Cygwin builds also. - add comment about which header/symbol detection depends on what header. Based on `./configure` mainly. - form `CURL_TEST_DEFINES` manually, and include only those macros which are actually used in `CMake/CurlTests.c`. - change `curl_internal_test()` to use `CMAKE_REQUIRED_DEFINITIONS`, instead of `CMAKE_REQUIRED_FLAGS` to simplify the logic, and to allow dropping the latter macro completely. - drop `windows.h` from header and symbol checks. - `./configure`: add comment about whether `netinet/in6.h`, `sys/un.h` are indeed meant to be included for all detections. There is a chance they were added there by accident. Detection resuls were cross-checked between 436bbbe (master) and 48ff4694e608ccfdedf7ce5bab2b96d6b2c23cda (this PR), for CI GHA Linux, Linux HTTP/3, non-native, macOS and Windows jobs. Closes curl#15164
pps83
pushed a commit
to pps83/curl
that referenced
this pull request
Apr 26, 2025
Enable `WIN32_LEAN_AND_MEAN` for all feature detections on Windows. (Also drop it from individual detections.) Cherry-picked from curl#15164
pps83
pushed a commit
to pps83/curl
that referenced
this pull request
Apr 26, 2025
A recent update caused CMake builds to mis-detect this symbol on iOS. Auto-detection also seems redundant given that it's a Windows-only function and most Windows builds were already opted-in. Drop detections and use it in all Windows builds with large file support enabled. Feature history: - pririotizing for Windows: aaacd02 curl#14678 - Windows opt-in cmake: 8e74c07 curl#11950 - Windows opt-in: aa6c94c curl#11944 - use in libcurl: 9c7165e curl#11918 - use in example: 817d1c0 Regression from 8e34505 curl#15164 Reported-by: Maarten Billemont Fixes curl#15525 Closes curl#15526
pps83
pushed a commit
to pps83/curl
that referenced
this pull request
Apr 26, 2025
Fix builds with CMake configured to falsely return successful detection when using `check_function_exists()` (and `check_library_exists()`, and anything based on `try_compile()` that's relying on the linker). After such mis-detection the build fails when trying to use the feature that doesn't in fact exist. The mis-detection is caused by this CMake setting: ``` set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) ``` It is set by default (or on conditions) when using 3rd-party toolchain: https://github.com/leetal/ios-cmake/blob/master/ios.toolchain.cmake After this patch the curl build overrides this setting for the duration of feature tests, and logs a message about it. Also preset and skip feature tests for known mis-detections: - `connect()` in `libsocket` - `getpass_r()` - `eventfd()` (did not cause an issue due to a separate bug) - `sendmmsg()` (did not cause an issue because it's Linux-only) If mis-detections are still seen, the workaround is to force-set the specific feature by passing `-DHAVE_*=OFF` to cmake. Also consider passing `-DENABLE_STRICT_TRY_COMPILE=ON` for `ios.toolchain.cmake` to fix the root cause. Interestingly curl itself uses this setting to speed up compile-only detections: be17f29 curl#3744 Also: - OtherTests.cmake: restore original value of `CMAKE_TRY_COMPILE_TARGET_TYPE`. Before this patch it reset it to empty. - OtherTests.cmake: unset a local variable after use, quote a string. Follow-up to 8e34505 curl#15164 Follow-up to 8b76a8a curl#15525 Ref: leetal/ios-cmake#47 Ref: https://gitlab.kitware.com/cmake/cmake/-/issues/18121 Ref: https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_TARGET_TYPE.html Reported-by: Dan Rosser Fixes curl#15557 Closes curl#15559
pps83
pushed a commit
to pps83/curl
that referenced
this pull request
Apr 26, 2025
Follow-up to 8e34505 curl#15164 Follow-up to 1c877a0 curl#1176 Closes curl#15845
pps83
pushed a commit
to pps83/curl
that referenced
this pull request
Apr 26, 2025
Syncing this with autotools. Follow-up to 8e34505 curl#15164 Closes curl#15869
pps83
pushed a commit
to pps83/curl
that referenced
this pull request
Apr 26, 2025
To make it build again with CMake + Android 20 and earlier. 8e34505 synced `getpwuid_r()` detection in cmake with autotools. It means cmake started detecting it with Android <21 just like autotools, and thus cmake builds also need to tackle the missing declaration with old Android SDK versions. Use a PP solution, allowing to drop the autotools-specific on used before this patch. Follow-up to 8e34505 curl#15164 Follow-up to 9c33813 curl#2609 Ref: curl#2058 Closes curl#15871
pps83
pushed a commit
to pps83/curl
that referenced
this pull request
Apr 26, 2025
Almost all feature detection results are pre-filled on Windows for performance, so none of the issues fixed here affected builds. For good measure, this patch add missing detections and fixes others to make sure they work even when omitting the pre-fill. It also fixes detecting IPv6 for MS-DOS. - fix `HAVE_STRUCT_TIMEVAL` detection for MSVC. Follow-up to c1bc090 curl#12495 - add `HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID` detection for Windows. - fix `HAVE_STRDUP` detection for MSVC. - fix `HAVE_SNPRINTF` detection for Windows. Regression from 8e34505 curl#15164 - fix `HAVE_IOCTLSOCKET` detection for non-UWP MSVC. - exclude `if_nametoindex` detection for Windows. Although it exists on Windows, detection, usage and availability is complicated, and curl doesn't use it on this platform. Regression from 8e34505 curl#15164 - move IPv6 detections so that pre-filling and MS-DOS Watt-32 configuration applies to them. This fixes `HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID` detection with MS-DOS. Ref: https://github.com/curl/curl/actions/runs/13260511764/job/37015877585#step:7:306 Follow-up to a3585c9 curl#15543 Also: - add debug option to test without pre-filling. - replace `NOT LESS` with `GREATER_EQUAL` Closes curl#16278
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
reduce
check_include_file_concat()use to those headers that eitherdepend on a previously detected header, or another header or symbol
detection depend on it.
replace
check_symbol_exists()withcheck_function_exists()forfunctions that are detected with
AC_CHECK_FUNCS()in./configure.This makes
setmode()no longer be detected with MSYS, syncingthis with
./configure. Instead_setmode()is used now also inCMake MSYS builds. This is consistent with Cygwin builds also.
add comment about which header/symbol detection depends on what
header. Based on
./configuremainly.form
CURL_TEST_DEFINESmanually, and include only those macros whichare actually used in
CMake/CurlTests.c.change
curl_internal_test()to useCMAKE_REQUIRED_DEFINITIONS,instead of
CMAKE_REQUIRED_FLAGSto simplify the logic, and to allowdropping the latter macro completely.
drop
windows.hfrom header and symbol checks../configure: add comment about whethernetinet/in6.h,sys/un.hare indeed meant to be included for all detections. There is a chance
they were added there by accident.
Detection resuls were cross-checked between
436bbbe (master) and
48ff4694e608ccfdedf7ce5bab2b96d6b2c23cda (this PR), for CI GHA Linux,
Linux HTTP/3, non-native, macOS and Windows jobs.
check_include_file_concat()for LDAP and GSS detection #15157WindowsCache.cmake.