-
Notifications
You must be signed in to change notification settings - Fork 320
Add initial jq-based templating engine #456
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still working through it, but figured starting with this is probably helpful to you.
Dockerfile.template
Outdated
| {{ if env.version | endswith("-rc") then ( -}} | ||
| case "$GHOST_VERSION" in *-alpha* | *-beta* | *-rc*) installCmd="$installCmd --channel next" ;; esac; \ | ||
| {{ ) else "" end -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably do this unilaterally for the -rc and not have the case anymore, but avoiding the change for now is fine too 👍
apply-templates.sh
Outdated
| [ -f versions.json ] # run "versions.sh" first | ||
|
|
||
| cd "$(dirname "$(readlink -f "$BASH_SOURCE")")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do the -f as a rough sanity check so that we don't have to do the cd 👀
(otherwise they're checking different directories entirely 😅)
| [ -f versions.json ] # run "versions.sh" first | |
| cd "$(dirname "$(readlink -f "$BASH_SOURCE")")" | |
| [ -f versions.json ] # run "versions.sh" first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept the cd as a defense in depth for the rm -rf $version to not accidentally delete something unexpected. But yeah, the order is wrong.
| if [ -d "$version" ]; then | ||
| rm -rf "$version" | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if [ -d "$version" ]; then | |
| rm -rf "$version" | |
| fi | |
| rm -rf "$version" |
-fwill make sure this doesn't fail when it doesn't exist,- and if it exists but isn't a directory, it would sometimes fail later (depending on the next conditional) so it's easier to manage/think about the drift if we just enforce the state at this level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
if [ -d "$version" ]; then rm -rf "$version" fi rm -rf "$version"
-fwill make sure this doesn't fail when it doesn't exist,- and if it exists but isn't a directory, it would sometimes fail later (depending on the next conditional) so it's easier to manage/think about the drift if we just enforce the state at this level
Did you decide against this change? 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 I don't like either. But I don't have a better solution to protect against bad input. Certain software of the past nuking user's home directories makes me nervous of rm -rf "$var".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, from that perspective rm -rf "$var" is the safest version we could possibly have, and the rm -rf "$var/" I often do is the unsafe version, because if $var ends up empty somehow (and doesn't trigger set -u), we'll run rm -rf '' which arguably should error, but in practice does nothing successfully instead:
$ ls
bar foo
$ rm -rf ''; echo $?
0
$ ls
bar foo(Those issues come from combining variables in rm -rf like rm -rf "$somedir/$somefile")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which arguably should error
From the perspective of "empty is an invalid filename" it's probably sane that it doesn't though 🤷
$ touch ''
touch: cannot touch '': No such file or directory
generate-stackbrew-library.sh
Outdated
| | xargs -r bashbrew cat --format '["{{ .RepoName }}:{{ .TagName }}"]="{{ join " " .TagEntry.Architectures }}"' | ||
| )" | ||
| eval "declare -g -A parentRepoToArches=( $parentRepoToArchesStr )" | ||
| : "${BASHBREW_LIBRARY:-https://github.com/docker-library/official-images/raw/HEAD/library}/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without an export this doesn't apply, and we don't actually support a URL path in BASBHREW_LIBRARY (we assume it points to a directory), which is why it was implemented the way it was in the previous version of this function. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was over aggressive on the removal of the find + awk. After testing with BASHBREW_LIBRARY set and unset, I have made an adjustment (coming soon).
generate-stackbrew-library.sh
Outdated
| getArches 'ghost' | ||
| parentArchesJson="$(getArchesJson 'ghost')" | ||
|
|
||
| commit="$(fileCommit '*/**')" # |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this # for a TODO of some kind or something? 😅
generate-stackbrew-library.sh
Outdated
| jq --raw-output '.[].variants[].from' versions.json \ | ||
| | sort -u \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| jq --raw-output '.[].variants[].from' versions.json \ | |
| | sort -u \ | |
| jq --raw-output '[ .[].variants[].from ] | unique[]' versions.json \ |
generate-stackbrew-library.sh
Outdated
|
|
||
| jq --raw-output '.[].variants[].from' versions.json \ | ||
| | sort -u \ | ||
| | xargs -r bashbrew cat --format '{ "{{ .RepoName }}:{{ .TagName }}": {{ json .TagEntry.Architectures }} }' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | xargs -r bashbrew cat --format '{ "{{ .RepoName }}:{{ .TagName }}": {{ json .TagEntry.Architectures }} }' \ | |
| | xargs -r bashbrew cat --format '{ {{ join ":" .RepoName .TagName | json }}: {{ json .TagEntry.Architectures }} }' \ |
generate-stackbrew-library.sh
Outdated
| jq --raw-output '.[].variants[].from' versions.json \ | ||
| | sort -u \ | ||
| | xargs -r bashbrew cat --format '{ "{{ .RepoName }}:{{ .TagName }}": {{ json .TagEntry.Architectures }} }' \ | ||
| | jq --compact-output --null-input '[ inputs ] | add' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just slurp 😅
| | jq --compact-output --null-input '[ inputs ] | add' | |
| | jq --compact-output --slurp 'add' |
generate-stackbrew-library.sh
Outdated
| EOE | ||
| done | ||
| done | ||
| jq --raw-output --argjson parentArches "$parentArchesJson" ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is long enough that we should move the body of it into a separate generate-stackbrew-library.jq file so we get nice syntax highlighting (from our editors and GitHub). 😇
exec jq \
--raw-output \
--argjson parentArches "$parentArchesJson" \
--from-file generate-stackbrew-library.jq \
versions.json \
--args -- "$@"
generate-stackbrew-library.sh
Outdated
| # returns a list of tags for this variant | ||
| # usage: variant_tags("version"; [sortedVersionList]; "variant"; [sortedVariantList]) | ||
| # e.g. variant_tags("1.2.3.4"; ["1.2.3.4", "0.1.2.3"]; "trixie"; ["trixie", "alpine3.22"]) -> ["1.2.3.4-trixie", "1.2.3-trixie", ...] | ||
| def variant_tags($version; $versionListSorted; $variant; $variantListSorted): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that each of these are only used once, what's the benefit to having functions over just embedding the logic below? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually trying to not embed all the logic inline so that it could be easier to understand and simpler to extend to other images. But it has kind of come back into basically one function. I think that it could be improved to be more generic and useful to any image repo we maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I find it a little harder to read this way and I'm not convinced it will actually be as reusable as we want it to be in practice, but I don't feel strongly about it and I'm willing to give it a chance. 😄 👍
95d3768 to
83ce252
Compare
|
I checked this out locally so I could redo your original diff now that it's had some changes/updates, and `bashbrew cat`:$ diff -u <(bashbrew cat ghost) <(bashbrew cat <(./generate-stackbrew-library.sh))
--- /dev/fd/63 2025-10-22 15:15:17.266193532 -0700
+++ /dev/fd/62 2025-10-22 15:15:17.266193532 -0700
@@ -1,22 +1,19 @@
Maintainers: Tianon Gravi <[email protected]> (@tianon), Joseph Ferguson <[email protected]> (@yosifkit), Austin Burdine <[email protected]> (@acburdine)
GitRepo: https://github.com/docker-library/ghost.git
+GitCommit: 83ce25215e22a36c646c0a6165a531f12d92339f
-Tags: 6.5.0, 6.5, 6, latest
-Architectures: amd64, arm32v7, arm64v8, ppc64le, s390x
-GitCommit: f952f7615274894d6efb0fbc765f5368580fb39a
-Directory: 6/debian
+Tags: 6.5.0-bookworm, 6.5-bookworm, 6-bookworm, bookworm, 6.5.0, 6.5, 6, latest
+Architectures: amd64, arm32v7, arm64v8, s390x
+Directory: 6/bookworm
-Tags: 6.5.0-alpine, 6.5-alpine, 6-alpine, alpine
-Architectures: amd64, arm32v6, arm32v7, arm64v8
-GitCommit: f952f7615274894d6efb0fbc765f5368580fb39a
-Directory: 6/alpine
+Tags: 6.5.0-alpine3.22, 6.5-alpine3.22, 6-alpine3.22, alpine3.22, 6.5.0-alpine, 6.5-alpine, 6-alpine, alpine
+Architectures: amd64, arm64v8
+Directory: 6/alpine3.22
-Tags: 5.130.5, 5.130, 5
-Architectures: amd64, arm32v7, arm64v8, ppc64le, s390x
-GitCommit: fdba3d80f50da610007165f5fe46f9b8af69764b
-Directory: 5/debian
+Tags: 5.130.5-bookworm, 5.130-bookworm, 5-bookworm, 5.130.5, 5.130, 5
+Architectures: amd64, arm32v7, arm64v8, s390x
+Directory: 5/bookworm
-Tags: 5.130.5-alpine, 5.130-alpine, 5-alpine
-Architectures: amd64, arm32v6, arm32v7, arm64v8
-GitCommit: fdba3d80f50da610007165f5fe46f9b8af69764b
-Directory: 5/alpine
+Tags: 5.130.5-alpine3.22, 5.130-alpine3.22, 5-alpine3.22, 5.130.5-alpine, 5.130-alpine, 5-alpine
+Architectures: amd64, arm64v8
+Directory: 5/alpine3.22`bashbrew list`:$ diff -u <(bashbrew list ghost | cut -d: -f2) <(bashbrew list <(./generate-stackbrew-library.sh) | cut -d: -f2)
--- /dev/fd/63 2025-10-22 15:16:27.434905448 -0700
+++ /dev/fd/62 2025-10-22 15:16:27.434905448 -0700
@@ -1,14 +1,28 @@
+6.5.0-bookworm
+6.5-bookworm
+6-bookworm
+bookworm
6.5.0
6.5
6
latest
+6.5.0-alpine3.22
+6.5-alpine3.22
+6-alpine3.22
+alpine3.22
6.5.0-alpine
6.5-alpine
6-alpine
alpine
+5.130.5-bookworm
+5.130-bookworm
+5-bookworm
5.130.5
5.130
5
+5.130.5-alpine3.22
+5.130-alpine3.22
+5-alpine3.22
5.130.5-alpine
5.130-alpine
5-alpine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hmm, I didn't notice these renames - this implies that we might support more than one version of a given base distribution -- is that an intentional change? (I certainly wasn't intending to support more than one each here 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no. I did add variants that could support multiple base distribution versions, but mostly to add extra tags. I don't think we should keep more than one Debian suite or Alpine version per ghost version.
|
This force push includes a partial rewrite of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some versions.sh comments/review 😇
versions.sh
Outdated
| dist: { | ||
| linux: [ | ||
| # list of debian versions, in descending order | ||
| #"trixie", # TODO | ||
| "bookworm", | ||
| empty # trailing comma | ||
| ], | ||
| linuxmusl: [ | ||
| # list of alpine versions, in descending order | ||
| "3.22", | ||
| empty | ||
| | "alpine" + . | ||
| ] | ||
| }[.[0]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant to the above conversation about supporting multiple variants, I think it's fine to leave the data model here able to support multiple, but we should update the code filling it in to make it more clear we won't accept more than one (unless something really exceptional happens, I guess), and so that we don't accidentally forget in the future; maybe something like this (very open to alternatives, but this is a straw man)?
| dist: { | |
| linux: [ | |
| # list of debian versions, in descending order | |
| #"trixie", # TODO | |
| "bookworm", | |
| empty # trailing comma | |
| ], | |
| linuxmusl: [ | |
| # list of alpine versions, in descending order | |
| "3.22", | |
| empty | |
| | "alpine" + . | |
| ] | |
| }[.[0]], | |
| dist: { | |
| # each of these should be a single distro version unless something *really* exceptional happens | |
| linux: [ "bookworm" ], | |
| linuxmusl: [ "alpine3.22" ], | |
| }[.[0]], |
versions.sh
Outdated
| | capture("^ *\"@img/sharp-(?<name>linux[a-z0-9-]+)\" \"[0-9.]+\"$") | ||
| | .name | ||
| | split("-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the immediate split to take off more data, couldn't this instead massage the regex a little so that .name is the exact value we need?
| | capture("^ *\"@img/sharp-(?<name>linux[a-z0-9-]+)\" \"[0-9.]+\"$") | |
| | .name | |
| | split("-") | |
| | capture("^ *\"@img/sharp-(?<dist>linux[a-z]*)-(?<arch>[a-z0-9]+)\" \"[0-9.]+\"$") |
(and then using .dist and .arch instead of .[0] and .[1] below - could even be .dist |= { ... }[.] | .arch |= { ... }[.])
versions.sh
Outdated
| }[.[1]], | ||
| } | ||
| ) as $item ({}; .[$item.dist[]].arches += [ $item.arch ]) | ||
| | with_entries(.value.arches |= sort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just be .[].arches |= sort:
$ jq -nc '{ foo: { bar: [ "c", "b", "a" ] } } | .[].bar |= sort'
{"foo":{"bar":["a","b","c"]}}
$ # still works if we get even more whack:
$ jq -nc '{ foo: { bar: [ "c", "b", "a" ] } } | .[][] |= sort'
{"foo":{"bar":["a","b","c"]}}However, the order should already be deterministic from yarn.lock, so I'm not convinced it's necessary for us to sort here. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deterministic from
yarn.lock
That puts them in alphabetical order based on the "arch" key, so arm > arm64 > s390x > x64. I don't like amd64 at the end so I've moved the sort up so that it is while the lists are created. This order will also control the order generated by generate-stackbrew-library.sh so that we don't rely on parent arch list order.
versions.sh
Outdated
| # move alpine variants to the end of the object | ||
| # debian suites and alpine versions are each relatively sorted already | ||
| | map(.value.variants |= ( to_entries | sort_by(.key | contains("alpine")) | from_entries)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels weird to munge variants on these objects so late instead of doing so above when we're generating them 😅
I'll add a suggestion above for a potential way to resolve this without an explicit sort 👀
versions.sh
Outdated
| reduce ( | ||
| inputs | ||
| | capture("^ *\"@img/sharp-(?<name>linux[a-z0-9-]+)\" \"[0-9.]+\"$") | ||
| | .name | ||
| | split("-") | ||
| | { | ||
| dist: { | ||
| linux: [ | ||
| # list of debian versions, in descending order | ||
| #"trixie", # TODO | ||
| "bookworm", | ||
| empty # trailing comma | ||
| ], | ||
| linuxmusl: [ | ||
| # list of alpine versions, in descending order | ||
| "3.22", | ||
| empty | ||
| | "alpine" + . | ||
| ] | ||
| }[.[0]], | ||
| arch: { | ||
| x64: "amd64", | ||
| arm64: "arm64v8", | ||
| arm: "arm32v7", | ||
| s390x: "s390x", | ||
| }[.[1]], | ||
| } | ||
| ) as $item ({}; .[$item.dist[]].arches += [ $item.arch ]) | ||
| | with_entries(.value.arches |= sort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's one possibility for fixing the ordering of Debian first, Alpine second without an explicit sort (incorporating the above suggestions too):
| reduce ( | |
| inputs | |
| | capture("^ *\"@img/sharp-(?<name>linux[a-z0-9-]+)\" \"[0-9.]+\"$") | |
| | .name | |
| | split("-") | |
| | { | |
| dist: { | |
| linux: [ | |
| # list of debian versions, in descending order | |
| #"trixie", # TODO | |
| "bookworm", | |
| empty # trailing comma | |
| ], | |
| linuxmusl: [ | |
| # list of alpine versions, in descending order | |
| "3.22", | |
| empty | |
| | "alpine" + . | |
| ] | |
| }[.[0]], | |
| arch: { | |
| x64: "amd64", | |
| arm64: "arm64v8", | |
| arm: "arm32v7", | |
| s390x: "s390x", | |
| }[.[1]], | |
| } | |
| ) as $item ({}; .[$item.dist[]].arches += [ $item.arch ]) | |
| | with_entries(.value.arches |= sort) | |
| reduce ( | |
| inputs | |
| | capture("^ *\"@img/sharp-(?<dist>linux[a-z]*)-(?<arch>[a-z0-9]+)\" \"[0-9.]+\"$") | |
| ) as $item ({ | |
| # this controls the variant ordering | |
| linux: [], # non-Alpine first | |
| linuxmusl: [], # Alpine second | |
| }; .[$item.dist] += [ $item.arch ]) | |
| | with_entries( | |
| select(.value | length > 0) | |
| | .key |= { | |
| # each of these should be a single distro version unless something *really* exceptional happens | |
| linux: [ "bookworm" ][], | |
| linuxmusl: [ "alpine3.22" ][], | |
| }[.] | |
| | .value |= { arches: [ { | |
| x64: "amd64", | |
| arm64: "arm64v8", | |
| arm: "arm32v7", | |
| s390x: "s390x", | |
| }[.[]] ] } # TODO maybe a "// empty" here so we ignore anything we are not explicitly handling? error() would be super reasonable too but would be worth swapping this to ".[] | {...}[.]" or "arches: map(...)" instead so we can include the offending value in the error | |
| ) |
versions.sh
Outdated
| to_entries | ||
| # sort by version number, descending | ||
| | sort_by(.value.version | split("[.-]"; "") | map(tonumber? // .) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a few instances of weird parenthesis "cuddling" throughout this PR (where a parenthesis and its pair don't have the same whitespace) 😅
I don't have a strong opinion on individual cases whether they should be func(foo) or func( foo ) or (foo(bar)) or ( foo(bar) ) or ( foo( bar ) ), but I do have a strong opinion that matching pairs should have the same whitespace at each end, and otherwise am fine with whatever's most readable/ergonomic (I personally usually default to "tight" unless things look exceptionally hard to read, but that's often my clue that I need newlines instead 😅)
versions.sh
Outdated
| | map(.value.variants |= ( to_entries | sort_by(.key | contains("alpine")) | from_entries)) | ||
| | from_entries | ||
| '> versions.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| '> versions.json | |
| ' > versions.json |
versions.sh
Outdated
| )" | ||
|
|
||
| export fullVersion cliVersion | ||
| json="$(jq <<<"$json" -c --argjson doc "$doc" ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| json="$(jq <<<"$json" -c --argjson doc "$doc" ' | |
| json="$(jq <<<"$json" --compact-output --argjson doc "$doc" ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fresh generate comments 😇
generate-stackbrew-library.jq
Outdated
| | ( $allVariants | map(select(contains("alpine"))))[0] as $latestAlpine | ||
| | ( $allVariants | map(select(contains("alpine") | not)))[0] as $latestDebian |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | ( $allVariants | map(select(contains("alpine"))))[0] as $latestAlpine | |
| | ( $allVariants | map(select(contains("alpine") | not)))[0] as $latestDebian | |
| | first( $allVariants[] | select(contains("alpine")) ) as $latestAlpine | |
| | first( $allVariants[] | select(contains("alpine") | not) ) as $latestDebian |
I think this is actually slightly more efficient inside jq if we redo the .[].variants | keys_unsorted[] inside the first() because it won't complete the full iteration and will actively short-circuit, but readability definitely suffers if we do that 😅
generate-stackbrew-library.jq
Outdated
| @@ -0,0 +1,73 @@ | |||
| # latest version is the first one since it is already in version order | |||
| keys_unsorted[0] as $latestMajor | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still true if we have pre-releases? Don't we need to filter them out? ie:
| keys_unsorted[0] as $latestMajor | |
| first(keys_unsorted[] | select(endswith("-rc") | not)) as $latestMajor |
Oh, and doesn't that also have issues if we have, for example, a 7-rc and 7 is null? So this needs to account for the value too?
| keys_unsorted[0] as $latestMajor | |
| first(to_entries[] | select(.value and (.key | endswith("-rc") | not)).key) as $latestMajor |
generate-stackbrew-library.jq
Outdated
|
|
||
|
|
||
| # loop over major versions | ||
| | to_entries[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, if we did to_entries sooner, we'd have less iteration above -- basically if this file started with to_entries right off the bat, we'd then have:
to_entries
| first(.[] | select(.value and (.key | endswith("-rc") | not)).key) as $latestMajor
| [ .[].value.variants | keys_unsorted[] ] as $allVariants
| first( $allVariants[] | select(contains("alpine")) ) as $latestAlpine
| first( $allVariants[] | select(contains("alpine") | not) ) as $latestDebian
| .[]
| .key as $major
| .value
...(I guess it really only helps with $latestMajor 🤷 but that's not nothing!)
generate-stackbrew-library.jq
Outdated
| # latest version is the first one since it is already in version order | ||
| keys_unsorted[0] as $latestMajor | ||
| # list of all variants in version.json order (debian suites followed by alpine versions, each in descending order) | ||
| | [ .[].variants | keys_unsorted[] ] as $allVariants |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess arguably, this should actually be .[$latestMajor].variants explicitly, right? Because we don't want to "default" any other aliases (like if a pre-release version had a new Alpine or Debian version that the latest release doesn't have yet). That also makes it less useful/meaningful/DRY to have $allVariants because that's a pretty static lookup. 😅
... and yes, this is in direct contrast to my suggestion that we move up the to_entries because then we don't have trivial lookup here unless we save the entire $latestMajor key/value pair instead of just the key 😂
So maybe that'd look something like this?
to_entries
| first(.[] | select(.value and (.key | endswith("-rc") | not))) as $latestMajor
| first( $latestMajor.value.variants[] | select(contains("alpine")) ) as $latestAlpine
| first( $latestMajor.value.variants[] | select(contains("alpine") | not) ) as $latestDebian
| $latestMajor.key as $latestMajor
| .[]
| .key as $major
| .value
...
generate-stackbrew-library.jq
Outdated
| # e.g. "1.2.3.4" -> [ "1.2.3.4", "1.2.3", "1.2", "1", "" ] | ||
| | ( | ||
| .version | ||
| # TODO improve rc suffix support (e.g. this won't work for -beta or -rc.1 versioned software) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is really something like if $major | endswith("-rc") then [ ., $major ] else ... instead?
| if $variant == $latestAlpine then | ||
| ($variant | sub("[0-9.]+$"; "")) # "alpine3.22" -> "alpine" | ||
| else empty end, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really verbose way to spell "alpine" 😅
Why the sub? 🤔
| if $variant == $latestAlpine then | |
| ($variant | sub("[0-9.]+$"; "")) # "alpine3.22" -> "alpine" | |
| else empty end, | |
| if $variant == $latestAlpine then "alpine" else empty end, |
|
Recent force push simplifies the optional dependencies logic by just ensuring that their system dependencies are installed so that they succeed to install during the (Working on the build failure: |
d4633e7 to
616959c
Compare
Dockerfile.template
Outdated
| gosu --version; \ | ||
| gosu nobody true | ||
| {{ if is_alpine then ( -}} | ||
| RUN set -eux; ln -svf gosu /usr/local/bin/su-exec; su-exec nobody true # backwards compatibility (TODO remove in Ghost 6+) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added in #417. Should we follow the comment and drop it for the 6+ images or wait for a different release? In either case, we should adjust the template to do it automatically, so we don't forget again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, oops. I think maybe we missed the boat? or maybe it's still defensible? I'm OK with either direction, but yeah, we should fix the template so we actually do it either now or later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with the "remove it at the next release" on both 5 and 6. The image has been using gosu in Alpine images for over a year so users have had time to migrate off of su-exec.
| apk add --no-cache --virtual .build-deps-ghost g++ linux-headers make python3 py3-setuptools; \ | ||
| {{ ) else ( -}} | ||
| savedAptMark="$(apt-mark showmanual)"; \ | ||
| apt-get update; \ | ||
| apt-get install -y --no-install-recommends g++ make python3; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible TODO: add libvips-dev or equivalent package to build deps plus a scanelf/ldd dance to keep linked libraries. This would allow the ghost install line to try building libvips on architectures that don't have prebuilt sharp/vips linking. And would maybe let us add back arm32vX Alpine images and ppc64le for Debian if we are on Trixie. That assumes that the sharp version in ghost's yarn.lock does not change from v0.34.2 because anything newer requires a newer libvips than either Trixie or Alpine 3.22 have.
The scanelf/ldd dance is complicated by the fact that some node modules contain libraries not valid for the current user space so you have things linked against libc.musl-armv7.so.1, libc.so.6, libresolv.so.2, and libvips-cpp.so.8.16.1 in an Alpine container and so they fail when passed to apk add --no-network:
+ apk add --no-network --virtual .ghost-rundeps so:ld-linux-armhf.so.3 so:libc.musl-armv7.so.1 so:libc.so.6 so:libdl.so.2 so:libgcc_s.so.1 so:libglib-2.0.so.0 so:libgobject-2.0.so.0 so:libm.so.6 so:libpthread.so.0 so:libresolv.so.2 so:libstdc++.so.6 so:libvips-cpp.so.42 so:libvips-cpp.so.8.16.1 so:libvips.so.42
...
so:ld-linux-armhf.so.3 (no such package):
required by: .ghost-rundeps-20251029.205402[so:ld-linux-armhf.so.3]
so:libc.so.6 (no such package):
required by: .ghost-rundeps-20251029.205402[so:libc.so.6]
so:libdl.so.2 (no such package):
required by: .ghost-rundeps-20251029.205402[so:libdl.so.2]
so:libm.so.6 (no such package):
required by: .ghost-rundeps-20251029.205402[so:libm.so.6]
so:libpthread.so.0 (no such package):
required by: .ghost-rundeps-20251029.205402[so:libpthread.so.0]
so:libresolv.so.2 (no such package):
required by: .ghost-rundeps-20251029.205402[so:libresolv.so.2]
so:libvips-cpp.so.8.16.1 (no such package):
required by: .ghost-rundeps-20251029.205402[so:libvips-cpp.so.8.16.1]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel all kinds of ways reading this, especially that latter half.
Given the historically aggressive relationship between Sharp and libvips, I'm very comfortable saying that if Sharp doesn't pre-build for it, we don't support it. I think that's a sane stance that still gets us a good, reasonable set of supported architectures, and even more importantly a set that is arguably supported by upstream (Sharp).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (some minor comments, but nothing worth blocking on and I'm happy if you want to merge this as-is)
Dockerfile.template
Outdated
| apt-mark showmanual | xargs apt-mark auto > /dev/null; \ | ||
| [ -z "$savedAptMark" ] || apt-mark manual $savedAptMark; \ | ||
| apt-get purge -y --auto-remove; \ | ||
| rm -rf /var/lib/apt/lists/*; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| apt-mark showmanual | xargs apt-mark auto > /dev/null; \ | |
| [ -z "$savedAptMark" ] || apt-mark manual $savedAptMark; \ | |
| apt-get purge -y --auto-remove; \ | |
| rm -rf /var/lib/apt/lists/*; \ | |
| apt-mark auto '.*' > /dev/null; \ | |
| [ -z "$savedAptMark" ] || apt-mark manual $savedAptMark > /dev/null; \ | |
| apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false; \ | |
| {{ clean_apt }}; \ |
| {{ ) else ( -}} | ||
| apt-mark auto '.*' > /dev/null; \ | ||
| [ -z "$savedAptMark" ] || apt-mark manual $savedAptMark > /dev/null; \ | ||
| apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should technically move clean_apt down here:
| apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false; \ | |
| apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false; \ | |
| {{ clean_apt }}; \ |
Dockerfile.template
Outdated
| WORKDIR $GHOST_INSTALL | ||
| VOLUME $GHOST_CONTENT | ||
|
|
||
| COPY docker-entrypoint.sh /usr/local/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| COPY docker-entrypoint.sh /usr/local/bin | |
| COPY docker-entrypoint.sh /usr/local/bin/ |
| if [ -d "$version" ]; then | ||
| rm -rf "$version" | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
if [ -d "$version" ]; then rm -rf "$version" fi rm -rf "$version"
-fwill make sure this doesn't fail when it doesn't exist,- and if it exists but isn't a directory, it would sometimes fail later (depending on the next conditional) so it's easier to manage/think about the drift if we just enforce the state at this level
Did you decide against this change? 😇
generate-stackbrew-library.jq
Outdated
|
|
||
| | ( | ||
| "", # newline between library entries | ||
| "Tags: \( $tags | join(", "))", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "Tags: \( $tags | join(", "))", | |
| "Tags: \($tags | join(", "))", |
or
| "Tags: \( $tags | join(", "))", | |
| "Tags: \( $tags | join(", ") )", |
generate-stackbrew-library.sh
Outdated
| getArches 'ghost' | ||
| parentArchesJson="$(getArchesJson 'ghost')" | ||
|
|
||
| commit="$(fileCommit '*/**')" # last commit that changed files related to a build context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this matches .github/**. Hmm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| commit="$(fileCommit '*/**')" # last commit that changed files related to a build context | |
| commit="$(git log -1 --format='format:%H' HEAD -- '[^.]*/**')" # last commit that changed files related to a build context |
generate-stackbrew-library.sh
Outdated
| cat <<-EOH | ||
| # this file is generated via https://github.com/docker-library/ghost/blob/$(fileCommit "$self")/$self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this too, we can drop fileCommit entirely:
| cat <<-EOH | |
| # this file is generated via https://github.com/docker-library/ghost/blob/$(fileCommit "$self")/$self | |
| selfCommit="$(git log -1 --format='format:%H' HEAD -- "$self")" | |
| cat <<-EOH | |
| # this file is generated via https://github.com/docker-library/ghost/blob/$selfCommit/$self |
Based on docker-library/php#1052 and related.
Notably this drops architectures that don't have prebuilt
sharplibraries (a dep of ghost that useslibvipsand requires a newer version than is in Alpine or Debian to build sharp).This also rewrites
generate-stackbrew-library.shto use lots ofjqinstead of bash string replacements. Maybe it is better, maybe it is not. This also brings in Debian suite and Alpine version tags (and then we could have more than one suite/version).