-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(windows): align static Foundation autolinks #5525
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
12271eb
0243902
2d06f93
fcd833c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,8 +58,16 @@ target_link_libraries(FoundationNetworking | |
| if(NOT BUILD_SHARED_LIBS) | ||
| target_compile_options(FoundationNetworking PRIVATE | ||
| "SHELL:$<$<COMPILE_LANGUAGE:Swift>:-Xfrontend -public-autolink-library -Xfrontend _CFURLSessionInterface>") | ||
| target_compile_options(FoundationNetworking PRIVATE | ||
| "SHELL:$<$<COMPILE_LANGUAGE:Swift>:-Xfrontend -public-autolink-library -Xfrontend curl>") | ||
| if(WIN32) | ||
| target_compile_options(FoundationNetworking PRIVATE | ||
| "SHELL:$<$<COMPILE_LANGUAGE:Swift>:-Xfrontend -public-autolink-library -Xfrontend libcurl>" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why the name changes here. It worked as curl there? That said the rename is likely better as the name is supposed to be the actual name on disk.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CMake path emitted an autolink for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but I mean, how has this been working? I suppose that this fix is for outside the CMake build as the curl target in CMake already points to the static library.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, exactly. This change is for consumers of the installed static SDK that are outside the CMake target graph. In the in-tree build An external swiftc consumer cannot see that CMake target. It only sees the |
||
| "SHELL:$<$<COMPILE_LANGUAGE:Swift>:-Xfrontend -public-autolink-library -Xfrontend zlibstatic>" | ||
| "SHELL:$<$<COMPILE_LANGUAGE:Swift>:-Xfrontend -public-autolink-library -Xfrontend brotlicommon>" | ||
| "SHELL:$<$<COMPILE_LANGUAGE:Swift>:-Xfrontend -public-autolink-library -Xfrontend brotlidec>") | ||
| else() | ||
| target_compile_options(FoundationNetworking PRIVATE | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why doesn't Linux need the extra auto linked libraries as well? The dependencies should be the same between Linux/Windows so I wouldn't expect anything to be windows specific here (except for maybe the name of the library if windows uses a different prefix for example)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Windows SDK builds and packages a static curl archive with zlib and Brotli enabled, but those archive dependencies are not encoded in its autolink metadata. Linux normally resolves the shared curl target and its dynamic dependencies; the existing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think this is true - the static Linux SDK includes a
I don't think the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You are right about the underlying mechanism and my earlier comment conflated archive naming with linkage mode. On Unix, I checked the published Swift 6.3.3 Static Linux SDK and the latest main snapshot from 2026-07-11 for both x86_64 and aarch64. In all four configurations:
The generated autolink file contains Windows uses a different curl configuration: Schannel, zlib and Brotli. Its observed unresolved symbols are exactly the zlib and Brotli set. Therefore the platform-specific lists are intentional: Windows needs
I agree that The cross-repository full static SDK build @compnerd requested remains the final integration check. My |
||
| "SHELL:$<$<COMPILE_LANGUAGE:Swift>:-Xfrontend -public-autolink-library -Xfrontend curl>") | ||
| endif() | ||
| target_compile_options(FoundationNetworking PRIVATE | ||
| "SHELL:$<$<COMPILE_LANGUAGE:Swift>:-Xfrontend -public-autolink-library -Xfrontend $<$<PLATFORM_ID:Windows>:${CMAKE_STATIC_LIBRARY_PREFIX_Swift}>swiftSynchronization>") | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere we use
if(CMAKE_SYSTEM_NAME STREQUAL "Windows")to conditionalize when building for Windows. How doesif(WIN32)behave differently (if at all) / should we use the other format used elsewhere instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 0243902 to use
CMAKE_SYSTEM_NAME STREQUAL "Windows", matching the condition style used elsewhere in this project.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify - I'm not 100% certain whether that is the correct syntax but rather I was asking why you chose
WIN32and whether there is a difference with theCMAKE_SYSTEM_NAMEcheckThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question - I checked this directly with CMake 4.3.2.
WIN32was true forWindows,WindowsStore,WindowsPhoneandWindowsCEwhileCMAKE_SYSTEM_NAME STREQUAL "Windows"selects only desktop Windows.I originally used
WIN32as conventional shorthand, not because this change needed the broader Windows family. There is no benefit to that breadth here, so the narrower check matching the project's existing style is preferable. That is why I updated it.