Refactor docker stuff

[LukeShu]

From Slack. Pasting it here so I don't lose it.

---

The docker-build script I [@lukeshu] sent you [@rhs] last week, I didn't like it because its form-factor felt funny; too many arguments. Re-thinking it, I think a good fit would be for it to behave exactly like docker build, but with an additional --output flag, and removing the -t/--tag flag in favor of --tag.name and --tag.version. It would be used like:

foo.docker: foo/Dockerfile foo/a foo/b foo/c
    docker-buildfile --output=$@ --tag.name='$(docker.LOCALHOST):31000/foo' --tag.version=$(VERSION) $(<D)
foo.docker.clean:
    rm -f foo/a foo/b foo/c
foo/a: bin/a
    cp $< $@
foo/b: bin/b
    cp $< $@
foo/c: bin/c
    cp $< $@

You suggesting having it copy in dependencies is a good suggestion, but it makes the form-factor funny. I think having a 2nd utility that composes well with it could solve that.

foo.docker: foo/Dockerfile bin/a bin/b bin/c
    with-dir-of-files $^ -- docker-buildfile --output=$@ --tag.name='$(docker.LOCALHOST):31000/foo' --tag.version=$(VERSION)

where with-dir-of-files is something like

#!/usr/bin/env bash
tmpdir=$(mktemp -d)
files=()
while (( $# > 0 )); do
    if [[ $1 == '--' ]]; then
        shift
        break
    else
        files+=("$1")
        shift
    fi
done
cp -t "$tmpdir" "$1"
"$@" "$tmpdir"
r=$?
rm -rf "$tmpdir"
exit $r

but with error handling

[LukeShu]

For completeness, the docker-build script I sent was

diff --git a/build-aux/docker-build b/build-aux/docker-build
new file mode 100644
index 00000000..53e12b4b
--- /dev/null
+++ b/build-aux/docker-build
@@ -0,0 +1,73 @@
+#!/usr/bin/env bash
+# Copyright 2019 Datawire
+set -euE
+
+# Usage: docker-build DIR NAME [VERSION]
+main() {
+   arg_dir=$1
+   arg_name=$2
+   arg_version=${3:-latest}
+
+   # Decide what to name the image
+   tags=("$arg_name:$arg_version")
+   # Build the image
+   docker build -t "${tags[0]}" "$arg_dir"
+   # Also decide on some extra tags for the image.  Don't
+   # actually tag these yet, we'll do that below; just decide
+   # what the tag names will be for now.
+   id=$(docker image inspect "${tags[0]}" --format='{{.Id}}')
+   tags+=("$arg_name:id-${id//:/-}")
+   if [[ $arg_version != latest ]]; then
+       tags+=("$arg_name:latest")
+   fi
+
+   # Write all known names for the image to a file.  The order of
+   # things in that file is important, as rules in docker.mk pick
+   # things out of it:
+   #
+   #  line 1: local tag name (version-based)
+   #  line 2: image hash
+   #  line 3: local tag name (hash-based)
+   #  line 4: local tag name (latest, optional)
+   #
+   # Do this in a 2-phase write-then-rename, for several reasons
+   # you'll discover below.
+   outfile="${arg_dir}.docker"
+   tmpfile="$(dirname "$outfile")/.tmp.${outfile##*/}.tmp"
+   printf '%s\n' "${tags[0]}" "$id" "${tags[@]:1}" > "$tmpfile"
+
+   # Reason 1: Avoid bumping the timestamp if the contents of the file
+   # didn't change.
+   if cmp -s "$tmpfile" "$outfile"; then
+       rm -f "$tmpfile"
+       return 0
+   fi
+
+   # Reason 2: Detecting when builds in CI are non-convergent.
+   if [[ -n "$CI" && -e "$outfile" ]]; then
+       # We have to do this _before_ do this tagging the new
+       # tag names, otherwise we lose important information
+       # on debugging the issue.
+       echo "error: This should not happen in CI: $outfile should not change" >&2
+       exit 1
+   fi
+       
+   # Create the tags we decided on earlier
+   for tag in "${tags[@]:1}"; do
+       docker tag "$id" "$tag"
+   done
+
+   # Reason 3: So that we can prune images/tags that are only in
+   # the old version of the file, to avoid leaking orphaned
+   # images.
+   if [[ -e "$outfile" ]]; then
+       # We have to do this _after_ updating any tag names
+       # that are the same (eg ":latest").
+       docker image rm $(grep -vFx -f "$tmpfile" "$outfile") || true
+   fi
+
+   # Finally, move the file in to the final location.
+   mv -f "$tmpfile" "$outfile"
+}
+
+main "$@"
diff --git a/build-aux/docker.mk b/build-aux/docker.mk
index 8d0133c4..24236419 100644
--- a/build-aux/docker.mk
+++ b/build-aux/docker.mk
@@ -112,28 +112,8 @@ _docker.port-forward = $(dir $(_docker.mk))docker-port-forward
 #  line 2: image hash
 #  line 3: local tag name (hash-based)
 #  line 4: local tag name (latest, optional)
-#
-# Note: We test for changes for CI early, but test for changes for
-# cleanup late.  If we did the cleanup test early because of :latest,
-# it would leave dangling untagged images.  If we did the CI test
-# late, it would remove the evidence for debugging.
 %.docker: %/Dockerfile
-   printf '%s\n' '$(docker.LOCALHOST):31000/$(notdir $*):$(or $(VERSION),latest)' > $(@D)/.tmp.$(@F).tmp
-   docker build -t "$$(sed -n 1p $(@D)/.tmp.$(@F).tmp)" $*
-   docker image inspect "$$(sed -n 1p $(@D)/.tmp.$(@F).tmp)" --format='{{.Id}}' >> $(@D)/.tmp.$(@F).tmp
-   printf '%s\n' '$(docker.LOCALHOST):31000/$(notdir $*)':"id-$$(sed -n '2{ s/:/-/g; p; }' $(@D)/.tmp.$(@F).tmp)" $(if $(VERSION),'$(docker.LOCALHOST):31000/$(notdir $*):latest') >> $(@D)/.tmp.$(@F).tmp
-   @{ \
-       PS4=''; set -x; \
-       if cmp -s $(@D)/.tmp.$(@F).tmp $@; then \
-           rm -f $(@D)/.tmp.$(@F).tmp || true; \
-       else \
-           $(if $(CI),if test -e $@; then false This should not happen in CI: $@ should not change; fi &&) \
-                           docker tag "$$(sed -n 2p $(@D)/.tmp.$(@F).tmp)" "$$(sed -n 3p $(@D)/.tmp.$(@F).tmp)" && \
-           $(if $(VERSION),docker tag "$$(sed -n 2p $(@D)/.tmp.$(@F).tmp)" "$$(sed -n 4p $(@D)/.tmp.$(@F).tmp)" &&) \
-           if test -e $@; then docker image rm $$(grep -vFx -f $(@D)/.tmp.$(@F).tmp $@) || true; fi && \
-           mv -f $(@D)/.tmp.$(@F).tmp $@; \
-       fi; \
-   }
+   $(dir $(_docker.mk))docker-build '$*' '$(docker.LOCALHOST):31000/$(notdir $*)' $(VERSION)

 %.docker.clean:
    if [ -e $*.docker ]; then docker image rm $$(cat $*.docker) || true; fi

I think, to get arg-parsing the same as docker build, that it makes sens to write the docker-build program in Go; something that is easier now with #7.

[LukeShu]

foo.docker: foo/Dockerfile bin/a bin/b bin/c
  with-dir-of-files $^ -- docker-buildfile --output=$@ --tag.name='$(docker.LOCALHOST):31000/foo' --tag.version=$(VERSION)

Or, a templated version of that:

docker.mk:

%.docker: %/Dockerfile
    with-dir-of-files $^ -- docker-buildfile --output=$@ --tag.name='$(docker.LOCALHOST):31000/$(notdir $*)' $(if $(VERSION),--tag.version=$(VERSION))

Makefile:

foo.docker: bin/a bin/b bin/c

[LukeShu]

Repeating a conversation that I don't remember if was in-person or on Slack:

I've come around to thinking that --iidfile combined with $(COPY_IFCHANGED) script is good enough for the build, and that wrapping docker build is unnecessary complexity.

[LukeShu]

Implemented.