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

feat: add dockerfile cross-compilation for multi-platform images #1018

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

salasberryfin
Copy link
Contributor

What this PR does / why we need it:

Use cross-compilation when building the image with buildx. This means that the process does not need to use emulation for the whole build which makes it considerably faster.

We can use the existing build arguments (which are globally available):

  • BUILDPLATFORM: make the image always native to the machine.
  • TARGETOS and TARGETARCH: configure the compiler process for the specific architecture.

Which issue(s) this PR fixes:
Fixes #933

Special notes for your reviewer:

I ran a quick test and image building is a few minutes faster after this change.

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@salasberryfin salasberryfin added kind/enhancement Categorizes issue or PR as related to a new feature. area/build-and-release Indicates issue or PR related to build or release labels Jan 20, 2025
@salasberryfin salasberryfin requested a review from a team as a code owner January 20, 2025 17:05
Copy link
Contributor

@yiannistri yiannistri left a comment

Choose a reason for hiding this comment

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

For reference, here is the list of multi-platform build arguments. The release GHA is calling make docker-build-and-push and uses the $(TARGET_PLATFORMS) variable which is set to linux/amd64,linux/arm64.

I need to test how Tilt behaves with this change.

@alexander-demicev alexander-demicev added area/build-and-release Indicates issue or PR related to build or release and removed area/build-and-release Indicates issue or PR related to build or release labels Jan 21, 2025
Copy link
Contributor

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

Have not run it locally yet, some comments/questions inline:

Dockerfile Outdated Show resolved Hide resolved
@@ -17,7 +17,8 @@
# Build the manager binary
ARG builder_image

FROM ${builder_image} as builder
#FROM ${builder_image} as builder
FROM --platform=$BUILDPLATFORM ${builder_image} as builder
Copy link
Contributor

Choose a reason for hiding this comment

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

where is BUILDPLATFORM arg defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BUILDPLATFORM is one of the pre-defined build arguments that is available to any build: https://docs.docker.com/build/building/variables/#multi-platform-build-arguments

Copy link
Contributor

@yiannistri yiannistri left a comment

Choose a reason for hiding this comment

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

For some reason, Tilt no longer picks up changes automatically. In my test, a change in a log statement was not reflected in Tilt logs, even though it appeared to re-build the binary.

@salasberryfin salasberryfin force-pushed the docker-faster-image-building branch from 011bb04 to fbf4894 Compare January 21, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build-and-release Indicates issue or PR related to build or release kind/enhancement Categorizes issue or PR as related to a new feature.
Projects
Development

Successfully merging this pull request may close these issues.

Investigate cross compilation
4 participants