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 (Try 2) #109707

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Nov 11, 2024

Unlike the first try, don't ship the brotli libs in two places. Since no runtime references the brotli components, only build brotli alongside the native libs components for each platform.

@jkoritzinsky
Copy link
Member Author

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

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

… always side-by-side (and we don't need to add it to the Mono build)
@jkoritzinsky
Copy link
Member Author

/azp run runtime-ios, runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -169,6 +171,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. -->
<ItemGroup Condition="'$(UseSystemBrotli)' != 'true'">
<NativeLibrary Include="$(IlcFrameworkNativePath)libbrotlienc.a" />
Copy link
Member

Choose a reason for hiding this comment

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

We get libz from IlcSdkPath. What's the reason for brotli to be different?

Copy link
Member Author

Choose a reason for hiding this comment

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

Getting libz from the IlcSdkPath works well because we end up building zlib with the Mono runtime and we build the zlib libs as part of the NativeAOT runtime build (that builds the libraries static libs).

For brotli, Mono doesn't currently build brotli in any capacity or link it into their runtime at all. As a result, putting the brotli libs into the ILC SDK and not into the "libraries native components static libs" folder causes Mono-based static linking scenarios to fail as they don't have anywhere to get the brotli symbols from. In particular, we hit this on iOS, a platform where we decided to continue using the system zlib implementation due to size constraints and linking considerations.

Copy link
Member

Choose a reason for hiding this comment

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

The brotli libs are still under IlcSdkPath on Windows. Is that right?

It does not look right to me that configuration of Mono targets is forcing a directory structure of NAOT packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

The brotli libs are next to the Compression.Native libs for all platforms. On Windows this is IlcSdkPath, on non-Windows it is not.

Copy link
Member

Choose a reason for hiding this comment

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

Should we have libz next to Compression.Native too to have some sort of consistency?

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 can try giving that a shot now that we aren't going to use a vendored zlib on mobile platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants