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
cmake: pre-fill rest of detection values for Windows #12044
Closed
Closed
Conversation
This file contains 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
vszakats
changed the title
cmake: tidy-up and Windows optimization
cmake: pre-cache rest of detection values for Windows
Oct 6, 2023
Also tested an experimental configure-vs-cmake feature-detection comparison macOS configure-vs-cmake patch: diff --git a/.github/workflows/configure-vs-cmake.yml b/.github/workflows/configure-vs-cmake.yml
index 3131bc128..0f20801a3 100644
--- a/.github/workflows/configure-vs-cmake.yml
+++ b/.github/workflows/configure-vs-cmake.yml
@@ -27,7 +27,7 @@ on:
permissions: {}
jobs:
- check:
+ check-linux:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
@@ -39,7 +39,28 @@ jobs:
- name: run cmake
run: |
- mkdir build && cd build && cmake ..
+ cmake -B build
+
+ - name: compare generated curl_config.h files
+ run: ./scripts/cmp-config.pl lib/curl_config.h build/lib/curl_config.h
+
+ check-macos:
+ runs-on: macos-latest
+ steps:
+ - uses: actions/checkout@v4
+
+ - name: install packages
+ run: |
+ while [[ $? == 0 ]]; do for i in 1 2 3; do brew update && brew install libtool autoconf automake && break 2 || { echo Error: wait to try again; sleep 10; } done; false Too many retries; done
+
+ - name: run configure --with-openssl
+ run: |
+ autoreconf -fi
+ ./configure --with-openssl
+
+ - name: run cmake
+ run: |
+ cmake -B build
- name: compare generated curl_config.h files
run: ./scripts/cmp-config.pl lib/curl_config.h build/lib/curl_config.h |
vszakats
force-pushed
the
cmake-tidy-2
branch
2 times, most recently
from
October 6, 2023 15:30
d031a14
to
c281b78
Compare
vszakats
force-pushed
the
cmake-tidy-2
branch
4 times, most recently
from
October 7, 2023 14:57
282d4da
to
1bf70e6
Compare
vszakats
changed the title
cmake: pre-cache rest of detection values for Windows
cmake: pre-fill rest of detection values for Windows
Oct 7, 2023
vszakats
force-pushed
the
cmake-tidy-2
branch
2 times, most recently
from
October 9, 2023 17:10
4c9e89c
to
cb87638
Compare
vszakats
added
the
feature-window
A merge of this requires an open feature window
label
Oct 11, 2023
It's same check as `HAVE_SIGSETJMP`.
Comment suggested it is special, but it used the same method as all the other symbol detections. It also suggested this was a secondary detection attempt, but there was no first one.
Based on results found on AppVeyor CI with VS2008-VS2010-VS2022 and mingw-w64 6 to 9, origina mingw-w64 1.0 release and current 11.0 release.
``` /usr/local/Cellar/mingw-w64/11.0.1/toolchain-x86_64/x86_64-w64-mingw32/include/winsock2.h:15:2: warning: #warning Please include winsock2.h before windows.h [-Wcpp] 15 | #warning Please include winsock2.h before windows.h | ^~~~~~~ ```
This seems to actually cause the warnings, after fixing the header include order to really include winsock2.h before windows.h: ``` Building C object CMakeFiles/cmTC_0da7d.dir/HAVE_IDN2_H.c.obj /usr/local/bin/x86_64-w64-mingw32-gcc -D_WINSOCKAPI_="" -I/usr/local/include -W -Wall -pedantic -Wbad-function-cast -Wconversion -Winline -Wmissing-declarations -Wmissing-prototypes In file included from .../curl/bld-cmake-win/CMakeFiles/CMakeScratch/TryCompile-7IYZyM/HAVE_IDN2_H.c:2: /usr/local/Cellar/mingw-w64/11.0.1/toolchain-x86_64/x86_64-w64-mingw32/include/winsock2.h:15:2: warning: #warning Please include winsock2.h before windows.h [-Wcpp] 15 | #warning Please include winsock2.h before windows.h | ^~~~~~~ Linking C executable cmTC_0da7d.exe ``` After both fixes, the test is warning-free: ``` Building C object CMakeFiles/cmTC_a2548.dir/HAVE_IDN2_H.c.obj /usr/local/bin/x86_64-w64-mingw32-gcc -I/usr/local/include -W -Wall -pedantic -Wbad-function-cast -Wconversion -Winline -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long -Wno-multichar -Wpointer-arith -Wshadow -Wsign-compare -Wundef -Wunused -Wwrite-strings -Wcast-align -Wdeclaration-after-statement -Wempty-body -Wendif-labels -Wfloat-equal -Wignored-qualifiers -Wno-format-nonliteral -Wno-sign-conversion -Wno-system-headers -Wstrict-prototypes -Wtype-limits -Wvla -Wmissing-parameter-type -Wold-style-declaration -Wstrict-aliasing=3 -Wno-pedantic-ms-format -Wformat=2 -Warray-bounds=2 -ftree-vrp -Wduplicated-cond -Wnull-dereference -fdelete-null-pointer-checks -Wshift-negative-value -Wshift-overflow=2 -Walloc-zero -Wduplicated-branches -Wformat-overflow=2 -Wformat-truncation=1 -Wrestrict -Warith-conversion -Wdouble-promotion -Wenum-conversion -Wunused-const-variable -o CMakeFiles/cmTC_a2548.dir/HAVE_IDN2_H.c.obj -c .../curl/bld-cmake-win/CMakeFiles/CMakeScratch/TryCompile-7JKzOa/HAVE_IDN2_H.c Linking C executable cmTC_a2548.exe ```
This breaks from autotools. autotools will need to be adapted to avoid the unnecessary header check in case the lib is missing. The curl source bits that use idn2 require BOTH to be detected.
Not ruling out a terrible custom mis-configuration that may end up with both WIN32 and UNIX set, but this was the only file we did consider this scenario, so it seems fairly unlikely. When building for Cygwin or MSYS2, WIN32 is NOT set and UNIX is set.
`CMAKE_VERSION` supported since CMake 2.6.3
Move it before including Windows headers.
`NOT UNIX` / `WIN32` / `HAVE_WINDOWS_H` / `HAVE_WIN*_H` are used interchangeably in the source. In practice these all mean the same thing and logic expects them to be in sync. We should end up using `WIN32` only in CMake sources to check for Windows. And `_WIN32` in C sources. There is Windows C compiler supported that seems to miss Windows socket header: `__SALFORDC__`. This C compiler seems to be long gone. If still around, it must support Windows sockets out of the box.
i.e. use windows.h directly, instead via `lib/setup-win32.h`. This makes it work without requiring `HAVE_WINDOWS_H` defined.
…ngw-w64 The pre-cached values for CMake and the `config-win32.h` ones were wrongly set as unsupported. mingw-w64 supported these since its 1.0 release. autotools got them right. I did not catch these when doing cross-build-tool reproducibility tests for Windows. Reason being that the `gettimeofday` branch is overridden by a `WIN32` one, so it's not used regardless of these settings. It means that this fix doesn't change the build results. Two new headers _will_ be included though and the detection results are correctly setup now.
vszakats
force-pushed
the
cmake-tidy-2
branch
from
October 16, 2023 05:29
cb87638
to
bd3e222
Compare
bagder
approved these changes
Oct 16, 2023
This is present in mingw-w64 on native Windows by default, otherwise it's only detected when manually adding pthread lib. But, the result of this feature check is never used on Windows by the source code, because the `WIN32` branches override it. It'd also make curl phtread dependent if it did use it.
Always overridden by `WIN32` branches in curl source code. Otherwise it needs pthread and a constant that is not present in mingw-w64 anyway.
Possibly clang-cl might support this, which might show up as MSVC in CMake. I'm not sure, so revert. This reverts commit 761361f.
vszakats
removed
the
feature-window
A merge of this requires an open feature window
label
Oct 25, 2023
vszakats
added a commit
to curl/curl-for-win
that referenced
this pull request
Oct 25, 2023
Switch from `curl-gnumake.sh` to `curl-cmake.sh` with upcoming curl release v8.5.0. cmake builds are now _faster_ for Windows builds than raw gnumake (aka `Makefile.mk`). They also use 'unity' mode, which makes builds run fast with the side-effect of also creating potentially more efficient binaries by allowing better compiler optimizations. This also makes curl-for-win use the same build system for all target platforms (`Makefile.mk` is not available for *nix platforms). Initially on 2022-07-04 cmake was 25% slower than gnumake. By 2022-09-26 this reduced to 20%, by 2023-07-29 to 18% and after the latest round of updates gnumake came out 7% slower than cmake. This is for Windows, for a triple x64, arm64 and x86 build. In absolute times this is 27m22s for cmake and 29m11s for gnumake. (FWIW autotools builds are 52% slower than cmake unity builds now.) Making cmake builds fast was a multi-year effort with these major steps: 1. add support for cmake builds in curl-for-win. 420e73c 2. bring it in-line with gnumake and autotools builds and tidy-up as much as possible. Scattered to a many commits. 3. delete a whole bunch of unused feature detections. curl/curl@4d73854 curl/curl#9044 (and a lot more smaller commits) 4. speed up picky warning option initialization by avoiding brute-force all options. (first in libssh2, then in curl, then applied also ngtcp2, nghttp3, nghttp2) curl/curl@9c543de curl/curl#10973 5. implement single build run for shared and static libcurl + tool (first in libssh2) curl/curl@1199308 curl/curl#11505 53dcd49 6. implement single build pass for shared and static libcurl (for Windows initially) curl/curl@2ebc74c curl/curl#11546 7. extend above to non-Windows (e.g. macOS) curl/curl@fc9bfb1 curl/curl#11627 bafa77d (mac) 1b27304 (linux) 8. implement unity builds: single compiler invocation for libcurl + tool curl/curl@3f8fc25 curl/curl#11095 curl/curl@f42a279 curl/curl#11928 67d7fd3 9. speed up 4x the cmake initialization phase (for Windows) curl/curl@2100d9f curl/curl#12044 10. speed up cmake initialization even more by pre-filling detection results for our well-known mingw-w64 environments. 74dd967 5a43c61 +1: speed up autotools initialization by mitigating a brute-force feature detection on Windows. This reduced total build times by 5 minutes at the time, for the 3 Windows targets in total. curl/curl@6adcff6 curl/curl#9591 Also update build times for cmake-unity and gnumake, based on runs: cmake-unity: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48357875 gnumake: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48358005
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.
The goal of this patch is to avoid unnecessary feature detection work
when doing Windows builds with CMake. Do this by pre-filling well-known
detection results for Windows and specifically for mingw-w64 and MSVC
compilers. Also limit feature checks to platforms where the results are
actually used. Drop a few redundant ones. And some tidying up.
pre-fill remaining detection values in Windows CMake builds.
Based on actual detection results observed in CI runs, preceding
similar work over libssh2 and matching up values with
lib/config-win32.h
.This brings down CMake configuration time from 58 to 14 seconds on the
same local machine.
On AppVeyor CI this translates to:
https://ci.appveyor.com/project/curlorg/curl/builds/48208419/job/4gw66ecrjpy7necb#L296
https://ci.appveyor.com/project/curlorg/curl/builds/48217440/job/8m4fwrr2fe249uo8#L186
https://ci.appveyor.com/project/curlorg/curl/builds/48208419/job/s1y8q5ivlcs7ub29?fullLog=true#L290
https://ci.appveyor.com/project/curlorg/curl/builds/48217440/job/pchpxyjsyc9kl13a?fullLog=true#L194
The formula is about 1-3 seconds delay for each detection. Almost all
of these trigger a full compile-link cycle behind the scenes, slow
even today, both cross and native, mingw-w64 and apparently MSVC too.
Enabling .map files or other custom build features slows it down
further. (Similar is expected for autotools configure.)
stop detecting
idn2.h
if idn2 was deselected.autotools does this.
stop detecting
idn2.h
if idn2 was not found.This deviates from autotools. Source code requires both header and
lib, so this is still correct, but faster.
limit
ADDRESS_FAMILY
detection to Windows.normalize
HAVE_WIN32_WINNT
value to lowercase0x0a12
format.pre-fill
HAVE_WIN32_WINNT
-dependent detection results.Saving 4 (slow) feature-detections in most builds:
getaddrinfo
,freeaddrinfo
,inet_ntop
,inet_pton
fix pre-filled
HAVE_SYS_TIME_H
,HAVE_SYS_PARAM_H
,HAVE_GETTIMEOFDAY
for mingw-w64.Luckily this do not change build results, as
WIN32
tookpriority over
HAVE_GETTIMEOFDAY
with the current sourcecode.
limit
HAVE_CLOCK_GETTIME_MONOTONIC_RAW
andHAVE_CLOCK_GETTIME_MONOTONIC
detections to non-Windows.We're not using these in the source code for Windows.
reduce compiler warning noise in CMake internal logs:
winsock2.h
beforewindows.h
.Apply it to autotools test snippets too.
-D_WINSOCKAPI_=
hack that aimed to fix the above.CMake/CurlTests.c
to emit less warnings.delete redundant
HAVE_MACRO_SIGSETJMP
feature check.It was the same check as
HAVE_SIGSETJMP
.delete "experimental" marking from
CURL_USE_OPENSSL
.show CMake version via
CMakeLists.txt
.Credit to the
zlib-ng
project for the idea:https://github.com/zlib-ng/zlib-ng/blob/61e181c8ae93dbf56040336179c9954078bd1399/CMakeLists.txt#L7
make
CMake/CurlTests.c
passchecksrc
.CMake/WindowsCache.cmake
tidy-ups.replace
WIN32
guard with_WIN32
inCMake/CurlTests.c
.Closes #12044
Clearer without whitespace changes: https://github.com/curl/curl/pull/12044/files?w=1