Revert "Remove runtime dependencies from slim and alpine variants"#497
Merged
yosifkit merged 2 commits intodocker-library:masterfrom Jan 14, 2025
Merged
Revert "Remove runtime dependencies from slim and alpine variants"#497yosifkit merged 2 commits intodocker-library:masterfrom
yosifkit merged 2 commits intodocker-library:masterfrom
Conversation
This reverts commit 7f078b1. It looks like this change is too disruptive. For example, modern rails apps all depend in `psych` which requires libyaml. I still think it makes sense, but should probably only be part of the next Ruby version so it doesn't suddently break for lots of people.
|
The |
|
Yeah, it looks like this has broken about a dozen projects for me 😢 |
|
When it's going to be merged? |
|
Sorry to be reiterative, but do we have a timeline for merging this? Some of our pipelines are currently broken, and while we can edit our Dockerfiles as a temporary fix, we’d prefer to avoid that if the change will soon be reversed. At the moment, this issue is blocking deployments of new features. Thanks! |
|
agreed this broke |
|
I'm sure most people coming here know this, but if you need to fix your builds while waiting for this to be merged, you can pin to the previous release sha, e.g. Alternatively, install the missing system packages. |
Member
|
The breakage is unfortunate but correct. The Please use this temporary breakage as a chance to prepare by adding any of the removed packages that you require to your own build-time or runtime dependencies (like Redmine). Edit: I meant next the patch release of each |
This adjusts the backwards-compatibility shim to be easier to remove in the future (and automatically removes it at each next minor release).
Member
|
I've added commit 1ea0c59
Diff: |
|
FYI - rails has already added now necessary packages to the default Dockerfile - rails/rails#54237 |
Member
|
(I'm working on another diff to compare the pure revert version / old version's packages to my change to validate that it correctly does bring everything we expect back in, which should be a smaller diff than what I posted above and easier to review for correctness.) |
Contributor
Author
|
@tianon what do you think about keeping it indefinitely for 3.1/3.2/3.3/3.4? I understand the inclusion of the dev packages is unintentional and I totally agree about removing them eventually. But considering they have been a part of the image for almost a decade and no one really noticed I don't think this needs to be pushed even in a patch version. |
rubys
added a commit
to fly-apps/dockerfile-rails
that referenced
this pull request
Jan 14, 2025
Member
|
For Alpine, these unintentional dependencies account for a ~25MiB image size increase (in a ~100MiB image, that's pretty sizeable). For Debian-based Slim variants, it's ~50MiB (on a ~220MiB image). Size isn't the end-all-be-all, of course, but both of these variants have minimal image size as an explicit goal, so it seems useful and even important that we correct our mistake. Doing it on a minor version bump means that (as noted above), it's easier for users to pin to a working version while they figure out which of these dependencies they need to fix their builds and don't have to dig up a content-digest, so it seems like a pretty fair compromise.
As promised: Diff: |
Contributor
Author
I guess most(?) do specify the patch version so if you have even the smallest amount of CI it should catch it. I just rank the size savings so very low since it went unnoticed for sooo long. I can't really tell if it would repeat with the next patch versions, so I would be more conservative, but it may be fine. |
|
I guess most should have already noticed the issue and added the build dependencies in the respective build stages (where they honestly belong). It's just unfortunate that it surfaced like this, so maybe take it as a note for the future to at least add a warning first, before adding breaking changes and/or add the breaking changes in the next patch or (conservatively) minor version(s). |
Member
|
We don't really have a better way to notify users of things like this -- a warning would've been lost in the logs, totally unnoticed. 😞 |
Member
|
(There's also not an obvious place to add a warning -- we'd have to add something nutty like a wrapper script around ... |
|
Yeah that's why it's probably better to just keep the patch versions backwards compatible. You're right, a warning would have most likely been lost in the logs. There's also no built in mechanism for notifying docker users of such changes. |
Member
|
I'll go ahead and merge this to at least get the revert in. LGTM. If you'd still like to discuss or change when it will come back, let's open a new PR/issue. |
docker-library-bot
added a commit
to docker-library-bot/official-images
that referenced
this pull request
Jan 14, 2025
Changes: - docker-library/ruby@2ba928a: Merge pull request docker-library/ruby#497 from Earlopain/revert-slim-trim - docker-library/ruby@1ea0c59: Remove packages again at next minor release of each series - docker-library/ruby@6f84caa: Revert "Remove runtime dependencies from slim and alpine variants"
|
@tianon Hi! It is true that even if it were included in the logs, it would probably result in the message being lost and unnoticed. Anyway, developers usually test newer images before bumping versions, so they would notice if something is failing. What we should avoid is introducing changes to already released and tagged versions when those are already in use. |
Contributor
|
When will new images be deployed? The latest ruby:3.3-slim was built before this was reverted. |
Member
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
12 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This reverts commit 7f078b1. (#493)
It looks like this change is too disruptive. For example, modern rails apps all transitively depend in
psychwhich requires libyaml. Coincidentally my own builds would fail with this.I still think this change makes sense, but should probably only be part of the next Ruby version so it doesn't suddently break for lots of people.
Closes #495, closes #496