Move to a single template for Debian and Alpine #366
Merged
Conversation
This file contains 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
tianon
requested changes
Jan 22, 2025
Dockerfile.template
Outdated
Comment on lines
30
to
39
| # common packages | ||
| "ca-certificates", | ||
| "ghostscript", # for creating PDF thumbnails | ||
| "git", | ||
| "imagemagick", | ||
| "mercurial", | ||
| "openssh-client", | ||
| "subversion", | ||
| "tini", # grab tini for signal processing and zombie killing | ||
| "wget", |
There was a problem hiding this comment.
I don't love this (relying on a set of packages with the same name), but the alternative (copying the full list) is worse. 😂 😭 ❤️
It's even more dramatic in the later set where there's barely any overlap because the two distros use very different naming schemes for dev packages. 🙈
Dockerfile.template
Outdated
Comment on lines
20
to
65
| RUN set -eux; \ | ||
| {{ if is_alpine then ( -}} | ||
| apk add --no-cache \ | ||
| {{ ) else ( -}} | ||
| apt-get update; \ | ||
| apt-get install -y --no-install-recommends \ | ||
| ca-certificates \ | ||
| curl \ | ||
| wget \ | ||
| \ | ||
| bzr \ | ||
| git \ | ||
| mercurial \ | ||
| openssh-client \ | ||
| subversion \ | ||
| \ | ||
| # we need "gsfonts" for generating PNGs of Gantt charts | ||
| # and "ghostscript" for creating PDF thumbnails (in 4.1+) | ||
| ghostscript \ | ||
| gsfonts \ | ||
| imagemagick \ | ||
| # grab tini for signal processing and zombie killing | ||
| tini \ | ||
| {{ ) end -}} | ||
| {{ | ||
| [ | ||
| [ | ||
| # common packages | ||
| "ca-certificates", | ||
| "ghostscript", # for creating PDF thumbnails | ||
| "git", | ||
| "imagemagick", | ||
| "mercurial", | ||
| "openssh-client", | ||
| "subversion", | ||
| "tini", # grab tini for signal processing and zombie killing | ||
| "wget", | ||
| if is_alpine then | ||
| # alpine packages | ||
| "bash", | ||
| "breezy", | ||
| "findutils", | ||
| "ghostscript-fonts", # for generating PNGs of Gantt charts | ||
| "tzdata", | ||
| empty | ||
| else | ||
| # debian packages | ||
| "bzr", | ||
| "gsfonts", # for generating PNGs of Gantt charts | ||
| empty | ||
| end | ||
| ] | sort[] | ( | ||
| -}} | ||
| {{ . }} \ | ||
| {{ | ||
| ) | ||
| ] | add | ||
| -}} | ||
| {{ if is_alpine then ( -}} | ||
| ; | ||
| {{ ) else ( -}} | ||
| ; \ | ||
| # allow imagemagick to use ghostscript for PDF -> PNG thumbnail conversion (4.1+) | ||
| sed -ri 's/(rights)="none" (pattern="PDF")/\1="read" \2/' /etc/ImageMagick-6/policy.xml; \ | ||
| rm -rf /var/lib/apt/lists/* | ||
| {{ ) end -}} |
There was a problem hiding this comment.
I think this block is slightly easier to read if we refactor it so we have the package list first, then all the conditionals for is_alpine vs Debian in one block, so the actual end-result is all together and not split by that huge jq block:
{{
[
# common packages
"ca-certificates",
"ghostscript", # for creating PDF thumbnails
"git",
"imagemagick",
"mercurial",
"openssh-client",
"subversion",
"tini", # grab tini for signal processing and zombie killing
"wget",
if is_alpine then
# alpine packages
"bash",
"breezy",
"findutils",
"ghostscript-fonts", # for generating PNGs of Gantt charts
"tzdata",
empty
else
# debian packages
"bzr",
"gsfonts", # for generating PNGs of Gantt charts
empty
end
] | sort | (
-}}
RUN set -eux; \
{{ if is_alpine then ( -}}
apk add --no-cache \
{{ map( -}}
{{ . }} \
{{ ) | add -}}
;
{{ ) else ( -}}
apt-get update; \
apt-get install -y --no-install-recommends \
{{ map( -}}
{{ . }} \
{{ ) | add -}}
; \
# allow imagemagick to use ghostscript for PDF -> PNG thumbnail conversion (4.1+)
sed -ri 's/(rights)="none" (pattern="PDF")/\1="read" \2/' /etc/ImageMagick-6/policy.xml; \
rm -rf /var/lib/apt/lists/*
{{ ) end -}}
{{ ) -}}(If you agree, I have this change locally and am happy to just commit it if you'd rather.)
Dockerfile.template
Outdated
Comment on lines
172
to
220
| {{ if is_alpine then ( -}} | ||
| apk add --no-cache --virtual .build-deps \ | ||
| {{ ) else ( -}} | ||
| savedAptMark="$(apt-mark showmanual)"; \ | ||
| apt-get update; \ | ||
| apt-get install -y --no-install-recommends \ | ||
| default-libmysqlclient-dev \ | ||
| freetds-dev \ | ||
| gcc \ | ||
| libpq-dev \ | ||
| libsqlite3-dev \ | ||
| libxml2-dev \ | ||
| libxslt-dev \ | ||
| libyaml-dev \ | ||
| make \ | ||
| patch \ | ||
| pkgconf \ | ||
| xz-utils \ | ||
| {{ ) end -}} | ||
| {{ | ||
| [ | ||
| [ | ||
| # common packages | ||
| "freetds-dev", | ||
| "gcc", | ||
| "make", | ||
| "patch", | ||
| if is_alpine then | ||
| # alpine packages | ||
| "coreutils", # required because "chmod +X" in busybox will remove +x on files (and coreutils leaves files alone with +X) | ||
| "mariadb-dev", | ||
| "musl-dev", | ||
| "postgresql-dev", | ||
| "sqlite-dev", | ||
| "ttf2ufm", | ||
| "yaml-dev", | ||
| "zlib-dev", | ||
| empty | ||
| else | ||
| # debian packages | ||
| "default-libmysqlclient-dev", | ||
| "libpq-dev", | ||
| "libsqlite3-dev", | ||
| "libxml2-dev", | ||
| "libxslt-dev", | ||
| "libyaml-dev", | ||
| "pkgconf", | ||
| "xz-utils", | ||
| empty | ||
| end | ||
| ] | sort[] | ( | ||
| -}} | ||
| {{ . }} \ | ||
| {{ | ||
| ) | ||
| ] | add | ||
| -}} | ||
| ; \ | ||
| {{ if is_alpine | not then ( -}} | ||
| rm -rf /var/lib/apt/lists/*; \ | ||
| {{ ) else "" end -}} |
There was a problem hiding this comment.
Again, I think this multi-conditional logic is a little harder to follow than it needs to be 🙈
With some slight reordering:
{{
[
# common packages
"freetds-dev",
"gcc",
"make",
"patch",
if is_alpine then
# alpine packages
"coreutils", # required because "chmod +X" in busybox will remove +x on files (and coreutils leaves files alone with +X)
"mariadb-dev",
"musl-dev",
"postgresql-dev",
"sqlite-dev",
"ttf2ufm",
"yaml-dev",
"zlib-dev",
empty
else
# debian packages
"default-libmysqlclient-dev",
"libpq-dev",
"libsqlite3-dev",
"libxml2-dev",
"libxslt-dev",
"libyaml-dev",
"pkgconf",
"xz-utils",
empty
end
] | sort | (
-}}
{{ if is_alpine then ( -}}
apk add --no-cache --virtual .build-deps \
{{ map( -}}
{{ . }} \
{{ ) | add -}}
; \
{{ ) else ( -}}
savedAptMark="$(apt-mark showmanual)"; \
apt-get update; \
apt-get install -y --no-install-recommends \
{{ map( -}}
{{ . }} \
{{ ) | add -}}
; \
rm -rf /var/lib/apt/lists/*; \
{{ ) end -}}
{{ ) -}}
Dockerfile.template
Outdated
Comment on lines
239
to
242
| {{ if is_alpine | not then ( -}} | ||
| # nokogiri's vendored libxml2 + libxslt do not build on mips64le, so use the apt packages when building | ||
| gosu redmine bundle config build.nokogiri --use-system-libraries; \ | ||
| {{ ) else "" end -}} |
There was a problem hiding this comment.
Instead of | not we can swap the else block:
Suggested change
| {{ if is_alpine | not then ( -}} | |
| # nokogiri's vendored libxml2 + libxslt do not build on mips64le, so use the apt packages when building | |
| gosu redmine bundle config build.nokogiri --use-system-libraries; \ | |
| {{ ) else "" end -}} | |
| {{ if is_alpine then "" else ( -}} | |
| # nokogiri's vendored libxml2 + libxslt do not build on mips64le, so use the apt packages when building | |
| gosu redmine bundle config build.nokogiri --use-system-libraries; \ | |
| {{ ) end -}} |
apply-templates.sh
Outdated
| { | ||
| generated_warning | ||
| gawk -f "$jqt" "$template" | ||
| gawk -f "$jqt" "Dockerfile.template" |
There was a problem hiding this comment.
Suggested change
| gawk -f "$jqt" "Dockerfile.template" | |
| gawk -f "$jqt" Dockerfile.template |
yosifkit
force-pushed
the
single-template
branch
from
c89003a to
b160f7a
Compare
tianon
approved these changes
Jan 23, 2025
Comment on lines
+138
to
+140
| apk add --no-cache patch; \ | ||
| {{ ) else ( -}} | ||
| savedAptMark="$(apt-mark showmanual)"; \ |
There was a problem hiding this comment.
Similar to savedAptMark, should we add --virtual here? 🤔
I don't think this is super important.
There was a problem hiding this comment.
I guess this is along the same lines as "is BusyBox's patch enough?" (and similarly unimportant)
docker-library-bot
added a commit
to docker-library-bot/official-images
that referenced
this pull request
Jan 23, 2025
Changes: - docker-library/redmine@8d36a1a: Merge pull request docker-library/redmine#366 from infosiftr/single-template - docker-library/redmine@b160f7a: Move to a single template for Debian and Alpine - docker-library/redmine@087f78a: Merge pull request docker-library/redmine#365 from infosiftr/better-keybase - docker-library/redmine@922b31e: Backport https://www.redmine.org/issues/42113 patch for 5.x - docker-library/redmine@a57cd24: Drop secrets.yml and just use `SECRET_KEY_BASE`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Minor package sorting and add
apt markto the temporary patch from #365Also, #249 (comment) seems to be fine now, so we can drop
curlfrom Debian.Edit: I don't mind backing out the package sorting or other bits causing extra generated Dockerfile changes if it is too much or undesirable.