Fix various issues with the "make buildx" target #40089

thaJeztah · 2019-10-14T14:11:08Z

relates to #39340 (and slightly related to #40088)

See individual commit messages for a description 😅

thaJeztah · 2019-10-14T14:11:20Z

thaJeztah · 2019-10-14T16:23:25Z

Hm... interesting; am I dropping an env-var here, causing it to fall back to go env ?

 + make bundles/buildx
 mkdir bundles
 docker build -f ${BUILDX_DOCKERFILE:-Dockerfile.buildx} -t "moby-buildx:${BUILDX_COMMIT:-latest}" \
 	--build-arg BUILDX_COMMIT \
 	--build-arg BUILDX_REPO \
 	--build-arg GOOS=$(if [ -n "" ]; then echo ; else go env GOHOSTOS || uname | awk '{print tolower($0)}' || true; fi) \
 	--build-arg GOARCH=$(if [ -n "" ]; then echo ; else go env GOHOSTARCH || true; fi) \
 	. && \
 	id=$(docker create moby-buildx:${BUILDX_COMMIT:-latest}); \
 	if [ -n "${id}" ]; then docker cp ${id}:/usr/bin/buildx bundles/buildx && touch bundles/buildx; docker rm -f ${id}; fi \
 && bundles/buildx version
 /bin/sh: 4: go: not found
 /bin/sh: 5: go: not found

Makefile

cpuguy83 · 2019-10-14T17:06:43Z

Makefile

@@ -35,6 +35,49 @@ export VALIDATE_REPO
 export VALIDATE_BRANCH
 export VALIDATE_ORIGIN_BRANCH

+ifneq "$(strip $(shell command -v go 2>/dev/null))" ""


This all seems a bit overkill.
What are we trying to solve here?

@cpuguy83 I ran into this problem;

/bin/sh: 4: go: not found /bin/sh: 5: go: not found

Likely because the GOOS and GOARCH make variables were not set

Yeah but doesn't what we already have solve that? And in the case no go the user just needs to set GOOS and GOARCH themselves.

But I just discovered what the actual problem was; we were now actually using buildx v0.3.0, so it tried to pull an image that doesn't exist for s390x/power. The go not found was (I think) just the output leaking from the go env GOHOSTOS || uname line; I'll remove that one commit

Oh, actually.. (sleepy head); it's because s390x/power are now using buildkit to build the image 🤔

thaJeztah · 2019-10-14T17:13:08Z

Yo, Jenkins, where did you go?

edit: aaaaaaand he's back

thaJeztah · 2019-10-14T17:35:30Z

Dockerfile.buildx

+
+FROM golang:${GO_VERSION}-stretch
+COPY --from=build /usr/bin/buildx /usr/bin/buildx


Ah! 💡 I know why it failed in this branch; before this, there was no COPY in the Dockerfile, and because the COPY was added, it started downloading the docker.io/tonistiigi/copy:v0.1.3 image. Apparently, that one is only available for amd64 ?

Yup; that one is only amd64, and armv7

What's the reasoning for doing this second image?
This only produces a runnable image if the specified GOOS/GOARCH == the daemon's GOOS/GOARCH

In effect this is creating a 2nd image and taking even more space since buildx is duplicated.

The first stage won't be tagged, so can be garbage collected, and only the second one is preserved; the final image is roughly 870MB whereas without that stage, we're tagging an image that's 1.71GB; see the commit message 39f1a4a

thaJeztah · 2019-10-14T17:42:58Z

OK, this builds now, but I'll amend one commit message, and re-order commits (enabling BUILDX before the commit that adds the COPY)

kolyshkin · 2019-10-17T20:15:57Z

Dockerfile.buildx

+RUN GOOS="${GOOS}" GOARCH="${GOARCH}" BUILDX_COMMIT="${BUILDX_COMMIT}" \
+ && export version="$(git describe --tags)" \


there should not be && in here as what is before (var assignments) is not a command.

kolyshkin · 2019-10-17T20:18:23Z

Dockerfile.buildx

+RUN GOOS="${GOOS}" GOARCH="${GOARCH}" BUILDX_COMMIT="${BUILDX_COMMIT}" \
+ && export version="$(git describe --tags)" \
+ && export commit="$(git rev-parse --short HEAD)" \


1 export allows for multiple var assignments
2 you don't need to export any variables as you're using them only here ("export" means make them available for child processes, and I don't see how it's necessary here)

Oh I see why you're using export... so you would use the vars later. Well, I still don't like the way it looks :(

Let me try again; I think I tried without export, and for some reason, things failed.

Makefile

kolyshkin · 2019-10-17T20:35:46Z

Makefile

 		--build-arg BUILDX_COMMIT \
 		--build-arg BUILDX_REPO \
 		--build-arg GOOS=$$(if [ -n "$(GOOS)" ]; then echo $(GOOS); else go env GOHOSTOS || uname | awk '{print tolower($$0)}' || true; fi) \
 		--build-arg GOARCH=$$(if [ -n "$(GOARCH)" ]; then echo $(GOARCH); else go env GOHOSTARCH || true; fi) \
 		. && \
-		id=$$(docker create moby-buildx:$(BUILDX_COMMIT)); \
-		if [ -n "$${id}" ]; then docker cp $${id}:/usr/bin/buildx $@ && touch $@; docker rm -f $${id}; fi
+		id=$$(docker create moby-buildx:$${BUILDX_COMMIT:-latest}); \


OK, so using && from makefile is redundant. If any command fails, make stops right there. I guess this practice comes from Dockerfile RUN use, but makefile is different and if we can make things simpler, let's do it.

In other words,

docker build .... id=$$(docker ...); if [ -n "$${id}" ]; .... $@ version

Ok I rewrote it a bit; also put the if over a couple of lines, but let me know if that's too verbose.

cpuguy83 · 2019-10-17T21:42:51Z

Makefile

-buildx: bundles/buildx # build buildx cli tool
-else
-buildx:
+buildx: bundles/buildx ## build buildx cli tool


In #40091 buildx is a required pre-req for all builds.
What happens if we don't define buildx:?

It should be covered by the .PHONY buildx

mkdir foo && cd foo echo ".PHONY: buildx" > Makefile make buildx # make: Nothing to be done for `buildx'. echo $? # 0 make nonexistingtarget # make: *** No rule to make target `nonexistingtarget'. Stop. echo $? # 2

kolyshkin · 2019-10-17T22:50:43Z

Dockerfile.buildx

+    -X \"github.com/docker/buildx/version.Revision=$(git rev-parse --short HEAD)\" \
+    -X \"github.com/docker/buildx/version.Package=github.com/docker/buildx\"" \
+ && go build -ldflags "${ldflags}" -o /usr/bin/buildx ./cmd/buildx


&& looks artificial here as we don't have any command to succeed before it.

I also see a repetition of github.com/docker/buildx string; why don't we do this

pkg="github.com/docker/buildx"; \ ldflags=" \ -X \"${pkg}/version.Version=$(git describe --tags)\" \ -X \"${pkg}/version.Revision=$(git rev-parse --short HEAD)\" \ -X \"${pkg}/version.Package=${pkg}\" \ "; \ go build -ldflags "${ldflags}" -o /usr/bin/buildx ./cmd/buildx

I checked it works in both bash and dash, and even passes shellcheck

&& looks artificial here as we don't have any command to succeed before it.

arf. that's me messing up rebasing

I also see a repetition of github.com/docker/buildx string; why don't we do this

👍 yup, I like that

Dockerfile.buildx

thaJeztah · 2019-10-17T23:22:20Z

Updated with the suggested changes

cpuguy83

LGTM

Dockerfile.buildx

Makefile

thaJeztah · 2019-10-21T18:07:44Z

@kolyshkin updated 👍

kolyshkin · 2019-10-21T19:52:27Z

Dockerfile.buildx

+      -X \"${pkg}/version.Version=$(git describe --tags)\" \
+      -X \"${pkg}/version.Revision=$(git rev-parse --short HEAD)\" \
+      -X \"${pkg}/version.Package=buildx\" \


hmm. shouldn't it be -X \"${pkg}/version.Package=${pkg}\" \?

See #40089 (comment)

This way, it looks like

buildx version buildx v0.3.0 c967f1d

instead of

buildx version github.com/docker/buildx v0.3.0 c967f1d

Commit 1be2cc2 updated the Makefile to force the use of BuildKit, if `USE_BUILDX` was not set. As a side-effect, Jenkins now started using BuildKit on s390x and ppc64le as well, because it overwrote the `DOCKER_BUILDKIT=0` that was set. This commit forces the use of buildx on s390x and ppc64le. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The variables were not substituted, because single-quotes were used. While at it; change the fixed version/commit to use the actual commit and version, using git. before this change: make buildx && ./bundles/buildx version github.com/docker/buildx ${BUILDX_COMMIT} ${BUILDX_COMMIT} after this change: make buildx && ./bundles/buildx version buildx v0.3.0 c967f1d Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The `BUILDX_COMMIT` variable was set as a Make variable, which isn't exported, and thus not available in scripts, unless referenced through `$(VAR)` (non-curly-brackets). As a result `--build-arg BUILDX_COMMIT` did not set the `BUILDX_COMMIT` build-arg, and the default from the Dockerfile (`master`) was used instead. This patch exports the default version that's set in the Makefile, so that it can be used as a regular environment variable. The script was also slighly modified to no longer use the `Make` variable. In addition, the `buildx` target now calls `buildx version`, which is useful to confirm if the binary was successfully built (and with the correct version). Before: rm -f bundles/buildx && make buildx && ./bundles/buildx version # => => naming to docker.io/library/moby-buildx:v0.3.0 github.com/docker/buildx v0.3.1 6db68d0 # using a make variable: rm -f bundles/buildx && make BUILDX_COMMIT=v0.2.1 buildx && ./bundles/buildx version # => => naming to docker.io/library/moby-buildx:v0.2.1 github.com/docker/buildx v0.3.1 6db68d0 # using an environment variable: rm -f bundles/buildx && BUILDX_COMMIT=v0.2.2 make buildx && ./bundles/buildx version # => => naming to docker.io/library/moby-buildx:v0.2.2 github.com/docker/buildx v0.3.1 6db68d0 After: # default rm -f bundles/buildx && make buildx # => => naming to docker.io/library/moby-buildx:v0.3.0 github.com/docker/buildx v0.3.0 c967f1d # using a make variable: rm -f bundles/buildx && make BUILDX_COMMIT=v0.2.1 buildx # => => naming to docker.io/library/moby-buildx:v0.2.1 github.com/docker/buildx v0.2.1 0eb2df5 # using an environment variable: rm -f bundles/buildx && BUILDX_COMMIT=v0.2.2 make buildx # => => naming to docker.io/library/moby-buildx:v0.2.2 github.com/docker/buildx v0.2.2 ab5fe3d Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This patch removes the `BUILDX_COMMIT` make variable. With the make variable removed, it no longer "masks" environment variables, and there is no longer a need to export the variable. A side effect of this change, is that (by default), the buildx image is tagged as `moby-buildx:latest`. This likely isn't a problem, because the build-cache would still be preserved in intermediate images. Having the image tagged as `:latest` also makes cleaning up easier (without having to remove the image for each version tagged. Otherwise, the behavior remains the same as before: # default rm -f bundles/buildx && make buildx # => => naming to docker.io/library/moby-buildx:latest github.com/docker/buildx v0.3.0 c967f1d # using a make variable: rm -f bundles/buildx && make BUILDX_COMMIT=v0.2.1 buildx # => => naming to docker.io/library/moby-buildx:v0.2.1 github.com/docker/buildx v0.2.1 0eb2df5 # using an environment variable: rm -f bundles/buildx && BUILDX_COMMIT=v0.2.2 make buildx # => => naming to docker.io/library/moby-buildx:v0.2.2 github.com/docker/buildx v0.2.2 ab5fe3d Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This simplifies the makefile a bit, while preserving the functionality. Using a non-existing Dockerfile to demonstrate: make buildx Successfully tagged moby-buildx:latest 92059305df7371f8b5b3638d4d405d49ff909031a7bc6d2f515cb0a0df03c2f4 github.com/docker/buildx v0.3.0 c967f1d make BUILDX_DOCKERFILE=foo buildx BUILDX_DOCKERFILE=foo buildx unable to prepare context: unable to evaluate symlinks in Dockerfile path: lstat /Users/sebastiaan/go/src/github.com/docker/docker/foo: no such file or directory Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Un-indent the comment, so that it doesn't get printed by the shell script (moved it above the target, as it looked slightly less cluttered) Also fixed the "help" comment, so that it shows up in `make help`, and removed the un-needed dummy `buildx:` target. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The build-stage would still be in the local cache (and can be cleaned up with `docker system prune`) but the tagged image (which usually wouldn't be removed) will take up less space now. Note that - the binary is not statically linked, so we cannot use a "from scratch" image - in cases where the binary is cross-compiled (e.g. on a non-linux machine), the image itself is not really useful (we may want to consider not tagging the image in that situation) Before: REPOSITORY TAG IMAGE ID CREATED SIZE moby-buildx latest c9b2af465baf 7 minutes ago 1.71GB After: REPOSITORY TAG IMAGE ID CREATED SIZE moby-buildx latest 345501e2df0a 2 minutes ago 820MB Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2019-10-21T22:02:49Z

rebased to kick CI (master was broken because of a linting issue)

kolyshkin

LGTM

thaJeztah · 2019-10-21T23:07:13Z

argh; failing on a flaky test;

 === Failed
[2019-10-21T23:02:53.486Z] === FAIL: amd64.integration-cli TestDockerSwarmSuite/TestAPISwarmNodeDrainPause (72.72s)
[2019-10-21T23:02:53.486Z]     --- FAIL: TestDockerSwarmSuite/TestAPISwarmNodeDrainPause (72.72s)
[2019-10-21T23:02:53.486Z]         daemon.go:26: Creating a new daemon at: "/go/src/github.com/docker/docker/bundles/test-integration/TestDockerSwarmSuite/TestAPISwarmNodeDrainPause"
[2019-10-21T23:02:53.486Z]         daemon.go:26: Creating a new daemon at: "/go/src/github.com/docker/docker/bundles/test-integration/TestDockerSwarmSuite/TestAPISwarmNodeDrainPause"
[2019-10-21T23:02:53.486Z]         check_test.go:358: [d48494135b48e] joining swarm manager [d1822cb98a9bc]@0.0.0.0:2477, swarm listen addr 0.0.0.0:2478
[2019-10-21T23:02:53.486Z]         docker_api_swarm_node_test.go:106: timeout hit after 1m0s: output: "b99585122cdb\ncdbb41970aad\n"
[2019-10-21T23:02:53.486Z]         daemon.go:83: assertion failed: error is not nil: exit status 1

thaJeztah added status/2-code-review area/testing area/project kind/bugfix PR's that fix bugs labels Oct 14, 2019

thaJeztah commented Oct 14, 2019

View reviewed changes

Makefile Show resolved Hide resolved

thaJeztah force-pushed the fix_buildx_target branch from 2ba8476 to 3e455bd Compare October 14, 2019 16:57

cpuguy83 reviewed Oct 14, 2019

View reviewed changes

thaJeztah force-pushed the fix_buildx_target branch from 736c144 to 8f7be89 Compare October 14, 2019 17:11

thaJeztah commented Oct 14, 2019

View reviewed changes

thaJeztah force-pushed the fix_buildx_target branch from 35a4447 to c0d8ebf Compare October 14, 2019 17:47

kolyshkin reviewed Oct 17, 2019

View reviewed changes

Makefile Show resolved Hide resolved

kolyshkin reviewed Oct 17, 2019

View reviewed changes

thaJeztah force-pushed the fix_buildx_target branch from c0d8ebf to 0f01951 Compare October 17, 2019 21:36

cpuguy83 reviewed Oct 17, 2019

View reviewed changes

kolyshkin reviewed Oct 17, 2019

View reviewed changes

Dockerfile.buildx Show resolved Hide resolved

thaJeztah force-pushed the fix_buildx_target branch from 0f01951 to ed69eae Compare October 17, 2019 23:21

cpuguy83 approved these changes Oct 17, 2019

View reviewed changes

kolyshkin reviewed Oct 21, 2019

View reviewed changes

Dockerfile.buildx Outdated Show resolved Hide resolved

kolyshkin reviewed Oct 21, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

thaJeztah force-pushed the fix_buildx_target branch from ed69eae to 182a02e Compare October 21, 2019 18:07

kolyshkin reviewed Oct 21, 2019

View reviewed changes

thaJeztah added 8 commits October 22, 2019 00:01

make buildx: update to buildx v0.3.1

7eb804c

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the fix_buildx_target branch from 182a02e to 7eb804c Compare October 21, 2019 22:02

kolyshkin approved these changes Oct 21, 2019

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Oct 21, 2019

thaJeztah merged commit 684d0ba into moby:master Oct 22, 2019

thaJeztah deleted the fix_buildx_target branch October 22, 2019 00:07

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix various issues with the "make buildx" target #40089

Fix various issues with the "make buildx" target #40089

thaJeztah commented Oct 14, 2019

thaJeztah commented Oct 14, 2019

thaJeztah commented Oct 14, 2019

cpuguy83 Oct 14, 2019

thaJeztah Oct 14, 2019

thaJeztah Oct 14, 2019

cpuguy83 Oct 14, 2019

thaJeztah Oct 14, 2019

thaJeztah Oct 14, 2019

thaJeztah commented Oct 14, 2019 •

edited

thaJeztah Oct 14, 2019

thaJeztah Oct 14, 2019

cpuguy83 Oct 15, 2019

thaJeztah Oct 16, 2019

thaJeztah commented Oct 14, 2019

kolyshkin Oct 17, 2019

kolyshkin Oct 17, 2019

kolyshkin Oct 17, 2019

thaJeztah Oct 17, 2019

kolyshkin Oct 17, 2019

thaJeztah Oct 17, 2019

cpuguy83 Oct 17, 2019

thaJeztah Oct 17, 2019 •

edited

kolyshkin Oct 17, 2019 •

edited

thaJeztah Oct 17, 2019

thaJeztah commented Oct 17, 2019

cpuguy83 left a comment

thaJeztah commented Oct 21, 2019

kolyshkin Oct 21, 2019

thaJeztah Oct 21, 2019

thaJeztah commented Oct 21, 2019

kolyshkin left a comment

thaJeztah commented Oct 21, 2019


		FROM golang:${GO_VERSION}-stretch
		COPY --from=build /usr/bin/buildx /usr/bin/buildx

		RUN GOOS="${GOOS}" GOARCH="${GOARCH}" BUILDX_COMMIT="${BUILDX_COMMIT}" \
		&& export version="$(git describe --tags)" \

Fix various issues with the "make buildx" target #40089

Fix various issues with the "make buildx" target #40089

Conversation

thaJeztah commented Oct 14, 2019

thaJeztah commented Oct 14, 2019

thaJeztah commented Oct 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Oct 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Oct 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah Oct 17, 2019 • edited

Choose a reason for hiding this comment

kolyshkin Oct 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Oct 17, 2019

cpuguy83 left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Oct 21, 2019

kolyshkin left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 21, 2019

thaJeztah commented Oct 14, 2019 •

edited

thaJeztah Oct 17, 2019 •

edited

kolyshkin Oct 17, 2019 •

edited