search

dotnet / dotnet-docker

Docker images for .NET and the .NET Tools.
https://hub.docker.com/_/microsoft-dotnet
MIT License
4.4k stars 1.92k forks source link

`zlib` statically linked for .NET 9; We need .NET 9-specific `runtime-deps` #5687

Closed richlander closed 1 week ago

richlander commented 1 month ago

That means we should create new runtime-deps images for .NET 9+, for runtime and AOT scenarios. .NET 9 reuses .NET 8 runtime-deps, currently.

https://github.com/dotnet/dotnet-docker/tree/main/src/runtime-deps/8.0

Sounds like we'll need to wait for P7 or RC1 to make this change

Related: https://github.com/dotnet/dotnet-buildtools-prereqs-docker/pull/1125

jkotas commented 1 month ago

cc @carlossanlop

lbussell commented 1 month ago

[Triage] @richlander or @carlossanlop, when would be the best time to make this change for container images? We can either make the change now for Preview 7 or wait until RC1.

carlossanlop commented 1 month ago

The P7 snaps will happen on July 22nd. Maybe we could wait to do such change on that day?

@richlander I see you are mentioning my PR here: https://github.com/dotnet/dotnet-buildtools-prereqs-docker/pull/1125

If I'm understanding your request correctly, I need to create a PR with similar changes in the dotnet-docker repo for each one of the images that won't be using system zlib anymore. Is this correct? Remove the zlib dependency?

Note that AFAIK, having the system zlib installed in the docker images of platforms where we will use the in-tree zlib-ng should not cause problems or conflicts. So I guess we want to make this change to reduce the size of the images, right?

MichaelSimons commented 1 month ago

If I'm understanding your request correctly, I need to create a PR with similar changes in the dotnet-docker repo for each one of the images that won't be using system zlib anymore. Is this correct? Remove the zlib dependency?

@lbussell can handle the changes needed in dotnet-docker. The dockerfiles are generated from a template so it won't be very difficult. The main question is when this change can/should be made.

Note that AFAIK, having the system zlib installed in the docker images of platforms where we will use the in-tree zlib-ng should not cause problems or conflicts. So I guess we want to make this change to reduce the size of the images, right?

Yes size is important for our docker user community. It directly affects container startup as well as reduces the attack surface area and can make them more secure.

carlossanlop commented 1 month ago

Ah ok got it. I'd be happy to help but if it's all done with a template, even better. I'll let @lbussell do it.

Let's do it when we snap, July 22nd, how does that sound? By then, we should be 100% sure the zlib change did not cause any problems anywhere so we didn't have to revert it.

lbussell commented 1 month ago

Let's do it when we snap, July 22nd, how does that sound?

Sounds good to me.

Is the initial implementation currently in the latest builds of the SDK? If so, I can prep the changes ahead of time to run them through PR validation before that date.

carlossanlop commented 1 month ago

Note that we are still using the system zlib in the following platforms: mobile (android, ios, tvos, maccatalyst), armv6.

https://github.com/dotnet/runtime/blob/e48c2b26e39de37c046981761224907c96783451/eng/native/configureplatform.cmake#L508

Edit: I updated to show the correct cmake lines that exclude the platforms. I mistakenly still added wasm/wasi to the list but no, they are consuming the in-tree zlib-ng too.

carlossanlop commented 1 month ago

Is the initial implementation currently in the latest builds of the SDK?

Yes, it should be. The deps flow from runtime to sdk that contained this zlib-ng change was merged today: https://github.com/dotnet/sdk/pull/42019

lbussell commented 1 week ago

This is now rolled out - breaking change announcements: https://github.com/dotnet/dotnet-docker/discussions/5792, https://github.com/dotnet/docs/issues/42136