Skip to content

Conversation

@carlossanlop
Copy link
Contributor

@carlossanlop carlossanlop self-assigned this Jan 29, 2025
@carlossanlop carlossanlop requested review from a team and jkotas January 29, 2025 18:57
@carlossanlop carlossanlop marked this pull request as ready for review January 29, 2025 19:08
@carlossanlop
Copy link
Contributor Author

Nothing broke but I think I should go back to draft and first test this change in a test PR in the runtime repo, just in case any CI leg fails.

@jkoritzinsky
Copy link
Member

I think to truly validate this, you need to build the docker images with the updates build-rootfs script and make sure that we can still build all branches we're still servicing with the new images.

@am11
Copy link
Member

am11 commented Jan 29, 2025

Nothing broke but I think I should go back to draft and first test this change in a test PR in the runtime repo, just in case any CI leg fails.

This script is not used in CI by arcade and runtime repos. It's used by https://github.com/dotnet/dotnet-buildtools-prereqs-docker and it's used for older versions of prereq containers from the main branch as well (servicing branches). Testing is done in multiple steps (build prereq image, then use that container image to build runtime), usually in local machine.

@carlossanlop
Copy link
Contributor Author

carlossanlop commented Jan 29, 2025

This script is not used in CI by arcade and runtime repos.

Alright. But can I still assume that the change needs to be made here in arcade? @am11

I don't know how the code flows from between arcade and the prereqs repo.

@am11
Copy link
Member

am11 commented Jan 29, 2025

@carlossanlop, I think there are two opposing thought processes:

  1. we want to keep non-portable build support in rootfs: for that reason we are still carrying libunwind dependency in rootfs script although it was removed in .NET Core 2.1 in favor of local copy dotnet/coreclr@d104270. By this logic, this PR should "replace" zlib with zlib-ng.
  2. we don't want non-portable build support in rootfs: by this logic, we should also remove libunwind.

@jkotas
Copy link
Member

jkotas commented Jan 29, 2025

@carlossanlop There is a conversation about this on Teams (Build Tools Prereqs V-Team)

@carlossanlop
Copy link
Contributor Author

Closing as the changes are more complex than just this.

@carlossanlop carlossanlop deleted the RemoveZlib branch January 29, 2025 22:16
@jkotas
Copy link
Member

jkotas commented Jan 30, 2025

@carlossanlop Can we keep the cleanup in install-dependencies.sh? I do not think that install-dependencies.sh suffers from the problematic use in the prereqs repo.

@carlossanlop
Copy link
Contributor Author

Sure.

@carlossanlop carlossanlop reopened this Jan 30, 2025
@carlossanlop carlossanlop changed the title Remove all zlib dependencies from eng/common scripts Remove zlib dependencies from eng/common/native/install-dependencies.sh Jan 30, 2025
@carlossanlop carlossanlop marked this pull request as ready for review January 30, 2025 17:22
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.

Thanks!

@carlossanlop carlossanlop enabled auto-merge (squash) January 30, 2025 17:39
@carlossanlop carlossanlop merged commit c14bfef into dotnet:main Jan 30, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove system zlib packages from unix native dependencies

4 participants