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

Imagemagick enable delegate build and disabled using shared libraries #889

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

featheredtoast
Copy link
Member

compiles to a single portable binary

Create a base builder package with compile time dependencies. Create an imagemagick_builder to build imagemagick.

The base builder can be extended for other builders.

Add imagemagick runtime dependencies to discourse_dependencies image avoid -dev libs

Statically compile as much as possible with --disable-shared and --enable-delegate-build flags.
References:
https://stackoverflow.com/questions/47031789/imagemagick-100-static-build-for-linux https://www.imagemagick.org/discourse-server/viewtopic.php?t=14259

Add fonts-urw-base35 for NimbusSans-Regular, needed for letter avatar generation

Copy over the resulting magick bin, as well as etc and share files from the compilation. etc is needed for magick to run, share is not, but contains translations for errors which Discourse tests are dependent on reading from.

Create symlinks for other magick tooling - imagemagick creates symlink tool names that Discourse uses. These could be dropped if Discourse decided to use magick {toolname} rather than {toolname}.

Add nginx compile dependency - building nginx still needs libfreetype6 This was implicitly installed previously. Removing the imagemagick build from base broke the next nginx build. Add this dependency back in. This dependency can be removed once we build nginx separately as well.

compiles to a single portable binary

Create a base builder package with compile time dependencies. Create an
imagemagick_builder to build imagemagick.

The base builder can be extended for other builders.

Add imagemagick runtime dependencies to discourse_dependencies image
avoid -dev libs

Statically compile as much as possible with --disable-shared and
--enable-delegate-build flags.
References:
https://stackoverflow.com/questions/47031789/imagemagick-100-static-build-for-linux
https://www.imagemagick.org/discourse-server/viewtopic.php?t=14259

Add fonts-urw-base35 for NimbusSans-Regular, needed for letter avatar generation

Copy over the resulting magick bin, as well as etc and share files from the
compilation. etc is needed for magick to run, share is not, but contains
translations for errors which Discourse tests are dependent on reading from.

Create symlinks for other magick tooling - imagemagick creates symlink tool
names that Discourse uses. These *could* be dropped if Discourse decided to use
`magick {toolname}` rather than `{toolname}`.

Add nginx compile dependency - building nginx still needs libfreetype6
This was implicitly installed previously. Removing the imagemagick build from
base broke the next nginx build. Add this dependency back in. This dependency
can be removed once we build nginx separately as well.
@@ -2,6 +2,21 @@
# VERSION: release

ARG DEBIAN_RELEASE=bookworm
FROM discourse/ruby:3.3.6-${DEBIAN_RELEASE}-slim AS builder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discourse/ruby:3.3.6-${DEBIAN_RELEASE}-slim

I don't think we need to depend on Ruby here and am wondering if we can just rely on the debian images?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could, it doesn't really matter too much - I figured we're throwing it away after moving the compiled binaries over and building from the ruby base image here is convenient as we already need that image to build all the discourse base image to begin with.

Comment on lines +55 to +58
# imagemagick runtime dependencies
libheif1 libjbig0 libtiff6 libpng16-16 libfontconfig1 \
libwebpdemux2 libwebpmux3 libxext6 librsvg2-2 libgomp1 \
fonts-urw-base35 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are duplicating the required runtime dependencies between the Dockerfile and the install-imagemagick script. Feels like it will be quite brittle.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also wondering... is there a way to statically compile imagick so we don't have to think about this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

install-imagemagick includes static libs and -dev libs as well which are only needed to compile - this is only a subset or a list of non-dev .so libs that magick needs at runtime.

I looked into an LD -static flag, but that felt like a whole other rabbit hole. The sources I've seen so far point to the compilation being able to not compile more dynamic libs so the actual magick binary is built statically, but sources I've found are if you need to enable extensions as we do, we still are depending on these libs.

Comment on lines +59 to +60
# nginx compile dependencies \
libfreetype6-dev && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this change out of the PR since it is for nginx and not imagemagick.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both imagemagick and nginx depends on this lib to be installed to compile. It's needed to allow nginx to continue to be compiled in the base lib. Right now, the dependency just so happens to be installed via imagemagick.

With the removal of imagemagick dependencies, this lib too is not installed, causing nginx compilation to fail. Adding this back again allows compilation to succeed again.

I'm planning on moving nginx to build in its own builder next, which will remove the requirement of this library in the base image, but since I'm not dealing with nginx in this PR, this is needed.

@tgxworld
Copy link
Contributor

@featheredtoast Sorry just realised that I missed your replies and this has been sitting around for a month.

Comment on lines +99 to +110
RUN ln -s /usr/local/bin/magick /usr/local/bin/animate &&\
ln -s /usr/local/bin/magick /usr/local/bin/compare &&\
ln -s /usr/local/bin/magick /usr/local/bin/composite &&\
ln -s /usr/local/bin/magick /usr/local/bin/conjure &&\
ln -s /usr/local/bin/magick /usr/local/bin/convert &&\
ln -s /usr/local/bin/magick /usr/local/bin/display &&\
ln -s /usr/local/bin/magick /usr/local/bin/identify &&\
ln -s /usr/local/bin/magick /usr/local/bin/import &&\
ln -s /usr/local/bin/magick /usr/local/bin/magick-script &&\
ln -s /usr/local/bin/magick /usr/local/bin/mogrify &&\
ln -s /usr/local/bin/magick /usr/local/bin/montage &&\
ln -s /usr/local/bin/magick /usr/local/bin/stream &&\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@featheredtoast Is there a list for us to check for all the various ways people can call the magick bin? I'm trying to determine if there is a better way for us to maintain this list long term because it is quite extensive.

Copy link
Contributor

@tgxworld tgxworld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a closer look at all the changes and this looks good to me. More importantly, this change eliminates a 400mb layer in the image.

@tgxworld tgxworld merged commit 5ed07ec into main Dec 17, 2024
5 checks passed
@tgxworld tgxworld deleted the imagemagick-compile-builder branch December 17, 2024 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants