mac: build arm64 binaries #540

tiborvass · 2021-04-05T23:02:59Z

Signed-off-by: Tibor Vass tibor@docker.com

This PR uses golang 1.16 In order to build plugins for darwin/arm64.
However, docker app plugin does not build with go 1.16 (could not figure out how to disable go modules), and it was meant to be removed anyway.
ARM binaries had issues building with cgo, so as a workaround, this PR sets CGO_ENABLED=0 for arm (32 bit, aka armel/armhf) binaries only.

thaJeztah · 2021-04-06T09:31:41Z

deb/common/rules

 	cd engine && TMP_GOPATH="/go" hack/dockerfile/install/install.sh rootlesskit dynamic
 	# Build the CLI
-	cd /go/src/github.com/docker/cli && \
-		LDFLAGS='' DISABLE_WARN_OUTSIDE_CONTAINER=1 make VERSION=$(VERSION) GITCOMMIT=$(CLI_GITCOMMIT) dynbinary manpages


Why did you remove the LDFLAGS='' here?

(perhaps we need the same comment as in the plugins install script?

I didn't think enough, and had too much confidence in Tonis's scripts :) I put it back.

thaJeztah · 2021-04-06T12:08:11Z

deb/common/rules

 	# Build the CLI
-	cd /go/src/github.com/docker/cli && \
-		LDFLAGS='' DISABLE_WARN_OUTSIDE_CONTAINER=1 make VERSION=$(VERSION) GITCOMMIT=$(CLI_GITCOMMIT) dynbinary manpages
+	cd /go/src/github.com/docker/cli && VERSION=$(VERSION) GITCOMMIT=$(CLI_GITCOMMIT) LDFLAGS='' GO_LINKMODE=dynamic ./scripts/build/binary && DISABLE_WARN_OUTSIDE_CONTAINER=1 make manpages


Suggested change

cd /go/src/github.com/docker/cli && VERSION=$(VERSION) GITCOMMIT=$(CLI_GITCOMMIT) LDFLAGS='' GO_LINKMODE=dynamic ./scripts/build/binary && DISABLE_WARN_OUTSIDE_CONTAINER=1 make manpages

cd /go/src/github.com/docker/cli && VERSION=$(VERSION) GITCOMMIT=$(CLI_GITCOMMIT) LDFLAGS='' GO_LINKMODE=dynamic ./scripts/build/binary && LDFLAGS='' DISABLE_WARN_OUTSIDE_CONTAINER=1 make manpages

Need to set it for both ./scripts/build/binary and for make manpages (we don't export LDFLAGS, so it's not set after && otherwise)

tonistiigi · 2021-04-07T03:38:13Z

static/Makefile

-	cp $(CLI_DIR)/build/docker-darwin-amd64 build/mac/docker/docker
-	tar -C build/mac -c -z -f build/mac/docker-$(GEN_STATIC_VER).tgz docker
+cross-mac: buildx cross-mac-plugins
+	cd $(CLI_DIR) && VERSION=$(GEN_STATIC_VER) docker buildx bake --set cross.platform=darwin/amd64,darwin/arm64 cross


why are you building cross?

thaJeztah

LGTM after fixing the windows stage

Signed-off-by: Tibor Vass <tibor@docker.com>

tonistiigi · 2021-04-08T19:08:54Z

Makefile

 static: checkout ## build static-compiled packages
 	for p in $(DOCKER_BUILD_PKGS); do \
-		$(MAKE) -C $@ VERSION=$(VERSION) GO_VERSION=$(GO_VERSION) $${p}; \
+		$(MAKE) -C $@ VERSION=$(VERSION) GO_VERSION=$(GO_VERSION) GOPLATFORM=$(GOPLATFORM) $${p}; \


why is this called GOPLATFORM?

i can call it PLATFORM if you want but in this repo it's confusing as it can also mean the stupid "Docker CE" PLATFORM string. Happy to change the name.

It's not GOARCH?

Could be GO_PLATFORM to be consistent with GO_VERSION

tonistiigi · 2021-04-08T19:19:42Z

static/Makefile

 .PHONY: static-cli
 static-cli:
-	$(MAKE) -C $(CLI_DIR) -f docker.Makefile VERSION=$(GEN_STATIC_VER) build
+	cd $(CLI_DIR) && VERSION=$(GEN_STATIC_VER) docker buildx bake --set binary.platform=$(GOPLATFORM) $(if $(findstring arm/,$(GOPLATFORM)),--set binary.args.CGO_ENABLED=0) binary


Why don't you just pass in CGO_ENABLED? Afaics it is not that arm binaries can't use cgo but that arm call happens on some specific environment/jenkins node set from the caller side that causes it. If this would be arm specific we would disable cgo in docker/cli for that case(as we disable pie for some arch) but don't think but I don't think there is any proof of that atm.

Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass · 2021-04-09T19:27:48Z

@tonistiigi fixed your concerns.

tiborvass · 2021-04-09T19:28:19Z

@tonistiigi arm workaround moved to https://github.com/docker/release-packaging/pull/574/files#diff-e6ffa5dc854b843b3ee3c3c28f8eae2f436c2df2b1ca299cca1fa5982e390cf8R226

Since we are building arm on arm64 machines we have to specify the desired platform and not rely on the host's architecture. Also when building arm on arm64 machines, there can be issues with cgo. So this patch makes sure CGO_ENABLED env var gets passed on as a build arg. Signed-off-by: Tibor Vass <tibor@docker.com>

Signed-off-by: Tibor Vass <tibor@docker.com>

In docker/cli#2993, Makefile target dynbinary changed from a host script not depending on docker to a dockerized script. Instead call the underlying script directly for deb/rpm. I still think we should build deb/rpms using docker. Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass · 2021-04-09T20:16:36Z

Ok this is green from https://ci-next.docker.com/teams-core/blue/organizations/jenkins/release-packaging/detail/PR-574/68/pipeline/

thaJeztah

LGTM

tiborvass force-pushed the darwin_arm64 branch 7 times, most recently from c31456e to 9e8e620 Compare April 6, 2021 09:19

thaJeztah reviewed Apr 6, 2021

View reviewed changes

tiborvass force-pushed the darwin_arm64 branch from 9e8e620 to 393c2f6 Compare April 6, 2021 09:35

thaJeztah requested changes Apr 6, 2021

View reviewed changes

tiborvass force-pushed the darwin_arm64 branch 8 times, most recently from ab9e47a to 4d88967 Compare April 7, 2021 03:30

tonistiigi reviewed Apr 7, 2021

View reviewed changes

tiborvass force-pushed the darwin_arm64 branch from 4d88967 to fca7039 Compare April 7, 2021 21:26

thaJeztah previously approved these changes Apr 8, 2021

View reviewed changes

tiborvass dismissed thaJeztah’s stale review via b620ba5 April 8, 2021 17:18

tiborvass force-pushed the darwin_arm64 branch from fca7039 to b620ba5 Compare April 8, 2021 17:18

Tibor Vass added 2 commits April 8, 2021 18:41

mac: build arm64 binaries

eb10cbb

Signed-off-by: Tibor Vass <tibor@docker.com>

plugins: support multiple architectures

7eb0491

Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the darwin_arm64 branch 2 times, most recently from 6866c36 to 85c07f0 Compare April 8, 2021 19:04

tonistiigi reviewed Apr 8, 2021

View reviewed changes

tiborvass force-pushed the darwin_arm64 branch 2 times, most recently from 88eb12b to 6fa4429 Compare April 8, 2021 20:12

tiborvass marked this pull request as ready for review April 9, 2021 17:12

plugins: update golang to 1.16.3 to build buildx on darwin/arm64

60e28c9

Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the darwin_arm64 branch 2 times, most recently from c3d6ef8 to eb9402b Compare April 9, 2021 19:15

Tibor Vass added 3 commits April 9, 2021 19:44

win: add arch in path for consistency with mac paths

06c788d

Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the darwin_arm64 branch from eb9402b to 5d52107 Compare April 9, 2021 19:46

tiborvass mentioned this pull request Apr 9, 2021

[20.10] mac: build arm64 binaries #541

Merged

tonistiigi approved these changes Apr 9, 2021

View reviewed changes

tiborvass merged commit b392198 into docker:master Apr 9, 2021

thaJeztah approved these changes Apr 9, 2021

View reviewed changes

	cd /go/src/github.com/docker/cli && VERSION=$(VERSION) GITCOMMIT=$(CLI_GITCOMMIT) LDFLAGS='' GO_LINKMODE=dynamic ./scripts/build/binary && DISABLE_WARN_OUTSIDE_CONTAINER=1 make manpages
	cd /go/src/github.com/docker/cli && VERSION=$(VERSION) GITCOMMIT=$(CLI_GITCOMMIT) LDFLAGS='' GO_LINKMODE=dynamic ./scripts/build/binary && LDFLAGS='' DISABLE_WARN_OUTSIDE_CONTAINER=1 make manpages

mac: build arm64 binaries #540

mac: build arm64 binaries #540

Uh oh!

Conversation

tiborvass commented Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Apr 9, 2021

Uh oh!

tiborvass commented Apr 9, 2021

Uh oh!

tiborvass commented Apr 9, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tiborvass commented Apr 5, 2021 •

edited

Loading