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.

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

Put brotli on the FetchContent plan #107166

Merged
merged 20 commits into from
Oct 21, 2024
Merged

Put brotli on the FetchContent plan #107166

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
fba095d
Upgrade brotli from v1.0.9 to v1.1.0 but do a full repo copy
jkoritzinsky May 29, 2024
75c20c1
Delete unused subfolders
jkoritzinsky May 29, 2024
c46e058
Integrate FetchContent-based discovery with system-brotli discovery v…
jkoritzinsky May 29, 2024
ff875b8
Update cg manifest
jkoritzinsky Aug 23, 2024
a8f604c
Hook up brotli linkage and NativeAOT targets
jkoritzinsky Aug 29, 2024
e1c1d10
Apply patch for MSVC warning
jkoritzinsky Aug 29, 2024
199888a
Merge branch 'main' of github.com:dotnet/runtime into brotli-upgrade
jkoritzinsky Aug 29, 2024
0f8202b
Link against brotli on non-Windows
jkoritzinsky Aug 30, 2024
d717e9d
Set the flags for brotli to allow exporting the brotli symbols from C…
jkoritzinsky Aug 30, 2024
72de3b8
Set export flags for brotlicommon and don't build the brotli target b…
jkoritzinsky Sep 3, 2024
72bc9f8
Try putting the brotli files eveywhere necessary for our various plat…
jkoritzinsky Sep 10, 2024
81e5520
Build brotlienc and brotlidec to expect to be statically linked again…
jkoritzinsky Sep 10, 2024
e9896c9
Try installing in the libs partition instead of pulling from the Nati…
jkoritzinsky Sep 10, 2024
599dc91
Revert "Use system brotli on Unix non-portable builds (#107225)"
jkoritzinsky Sep 11, 2024
a575d28
Merge commit '599dc9149e9' into brotli-upgrade
jkoritzinsky Sep 11, 2024
3bddbf5
Add to platform manifest
jkoritzinsky Sep 12, 2024
9087c81
Merge branch 'main' of https://github.com/dotnet/runtime into brotli-…
jkoritzinsky Sep 12, 2024
adaa23c
Merge branch 'main' of https://github.com/dotnet/runtime into brotli-…
jkoritzinsky Oct 17, 2024
470a561
Let's try adding the brotli libs after the compression libs in all ca…
jkoritzinsky Oct 17, 2024
c5bfcc4
Add back the comment for the .cmake include files
jkoritzinsky Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eng/DotNetBuild.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,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>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

<InnerBuildArgs Condition="$(UseSystemLibs.Contains('+libunwind+'))">$(InnerBuildArgs) --cmakeargs -DCLR_CMAKE_USE_SYSTEM_LIBUNWIND=true</InnerBuildArgs>
<!-- TODO: llvm-libunwind -->
<!-- TODO: LinuxTracepoints -->
Expand Down
6 changes: 6 additions & 0 deletions eng/testing/tests.ioslike.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,12 @@

<AppleNativeFilesToBundle Include="$(PublishDir)\**\*.*" Exclude="@(AppleAssembliesToBundle);@(ApplePdbsToBundle);@(AppleXmlsToBundle);@(_SatelliteAssemblies)" />
</ItemGroup>

<ItemGroup Condition="'$(RuntimeFlavor)' == 'coreclr'">
<AppleNativeFilesToBundle Include="$(CoreCLRArtifactsPath)\aotsdk\libbrotlienc.a" />
<AppleNativeFilesToBundle Include="$(CoreCLRArtifactsPath)\aotsdk\libbrotlidec.a" />
<AppleNativeFilesToBundle Include="$(CoreCLRArtifactsPath)\aotsdk\libbrotlicommon.a" />
</ItemGroup>
</Target>

<Target Name="_CopyTestArchive"
Expand Down
16 changes: 11 additions & 5 deletions src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ The .NET Foundation licenses this file to you under the MIT license.

<PropertyGroup>
<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>
<FullRuntimeName>libRuntime.WorkstationGC</FullRuntimeName>
<FullRuntimeName Condition="'$(ServerGarbageCollection)' == 'true' or '$(IlcLinkServerGC)' == 'true'">libRuntime.ServerGC</FullRuntimeName>

Expand Down Expand Up @@ -168,6 +170,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. -->
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

<ItemGroup Condition="'$(UseSystemBrotli)' != 'true'">
<NativeLibrary Include="$(IlcSdkPath)libbrotlienc.a" />
<NativeLibrary Include="$(IlcSdkPath)libbrotlidec.a" />
<NativeLibrary Include="$(IlcSdkPath)libbrotlicommon.a" />
</ItemGroup>


<ItemGroup Condition="'$(StaticICULinking)' == 'true' and '$(NativeLib)' != 'Static' and '$(InvariantGlobalization)' != 'true'">
<NativeLibrary Include="$(IntermediateOutputPath)libs/System.Globalization.Native/build/libSystem.Globalization.Native.a" />
<DirectPInvoke Include="libSystem.Globalization.Native" />
Expand All @@ -181,11 +191,6 @@ The .NET Foundation licenses this file to you under the MIT license.
<NativeSystemLibrary Include="crypto" />
</ItemGroup>

<ItemGroup Condition="'$(UseSystemBrotli)' != 'false' and Exists('$(IlcSdkPath)nonportable.txt')">
<NativeSystemLibrary Include="brotlienc" />
<NativeSystemLibrary Include="brotlidec" />
</ItemGroup>

<ItemGroup Condition="'$(StaticOpenSslLinking)' == 'true' and '$(NativeLib)' != 'Static'">
<NativeLibrary Include="$(IntermediateOutputPath)libs/System.Security.Cryptography.Native/build/libSystem.Security.Cryptography.Native.OpenSsl.a" />
<DirectPInvoke Include="libSystem.Security.Cryptography.Native.OpenSsl" />
Expand All @@ -210,6 +215,7 @@ The .NET Foundation licenses this file to you under the MIT license.
<NativeSystemLibrary Include="swiftCore" Condition="'$(_IsApplePlatform)' == 'true'" />
<NativeSystemLibrary Include="swiftFoundation" Condition="'$(_IsApplePlatform)' == 'true'" />
<NativeSystemLibrary Include="z" Condition="'$(UseSystemZlib)' == 'true'" />
<NativeSystemLibrary Include="brotlienc;brotlidec;brotlicommon" Condition="'$(UseSystemBrotli)' == 'true'" />
<NativeSystemLibrary Include="rt" Condition="'$(_IsApplePlatform)' != 'true' and '$(_linuxLibcFlavor)' != 'bionic'" />
<NativeSystemLibrary Include="log" Condition="'$(_linuxLibcFlavor)' == 'bionic'" />
<NativeSystemLibrary Include="icucore" Condition="'$(_IsApplePlatform)' == 'true'" />
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Windows.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)" />
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
</ItemGroup>

<ItemGroup>
Expand Down
7 changes: 7 additions & 0 deletions src/installer/pkg/sfx/Microsoft.NETCore.App/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,13 @@
<!-- zlib-specific files -->
<PlatformManifestFileEntry Include="libz.a" IsNative="true" />
<PlatformManifestFileEntry Include="zlibstatic.lib" IsNative="true" />
<!-- brotli-specific files -->
<PlatformManifestFileEntry Include="libbrotlienc.a" IsNative="true" />
<PlatformManifestFileEntry Include="libbrotlidec.a" IsNative="true" />
<PlatformManifestFileEntry Include="libbrotlicommon.a" IsNative="true" />
<PlatformManifestFileEntry Include="brotlienc.lib" IsNative="true" />
<PlatformManifestFileEntry Include="brotlidec.lib" IsNative="true" />
<PlatformManifestFileEntry Include="brotlicommon.lib" IsNative="true" />
</ItemGroup>

<ItemGroup>
Expand Down
5 changes: 3 additions & 2 deletions src/mono/msbuild/android/build/AndroidBuild.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,14 +203,15 @@
<ProfiledAOTProfilePaths Include="$(MibcFilePath)" />
</ItemGroup>

<!--
In order for the runtime to work when static linking, we must supply
<!--
In order for the runtime to work when static linking, we must supply
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" />
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
</ItemGroup>

<PropertyGroup>
Expand Down
21 changes: 11 additions & 10 deletions src/mono/msbuild/apple/build/AppleBuild.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,20 @@
</PropertyGroup>

<UsingTask Condition="'$(AppleGenerateAppBundle)' == 'true'"
TaskName="AppleAppBuilderTask"
TaskName="AppleAppBuilderTask"
AssemblyFile="$(AppleAppBuilderTasksAssemblyPath)" />
<UsingTask Condition="'$(RunAOTCompilation)' == 'true'"
TaskName="ILStrip"
AssemblyFile="$(MonoTargetsTasksAssemblyPath)" />
<UsingTask TaskName="MonoTargetsTasks.MarshalingPInvokeScanner"
<UsingTask TaskName="MonoTargetsTasks.MarshalingPInvokeScanner"
AssemblyFile="$(MonoTargetsTasksAssemblyPath)" />

<Import Condition="Exists('$(ILCompilerTargetsPath)') and '$(UseNativeAOTRuntime)' == 'true'"
Project="$(ILCompilerTargetsPath)" />
<Import Condition="Exists('$(ILLinkTargetsPath)') and '$(UseNativeAOTRuntime)' == 'true'"
Project="$(ILLinkTargetsPath)" />

<Target Name="_CleanPublish"
<Target Name="_CleanPublish"
BeforeTargets="Build">
<RemoveDir Directories="$(PublishDir)" />
</Target>
Expand Down Expand Up @@ -196,7 +196,7 @@
<_IsNative>false</_IsNative>
</_AssembliesToBundleInternal>

<_AotInputAssemblies Include="@(_AssembliesToBundleInternal)"
<_AotInputAssemblies Include="@(_AssembliesToBundleInternal)"
Condition="'%(_AssembliesToBundleInternal._InternalForceInterpret)' != 'true'">
<AotArguments>$(AotArguments)</AotArguments>
<ProcessArguments>$(ProcessArguments)</ProcessArguments>
Expand All @@ -205,7 +205,7 @@
<_AOT_InternalForceInterpretAssemblies Include="@(_AssembliesToBundleInternal->WithMetadataValue('_InternalForceInterpret', 'true'))" />
<_AssembliesToBundleInternal Remove="@(_AssembliesToBundleInternal)" />
</ItemGroup>

<MakeDir Directories="$(_MobileIntermediateOutputPath)" />

<PropertyGroup Condition="'$(iOSLikeDedup)' == 'true'">
Expand All @@ -228,15 +228,16 @@
<_ExcludeFromAppDir Include="$(_iOSLikeDedupAssembly)" />
</ItemGroup>

<!--
In order for the runtime to work when static linking, we must supply
<!--
In order for the runtime to work when static linking, we must supply
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.Net.Security.Native" />
<DirectPInvokes Include="libSystem.Security.Cryptography.Native.Apple" />
<DirectPInvokes Include="libbrotlienc;libbrotlidec" />
</ItemGroup>

<PropertyGroup>
Expand Down Expand Up @@ -274,7 +275,7 @@
<Target Name="_AppleNativeAotCompile"
DependsOnTargets="SetupProperties;ComputeIlcCompileInputs;IlcCompile;$(_IlcLibraryBuildDependsOn)" />

<Target Name="_AppleGenerateAppBundle"
<Target Name="_AppleGenerateAppBundle"
Condition="'$(AppleGenerateAppBundle)' == 'true'"
DependsOnTargets="_AppleGenerateRuntimeConfig">
<!-- Run App bundler, it uses AOT libs (if needed), link all native bits, compile simple UI (written in ObjC)
Expand Down Expand Up @@ -336,7 +337,7 @@
</Target>

<Target Name="_AfterAppleBuild">

</Target>

<Target Name="_AppleGenerateRuntimeConfig"
Expand All @@ -355,4 +356,4 @@
RuntimeConfigReservedProperties="@(_RuntimeConfigReservedProperties)">
</RuntimeConfigParserTask>
</Target>
</Project>
</Project>
4 changes: 4 additions & 0 deletions src/native/corehost/apphost/static/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@ if(CLR_CMAKE_TARGET_WIN32)
delayimp.lib
)

# additional requirements for System.IO.Compression.Native
include(${CLR_SRC_NATIVE_DIR}/libs/System.IO.Compression.Native/extra_libs.cmake)
append_extra_compression_libs(NATIVE_LIBS)

set(RUNTIMEINFO_LIB runtimeinfo)

else()
Expand Down
5 changes: 2 additions & 3 deletions src/native/external/brotli-version.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
1.1.0
https://github.com/google/brotli/releases/tag/v1.1.0

Copy the c/dec, c/enc, c/common, and c/include folders into the root and remove all other files.

Apply https://github.com/google/brotli/commit/85d88cbfc2b742e0742286ec277b73bdbf7be433.patch
Deleted tests, python, docs folders
Apply https://github.com/google/brotli/commit/85d88cbfc2b742e0742286ec277b73bdbf7be433.patch
63 changes: 27 additions & 36 deletions src/native/external/brotli.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
include(FetchContent)

include_directories(BEFORE "${CMAKE_CURRENT_LIST_DIR}/brotli/include")

set (BROTLI_SOURCES_BASE
common/constants.c
common/context.c
common/dictionary.c
common/platform.c
common/shared_dictionary.c
common/transform.c
dec/bit_reader.c
dec/decode.c
dec/huffman.c
dec/state.c
enc/backward_references.c
enc/backward_references_hq.c
enc/bit_cost.c
enc/block_splitter.c
enc/brotli_bit_stream.c
enc/cluster.c
enc/command.c
enc/compound_dictionary.c
enc/compress_fragment.c
enc/compress_fragment_two_pass.c
enc/dictionary_hash.c
enc/encode.c
enc/encoder_dict.c
enc/entropy_encode.c
enc/fast_log.c
enc/histogram.c
enc/literal_cost.c
enc/memory.c
enc/metablock.c
enc/static_dict.c
enc/utf8_util.c
FetchContent_Declare(
brotli
SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/brotli
)

addprefix(BROTLI_SOURCES "${CMAKE_CURRENT_LIST_DIR}/brotli" "${BROTLI_SOURCES_BASE}")
set(BROTLI_DISABLE_TESTS ON)
set(__CURRENT_BUILD_SHARED_LIBS ${BUILD_SHARED_LIBS})
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

set(BUILD_SHARED_LIBS OFF)
FetchContent_MakeAvailable(brotli)
set(BUILD_SHARED_LIBS ${__CURRENT_BUILD_SHARED_LIBS})

target_compile_options(brotlicommon PRIVATE $<$<COMPILE_LANG_AND_ID:C,MSVC>:/guard:cf>)
target_compile_options(brotlienc PRIVATE $<$<COMPILE_LANG_AND_ID:C,MSVC>:/guard:cf>)
target_compile_options(brotlidec PRIVATE $<$<COMPILE_LANG_AND_ID:C,MSVC>:/guard:cf>)
target_compile_options(brotlienc PRIVATE $<$<COMPILE_LANG_AND_ID:C,Clang,AppleClang,GNU>:-Wno-implicit-fallthrough>)
target_compile_options(brotlidec PRIVATE $<$<COMPILE_LANG_AND_ID:C,Clang,AppleClang,GNU>:-Wno-implicit-fallthrough>)

# Even though we aren't building brotli as shared libraries, we still need to be able to export the symbols
# from the brotli libraries so that they can be used by System.IO.Compression.
# Since we link all of the static libraries into a single shared library, we need to define BROTLICOMMON_SHARED_COMPILATION
# for all targets so brotlienc and brotlidec don't expect to link against a separate brotlicommon shared library.
target_compile_definitions(brotlienc PRIVATE BROTLI_SHARED_COMPILATION BROTLIENC_SHARED_COMPILATION BROTLICOMMON_SHARED_COMPILATION)
target_compile_definitions(brotlidec PRIVATE BROTLI_SHARED_COMPILATION BROTLIDEC_SHARED_COMPILATION BROTLICOMMON_SHARED_COMPILATION)
target_compile_definitions(brotlicommon PRIVATE BROTLI_SHARED_COMPILATION BROTLICOMMON_SHARED_COMPILATION)

# Don't build the brotli command line tool unless explicitly requested
# (which we never do)
set_target_properties(brotli PROPERTIES EXCLUDE_FROM_ALL ON)
Loading
Loading