-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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: add pre-fill for Unix, enable in GHA/macos, verify pre-fills #15841
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
d226ce9
to
b850285
Compare
This comment was marked as outdated.
This comment was marked as outdated.
e87f86e
to
3468cd0
Compare
823ac56
to
64c70a7
Compare
7826f74
to
b97db64
Compare
62a6057
to
bd577bb
Compare
This reverts commit 2ec1572. It's 0 because it's skipped in cross-builds like iOS ones.
2a5d65d
to
805d23c
Compare
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Feb 17, 2025
To help debugging builds where the actual feature check is broken. Follow-up to e7adf3e curl#15841
1 task
vszakats
added a commit
that referenced
this pull request
Feb 24, 2025
- appveyor: restore VS2008 job, after fixing its issues. Enable OpenSSL in it. It takes 1 minute. Follow-up to 9b0467b #16453 Follow-up to edfa537 #16456 - appveyor: make a copy of OpenSSL DLLs to have them picked up as an artifact (disabled by default) to aid local tests. - appveyor: dump CMake configuration logs on failure. - appveyor: tidy up job parameter defaults. - GHA/windows: add pre-fill check option for dl-mingw jobs. - GHA/windows: fix pre-fill check option for MSYS jobs by installing `diffutils`. Follow-up to e7adf3e #15841 - GHA/windows: de-duplicate to `PATH` commands for Cygwin. - GHA/windows: drop `$SYSTEMROOT/System32` from `PATH` for Cygwin configure. It's not needed. Follow-up to 36fd2dd #13599 - list `.pdb` files in curl version step for MSVC. Ref: #16439 Cherry-picked from #16394 Closes #16458
icing
pushed a commit
to icing/curl
that referenced
this pull request
Feb 25, 2025
- appveyor: restore VS2008 job, after fixing its issues. Enable OpenSSL in it. It takes 1 minute. Follow-up to 9b0467b curl#16453 Follow-up to edfa537 curl#16456 - appveyor: make a copy of OpenSSL DLLs to have them picked up as an artifact (disabled by default) to aid local tests. - appveyor: dump CMake configuration logs on failure. - appveyor: tidy up job parameter defaults. - GHA/windows: add pre-fill check option for dl-mingw jobs. - GHA/windows: fix pre-fill check option for MSYS jobs by installing `diffutils`. Follow-up to e7adf3e curl#15841 - GHA/windows: de-duplicate to `PATH` commands for Cygwin. - GHA/windows: drop `$SYSTEMROOT/System32` from `PATH` for Cygwin configure. It's not needed. Follow-up to 36fd2dd curl#13599 - list `.pdb` files in curl version step for MSVC. Ref: curl#16439 Cherry-picked from curl#16394 Closes curl#16458
vszakats
added a commit
that referenced
this pull request
Mar 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
appleOS
specific to an Apple operating system
CI
Continuous Integration
cmake
performance
Windows
Windows-specific
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.
TL;DR: Save 10 minutes of CI time for GHA/macos jobs using pre-fills and
add pre-fill verification for Apple and Windows. Also restores Xcode job
and saves 1.5-10 minutes configuring iOS jobs.
Pre-filling feature detection results can bring down the CMake configure
step to ~5 seconds on most GHA runners, ~10 seconds in slow envs like
Cygwin/MSYS2.
The potential savings per job are:
On native Windows pre-filling has been in place for a long time and
saving 8 minutes (VS2019-VS2015) to 1.5-2 minutes (VS2022), 3 minutes
(VS2022 UWP), and 30-60 seconds (MinGW), per CI job.
The downside is that detection results need to be manually collected and
filtered to those that universally apply to all platforms that they are
enabled on. Another downside is that by using a cache, we're not running
the actual detections, and thus won't catch regressions in them. It
means we must make sure that the cache is solid and matches with actual
detections results. An upside is that it gives a rough overview of which
features are available on which platforms. Another upside is pre-filled
values do work for feature detections skipped for cross-builds, e.g.
HAVE_WRITABLE_ARGV
.This PR adds a pre-fill cache that supports all Unixes (except OmniOS)
used in CI, and makes it usable with an internal option. It also enables
it for GHA/macos CI jobs, where the maximum savings are. And also for
the two iOS [1] and two Cygwin/MSYS2 jobs. The latters don't have
pre-fill checks and we can drop them if they turn into a hassle.
Saving:
(offsetting the cost of pre-fill verifications)
You can enable pre-fill locally with
-D_CURL_PREFILL=ON
. It'sexperimental, and if you experience a problem, file a PR or an Issue.
This PR also adds a pre-fill checker for macOS and MinGW/MSVC Windows
GHA jobs to catch if the cache diverges from real detections. It also
adds this logic to AppVeyor, but doesn't enable it due to the perf
penalty of 2 minutes mininum.
The pre-fill checker works by configuring out-of-tree with and without
pre-fill, then diffing their
lib/curl_config.h
outputs.Exceptions are 3 detection results exposed indirectly [4], and missing
to expose 2, of which one is the C89 header
stddef.h
. While we assumethe C99
stdint.h
available outside iOS. We can expose them in thefuture, if necessary.
The pre-fill checks cost in total:
An extra time saving potential is caching type sizes. They are
well-known, and seldom change, esp. in CI. GHA/Windows jobs spend 8-17
seconds per job on these ~12 feature checks. ~5s on Cygwin/MSYS2. Couple
of seconds on other platforms. (This PR doesn't make this optimization.)
Another opportunity is doing the same for autotools, which typically
spends more time in the configuration step than cmake.
[1] Xcode job restored as a
follow-up to be5f202 #16302
[2] GHA/macos cmake configure times in seconds:
[3] iOS:
Before: https://github.com/curl/curl/actions/runs/13326401704?pr=15841
After: https://github.com/curl/curl/actions/runs/13332177764?pr=15841
[4] detection results exposed indirectly in
curl_config.h
:HAVE_FILE_OFFSET_BITS
via_FILE_OFFSET_BITS
HAVE_GETHOSTBYNAME_R_*_REENTRANT
viaNEED_REENTRANT
HAVE_SOCKADDR_IN6_SIN6_ADDR
viaUSE_IPV6
w/o ws: https://github.com/curl/curl/pull/15841/files?w=1
(possibly to configure-vs-cmake).CMakeCache.txt
). [SKIP, the pre-fill checker output should be enough.]