Skip to content

Add brotli support to FoundationNetworking #5251

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zhenchaoli
Copy link

@zhenchaoli zhenchaoli commented Jul 30, 2025

Add brotli as a dependency to enable brotli decoding for curl.

On Windows, only brotlicommon.lib and brotlidec.lib is linked.
brotlienc.lib is intentionally left out as curl does not need it. brotlienc.lib is also much larger in size (~3.8MB).

@parkera parkera requested a review from compnerd July 30, 2025 02:22
@zhenchaoli zhenchaoli changed the title Add brotli support to FoundationNetworking on Windows Add brotli support to FoundationNetworking on Windows [WIP[ Jul 30, 2025
@zhenchaoli zhenchaoli changed the title Add brotli support to FoundationNetworking on Windows [WIP[ Add brotli support to FoundationNetworking on Windows [WIP] Jul 30, 2025
@zhenchaoli
Copy link
Author

swiftlang/swift#83441
@swift-ci please test

@zhenchaoli
Copy link
Author

Seems build failed. I see that brotli is not checked out. Do I need a 'clean' test?

According to https://github.com/swiftlang/swift/blob/main/docs/ContinuousIntegration.md, this is a thing, but unsure from reading it whether that will make a difference.

@zhenchaoli
Copy link
Author

swiftlang/swift#83441
@swift-ci please clean test

@zhenchaoli zhenchaoli changed the title Add brotli support to FoundationNetworking on Windows [WIP] Add brotli support to FoundationNetworking on Windows Aug 3, 2025
@zhenchaoli zhenchaoli changed the title Add brotli support to FoundationNetworking on Windows Add brotli support to FoundationNetworking Aug 4, 2025
@zhenchaoli zhenchaoli requested a review from jrflat August 4, 2025 19:46
@zhenchaoli
Copy link
Author

zhenchaoli commented Aug 4, 2025

the related pull request is at swiftlang/swift#83441

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows builds the toolchain without SPM and without the dependency automation in WindowsSwiftPMDependencies.cmake. We need this to be wired up into the CMake build properly.

@jmschonfeld
Copy link
Contributor

Windows builds the toolchain without SPM and without the dependency automation in WindowsSwiftPMDependencies.cmake. We need this to be wired up into the CMake build properly.

What extra pieces would need to be wired up in this build here? I know it's expected that the Windows toolchain build wouldn't use WindowsSwiftPMDependencies, but I think that's fine? I don't see any references to zlib in this project and I believe the brotli setup is largely the same, are the build.ps1 updates to pass the appropriate paths in as cmake variables insufficient for CMake to find the project in the dependency graph automatically?

@zhenchaoli
Copy link
Author

Windows builds the toolchain without SPM and without the dependency automation in WindowsSwiftPMDependencies.cmake. We need this to be wired up into the CMake build properly.

My reading (and discussion with Jeremy) is that cmake/modules/WindowsSwiftPMDependencies.cmake is for local developments, and that the actual cmake changes are made in swiftlang/swift#83441

The meaty piece in this PR is just the 2 test cases in my opinion.

@zhenchaoli zhenchaoli requested a review from compnerd August 6, 2025 18:13
@compnerd
Copy link
Member

compnerd commented Aug 6, 2025

Ah, so Foundation itself won't need to link to brotli because curl will automatically ensure that the paths are exported and passed along to the linker?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants