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

Split system packages into two sections: required to build python vs required to build users' python packages #7877

Conversation

jeffwidman
Copy link
Member

@jeffwidman jeffwidman commented Aug 23, 2023

  1. Reorder the list of package lexicographically
  2. Assume the only packages needed for pyenv to install python are
    the ones listed in https://github.com/pyenv/pyenv/wiki#suggested-build-environment
  3. Move the remaining packages to a separate section of packages that
    are installed in the final image, not the builder image (they're not
    needed for building the various python's)
  4. Go through the list of packages that are potentially used for user
    packages, and search the repo issues/PR's/commit log for any
    mentions. Also google to see if any common python libraries require
    them. If so, document why.
  5. Delete the remaining packages, they're probably not needed.

This should slightly shrink our build times/image size, but there is a
risk that one or more of the removed libraries are actually needed. If
so, we'll probably see it in Sentry error logs once we deploy and jobs
start running against user repos, and then we can re-add the ones that
are still needed, except this time we'll actually be able to document
why they're needed.

I was a little surprised how many packages I was able to delete... 10 packages are gone!
Some of them may be part of the existing base image, but many of them looked
like things that may have been needed for other parts of the monolithic
docker image, but are no longer needed for just the Python bits.

Stacked on top of:

@jeffwidman jeffwidman force-pushed the split-debian-packages-into-required-for-build-vs-users-packages branch 2 times, most recently from 99c04c0 to 28ea367 Compare August 23, 2023 04:22
@jeffwidman jeffwidman changed the base branch from main to DRY-up-apt-installs-in-python-dockerfile August 23, 2023 04:23
@jeffwidman jeffwidman force-pushed the split-debian-packages-into-required-for-build-vs-users-packages branch from 28ea367 to 9775313 Compare August 23, 2023 04:23
@jeffwidman jeffwidman marked this pull request as ready for review August 23, 2023 04:24
@jeffwidman jeffwidman requested a review from a team as a August 23, 2023 04:24
@deivid-rodriguez
Copy link
Contributor

Nice work!

Could the test failure be related? I've never seen it before.

@jeffwidman jeffwidman force-pushed the DRY-up-apt-installs-in-python-dockerfile branch from 55d01b5 to 3413284 Compare August 23, 2023 06:32
@jeffwidman
Copy link
Member Author

jeffwidman commented Aug 23, 2023

Hmm, that's tough to know, normally I'd assume yes, but the error message looks more like a flake network connection... lemme try running it again.

For posterity, here's the failing run: https://github.com/dependabot/dependabot-core/actions/runs/5946994350/job/16128413270?pr=7877

Base automatically changed from DRY-up-apt-installs-in-python-dockerfile to main August 23, 2023 08:35
@jeffwidman jeffwidman force-pushed the split-debian-packages-into-required-for-build-vs-users-packages branch from 9775313 to 363999f Compare August 23, 2023 15:41
@jeffwidman
Copy link
Member Author

Looks like the CI failure was indeed a network flake, it worked fine the second time around.

@jeffwidman jeffwidman force-pushed the split-debian-packages-into-required-for-build-vs-users-packages branch from 363999f to 736bc21 Compare August 23, 2023 15:49
1. Reorder the list of package lexicographically
2. Assume the only packages needed for `pyenv` to install `python` are
   the ones listed in https://github.com/pyenv/pyenv/wiki#suggested-build-environment
3. Move the remaining packages to a separate section of packages that
   are installed in the final image, not the builder image (they're not
   needed for building the various python's)
4. Go through the list of packages that are _potentially_ used for user
   packages, and search the repo issues/PR's/commit log for any
   mentions. Also google to see if any common python libraries require
   them. If so, document why.
5. Delete the remaining packages, they're probably not needed.

This should slightly shrink our build times/image size, but there is a
risk that one or more of the removed libraries are actually needed. If
so, we'll probably see it in Sentry error logs once we deploy and jobs
start running against user repos, and then we can re-add the ones that
are still needed, except this time we'll actually be able to document
_why_ they're needed.

I was a little surprised how many packages I was able to delete... some
of them may be part of the existing base image, but many of them looked
like things that may have been needed for other parts of the monolithic
docker image, but are no longer needed for just the Python bits.
@jeffwidman jeffwidman force-pushed the split-debian-packages-into-required-for-build-vs-users-packages branch from 736bc21 to 1695861 Compare August 23, 2023 16:27
@jeffwidman jeffwidman merged commit 9cb48a8 into main Aug 23, 2023
@jeffwidman jeffwidman deleted the split-debian-packages-into-required-for-build-vs-users-packages branch August 23, 2023 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants