Put brotli on the FetchContent plan #107166

jkoritzinsky · 2024-08-29T23:05:08Z

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

…ia CMake's PkgConfig model

am11 · 2024-08-30T10:20:43Z

src/native/external/brotli/BUILD.bazel

We can keep custom code out of the vendor's directory (so it looks as close to the upstream as possible).

The Bazel file is from upstream.

tmds · 2024-08-30T16:47:17Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Windows.targets

@@ -47,6 +47,9 @@ The .NET Foundation licenses this file to you under the MIT license.
      <NativeLibrary Condition="'$(IlcMultiModule)' == 'true'" Include="$(SharedLibrary)" />
      <NativeLibrary Include="$(IlcSdkPath)$(StandaloneGCSupportName)$(LibrarySuffix)" />
      <NativeLibrary Include="$(IlcSdkPath)zlibstatic$(LibFileExt)" />
+      <NativeLibrary Include="$(IlcSdkPath)brotlicommon$(LibFileExt)" />
+      <NativeLibrary Include="$(IlcSdkPath)brotlienc$(LibFileExt)" />
+      <NativeLibrary Include="$(IlcSdkPath)brotlidec$(LibFileExt)" />


When we build .NET with system brotli, we don't include broticommon:

runtime/src/native/libs/System.IO.Compression.Native/extra_libs.cmake

Lines 23 to 28 in f0405a9

if (CLR_CMAKE_USE_SYSTEM_BROTLI)

find_library(BROTLIDEC brotlidec REQUIRED)

find_library(BROTLIENC brotlienc REQUIRED)

list(APPEND ${NativeLibsExtra} ${BROTLIDEC} ${BROTLIENC})

endif ()

When building against system brotli, brotlicommon is a private dependency of the brotlienc and brotlidec shared libraries, so it can be omitted. When statically linking, we need to specify all three.

tmds · 2024-08-30T16:48:36Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets

@@ -25,6 +25,8 @@ The .NET Foundation licenses this file to you under the MIT license.
    <LinkerFlavor Condition="'$(LinkerFlavor)' == '' and '$(_targetOS)' == 'linux'">bfd</LinkerFlavor>
    <IlcDefaultStackSize Condition="'$(IlcDefaultStackSize)' == '' and '$(_linuxLibcFlavor)' == 'musl'">1572864</IlcDefaultStackSize>
    <UseSystemZlib Condition="'$(UseSystemZlib)' == '' and !Exists('$(IlcSdkPath)libz.a')">true</UseSystemZlib>
+    <!-- Use libbrotlicommon.a as the sentinel for the three brotli libs. -->
+    <UseSystemBrotli Condition="'$(UseSystemBrotli)' == '' and !Exists('$(IlcSdkPath)libbrotlicommon.a')">true</UseSystemBrotli>


Nice and simple! 👍

tmds · 2024-08-30T16:52:21Z

@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)

…form experiences

…st brotlicommon instead of dynamically linked against it.

…veAOT build for mono Apple scenarios.

This reverts commit 3979ef8.

…upgrade

carlossanlop · 2024-09-13T03:37:17Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets

@@ -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. -->


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.

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).

carlossanlop · 2024-09-13T03:39:46Z

src/mono/msbuild/android/build/AndroidBuild.targets

      a list of direct pinvokes otherwise the runtime will crash
    -->
    <ItemGroup Condition="'$(_IsLibraryMode)' == 'true'">
      <DirectPInvokes Include="libSystem.Native" />
      <DirectPInvokes Include="libSystem.IO.Compression.Native" />
      <DirectPInvokes Include="libSystem.Security.Cryptography.Native.Android" />
+      <DirectPInvokes Include="libbrotlienc;libbrotlidec" />


Do we need a comment here to indicate that this brotli line needs to be after System.IO.Compression.Native? I am assuming order would also matter here but I might be wrong.

I'm curious - why did you need to add this line here and in AppleBuild.targets? For zlib-ng, we decided to continue using the system zlib-ng in mobile platforms and in wasm/wasi to ensure that we could benefit from quick security updates provided by the OS, as we would not be able to provide them ourselves in those platforms.

Shouldn't we do the same with brotli?

I'm curious - why did you need to add this line here and in AppleBuild.targets? For zlib-ng, we decided to continue using the system zlib-ng in mobile platforms and in wasm/wasi to ensure that we could benefit from quick security updates provided by the OS, as we would not be able to provide them ourselves in those platforms.

Shouldn't we do the same with brotli?

There is no system brotli for mobile platforms (even on Android) and wasm or wasi, so we can't use a system implementation.

carlossanlop · 2024-09-13T03:46:29Z

src/native/external/brotli.cmake

@@ -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


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.

carlossanlop · 2024-09-13T03:47:41Z

src/native/external/brotli.cmake

 )

-addprefix(BROTLI_SOURCES "${CMAKE_CURRENT_LIST_DIR}/brotli" "${BROTLI_SOURCES_BASE}")
+set(BROTLI_DISABLE_TESTS ON)


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.

Once I have regular CI working I'll make sure to do so.

carlossanlop · 2024-09-13T03:56:54Z

eng/DotNetBuild.props

@@ -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>


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.

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.

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 ).

It would be nice if we can make the vmr default to the configuration that is preferred by most distro maintainers.

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.

carlossanlop · 2024-09-13T04:05:43Z

src/native/libs/System.IO.Compression.Native/CMakeLists.txt

+    find_package(PkgConfig REQUIRED)
+    pkg_check_modules(BROTLI REQUIRED brotlidec brotlienc brotlicommon)


I was looking for these two!

Why here and not in extra_libs like with zlib-ng?

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.

carlossanlop · 2024-09-13T04:07:55Z

@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 · 2024-09-13T04:12:46Z

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 · 2024-09-13T04:19:25Z

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 · 2024-09-13T04:30:06Z

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 added 7 commits August 23, 2024 13:52

Upgrade brotli from v1.0.9 to v1.1.0 but do a full repo copy

fba095d

Delete unused subfolders

75c20c1

Integrate FetchContent-based discovery with system-brotli discovery v…

c46e058

…ia CMake's PkgConfig model

Update cg manifest

ff875b8

Hook up brotli linkage and NativeAOT targets

a8f604c

Apply patch for MSVC warning

e1c1d10

Merge branch 'main' of github.com:dotnet/runtime into brotli-upgrade

199888a

jkoritzinsky added the area-System.IO.Compression label Aug 29, 2024

jkoritzinsky added this to the 10.0.0 milestone Aug 29, 2024

jkoritzinsky requested a review from jkotas August 29, 2024 23:05

jkoritzinsky requested a review from MichalStrehovsky as a code owner August 29, 2024 23:05

jkoritzinsky requested review from carlossanlop, omajid and tmds August 29, 2024 23:05

dotnet-policy-service bot assigned jkoritzinsky Aug 29, 2024

jkoritzinsky mentioned this pull request Aug 29, 2024

source-built NativeAOT doesn't work with system brotli #107020

Closed

am11 reviewed Aug 30, 2024

View reviewed changes

Link against brotli on non-Windows

0f8202b

tmds reviewed Aug 30, 2024

View reviewed changes

Set the flags for brotli to allow exporting the brotli symbols from C…

d717e9d

…ompression.Native.

build-analysis bot mentioned this pull request Aug 30, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

jkotas mentioned this pull request Aug 31, 2024

Use system brotli on Unix non-portable builds #107225

Merged

Set export flags for brotlicommon and don't build the brotli target b…

72de3b8

…y default (doesn't work on wasi)

build-analysis bot mentioned this pull request Sep 4, 2024

System.Runtime.Serialization.Formatters CI failure. #107309

Closed

Try putting the brotli files eveywhere necessary for our various plat…

72bc9f8

…form experiences

jkoritzinsky requested a review from marek-safar as a code owner September 10, 2024 22:20

jkoritzinsky added 2 commits September 10, 2024 16:11

Build brotlienc and brotlidec to expect to be statically linked again…

81e5520

…st brotlicommon instead of dynamically linked against it.

Try installing in the libs partition instead of pulling from the Nati…

e9896c9

…veAOT build for mono Apple scenarios.

jkoritzinsky added 4 commits September 11, 2024 14:07

Revert "Use system brotli on Unix non-portable builds (dotnet#107225)"

599dc91

This reverts commit 3979ef8.

Merge commit '599dc9149e9' into brotli-upgrade

a575d28

Add to platform manifest

3bddbf5

Merge branch 'main' of https://github.com/dotnet/runtime into brotli-…

9087c81

…upgrade

carlossanlop reviewed Sep 13, 2024

View reviewed changes

tmds mentioned this pull request Sep 24, 2024

system-libs default [.NET 10] dotnet/source-build#4625

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put brotli on the FetchContent plan #107166

Put brotli on the FetchContent plan #107166

jkoritzinsky commented Aug 29, 2024

am11 Aug 30, 2024

jkoritzinsky Aug 30, 2024

tmds Aug 30, 2024

jkoritzinsky Aug 30, 2024

tmds Aug 30, 2024

tmds commented Aug 30, 2024 •

edited

Loading

carlossanlop Sep 13, 2024

jkoritzinsky Sep 13, 2024

carlossanlop Sep 13, 2024

jkoritzinsky Sep 13, 2024

carlossanlop Sep 13, 2024

carlossanlop Sep 13, 2024

jkoritzinsky Sep 13, 2024

carlossanlop Sep 13, 2024

jkoritzinsky Sep 13, 2024

omajid Sep 16, 2024

tmds Sep 17, 2024

tmds Sep 24, 2024

carlossanlop Sep 13, 2024

jkoritzinsky Sep 13, 2024

carlossanlop commented Sep 13, 2024

jkotas commented Sep 13, 2024 •

edited

Loading

carlossanlop commented Sep 13, 2024

jkotas commented Sep 13, 2024

	if (CLR_CMAKE_USE_SYSTEM_BROTLI)
	find_library(BROTLIDEC brotlidec REQUIRED)
	find_library(BROTLIENC brotlienc REQUIRED)

	list(APPEND ${NativeLibsExtra} ${BROTLIDEC} ${BROTLIENC})
	endif ()

		@@ -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

		find_package(PkgConfig REQUIRED)
		pkg_check_modules(BROTLI REQUIRED brotlidec brotlienc brotlicommon)

Put brotli on the FetchContent plan #107166

Are you sure you want to change the base?

Put brotli on the FetchContent plan #107166

Conversation

jkoritzinsky commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmds commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop commented Sep 13, 2024

jkotas commented Sep 13, 2024 • edited Loading

carlossanlop commented Sep 13, 2024

jkotas commented Sep 13, 2024

tmds commented Aug 30, 2024 •

edited

Loading

jkotas commented Sep 13, 2024 •

edited

Loading