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

Move UseSystemZlib into SetupOSSpecificProps #107666

Merged
merged 6 commits into from
Sep 12, 2024

Conversation

hwoodiwiss
Copy link
Contributor

@hwoodiwiss hwoodiwiss commented Sep 11, 2024

I have a hunch that the line I've moved is being evaluated before SetupProperties, as it's not currenlty in any target.
It relies on SetupProperties for IlcSdkPath to have a value, so this would cause it to set UseSystemZlib=true in all cases.

My hope is that moving it into SetupOSSpecificProps will fix this ordering issue, as SetupProperties is part of IlcDynamicBuildPropertyDependencies

Hopefully, fixes #106566

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 11, 2024
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you for submitting the fix! I have confirmed that it is fixing the problem. @carlossanlop Could you please check that it is fixing the problem as well?

@carlossanlop
Copy link
Member

That was quick! Truly appreciated. Thank you! I'll take a look tomorrow as soon as I get a chance.

@carlossanlop carlossanlop self-assigned this Sep 11, 2024
Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

@hwoodiwiss, is it so that before:

$ dotnet new console -n foo
$ cd foo
$ dotnet publish -p:PublishAot=true -o dist
$ ldd dist/foo

is showing libz and this PR is going to remove it? It doesn't seems to repro with current RC2 daily build (dotnet --version -> 9.0.100-rc.2.24460.28). MSBuild should propagate properties to all context and there are more properties propagated from global contexts which affect publishing. Perhaps the fix didn't made it in preview7, could you please try with daily build to confirm the issue persists in your environment?

@hwoodiwiss
Copy link
Contributor Author

hwoodiwiss commented Sep 11, 2024

I'll give it a try with the daily build, based on the binlogs, I can see that it's definitely using the version of this file that was backported in #106673 in my RC1 builds

@am11
Copy link
Member

am11 commented Sep 11, 2024

Yup, some fixes went in ratehr recently, so it would be good to confirm it with latest .NET9 build.
Also, if you haven't already, delete obj/ dir before publishing.

@hwoodiwiss
Copy link
Contributor Author

My test builds have been in github actions, so existing intermediate files shouldn't be an issue.

@martincostello
Copy link
Member

With version 9.0.100-rc.2.24461.4 of the SDK, I can still repro the issue in Azure App Service:

2024-09-11T10:40:31.830250750Z /app/Website: error while loading shared libraries: libz.so.1: cannot open shared object file: No such file or directory

@am11
Copy link
Member

am11 commented Sep 11, 2024

@martincostello could you share the output of ldd /app/Website?

@martincostello
Copy link
Member

Not easily, no. It was built from this commit if you want to build the image locally and inspect it: martincostello/website@41e4bb4

@hwoodiwiss
Copy link
Contributor Author

hwoodiwiss commented Sep 11, 2024

Testing Notes

This comment will consolidate the various testing I had spread across a few comments:

RC2

Tested publish from RC2, arm64 and amd64, both seemed to have libz.so.1 still referenced. (Test PR and build)

Checked arm64 using strings HwoodiwissHelper | grep libz.so as I didn't have an arm device to hand, or an arm version of ldd installed. That found libz.so.1

Checked amd64 using ldd HwoodiwissHelper, which gave the result:

        linux-vdso.so.1 (0x00007ffff04b9000)
        libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007ff089b00000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007ff089a10000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007ff0897e0000)
        /lib64/ld-linux-x86-64.so.2 (0x00007ff095baa000)

This change

I've tested this change by manually updating the file in .nuget/packages/microsoft.dotnet.ilcompiler/9.0.0-rc.1.24431.7/build/Microsoft.NETCore.Native.Unix.targets and doing a clean aot publish in WSL. This seemed to work, and the result of ldd HwoodiwissHelper is

        linux-vdso.so.1 (0x00007ffee5d17000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f19fdbd7000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f19fd9ae000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f1a09d56000)

@am11
Copy link
Member

am11 commented Sep 11, 2024

Thanks for validating the change and taking additional steps. I'd love to understand why it is not reproducible with console, webapi and webapiaot template apps published with PublishAot=true, I think it might be something pulling it up in container app model.

as I didn't have an arm device to hand

It is enough for testing I think, but as a tip for future, there are multiple ways to go about it, two of which are:

  1. install arch-specific binutils for your target (e.g. apt install binutils-aarch64-linux-gnu) and then use aarch64-linux-gnu-readelf -a path/to/app | grep "Shared library:"
  2. install multi-arch llvm (apt install llvm), then use llvm-readelf --needed-libs path/to/app

@hwoodiwiss
Copy link
Contributor Author

I'd love to understand why it is not reproducible with console, webapi and webapiaot template apps

I had a hunch on this too, they're not using any compression, so System.Io.Compression gets trimmed

Here's an example without usage - https://github.com/hwoodiwiss/ZlibDependencyTest/actions/runs/10819324810/job/30016934658#step:7:20

Example from a PR that adds usage -
https://github.com/hwoodiwiss/ZlibDependencyTest/actions/runs/10819331997/job/30016956732#step:7:21

@jkotas
Copy link
Member

jkotas commented Sep 11, 2024

Merge branch 'main' into hwoodiwiss-106566

@hwoodiwiss You do not have to be doing this. The repo maintainers will do it as necessary once the PR is ready to merge.

@MichalStrehovsky
Copy link
Member

Thanks for validating the change and taking additional steps. I'd love to understand why it is not reproducible with console, webapi and webapiaot template apps published with PublishAot=true, I think it might be something pulling it up in container app model.

Last time I was looking at it, webapi/webapiaot didn't use libz. Console doesn't use it for sure.

@am11
Copy link
Member

am11 commented Sep 12, 2024

Ah, didn't knew that linker optimization is this thorough that it will drop the the entire dependency. :)

Changing it to no-as-needed persists the libz dependency in hello world:

<LinkerArg Include="-Wl,--as-needed" Condition="'$(_IsApplePlatform)' != 'true'" />

	/lib/ld-musl-aarch64.so.1 (0xffffa794c000)
	libz.so.1 => /lib/libz.so.1 (0xffffa791b000)
	libc.musl-aarch64.so.1 => /lib/ld-musl-aarch64.so.1 (0xffffa794c000)

MSBuild should propagate properties to all context and there are more properties propagated from global contexts which affect publishing.

!Exists('$(IlcSdkPath)libz.a') was always failing due to IlcSdkPath being empty at global prop evaluation because it is assigned in SetupProperties targets the first time.

Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

I tested this and I can confirm the issue is fixed. Here's what I did:

  • OS: Ubuntu 24.04 x64
  • Uninstall system zlib: sudo apt remove zlib1g-dev
  • Install RC2 nightly: ./dotnet-install.sh -v 9.0.100-rc.2.24462.22
  • Confirm I'm using the correct version of dotnet:
    dotnet --list-sdks
    9.0.100-rc.2.24462.22 [/home/carlos/.dotnet/sdk]
    
  • Create console app: dotnet new console -o naot
  • Added <PublishAot>true</PublishAot> to the csproj
  • Ran the publish command using: dotnet publish naot.csproj

I got the expected error:

$ dotnet publish

Restore complete (0.9s)
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  naot failed with 2 error(s) (2.9s) → bin/Release/net9.0/linux-x64/naot.dll
    clang : error : linker command failed with exit code 1 (use -v to see invocation)
    /home/carlos/.nuget/packages/microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/build/Microsoft.NETCore.Native.targets(376,5): error MSB3073: The command ""clang" "obj/Release/net9.0/linux-x64/native/naot.o" -o "bin/Release/net9.0/linux-x64/native/naot" -Wl,--version-script=obj/Release/net9.0/linux-x64/native/naot.exports -Wl,--export-dynamic -gz=zlib -fuse-ld=bfd /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/sdk/libbootstrapper.o /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/sdk/libRuntime.WorkstationGC.a /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/sdk/libeventpipe-disabled.a /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/sdk/libRuntime.VxsortEnabled.a /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/sdk/libstandalonegc-disabled.a /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/sdk/libstdc++compat.a /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/framework/libSystem.Native.a /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/framework/libSystem.Globalization.Native.a /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/framework/libSystem.IO.Compression.Native.a /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/framework/libSystem.Net.Security.Native.a /home/carlos/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/framework/libSystem.Security.Cryptography.Native.OpenSsl.a -g -Wl,-rpath,'$ORIGIN' -Wl,--build-id=sha1 -Wl,--as-needed -pthread -ldl -lz -lrt -lm -pie -Wl,-pie -Wl,-z,relro -Wl,-z,now -Wl,--eh-frame-hdr -Wl,--discard-all -Wl,--gc-sections" exited with code 1.

Build failed with 2 error(s) in 4.0s
  • I edited this file: /home/carlos/.nuget/packages/microsoft.dotnet.ilcompiler/9.0.0-rc.2.24459.11/build/Microsoft.NETCore.Native.Unix.targets
  • Deleted the obj/ and bin/ folders.
  • Ran dotnet publish naot.csproj again.
  • Success!
    $ dotnet publish
    Restore complete (0.8s)
    You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
      naot succeeded (3.4s) → bin/Release/net9.0/linux-x64/publish/
    
    Build succeeded in 4.4s
    

Thanks, @hwoodiwiss !

@carlossanlop
Copy link
Member

/ba-g The runtime-dev-innerloop (Build linux-x64 debug Libraries_AllConfigurations) CI leg is stuck. It has this error in the log:

##[error]Agent failed with exception: The machine running request a5063779-f4ce-42b5-999d-94d58c5cdd68 restarted. Azure DevOps can't recover from restarts.
,##[warning]Received request to deprovision: The request was cancelled by the remote provider.

@carlossanlop carlossanlop merged commit 3948052 into dotnet:main Sep 12, 2024
84 of 88 checks passed
@carlossanlop
Copy link
Member

/backport to release/9.0

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr 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.

Native AOT Binaries still depend on libz.so
6 participants