-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Conversation
There was a problem hiding this 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?
That was quick! Truly appreciated. Thank you! I'll take a look tomorrow as soon as I get a chance. |
There was a problem hiding this 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?
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 |
Yup, some fixes went in ratehr recently, so it would be good to confirm it with latest .NET9 build. |
My test builds have been in github actions, so existing intermediate files shouldn't be an issue. |
With version 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 |
@martincostello could you share the output of |
Not easily, no. It was built from this commit if you want to build the image locally and inspect it: martincostello/website@41e4bb4 |
Testing NotesThis comment will consolidate the various testing I had spread across a few comments: RC2Tested publish from RC2, arm64 and amd64, both seemed to have Checked arm64 using Checked amd64 using
This changeI'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
|
Thanks for validating the change and taking additional steps. I'd love to understand why it is not reproducible with
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:
|
I had a hunch on this too, they're not using any compression, so 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 - |
@hwoodiwiss You do not have to be doing this. The repo maintainers will do it as necessary once the PR is ready to merge. |
Last time I was looking at it, webapi/webapiaot didn't use libz. Console doesn't use it for sure. |
Ah, didn't knew that linker optimization is this thorough that it will drop the the entire dependency. :) Changing it to runtime/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets Line 219 in 4930e1b
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this 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/
andbin/
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 !
/ba-g The
|
/backport to release/9.0 |
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10839382520 |
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