Fix zlib-ng build failure caught in the sdk CI

[carlossanlop]

Fixed by converting its NativeLibrary entry in Microsoft.NETCore.Native.Unix.targets to a NetCoreAppNativeLibrary, and place it after System.IO.Compression.Native, to ensure the linker detects the symbols, since it is unable to look back.

Fixes this build failure: https://dev.azure.com/dnceng-public/public/_build/results?buildId=734455&view=logs&j=3a02e3c5-16cf-5fed-f87e-ba752007640e&t=0d50ed43-4be9-5564-4350-aed7d4fced7d

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

[am11]

In #104454 (comment), you mentioned that the failing test has UseSystemZlib = true. If that is the case, how is changing UseSystemZlib != true code fixing the issue?

We don't have anything resembling -L $(IlcSdkPath) in nativeaot publishing, so this change is going to select z from whatever is on LD_LIBRARY_PATH (i.e. system installation), which is not what we want here?

I think the SDK issue is that not all distros have same version of zlib-ng, so we will probably need a shim (like icushim, opensslshim) to require the set of APIs we need from the lib and polyfill the differences.

[am11]

We don't have anything resembling -L $(IlcSdkPath) in nativeaot publishing, so this change is going to select z from whatever is on LD_LIBRARY_PATH (i.e. system installation), which is not what we want here?

Which is as good as not having UseSystemZlib in nativeaot at all. Also, this order dependent comment is not correct; these args are not order dependent, this is how library lookup path works on unix.

I suggest we remove UseSystemZlib from nativeaot for now, until there is a better solution. It seems like an unnecessary risk to take this late in the release cycle.

[carlossanlop]

In #104454 (comment), you mentioned that the failing test has UseSystemZlib = true. If that is the case, how is changing UseSystemZlib != true code fixing the issue?

The condition check is not changed. It is still checking for != true.

We don't have anything resembling -L $(IlcSdkPath) in nativeaot publishing, so this change is going to select z from whatever is on LD_LIBRARY_PATH (i.e. system installation), which is not what we want here?

I just pushed a commit to fix the folder where the UseSystemZlib condition is checking on Unix. It should now be IlcFrameworkNativePath. This matches the expected folder we use in EscapePath in line 152.

I think the SDK issue is that not all distros have same version of zlib-ng, so we will probably need a shim (like icushim, opensslshim) to require the set of APIs we need from the lib and polyfill the differences.

All Unix desktop distros should use the newly added copy of zlib-ng, which we build ourselves It's under src/native/external/zlib-ng. The exceptions are mobile platforms (android, ios, tvos, maccatalyst): they should find and use the system-installed zlib, whatever it is. There should not be an issue with that since all zlib versions expose the same public methods.

[am11]

The condition check is not changed. It is still checking for != true.

This is not what I meant? You are not addressing the issue SDK is having, but un-fixing something which is kind of working. Current state of this PR is actually never picking up zlib-ng which we compile in-tree..

[carlossanlop]

We don't have anything resembling -L $(IlcSdkPath) in nativeaot publishing, so this change is going to select z from whatever is on LD_LIBRARY_PATH (i.e. system installation), which is not what we want here?

Which is as good as not having UseSystemZlib in nativeaot at all. Also, this order dependent comment is not correct; these args are not order dependent, this is how library lookup path works on unix.

I fixed it with 5daaa28, but didn't notice your comment until after I pushed it. We're not supposed to check there anymore with this fix.

I suggest we remove UseSystemZlib from nativeaot for now, until there is a better solution. It seems like an unnecessary risk to take this late in the release cycle.

It's within our plans to introduce zlib-ng in Preview7. It works in nativeaot. Bugs were expected, we're fixing them.

[carlossanlop]

This is not what I meant? You are not addressing the issue SDK is having, but un-fixing something which is kind of working. Current state of this PR is actually never picking up zlib-ng which we compile in-tree..

Sorry, I'm not following. I'm not changing the condition in which we decide to use system zlib or not. We just needed to check in the correct folder.

[am11]

z is picked up from LD_LIBRARY_PATH or path pointed by -L arg (which we don't set).

The current state of this PR is using system libz in both UseSystemZlib true and false cases.

You probably need:

-       <NativeLibrary Condition="'$(UseSystemZlib)' != 'true'" Include="$(IlcSdkPath)libz.a" />
+      <NativeLibrary Condition="'$(UseSystemZlib)' != 'true'" Include="$(IlcFrameworkNativePath)libz.a" />

[carlossanlop]

No, we don't want to use NativeLibrary. As I mentioned before, the order matters. We already investigated this and determined that was the root cause of the problem. The zlib library, whatever it is, needs to be added after System.IO.Compression.Native, and it needs to be a NetCoreAppNartiveLibrary, not a NativeLibrary.

It needs to be z because that's the convention to include libz, whether it's from the in-tree zlib-ng or from the system zlib. Line 151 will tell you why it needs to be z.

Line 193 does the insertion of actual system zlib, with the condition reversed.

[am11]

It needs to be z because that's the convention to include libz, whether it's from the in-tree zlib-ng or from the system zlib.

clang -lz, clang -l:libz.a and clang -L . -lz are three different things. Expecting the first one to magically behave like second or third is the mistake I was trying to point out.

[carlossanlop]

clang -lz, clang -l:libz.a and clang -L . -lz are three different things. Expecting the first one to magically behave like second or third is the mistake I was trying to point out.

I see what you're trying to say, and I can understand adding a z library could be misinterpreted as "trying to consume the system zlib". But I hope I explained myself when I said that line 151 takes care of forming the full path of the lib*.a to append.