Skip to content

Add brotli support to FoundationNetworking #83441

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 10, 2025
Merged

Add brotli support to FoundationNetworking #83441

merged 1 commit into from
Oct 10, 2025

Conversation

@zhenchaoli
Copy link
Contributor

@zhenchaoli zhenchaoli commented Jul 30, 2025 •
edited
Loading

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

brotli is a general purpose compression algorithm used extensively on web. (https://datatracker.ietf.org/doc/html/rfc7932)

brotli support for curl is enabled by integrating the brotli compression library (https://github.com/google/brotli). I've also added some tests for FoundationNetworking in swiftlang/swift-corelibs-foundation#5251 which exercises this feature by

  • looking at the accept-encoding http header
  • decoding a brotli encoded payload

@zhenchaoli zhenchaoli mentioned this pull request Jul 30, 2025
Merged
@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@zhenchaoli
Copy link
Contributor Author

Seems windows ci passed. Linux failed due to test compilation error. I've added one more commit to enable test on Linux for swiftlang/swift-corelibs-foundation#5251

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

1 similar comment
@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@zhenchaoli
Copy link
Contributor Author

Linux test failure should be fixed now on swiftlang/swift-corelibs-foundation#5251

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test Linux platform

@zhenchaoli zhenchaoli force-pushed the zcli/brotli branch 3 times, most recently from cf08366 to 39f056b Compare August 2, 2025 02:21
@zhenchaoli zhenchaoli force-pushed the zcli/brotli branch 2 times, most recently from cf08366 to a01f1d5 Compare August 2, 2025 02:29
@zhenchaoli
Copy link
Contributor Author

rebased.

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@zhenchaoli zhenchaoli changed the title Add brotli support to FoundationNetworking [WIP] Add brotli support to FoundationNetworking Aug 3, 2025
compnerd
compnerd reviewed Aug 4, 2025
View reviewed changes
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.

It would be nice to get a bit more background on what is being done and why.

utils/build.ps1
Build-CMakeProject `
-Src $SourceCache\brotli `
-Bin "$BinaryCache\$($Platform.Triple)\brotli" `
-InstallTo "$BinaryCache\$($Platform.Triple)\usr" `
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to install it? Can we not use it from the build tree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm following the example of zlib here, and it feels cleaner to rely on build artifacts being in a consistent place for all dependency outputs. It seems all other dependencies already do this, so I think we should be consistent. (Maybe let's do these kind of changes in a separate PR for all dependencies if you feel strongly about it)

Copy link
Member

@compnerd compnerd Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

zlib is the only one doing this because it cannot be used from the build tree (I would love to migrate to that model for zlib as well). We no longer install any library which is not being distributed as a DLL - but that requires switching to the master branch rather than a release.

Copy link
Contributor Author

@zhenchaoli zhenchaoli Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Just to be clear, by "use it from the build tree", we'll be referencing .lib object files in brotli build tree like brotli\out\Release\brotlicommon.lib, and header files in its source tree at paths like brotli\c\include\brotli\decode.h?
I could try but I doubt it'll play well with the goal in the other comment which is using Brotli_DIR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, except, the idea with Brotli_DIR is that you tell CMake the directory to look into for BrotliConfig.cmake and it will wire up the headers and libraries appropriately. You don't need to pass flags or paths to the libraries or headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I will experiment and report back.

utils/build.ps1 Outdated
@@ -2334,6 +2349,8 @@ function Build-CURL([Hashtable] $Platform) {
USE_WIN32_LDAP = "NO";
ZLIB_ROOT = "$BinaryCache\$($Platform.Triple)\usr";
ZLIB_LIBRARY = "$BinaryCache\$($Platform.Triple)\usr\lib\zlibstatic.lib";
BROTLIDEC_LIBRARY = "$BinaryCache\$($Platform.Triple)\usr\lib\brotlidec.lib"
BROTLICOMMON_LIBRARY = "$BinaryCache\$($Platform.Triple)\usr\lib\brotlicommon.lib"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to pass in the library paths or do they generate a config file that we can use with Brotli_DIR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The build output of brotli does include 3 .pc files, but unsure how well curl supports BROTLI_DIR. The input variables for curl's brotli cmake are BROTLI_INCLUDE_DIR, BROTLICOMMON_LIBRARY, BROTLIDEC_LIBRARY according to https://github.com/curl/curl/blob/master/CMake/FindBrotli.cmake

Copy link
Member

Choose a reason for hiding this comment

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

The .pc files are for pkg-config, which we do not use or support as that is not something that is available on Windows by default. https://github.com/curl/curl/blob/master/CMake/FindBrotli.cmake#L62-L68 is the bit that matters - it should hopefully allow us to specify just the _DIR suffixed variable to find everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in ab40c18 . I'm not sure why this works as I don't find a BrotliConfig.cmake in brotli's output. According to https://cmake.org/cmake/help/latest/command/find_package.html#full-signature , for this <package>_DIR pattern to work, BrotliConfig.cmake needs to be found in that directory.

@compnerd compnerd added the Windows Platform: Windows label Aug 4, 2025
@zhenchaoli
Copy link
Contributor Author

It would be nice to get a bit more background on what is being done and why.

Updated the overview for this PR. Let me know if any details are missing.

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

compnerd
compnerd reviewed Aug 11, 2025
View reviewed changes
utils/build.ps1 Outdated
@@ -2334,6 +2350,7 @@ function Build-CURL([Hashtable] $Platform) {
USE_WIN32_LDAP = "NO";
ZLIB_ROOT = "$BinaryCache\$($Platform.Triple)\usr";
ZLIB_LIBRARY = "$BinaryCache\$($Platform.Triple)\usr\lib\zlibstatic.lib";
BROTLI_DIR = "$BinaryCache\$($Platform.Triple)";
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong. If this is meant to be an installed image, that should have the \usr suffix (like zlib). Otherwise, this should include the product name in the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It should have the \usr suffix. Fixed in 11fa0bc

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

compnerd
compnerd approved these changes Aug 11, 2025
View reviewed changes
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.

I think that this set of changes look correct. I think that we will need some help from @shahmishal to get the repository handling worked out.

@zhenchaoli
Copy link
Contributor Author

rebased resolving conflicts.

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@finagolfin
Copy link
Member

Please turn this on in build-script too. Nobody is currently using that curl build config, but I plan to start trying it out soon.

@zhenchaoli
Copy link
Contributor Author

Please turn this on in build-script too. Nobody is currently using that curl build config, but I plan to start trying it out soon.

added in f27db2b8ae9f45c00f1cb58740f43403731253cc

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@zhenchaoli
Copy link
Contributor Author

@swift-ci Please clean test

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@zhenchaoli zhenchaoli mentioned this pull request Sep 30, 2025
Draft
@speednoisemovement speednoisemovement mentioned this pull request Oct 2, 2025
Merged
@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

2 similar comments
@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@zhenchaoli
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#5251

@swift-ci Please clean test

@parkera parkera merged commit ed6ef79 into swiftlang:main Oct 10, 2025
5 checks passed
@parkera
Copy link
Contributor

parkera commented Oct 10, 2025

Merged along with swiftlang/swift-corelibs-foundation#5251

dendiz pushed a commit to dendiz/swift that referenced this pull request Oct 21, 2025
@zhenchaoli
Add brotli support to FoundationNetworking (swiftlang#83441)
1d2f6b2 
<!--
If this pull request is targeting a release branch, please fill out the
following form:

https://github.com/swiftlang/.github/blob/main/PULL_REQUEST_TEMPLATE/release.md?plain=1

Otherwise, replace this comment with a description of your changes and
rationale. Provide links to external references/discussions if
appropriate.
If this pull request resolves any GitHub issues, link them like so:

  Resolves <link to issue>, resolves <link to another issue>.

For more information about linking a pull request to an issue, see:

https://docs.github.com/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue
-->

<!--
Before merging this pull request, you must run the Swift continuous
integration tests.
For information about triggering CI builds via @swift-ci, see:

https://github.com/apple/swift/blob/main/docs/ContinuousIntegration.md#swift-ci

Thank you for your contribution to Swift!
-->

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

brotli is a general purpose compression algorithm used extensively on
web. (https://datatracker.ietf.org/doc/html/rfc7932)

brotli support for curl is enabled by integrating the brotli compression
library (https://github.com/google/brotli). I've also added some tests
for FoundationNetworking in
swiftlang/swift-corelibs-foundation#5251 which
exercises this feature by
- looking at the accept-encoding http header 
- decoding a brotli encoded payload
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@compnerd compnerd compnerd approved these changes

@jmschonfeld jmschonfeld jmschonfeld approved these changes

@shahmishal shahmishal Awaiting requested review from shahmishal shahmishal is a code owner

@parkera parkera Awaiting requested review from parkera

@jrflat jrflat Awaiting requested review from jrflat

@etcwilde etcwilde Awaiting requested review from etcwilde etcwilde is a code owner

@justice-adams-apple justice-adams-apple Awaiting requested review from justice-adams-apple justice-adams-apple is a code owner

Assignees

No one assigned

Labels

Windows Platform: Windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@zhenchaoli @compnerd @Steelskin @finagolfin @parkera @jmschonfeld