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

Migrate to zlib-ng, part 3: Remove zlib and zlib-intel source code and license mentions #104399

Merged
merged 11 commits into from
Jul 22, 2024

Conversation

carlossanlop
Copy link
Member

Contributes to: #101465

Split in commits for easier review.

Copy link
Contributor

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

@carlossanlop carlossanlop mentioned this pull request Jul 3, 2024
18 tasks
@jkotas
Copy link
Member

jkotas commented Jul 3, 2024

You should be also able to delete zlib-dev package from the Linux build prereqs at https://github.com/dotnet/runtime/blob/main/docs/workflow/requirements/linux-requirements.md#toolchain-setup

@jkotas
Copy link
Member

jkotas commented Jul 3, 2024

@jkotas
Copy link
Member

jkotas commented Jul 3, 2024

@carlossanlop
Copy link
Member Author

You should be also able to delete zlib-dev package from the Linux build prereqs at https://github.com/dotnet/runtime/blob/main/docs/workflow/requirements/linux-requirements.md#toolchain-setup

Questions:

  • Should we change this doc to also explain that system zlib is used in armv6, android, ios, tvos and maccatalyst?
  • Is system zlib required for a cross build?

@jkotas
Copy link
Member

jkotas commented Jul 3, 2024

Should we change this doc to also explain that system zlib is used in armv6, android, ios, tvos and maccatalyst?

From the doc: "These instructions are written assuming the current Ubuntu LTS.". armv6, android, ios, tvos and maccatalyst concerns are not relevant for this doc.

Is system zlib required for a cross build?

Yes - if the target platform is configured to system zlib. I cannot think about any docs where it would need to be mentioned.

@carlossanlop
Copy link
Member Author

carlossanlop commented Jul 3, 2024

I cannot think about any docs where it would need to be mentioned.

I found one for android coreclr cross building, but couldn't find one for the apple mobile platforms: https://github.com/dotnet/runtime/blob/898ffc5f74fad1593ed4036a69da039937224f99/docs/workflow/building/coreclr/android.md

I also found a general one for coreclr cross building: https://github.com/dotnet/runtime/blob/898ffc5f74fad1593ed4036a69da039937224f99/docs/workflow/building/coreclr/cross-building.md

The mono build document is very generic but has links to android and ios testing (not cross building): https://github.com/dotnet/runtime/tree/898ffc5f74fad1593ed4036a69da039937224f99/docs/workflow/building/mono

Do any of those look good for mentioning the system zlib requirement?

@carlossanlop
Copy link
Member Author

@jkotas Oh, the first doc you shared has a cross build section a few lines below, we could move zlib1g-dev there: https://github.com/dotnet/runtime/blob/main/docs/workflow/requirements/linux-requirements.md#additional-requirements-for-cross-building

@carlossanlop
Copy link
Member Author

And from under https://github.com/dotnet/dotnet-buildtools-prereqs-docker/tree/main/src/azurelinux/3.0/net9.0

OK I see that azurelinux is the only distro we split depending on the .NET 9.0 version. I mention this because I see zlib-devel and zlib1g-dev being installed in most of the other Dockerfiles but they should remain untouched as these images are shared by all the servicing branches.

@carlossanlop carlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 4, 2024
@carlossanlop
Copy link
Member Author

carlossanlop commented Jul 4, 2024

Can't merge this yet because part 2 was just reverted: #104414

I'm working on a fix for wasm and will come back to this PR after the fix is merged.

@carlossanlop carlossanlop removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 19, 2024
@carlossanlop
Copy link
Member Author

@jkotas I removed the no merge label as it seems we're in a good state.
If there are no objections, I'll merge this today (the CI is restarting because of a merge conflict with an *.md file).

@carlossanlop
Copy link
Member Author

This is removing dead code + adding some markdown changes. The previous CI runs passed (the dead code removal does not affect the build), and the latest comment simply fixed a couple of markdown files. I'll fast merge this.

@carlossanlop
Copy link
Member Author

carlossanlop commented Jul 22, 2024

@carlossanlop carlossanlop merged commit 4d2d3d3 into dotnet:main Jul 22, 2024
65 of 126 checks passed
@carlossanlop carlossanlop deleted the removeOldZlibs branch July 22, 2024 19:23
@jakobbotsch
Copy link
Member

@carlossanlop After this PR we are getting build failures on a clean ubuntu-22 image:

  /usr/bin/ld.bfd: cannot find -lz: No such file or directory
clang : error : linker command failed with exit code 1 (use -v to see invocation) 
...libSystem.Security.Cryptography.Native.OpenSsl.a -g -Wl,-rpath,'$ORIGIN' -Wl,--build-id=sha1 -Wl,--as-needed -Wl,-e0x0 -pthread -ldl -lz -lrt -lm -shared -Wl,-z,relro -Wl,-z,now -Wl,--eh-frame-hdr -Wl,--discard-all -Wl,--gc-sections" exited with code 1.

This is when running eng/install-native-dependencies.sh && ./build.sh ... (full script is https://gist.github.com/EgorBo/0e961d7fdf9a763fe4f78cf502115f29).
It succeeds if zlib1g-dev is manually installed.

@carlossanlop
Copy link
Member Author

Okay, I'm reverting it to avoid blocking today's snap.

@carlossanlop
Copy link
Member Author

@jakobbotsch reverted, sorry about the noise.

@ericstj
Copy link
Member

ericstj commented Jul 22, 2024

Is this another case where folks are using scripts that aren't represented in our actual build process? Or is it a public build vs CI validation thing?

@EgorBo
Copy link
Member

EgorBo commented Jul 22, 2024

Is this another case where folks are using scripts that aren't represented in our actual build process? Or is it a public build vs CI validation thing?

It's probably not represented, yes, but it follows the official build doc. Basically, a clean new Azure VM with Ubuntu 22.04 (either x64 or arm64):

eng/./install-native-dependencies.sh
./build.sh Clr+Libs -c Release

fails with the error posted above ^, after I added a manual apt install -y zlib1g-dev it has fixed the issue

@jkotas
Copy link
Member

jkotas commented Jul 23, 2024

We would run into the same build break in the CI and official builds once dotnet/dotnet-buildtools-prereqs-docker#1125 is merged and propagates through the system.

Is this another case where folks are using scripts that aren't represented in our actual build process?

We have two documented Linux build instructions: https://github.com/dotnet/runtime/blob/main/docs/workflow/building/coreclr/linux-instructions.md:

  • Containers that are used and validated in CI and official builds.
  • "Local" build environment that just requires installing several packages. It is more productive setup for development. It is not validated by dotnet/runtime CI. It is sort of validated by source build.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants