Skip to content

Clean up docker/build.sh #197

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
Aug 2, 2024
Merged

Clean up docker/build.sh #197

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
4 changes: 1 addition & 3 deletions .github/workflows/build-domjudge-container-PR.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ jobs:
- name: Build the container
run: |
cd docker
set -x
sh ./build.sh "${{ env.DOMJUDGE_VERSION }}"
set +x
./build.sh "${{ env.DOMJUDGE_VERSION }}"

- name: Build and push
run: |
Expand Down
4 changes: 1 addition & 3 deletions .github/workflows/build-domjudge-container-release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ jobs:
- name: Build the container
run: |
cd docker
set -x
sh ./build.sh "${{ env.DOMJUDGE_VERSION }}"
set +x
./build.sh "${{ env.DOMJUDGE_VERSION }}"

- name: Build and push
run: |
Expand Down
73 changes: 30 additions & 43 deletions docker/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,72 +1,59 @@
#!/bin/sh -eux
#!/bin/sh -eu

# Placeholders to annotate the Github actions logs
trace_on () { true; }
trace_off () { true; }
section_start () { true; }
section_end () { true; }
if [ "$#" -eq 0 ] || [ "$#" -gt 2 ]
then
echo "Usage: $0 domjudge-version <namespace>"
echo " For example: $0 5.3.0"
echo " or: $0 5.3.0 otherNamespace"
exit 1
fi

# Placeholders for grouping log lines
# (the body is a nested function declaration so it won't appear in the trace when using `set -x`)
section_start() { _() { :; }; }
section_end() { _() { :; }; }

if [ -n "${CI+x}" ]
then
if [ -n "${GITHUB_ACTION+x}" ]
set -x
then
# Functions to annotate the Github actions logs
trace_on () { set -x; }
trace_off () {
{ set +x; } 2>/dev/null
}

section_start_internal () {
# Functions for grouping log lines on GitHub Actions
trace_on() { set -x; }
# trace_off is manually inlined so it won't appear in the trace
section_start() {
{ set +x; } 2>/dev/null # trace_off
echo "::group::$1"
trace_on
}

section_end_internal () {
section_end() {
{ set +x; } 2>/dev/null # trace_off
echo "::endgroup::"
trace_on
}

section_start () {
trace_off
section_start_internal "$@"
}
section_end () {
trace_off
section_end_internal
}
else
export PS4='(${0}:${LINENO}): - [$?] $ '
# Redirect stderr to stdout as a workaround so they won't be out-of-order; see
Comment on lines -38 to -39
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out that LINENO doesn't work in sh. How about switching to bash?

Copy link
Member

Choose a reason for hiding this comment

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

We try by default ship sh scripts instead of bash, I don't think the LINENO is imporant enough to switch as this is only for CI and only in a legacy part of CI as this was used for gitlab-ci.

So in this case moving to bash is not worth it IMO, if you find another section where this makes sense I'm fine with (reconsider) moving. But to me the line numbers are not relevant when checking the CI log as this logging was only copied from the main repository but is not relevant for such a short script.

Copy link
Contributor Author

@tom93 tom93 Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Switching to bash would also allow hiding the section_start (and section_end) lines from the trace, but it's not a big deal. Currently it looks like this without PS4 (the "▸" lines are collapsed groups):

▶ Run cd docker
+ VERSION=8.2.3
+ NAMESPACE=domjudge
+ URL=https://www.domjudge.org/releases/domjudge-8.2.3.tar.gz
+ FILE=domjudge.tar.gz
+ section_start Download DOMjudge tarball
▶ Download DOMjudge tarball
+ section_start Build domserver container
▶ Build domserver container
+ section_start Build judgehost container (with intermediate image)
▸ Build judgehost container (with intermediate image)
+ section_start Build judgehost container (judging chroot)
▶ Build judgehost container (judging chroot)
+ section_start Push instructions
▶ Push instructions

(Another option is to only enable tracing between section_start and section_end, which would hide the section_start lines even in sh.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed PS4 (and put the variable assignments in a log group to make things look nicer).

# https://github.com/orgs/community/discussions/116552
# https://web.archive.org/web/20220430214837/https://github.community/t/stdout-stderr-output-not-in-correct-order-in-logs/16335
# (GitHub Actions displays stderr in the same style as stdout anyway, so
# there is no harm in us merging them.)
exec 2>&1
fi
set -x
fi

if [ "$#" -eq 0 ] || [ "$#" -gt 2 ]
then
echo "Usage: $0 domjudge-version <namespace>"
echo " For example: $0 5.3.0"
echo " or: $0 5.3.0 otherNamespace"
exit 1
fi

section_start "Variables"
VERSION="$1"
NAMESPACE="domjudge"
if [ -n "${2+x}" ]
then
NAMESPACE="$2"
fi

NAMESPACE="${2-domjudge}"
URL=https://www.domjudge.org/releases/domjudge-${VERSION}.tar.gz
FILE=domjudge.tar.gz
section_end

section_start "Download DOMjudge tarball"
echo "[..] Downloading DOMjudge version ${VERSION}..."

if ! wget --quiet "${URL}" -O ${FILE}
then
echo "[!!] DOMjudge version ${VERSION} file not found on https://www.domjudge.org/releases"
exit 1
fi

echo "[ok] DOMjudge version ${VERSION} downloaded as domjudge.tar.gz"; echo
section_end

Expand Down
Loading