-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
add curlver.h include to umbrella header to ensure version checks work #5159
base: main
Are you sure you want to change the base?
Conversation
Let us know if this works for you. |
I might need your help in understanding how to test this. I haven't built a static sdk from scratch. |
I was able to build my own static sdk. with this change:
without this change:
|
@swift-ci test |
Looks like I've broken the Windows build:
I don't know enough about the windows builders to understand why it is missing, though it doesn't seem like it should be based on Daniel's reaction to here: https://curl.se/mail/archive-2005-01/0089.html |
Looks like CI is building its own copy of curl 8.9.1:
|
And I see it installed
|
|
OK, my theory on why this is happening is that the CMake build being used on Windows for Foundation only includes libcurl for the
However, because this is marked
I suspect it is done this way because Foundation is going to great pains to prevent any leakage of libcurl into the ABI. I noticed this and made sure to use Options:
|
attempting to fix #5157
I believe the issue is that the version checks here are failing:
swift-corelibs-foundation/Sources/_CFURLSessionInterface/include/CFURLSessionInterface.h
Lines 39 to 43 in c466923
because
LIBCURL_VERSION_MAJOR
etc have not been defined.because
<curl/curlver.h>
hasn't been included anywhere yet.It is included here, but after
CFURLSessionInterface.h
has already been processed.swift-corelibs-foundation/Sources/_CFURLSessionInterface/CFURLSessionInterface.c
Lines 22 to 24 in c466923
swift-corelibs-foundation/Sources/_CFURLSessionInterface/include/static-module.map
Line 2 in c466923