Use system brotli on Unix non-portable builds #107225

agocke · 2024-08-31T02:37:23Z

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

…e.Unix.targets Co-authored-by: Jan Kotas <[email protected]>

jkotas · 2024-08-31T06:42:10Z

@tmds Could you please validate that this is fixing #107020?

tmds · 2024-08-31T07:02:17Z

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

@@ -164,6 +164,11 @@ 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')">


Can we extend this with something that is similar to https://github.com/dotnet/runtime/pull/107166/files#r1739122333? Then source-build can be built with bundled or system brotli.

Otherwise we need to add --cmakeargs -DCLR_CMAKE_USE_SYSTEM_BROTLI=true (similar to what is in the diff in #107020 (comment)) and source-building requires system brotli.

For .NET 10, we will get it with #107166.

For .NET 9, it would require build refactoring that is hard to justify for a backport at this point.

ok, then we need to add something that sets CLR_CMAKE_USE_SYSTEM_BROTLI=true so maintainers provide brotli in their non-portable builds.

Without it, the brotli the bootstrap build will work (since Microsoft's portable SDK doesn't have a nonportable.txt breadcrumb) but successive builds using the produced non-portable SDK won't, because NativeAOT will fail when trying to find brotli on a system that doesn't provide it.

Why is this not sufficient https://github.com/dotnet/runtime/blob/main/eng/DotNetBuild.props#L91 ?

That doesn't do anything unless the maintainer builds the vmr with --with-system-libs brotli.

If he doesn't add it, his bootstrap build works, but he is not aware that successive builds using that SDK will fail unless he provides brotli.

By adding CLR_CMAKE_USE_SYSTEM_BROTLI=true for non-portable, we make this issue visible also on the bootstrap build.

That doesn't do anything unless the maintainer builds the vmr with --with-system-libs brotli.

Are there any distros that do not want to use --with-system-libs brotli? I think that the conversation in #107020 suggested that all interested distro builds want to use --with-system-libs brotli.

Yes, there are no maintainers that have objected to using system brotli, and we are assuming brotli to be a common available system library.

Note that this change will also require a brotli development package to be added to the source-build CI jobs.

By that you mean added to the container?

tmds · 2024-08-31T07:05:24Z

@tmds Could you please validate that this is fixing #107020?

Yes, I will do some builds to validate this.

tmds · 2024-09-02T10:09:52Z

cc @omajid @MichaelSimons

tmds · 2024-09-03T07:41:59Z

Yes, I will do some builds to validate this.

#107225 (comment) needs to be tackled first.

agocke · 2024-09-05T17:52:04Z

@tmds Have you validated that this fix works locally? I can start upgrading the containers separately.

tmds · 2024-09-05T18:32:45Z

Have you validated that this fix works locally? I can start upgrading the containers separately.

Can we address #107225 (comment) first?

I'll validate this by building the vmr, and then building it again using the output of the first build.
It's going to take a few hours to build, so it would be preferable to validate the complete patch.

tmds · 2024-09-05T20:38:20Z

Can we address #107225 (comment) first?

By this, I mean: add something to DotNetBuild.props similar to what is shown in this diff: #107020 (comment).

Part of dotnet/runtime#107225

tmds · 2024-09-06T14:29:03Z

@agocke I've validated this change, and it is working as expected both for the portable publish as the non-portable publish.

agocke · 2024-09-10T22:15:03Z

It looks like this works now, after the update to the containers. From the build log:

Executing "/__w/1/s/artifacts/sb/src/src/coreclr/build-runtime.sh" -cmakeargs " -DCLR_CMAKE_USE_SYSTEM_BROTLI=true" -x64 -release -ci -portablebuild=false -keepnativesymbols -os linux -outputrid linux-x64 -cmakeargs "-DCLR_DOTNET_HOST_PATH=/__w/1/s/.dotnet/dotnet" -cmakeargs "-DCDAC_BUILD_TOOL_BINARY_PATH=/__w/1/s/artifacts/sb/src/artifacts/bin/coreclr/linux.x64.Release/cdac-build-tool/cdac-build-tool.dll"

agocke · 2024-09-11T17:34:08Z

@tmds This is now waiting on your review, let me know if there's anything else needed to unblock the build.

tmds

Lgtm! Thanks!

agocke · 2024-09-11T17:53:05Z

@jkotas Anything else I should cover?

jkotas

LGTM

jkotas · 2024-09-11T18:27:11Z

@jkoritzinsky This change should be reverted in your PR

agocke · 2024-09-11T20:37:24Z

/backport to release/9.0

github-actions · 2024-09-11T20:37:35Z

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10819302776

This reverts commit 3979ef8.

Co-authored-by: Jan Kotas <[email protected]>

am11 · 2024-09-25T21:58:16Z

This is failing Android build in SDK codeflow: https://dev.azure.com/dnceng-public/public/_build/results?buildId=818433&view=logs&jobId=66673ce3-9c46-54b2-cf79-862a2d55b22a&j=66673ce3-9c46-54b2-cf79-862a2d55b22a&t=f73a763b-d161-54c1-b189-48197f51b0b6

dotnet/sdk#43070

      Running '   cmake -DCMAKE_INSTALL_PREFIX="/vmr/src/runtime/artifacts/obj/mono/android.arm64.Release/out" -DCMAKE_INSTALL_LIBDIR=lib -DCMAKE_BUILD_TYPE=Release  -DCLR_CMAKE_USE_SYSTEM_BROTLI=true -DGC_SUSPEND=hybrid -DMONO_LIB_NAME=monosgen-2.0 -DMONO_SHARED_LIB_NAME=monosgen-2.0 -DCMAKE_TOOLCHAIN_FILE=/usr/local/android-ndk/build/cmake/android.toolchain.cmake -DANDROID_NDK=/usr/local/android-ndk -DANDROID_STL=none -DANDROID_CPP_FEATURES="no-rtti no-exceptions" -DANDROID_PLATFORM=android-21 -DANDROID_ABI=arm64-v8a -DENABLE_MINIMAL=ssa,logging -DFEATURE_PERFTRACING_PAL_TCP=1 -DFEATURE_PERFTRACING_DISABLE_DEFAULT_LISTEN_PORT=1 -DMONO_COMPONENTS_RID=android-arm64 -DCLR_CMAKE_HOST_ARCH=arm64 -DCMAKE_C_FLAGS=" -fpic -fstack-protector -DANDROID64 -DL_cuserid=9 -D__POSIX_VISIBLE=201002 -DSK_RELEASE -DNDEBUG -UDEBUG -Wl,--build-id=sha1" -DCMAKE_CXX_FLAGS=" -fpic -fstack-protector -DANDROID64 -DL_cuserid=9 -D__POSIX_VISIBLE=201002 -DSK_RELEASE -DNDEBUG -UDEBUG -Wl,--build-id=sha1"  "/vmr/src/runtime/src/mono"' in '/vmr/src/runtime/artifacts/obj/mono/android.arm64.Release/'
      -- The C compiler identification is Clang 12.0.9
      -- The CXX compiler identification is Clang 12.0.9
      -- Detecting C compiler ABI info
      -- Detecting C compiler ABI info - done
      -- Check for working C compiler: /usr/local/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/clang - skipped
      -- Detecting C compile features
      -- Detecting C compile features - done
      -- Detecting CXX compiler ABI info
      -- Detecting CXX compiler ABI info - done
      -- Check for working CXX compiler: /usr/local/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ - skipped
      -- Detecting CXX compile features
      -- Detecting CXX compile features - done
      -- The linker identification is LLD 12.0.9 (/buildbot/src/android/llvm-r416183/out/llvm-project/lld c935d99d7cf2016289302412d708641d52d2f7ee) (compatible with GNU linkers)
      CMAKE_SYSTEM_NAME=Android
      CMAKE_SYSTEM_VARIANT=
      CMAKE_SYSTEM_PROCESSOR=arm64
      TARGET_ARCH=arm64
      CMAKE_CROSSCOMPILING=TRUE
      CMake Error at /vmr/src/runtime/src/native/libs/System.IO.Compression.Native/extra_libs.cmake:24 (find_library):
        Could not find BROTLIDEC using the following names: brotlidec
      Call Stack (most recent call first):
        CMakeLists.txt:517 (append_extra_compression_libs)

cc @lewing

am11 · 2024-09-25T22:22:44Z

Fix: dotnet/arcade#15107. After merging, we will need to trigger https://github.com/dotnet/dotnet-buildtools-prereqs-docker/tree/main/src/azurelinux/3.0/net9.0/cross/amd64 and then https://github.com/dotnet/dotnet-buildtools-prereqs-docker/tree/main/src/azurelinux/3.0/net9.0/cross/android build. cc @mthalman

mthalman · 2024-09-27T13:15:34Z

Fix: dotnet/arcade#15107. After merging, we will need to trigger https://github.com/dotnet/dotnet-buildtools-prereqs-docker/tree/main/src/azurelinux/3.0/net9.0/cross/amd64 and then https://github.com/dotnet/dotnet-buildtools-prereqs-docker/tree/main/src/azurelinux/3.0/net9.0/cross/android build. cc @mthalman

These images have been published with the updates.

am11 · 2024-09-27T13:56:12Z

Thanks! "created": "2024-09-26T17:13:41.4665931Z", (20 hrs. ago) and the job that ran afterwards (15 hrs. ago) is passing, while the previous one failed. 👍

Co-authored-by: Jan Kotas <[email protected]>

Use system brotli on Unix non-portable builds

edea6f9

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label Aug 31, 2024

dotnet-policy-service bot assigned agocke Aug 31, 2024

agocke mentioned this pull request Aug 31, 2024

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

Closed

agocke marked this pull request as ready for review August 31, 2024 02:37

agocke requested a review from MichalStrehovsky as a code owner August 31, 2024 02:37

jkotas reviewed Aug 31, 2024

View reviewed changes

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets Outdated Show resolved Hide resolved

Update src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Nativ…

a6c4285

…e.Unix.targets Co-authored-by: Jan Kotas <[email protected]>

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

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

Open

3 tasks

tmds reviewed Aug 31, 2024

View reviewed changes

Use system brotli for non-portable builds

f3172f9

agocke added a commit to dotnet/dotnet-buildtools-prereqs-docker that referenced this pull request Sep 6, 2024

Add brotli to sourcebuild container

66dfbf3

Part of dotnet/runtime#107225

agocke mentioned this pull request Sep 6, 2024

Add brotli to sourcebuild container dotnet/dotnet-buildtools-prereqs-docker#1197

Merged

tmds approved these changes Sep 11, 2024

View reviewed changes

jkotas approved these changes Sep 11, 2024

View reviewed changes

agocke merged commit 3979ef8 into dotnet:main Sep 11, 2024
150 checks passed

agocke deleted the brotli-nonportable branch September 11, 2024 20:36

github-actions bot mentioned this pull request Sep 11, 2024

[release/9.0] Use system brotli on Unix non-portable builds #107708

Merged

4 tasks

jkoritzinsky added a commit to jkoritzinsky/runtime that referenced this pull request Sep 12, 2024

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

599dc91

This reverts commit 3979ef8.

omajid mentioned this pull request Sep 16, 2024

Put brotli on the FetchContent plan #107166

Merged

jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 17, 2024

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

7d8c33f

Co-authored-by: Jan Kotas <[email protected]>

tmds mentioned this pull request Sep 24, 2024

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

Open

sirntar pushed a commit to sirntar/runtime that referenced this pull request Sep 30, 2024

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

c50fb5e

Co-authored-by: Jan Kotas <[email protected]>

github-actions bot locked and limited conversation to collaborators Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use system brotli on Unix non-portable builds #107225

Use system brotli on Unix non-portable builds #107225

agocke commented Aug 31, 2024 •

edited by jkotas

Loading

jkotas commented Aug 31, 2024

tmds Aug 31, 2024

jkotas Sep 3, 2024

tmds Sep 4, 2024

jkotas Sep 4, 2024

tmds Sep 4, 2024

jkotas Sep 4, 2024

tmds Sep 4, 2024

agocke Sep 4, 2024

tmds Sep 4, 2024

tmds commented Aug 31, 2024

tmds commented Sep 2, 2024

tmds commented Sep 3, 2024

agocke commented Sep 5, 2024

tmds commented Sep 5, 2024

tmds commented Sep 5, 2024

tmds commented Sep 6, 2024

agocke commented Sep 10, 2024

agocke commented Sep 11, 2024

tmds left a comment

agocke commented Sep 11, 2024

jkotas left a comment

jkotas commented Sep 11, 2024

agocke commented Sep 11, 2024

github-actions bot commented Sep 11, 2024

am11 commented Sep 25, 2024

am11 commented Sep 25, 2024 •

edited

Loading

mthalman commented Sep 27, 2024

am11 commented Sep 27, 2024

Use system brotli on Unix non-portable builds #107225

Use system brotli on Unix non-portable builds #107225

Conversation

agocke commented Aug 31, 2024 • edited by jkotas Loading

jkotas commented Aug 31, 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

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 31, 2024

tmds commented Sep 2, 2024

tmds commented Sep 3, 2024

agocke commented Sep 5, 2024

tmds commented Sep 5, 2024

tmds commented Sep 5, 2024

tmds commented Sep 6, 2024

agocke commented Sep 10, 2024

agocke commented Sep 11, 2024

tmds left a comment

Choose a reason for hiding this comment

agocke commented Sep 11, 2024

jkotas left a comment

Choose a reason for hiding this comment

jkotas commented Sep 11, 2024

agocke commented Sep 11, 2024

github-actions bot commented Sep 11, 2024

am11 commented Sep 25, 2024

am11 commented Sep 25, 2024 • edited Loading

mthalman commented Sep 27, 2024

am11 commented Sep 27, 2024

agocke commented Aug 31, 2024 •

edited by jkotas

Loading

am11 commented Sep 25, 2024 •

edited

Loading