Put brotli on the FetchContent plan #107166
Conversation
…ia CMake's PkgConfig model
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Windows.targets
Show resolved
Hide resolved
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets
Outdated
Show resolved
Hide resolved
|
@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. |
…ompression.Native.
…y default (doesn't work on wasi)
…st brotlicommon instead of dynamically linked against it.
…veAOT build for mono Apple scenarios.
| @@ -155,6 +157,14 @@ The .NET Foundation licenses this file to you under the MIT license. | |||
| <NativeLibrary Condition="'$(UseSystemZlib)' != 'true'" Include="$(IlcSdkPath)libz.a" /> | |||
| </ItemGroup> | |||
|
|
|||
| <!-- brotli must be added after System.IO.Compression.Native and brotlicommon must be added last, order matters. --> | |||
There was a problem hiding this comment.
Was there a reason to move this ItemGroup from line 171 to here? I suspect something about the ItemGroup in line 168 was causing trouble.
There was a problem hiding this comment.
I had just added the itemgroup here initially before @agocke did the implementation that uses nonportable.txt. I didn't move it down when I merged in his changes as I just immediately reverted them (as my changes are a replacement for the nonportable.txt workflow).
| @@ -1,39 +1,30 @@ | |||
| # IMPORTANT: do not use add_compile_options(), add_definitions() or similar functions here since it will leak to the including projects | |||
There was a problem hiding this comment.
Please bring this line back 😄 I removed it when working on zlib-ng, then forgot about that warning and ended up adding an add_compile_options. It was really annoying to get this fixed in source build when my change flowed there.
| ) | ||
|
|
||
| addprefix(BROTLI_SOURCES "${CMAKE_CURRENT_LIST_DIR}/brotli" "${BROTLI_SOURCES_BASE}") | ||
| set(BROTLI_DISABLE_TESTS ON) |
There was a problem hiding this comment.
Don't forget to run all the exotic azp runs: runtime-community, runtime-extra-platforms, the nativeaot and wasm ones. They might capture some interesting error messages.
There was a problem hiding this comment.
Once I have regular CI working I'll make sure to do so.
| @@ -88,7 +88,7 @@ | |||
|
|
|||
| <!-- Handle system libraries --> | |||
| <UseSystemLibs Condition="'$(UseSystemLibs)' != ''">+$(UseSystemLibs)+</UseSystemLibs> | |||
| <InnerBuildArgs Condition="'$(PortableBuild)' != 'true'">$(InnerBuildArgs) --cmakeargs -DCLR_CMAKE_USE_SYSTEM_BROTLI=true</InnerBuildArgs> | |||
| <InnerBuildArgs Condition="$(UseSystemLibs.Contains('+brotli+'))">$(InnerBuildArgs) --cmakeargs -DCLR_CMAKE_USE_SYSTEM_BROTLI=true</InnerBuildArgs> | |||
There was a problem hiding this comment.
Where does UseSystemLibs acquire its value? Or rather, who is in charge of appending "brotli"? I mainly want to know if we're properly deciding to use system brotli in mobile platforms and wasm/wasi.
BTW, if you run runtime-community, I'd be curious to see if armv6 will give us trouble like with zlib-ng.
There was a problem hiding this comment.
UseSystemLibs is used in source-build for our distro partners to build against system brotli.
I don't expect issues with armv6 like zlib-ng as brotli isn't accelerated like zlib-ng is on various platforms.
There was a problem hiding this comment.
AFAIK, it's not set automatically anywhere. Even in source-build, we are manually passing it at build-time when we want to build the full .NET SDK for a platform. The only exception is for 9.0 (only) where we assume system brotli (#107225 ).
There was a problem hiding this comment.
It would be nice if we can make the vmr default to the configuration that is preferred by most distro maintainers.
There was a problem hiding this comment.
It would be nice if we can make the vmr default to the configuration that is preferred by most distro maintainers.
I've created an issue for this: dotnet/source-build#4625.
| find_package(PkgConfig REQUIRED) | ||
| pkg_check_modules(BROTLI REQUIRED brotlidec brotlienc brotlicommon) |
There was a problem hiding this comment.
I was looking for these two!
Why here and not in extra_libs like with zlib-ng?
There was a problem hiding this comment.
I was having weird issues with it being in extra_libs, and using PUBLIC in the target_link_libraries and target_include_directories below got the values to propagate such that I didn't need to include brotli.cmake in any other place than here.
|
@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? |
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 |
|
We do not set 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. |
…ses (thanks single-pass linker ordering...)
|
/azp run runtime-community, runtime-extra-platforms, runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
All failures look unrelated. Looks like we're good to go here! |
This reverts commit a7fc1ea.
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`
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:
nonportable.txtworkaround as we have files on disk we can look for, just like zlib-ng.Fixes #107020