Skip to content
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

docker: Multiplatform cross-compilation #3323

Merged
merged 3 commits into from
Sep 12, 2023
Merged

Conversation

codebien
Copy link
Contributor

@codebien codebien commented Sep 6, 2023

What?

Move to use the cross compilation for docker multi-arch builds.

Why?

https://www.docker.com/blog/faster-multi-platform-builds-dockerfile-cross-compilation-guide/

Related PR(s)/Issue(s)

#3301

@codebien codebien added this to the v0.47.0 milestone Sep 6, 2023
@codebien codebien self-assigned this Sep 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01% 🎉

Comparison is base (53b6f22) 73.16% compared to head (4bbdbc2) 73.18%.

❗ Current head 4bbdbc2 differs from pull request most recent head 3eb4e39. Consider uploading reports for the commit 3eb4e39 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3323      +/-   ##
==========================================
+ Coverage   73.16%   73.18%   +0.01%     
==========================================
  Files         258      258              
  Lines       19912    19912              
==========================================
+ Hits        14569    14572       +3     
+ Misses       4418     4416       -2     
+ Partials      925      924       -1     
Flag Coverage Δ
ubuntu 73.10% <ø> (-0.01%) ⬇️
windows 73.03% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codebien codebien force-pushed the docker-buildx-cross-compile branch 2 times, most recently from 6713a15 to bc232b5 Compare September 8, 2023 16:12
Dockerfile Outdated
RUN CGO_ENABLED=0 go install -a -trimpath -ldflags "-s -w -X go.k6.io/k6/lib/consts.VersionDetails=$(date -u +"%FT%T%z")/$(git describe --tags --always --long --dirty)"
RUN --mount=target=. \
--mount=type=cache,target=/root/.cache/go-build \
--mount=type=cache,target=/go/pkg \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
--mount=type=cache,target=/go/pkg \

We don't need it, we already include the vendor folder

@codebien codebien force-pushed the docker-buildx-cross-compile branch from bc232b5 to 6d572b3 Compare September 11, 2023 07:43
@codebien codebien requested review from a team, oleiade, olegbespalov and mstoykov and removed request for a team and oleiade September 11, 2023 08:23
@codebien codebien marked this pull request as ready for review September 11, 2023 08:30
Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I'm OK with merging this even as it is, but if we go with this option, please remove the Makefile target.

Dockerfile Show resolved Hide resolved
Dockerfile Outdated
RUN CGO_ENABLED=0 go install -a -trimpath -ldflags "-s -w -X go.k6.io/k6/lib/consts.VersionDetails=$(date -u +"%FT%T%z")/$(git describe --tags --always --long --dirty)"
RUN --mount=target=.,readonly \
--mount=type=cache,target=/root/.cache/go-build \
CGO_ENABLED=0 GOOS=$TARGETOS GOARCH=$TARGETARCH go build -a -trimpath -o /usr/bin/k6
Copy link
Contributor

Choose a reason for hiding this comment

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

Additionalyl I think -a compeltely negates the fact that we are havging a cache of the builds as -a basically saays to ignore that and rebuild

@codebien codebien force-pushed the docker-buildx-cross-compile branch from 6d572b3 to 2871ff5 Compare September 11, 2023 16:25
olegbespalov
olegbespalov previously approved these changes Sep 11, 2023
mstoykov
mstoykov previously approved these changes Sep 11, 2023
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM! Just one nit

Dockerfile Outdated
RUN apk --no-cache add git=~2
RUN CGO_ENABLED=0 go install -a -trimpath -ldflags "-s -w -X go.k6.io/k6/lib/consts.VersionDetails=$(date -u +"%FT%T%z")/$(git describe --tags --always --long --dirty)"
RUN CGO_ENABLED=0 GOOS=$TARGETOS GOARCH=$TARGETARCH go build -a -trimpath -o /usr/bin/k6
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is there a reason we want the -a? I think it rebuilds the stdlib which seems .. pointless 🤷

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 was commenting the same, but you were faster!!

I researched about it and I didn't find any reason, I guess there was some historical reason for it considering how old this command is.
I'm going to remove it as it isn't needed at the moment.

Base automatically changed from build-docker-arm64 to master September 12, 2023 08:33
@codebien codebien dismissed stale reviews from mstoykov and olegbespalov September 12, 2023 08:33

The base branch was changed.

Removed the -a flag as we don't think it is required. Probably, it had
been set for some historical reason related to the old CI or the old go
toolchain.
At the moment, there isn't a requirement for maintaing it.
@codebien codebien force-pushed the docker-buildx-cross-compile branch from 1231ce1 to 3eb4e39 Compare September 12, 2023 08:38
@codebien codebien requested a review from mstoykov September 12, 2023 08:59
@codebien codebien merged commit 4147538 into master Sep 12, 2023
@codebien codebien deleted the docker-buildx-cross-compile branch September 12, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants