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

Ensure Dockerfile installs commonly needed packages #46953

Merged
merged 1 commit into from Jan 10, 2023

Conversation

rubys
Copy link
Contributor

@rubys rubys commented Jan 10, 2023

Motivation / Background

While libvips is an example of a dependency needed to build a native gem, it is not an exhaustive list.

This pull request always includes build-essential as installing native gems is common need, not just for applications that include active storage.

Detail

  • Starts from the ruby:slim base instead of full ruby
  • Always install what is needed to build native packages
  • Include libraries for sqlite3, postgresql, mysql, and redis

Additional information

While this started out simple, it became a minor yak shaving event.

Node also has native modules, and in order to install them you need node-gyp, pkg-config, and python. And the way that python is packaged changed from debian buster to debian bullseye. So heads up: wart warning here, but one that only affects node users, and one that is unlikely to be visible externally as it merely substitutes the name of one python package for another based on the ruby version. See code/comments in app_base.rb for details.

Taking a step back, if we are installing packages, ruby:slim is a better starting point than ruby. It installs a few less packages by default, but also removed docs, man pages, etc. See https://stackoverflow.com/questions/59794891/how-does-debian-differ-from-debian-slim

This pull request explicitly installs libraries for sqlite3, postgresql, mysql, and redis as they are common requirements and don't take up much space.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.

A changelog entry is not needed for this change.

* Starts from the ruby:slim base instead of full ruby
* Always install what is neede to build native packages
* Include libraries for sqlite3, postgresql, and mysql
@rails-bot rails-bot bot added the railties label Jan 10, 2023
@rubys
Copy link
Contributor Author

rubys commented Jan 10, 2023

attn: @dhh

FYI: a future pull request will explore reducing deployment image size by using multi-stage builds to remove things like build-essentials which are not needed at runtime. For a preview, see https://fly.io/docs/rails/cookbooks/minimal/

Also it seems that this commit picked up a previously uncommitted change - merging two environment variables into one statement prevents the second setting from referencing the value of the first. I caught this in testing, but failed to included it in the prior commit. If this pull request is not merged, I'll submit a separate PR.

buildkite activerecord failures appear to be unrelated to the changes in this PR.

@dhh dhh merged commit ff224e6 into rails:main Jan 10, 2023
@dhh
Copy link
Member

dhh commented Jan 10, 2023

Lovely work, Sam. Very clear what packages are added for what purpose. Looking forward to see how slim we can go with a separate build step 👍

byroot added a commit that referenced this pull request Feb 27, 2023
Ref: #46953

None of the Ruby redis client need that package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants