Skip to content

Imagemagick enable delegate build and disabled using shared libraries #889

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Imagemagick enable delegate build and disabled using shared libraries #889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions image/base/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

RUN apt update && \
DEBIAN_FRONTEND=noninteractive apt-get -y install wget \
autoconf build-essential \
git \
cmake \
gnupg \
libpcre3-dev \
libfreetype6-dev \
libbrotli-dev

FROM builder AS imagemagick_builder
ADD install-imagemagick /tmp/install-imagemagick
RUN /tmp/install-imagemagick

FROM discourse/ruby:3.3.6-${DEBIAN_RELEASE}-slim AS discourse_dependencies

ARG DEBIAN_RELEASE
Expand Down Expand Up @@ -36,7 +51,13 @@ RUN --mount=type=tmpfs,target=/var/log \
libreadline-dev anacron wget \
psmisc whois brotli libunwind-dev \
libtcmalloc-minimal4 cmake \
pngcrush pngquant ripgrep poppler-utils; \
pngcrush pngquant ripgrep poppler-utils \
# imagemagick runtime dependencies
libheif1 libjbig0 libtiff6 libpng16-16 libfontconfig1 \
libwebpdemux2 libwebpmux3 libxext6 librsvg2-2 libgomp1 \
fonts-urw-base35 \
Comment on lines +55 to +58
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.

# nginx compile dependencies \
libfreetype6-dev && \
Comment on lines +59 to +60
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.

# install these without recommends to avoid pulling in e.g.
# X11 libraries, mailutils
DEBIAN_FRONTEND=noninteractive apt-get -y install --no-install-recommends git rsyslog logrotate cron ssh-client less; \
Expand Down Expand Up @@ -70,8 +91,24 @@ RUN sed -i "s/^# $LANG/$LANG/" /etc/locale.gen; \
RUN --mount=type=tmpfs,target=/root/.npm \
npm install -g terser uglify-js pnpm

ADD install-imagemagick /tmp/install-imagemagick
RUN /tmp/install-imagemagick
# Copy binary and configuration files for magick
COPY --from=imagemagick_builder /usr/local/bin/magick /usr/local/bin/magick
COPY --from=imagemagick_builder /usr/local/etc/ImageMagick-7 /usr/local/etc/ImageMagick-7
COPY --from=imagemagick_builder /usr/local/share/ImageMagick-7 /usr/local/share/ImageMagick-7
# Create symlinks to imagemagick tools
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 &&\
Comment on lines +99 to +110
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.

test $(magick -version | grep -o -e png -e tiff -e jpeg -e freetype -e heic -e webp | wc -l) -eq 6

ADD install-jemalloc /tmp/install-jemalloc
RUN /tmp/install-jemalloc
Expand Down
2 changes: 2 additions & 0 deletions image/base/install-imagemagick
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ tar zxf $WDIR/ImageMagick.tar.gz -C $WDIR
cd $IMDIR
PKG_CONF_LIBDIR=$PREFIX/lib LDFLAGS=-L$PREFIX/lib CFLAGS='-O2 -I$PREFIX/include' ./configure \
--prefix=$PREFIX \
--disable-shared \
--enable-delegate-build \
--enable-static \
--enable-bounds-checking \
--enable-hdri \
Expand Down
Loading