Skip to content

Fix build with -DCLR_CMAKE_USE_SYSTEM_BROTLI=true - #110816

Merged
jkoritzinsky merged 1 commit into
dotnet:mainfrom
omajid:another-attempt-at-fixing-system-brotli
Dec 20, 2024
Merged

Fix build with -DCLR_CMAKE_USE_SYSTEM_BROTLI=true#110816
jkoritzinsky merged 1 commit into
dotnet:mainfrom
omajid:another-attempt-at-fixing-system-brotli

Conversation

@omajid

@omajid omajid commented Dec 18, 2024

Copy link
Copy Markdown
Member

Currently, this fails on Fedora 41:

$ ./build.sh  --cmakeargs  -DCLR_CMAKE_USE_SYSTEM_BROTLI=true /p:FullAssemblySigningSupported=false
...
[100%] Linking CXX executable singlefilehost
ld.lld: error: undefined symbol: BrotliDecoderCreateInstance
>>> referenced by entrypoints.c
>>> entrypoints.c.o:(s_compressionNative) in archive ../libs-native/System.IO.Compression.Native/libSystem.IO.Compression.Native.a

This seems to be a regression introduced when this bit was accidentally dropped:
https://github.com/dotnet/runtime/pull/109707/files#diff-7a160e52815fdd808d9415ada41dd5b1748826b27c78277e14f4adcf1ce61511

Fix the build by re-introducing it.

Fixes: #110751

@omajid
omajid requested a review from jkoritzinsky December 18, 2024 16:27
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Dec 18, 2024
@omajid
omajid force-pushed the another-attempt-at-fixing-system-brotli branch from 88f19ab to 5e42b11 Compare December 18, 2024 16:27
Comment on lines +24 to +25
find_library(BROTLIDEC brotlidec REQUIRED)
find_library(BROTLIENC brotlienc REQUIRED)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we move these into Compression.Native/CMakeLists.txt and add the two libraries to the BROTLI_LIBRARIES variable? That would keep all of the brotli handling together.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Does the updated change look okay?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Wait, this doesn't work at all :(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, because src/native/corehost/apphost/static/CMakeLists.txt only includes NATIVE_LIBS

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

Should it include BROTLI_LIBRARIES under a CLR_CMAKE_USE_SYSTEM_BROTL conditional?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The BROTLI_LIBRARIES values should be propagated up to the static apphost due to

target_link_libraries(System.IO.Compression.Native-Static PUBLIC ${BROTLI_LIBRARIES})

and

target_link_libraries(System.IO.Compression.Native-Static PUBLIC ${BROTLI_LIBRARIES})

You can change it back to how it was before to unblock and I can investigate later.

@omajid
omajid force-pushed the another-attempt-at-fixing-system-brotli branch from 5e42b11 to aa6a70b Compare December 18, 2024 16:39
@omajid
omajid marked this pull request as draft December 18, 2024 17:20
Currently, this fails on Fedora 41:

    $ ./build.sh  --cmakeargs  -DCLR_CMAKE_USE_SYSTEM_BROTLI=true /p:FullAssemblySigningSupported=false
    ...
    [100%] Linking CXX executable singlefilehost
    ld.lld: error: undefined symbol: BrotliDecoderCreateInstance
    >>> referenced by entrypoints.c
    >>> entrypoints.c.o:(s_compressionNative) in archive ../libs-native/System.IO.Compression.Native/libSystem.IO.Compression.Native.a

This seems to be a regression introduced when this bit was accidentally
dropped:
https://github.com/dotnet/runtime/pull/109707/files#diff-7a160e52815fdd808d9415ada41dd5b1748826b27c78277e14f4adcf1ce61511

Fix the build by re-introducing it.

Fixes: dotnet#110751
@omajid
omajid force-pushed the another-attempt-at-fixing-system-brotli branch from aa6a70b to 6acd94c Compare December 18, 2024 19:53
@omajid
omajid marked this pull request as ready for review December 18, 2024 20:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Compression community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build fails with -DCLR_CMAKE_USE_SYSTEM_BROTLI=true

2 participants