Skip to content
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

Put brotli on the FetchContent plan #107166

Merged
merged 20 commits into from
Oct 21, 2024
Merged

Conversation

jkoritzinsky
Copy link
Member

Consume our brotli dependency through FetchContent like zlib-ng. Also, update the "system brotli" consumption to use CMake's PkgConfig support. This provides us with a few benefits:

  • We don't need to maintain a list of source files that we want to include from brotli.
  • Consuming system vs in-tree brotli becomes much more similar as we use the same properties to specify targets and include directories.
  • Using system brotli in NativeAOT comes easily when building against system brotli. No need for the nonportable.txt workaround as we have files on disk we can look for, just like zlib-ng.

Fixes #107020

@tmds
Copy link
Member

tmds commented Aug 30, 2024

@jkoritzinsky let me know when the PR is "ready", and then I will locally backport it to 9.0 and do some .NET builds to verify it fixes #107020.

@carlossanlop
Copy link
Member

@akoeplinger @jkotas should we keep using the system brotli in mobile platforms and wasi/wasm like we did with zlib-ng? Why or why not?

@jkotas
Copy link
Member

jkotas commented Sep 13, 2024

should we keep using the system brotli in mobile platforms and wasi/wasm

Does the system brotli exist on these platforms? Do we use it today?

@carlossanlop
Copy link
Member

Does the system brotli exist on these platforms? Do we use it today?

I do not know if system brotli exists in those platforms but we do use it today with find_package, formerly located in extra_libs.cmake, but moved in this PR to here: #107166 (comment)

@jkotas
Copy link
Member

jkotas commented Sep 13, 2024

We do not set CLR_CMAKE_USE_SYSTEM_BROTLI to 1 anywhere by default (different from zlib).

I think that system brotli does not exist on those platforms and we should be doing what we have been doing so far ie. use our bundled one. There is no other option.

@jkoritzinsky
Copy link
Member Author

/azp run runtime-community, runtime-extra-platforms, runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jkoritzinsky
Copy link
Member Author

All failures look unrelated. Looks like we're good to go here!

@jkoritzinsky jkoritzinsky merged commit a7fc1ea into dotnet:main Oct 21, 2024
216 of 233 checks passed
@jkoritzinsky jkoritzinsky deleted the brotli-upgrade branch October 21, 2024 20:16
steveisok added a commit that referenced this pull request Oct 22, 2024
steveisok added a commit that referenced this pull request Oct 22, 2024
This reverts commit a7fc1ea.

It broke the official build with an arch variant of `bin/native/net10.0-linux-Release-arm/libbrotlicommon.a' is not added because the package already contains file 'runtimes/linux-arm/native/libbrotlicommon.a`
jkoritzinsky added a commit to jkoritzinsky/runtime that referenced this pull request Nov 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

source-built NativeAOT doesn't work with system brotli
6 participants