Fix name of zlib that the linker needs #104904

omajid · 2024-07-15T16:23:23Z

It needs to be z so the linker argument becomes -lz. Otherwise the linker sees -lzlib which fails on Linux.

To reproduce the error, I used:

$ ./build.sh --cmakeargs -DCLR_CMAKE_USE_SYSTEM_ZLIB=true

It needs to be `z` so the linker argument becomes `-lz`. Otherwise the linker sees `-lzlib` which fails on Linux. To reproduce the error, I used: $ ./build.sh --cmakeargs -DCLR_CMAKE_USE_SYSTEM_ZLIB=true

omajid · 2024-07-15T16:23:33Z

I think this was introduced by https://github.com/dotnet/runtime/pull/104454/files#diff-7a160e52815fdd808d9415ada41dd5b1748826b27c78277e14f4adcf1ce61511R16

dotnet-policy-service · 2024-07-15T16:23:55Z

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

omajid · 2024-07-15T16:28:13Z

cc @carlossanlop

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

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

carlossanlop · 2024-07-15T21:49:27Z

/azp run runtime-native-aot

carlossanlop · 2024-07-15T21:49:51Z

/azp runtime-nativeaot-outerloop

carlossanlop · 2024-07-15T21:50:13Z

/azp run runtime-community

carlossanlop · 2024-07-15T21:50:45Z

/azp run runtime-nativeaot-outerloop

carlossanlop · 2024-07-15T21:52:04Z

/azp run runtime-extra-platforms

carlossanlop

Approving after confirming the CI looks good.

jkotas · 2024-07-16T04:09:46Z

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

@@ -13,7 +13,7 @@ macro(append_extra_compression_libs NativeLibsExtra)
      find_package(ZLIB REQUIRED)
      list(APPEND ZLIB_LIBRARIES m)
  else()
-    list(APPEND ZLIB_LIBRARIES zlib)
+    list(APPEND ZLIB_LIBRARIES $<IF:$<BOOL:CLR_CMAKE_USE_SYSTEM_ZLIB>,z,zlib>)


We should be able to delete the ANDROID special-case above since it should be handled by this condition just fine now.

Good point. Done. I also added a comment.

…re used.

carlossanlop · 2024-07-16T18:17:47Z

/ba-g Failure is an unrelated timeout to download infra resources in a wasm leg. Other wasm legs passed:

src/mono/nuget/Microsoft.NET.Runtime.WorkloadTesting.Internal/Sdk/WorkloadTesting.Core.targets(82,5): error MSB3923: Failed to download file "https://dot.net/v1/dotnet-install.sh". The request was canceled due to the configured HttpClient.Timeout of 100 seconds elapsing. ---> A task was canceled. ---> A task was canceled.

carlossanlop · 2024-07-23T17:44:59Z

@LoopedBard3 what this change did was make sure that when building, the CLR_CMAKE_USE_SYSTEM_ZLIB cmake property can be set from the command line. Are we passing that variable to the command for perf runs on Linux?

Edit: Context is #105331 (comment)

LoopedBard3 · 2024-07-23T17:55:16Z

@carlossanlop We should be using the same build steps as the runtime runs unless something has updated for in the steps that we have not gotten. The current command line for building linux x64 ends up being build.sh -ci -arch x64 -os linux -cross -s clr+libs+host+packs -c Release with the top level build.sh in the runtime repo (https://dev.azure.com/dnceng/internal/_build/results?buildId=2500691&view=results). This is the specific yml we use to build the runtime: https://github.com/dotnet/runtime/blob/main/eng/pipelines/coreclr/perf-non-wasm-jobs.yml#L5-L28.

omajid · 2024-07-23T18:07:59Z

I did some quick testing:

$ ./build.sh
$ find -iname '*Compression*so'
...
./artifacts/bin/coreclr/linux.x64.Debug/ilc/libSystem.IO.Compression.Native.so
...
$  ldd ./artifacts/bin/coreclr/linux.x64.Debug/ilc/libSystem.IO.Compression.Native.so
        linux-vdso.so.1 (0x00007f23b178d000)
        libz.so.1 => /lib64/libz.so.1 (0x00007f23b1680000)
        libm.so.6 => /lib64/libm.so.6 (0x00007f23b159c000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f23b13ab000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f23b178f000)

Looks like we still link to libz in a default build configuration?

mtl1979 · 2024-07-23T18:59:45Z

Could be that BOOL:CLR_CMAKE_USE_SYSTEM_ZLIB here always returns true... Easy test would be to try changing the generator expression to normal if block.

carlossanlop · 2024-07-23T20:05:00Z

Good find @omajid.
Checking your suggestion now, @mtl1979.

am11 · 2024-07-23T20:05:53Z

We are using generator pattern in many places in the repo, I don't think it's due to that.

According to artifacts/obj/native/linux-x64-Debug/CMakeFiles/CMakeConfigureLog.yaml, it's coming from the first one here:

% git grep relro
eng/native/configurecompiler.cmake:  add_linker_flag(-Wl,-z,relro,-z,now)
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets:      <LinkerArg Include="-Wl,-z,relro" Condition="'$(_IsApplePlatform)' != 'true'" />
src/mono/CMakeLists.txt:  add_link_options(-Wl,-z,relro)
src/mono/CMakeLists.txt:  add_compile_options(-Wl,-z,relro)

jkoritzinsky · 2024-07-23T20:26:44Z

@am11 can you share your configure log?

am11 · 2024-07-23T20:30:14Z

Sorry, my bad. It was actually a syntax error. :(
This is the fix: #105352.

carlossanlop · 2024-07-23T20:45:54Z

Sorry, my bad. It was actually a syntax error. :(

No worries, we found it on time :)

Fix name of zlib that the linker needs

7038c04

It needs to be `z` so the linker argument becomes `-lz`. Otherwise the linker sees `-lzlib` which fails on Linux. To reproduce the error, I used: $ ./build.sh --cmakeargs -DCLR_CMAKE_USE_SYSTEM_ZLIB=true

dotnet-issue-labeler bot added the area-System.IO.Compression label Jul 15, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 15, 2024

ViktorHofer requested a review from carlossanlop July 15, 2024 16:27

carlossanlop requested changes Jul 15, 2024

View reviewed changes

src/native/libs/System.IO.Compression.Native/extra_libs.cmake Outdated Show resolved Hide resolved

Conditionally use z or zlib depending on CLR_CMAKE_USE_SYSTEM_ZLIB

add5b8e

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

carlossanlop approved these changes Jul 15, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jul 16, 2024

[NativeAOT] Object synchronization method was called from an unsynchronized block of code. #104340

Closed

This was referenced Jul 16, 2024

runtime-community: Failure to build System.Security.Cryptography.Native/opensslshim.c due to overflow in expression with type long #104333

Open

Test failure System.Diagnostics.Tests.ProcessStartInfoTests.StartInfo_BadExe: Values differ #104941

Closed

build-analysis bot mentioned this pull request Jul 16, 2024

Test failure: System.Diagnostics.Tests.ProcessStartInfoTests.StartInfo_BadExe(useShellExecute: False) #104770

Closed

jkotas reviewed Jul 16, 2024

View reviewed changes

Remove the android branch, add a comment to explain when z and zlib a…

95a5938

…re used.

carlossanlop merged commit f1d9579 into dotnet:main Jul 16, 2024
157 of 159 checks passed

sebastienros mentioned this pull request Jul 17, 2024

[main] Update dependencies from dotnet/efcore, dotnet/runtime dotnet/aspnetcore#56684

Merged

LoopedBard3 mentioned this pull request Jul 23, 2024

[Perf] Linux/x64: Regressions in System.IO.Compression #105331

Closed

jkurdek mentioned this pull request Aug 7, 2024

[Perf] Linux/x64: 24 Regressions on 7/16/2024 9:50:36 PM dotnet/perf-autofiling-issues#38789

Closed

github-actions bot locked and limited conversation to collaborators Aug 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix name of zlib that the linker needs #104904

Fix name of zlib that the linker needs #104904

omajid commented Jul 15, 2024

omajid commented Jul 15, 2024

dotnet-policy-service bot commented Jul 15, 2024

omajid commented Jul 15, 2024

carlossanlop commented Jul 15, 2024

This comment was marked as outdated.

carlossanlop commented Jul 15, 2024

This comment was marked as resolved.

carlossanlop commented Jul 15, 2024

This comment was marked as outdated.

carlossanlop commented Jul 15, 2024

This comment was marked as outdated.

carlossanlop commented Jul 15, 2024

This comment was marked as outdated.

carlossanlop left a comment

jkotas Jul 16, 2024

carlossanlop Jul 16, 2024

carlossanlop commented Jul 16, 2024

carlossanlop commented Jul 23, 2024 •

edited

Loading

LoopedBard3 commented Jul 23, 2024 •

edited

Loading

omajid commented Jul 23, 2024

mtl1979 commented Jul 23, 2024

carlossanlop commented Jul 23, 2024

am11 commented Jul 23, 2024

jkoritzinsky commented Jul 23, 2024

am11 commented Jul 23, 2024

carlossanlop commented Jul 23, 2024

Fix name of zlib that the linker needs #104904

Fix name of zlib that the linker needs #104904

Conversation

omajid commented Jul 15, 2024

omajid commented Jul 15, 2024

dotnet-policy-service bot commented Jul 15, 2024

omajid commented Jul 15, 2024

carlossanlop commented Jul 15, 2024

This comment was marked as outdated.

carlossanlop commented Jul 15, 2024

This comment was marked as resolved.

carlossanlop commented Jul 15, 2024

This comment was marked as outdated.

carlossanlop commented Jul 15, 2024

This comment was marked as outdated.

carlossanlop commented Jul 15, 2024

This comment was marked as outdated.

carlossanlop left a comment

Choose a reason for hiding this comment

jkotas Jul 16, 2024

Choose a reason for hiding this comment

carlossanlop Jul 16, 2024

Choose a reason for hiding this comment

carlossanlop commented Jul 16, 2024

carlossanlop commented Jul 23, 2024 • edited Loading

LoopedBard3 commented Jul 23, 2024 • edited Loading

omajid commented Jul 23, 2024

mtl1979 commented Jul 23, 2024

carlossanlop commented Jul 23, 2024

am11 commented Jul 23, 2024

jkoritzinsky commented Jul 23, 2024

am11 commented Jul 23, 2024

carlossanlop commented Jul 23, 2024

carlossanlop commented Jul 23, 2024 •

edited

Loading

LoopedBard3 commented Jul 23, 2024 •

edited

Loading