Skip to content

Conversation

@jgehrcke
Copy link
Collaborator

@jgehrcke jgehrcke commented May 20, 2025

Run Golang build binaries native to the current build platform.

That was already the goal in #323. However, for that patch I erroneously assumed that the default value for --platform is the build platform. It's not, it's the target platform (docs).

Fallout: Golang compilation was very slow when building on amd64 (typical dev platform) for arm64 (typical prod platform), as observed by @klueska (because it was executed in a virtualized environment).

Other notes:

  • Renamed ARCH to TARGETARCH for clarity, renamed native-only.mk to single-arch.mk to better express intent (update: removed that change again, let's discuss separately).
  • At build time we do not need to detect the build architecture with uname but can use BuildKit's BUILDARCH. It uses the same architecture descriptors as seen in Golang tarball URLs.

Tested on my (amd64) laptop by running

$ time ARCH=arm64 make -f deployments/container/Makefile

After caching layers once and then doing a Golang code change this takes about ~45 seconds on my machine, which is just about the same time it takes to do a same-platform build.

@jgehrcke jgehrcke force-pushed the jp/fix-cross-platform-build branch from 442127c to 409d7ee Compare May 20, 2025 12:26

PUSH_ON_BUILD ?= false

# Override TARGETARCH in env to set the target build platform
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will update the comment to say ARCH instead of TARGETARCH

@jgehrcke jgehrcke force-pushed the jp/fix-cross-platform-build branch from 409d7ee to 5292c24 Compare May 20, 2025 12:28
esac; \
wget -nv -O - https://storage.googleapis.com/golang/go${GOLANG_VERSION}.linux-${ARCH}.tar.gz \
# BUILDARCH, TARGETARCH, etc are defined in the global scope by BuiltKit.
# BUILDARCH is reflecting the architecture of the build platform. TARGETARCH is
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
# BUILDARCH is reflecting the architecture of the build platform. TARGETARCH is
# BUILDARCH is the architecture of the build platform. TARGETARCH is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Comment on lines 30 to 37
RUN apt-get update && \
apt-get install -y wget make git gcc-aarch64-linux-gnu gcc && \
rm -rf /var/lib/apt/lists/*
Copy link
Member

Choose a reason for hiding this comment

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

I know that this was just moved, but does it make sense to have one dependency on a line to make future diffs cleaner?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, will do!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


ENV GOPATH=/go
ENV PATH=$GOPATH/bin:/usr/local/go/bin:$PATH

Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason for the removal?

Comment on lines 45 to 46
ARG VERSION="N/A"
ARG GIT_COMMIT="unknown"
Copy link
Member

Choose a reason for hiding this comment

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

VERSION and GIT_COMMIT are used by the make target below to ensure that these are embedded in the executable. Was there a specific reason to remove them?

Copy link
Collaborator Author

@jgehrcke jgehrcke May 20, 2025

Choose a reason for hiding this comment

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

are used by the make target below

Oh. Thank you! Will add them back.

It appeared as if they are only consumed in the other stage for setting image labels (e.g. LABEL version=${VERSION}). And nothing obvious broke. So I wrongly assumed that I can remove those.

Maybe we can make something fail during the build when these are not set.

I justed tested this, and gpu-kubelet-plugin --help shows version and commit information when the GIT_COMMIT and VERSION environment variables are not set during make.

In that case, commit information came from here (we also copy the git repo from the build context, and not only the source):

GIT_COMMIT ?= $(shell git describe --match="" --dirty --long --always --abbrev=40 2> /dev/null || echo "")

We probably want to support a build where no git repository is present and GIT_COMMIT is set externally.

RUN apt-get update && \
apt-get install -y wget make git gcc-aarch64-linux-gnu gcc && \
rm -rf /var/lib/apt/lists/*
RUN wget -nv -O - https://storage.googleapis.com/golang/go${GOLANG_VERSION}.linux-$BUILDARCH.tar.gz \
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
RUN wget -nv -O - https://storage.googleapis.com/golang/go${GOLANG_VERSION}.linux-$BUILDARCH.tar.gz \
RUN wget -nv -O - https://storage.googleapis.com/golang/go${GOLANG_VERSION}.linux-${BUILDARCH}.tar.gz \

Copy link
Collaborator Author

@jgehrcke jgehrcke May 20, 2025

Choose a reason for hiding this comment

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

yes, done!

(at first I thought there's a distinction between env variables and Docker ARGs, but Docker ARGs are just env variables after all)

Copy link
Member

Choose a reason for hiding this comment

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

They are env variables that are ONLY visible at build time!

@jgehrcke jgehrcke force-pushed the jp/fix-cross-platform-build branch 3 times, most recently from fc9c4c1 to 454c79e Compare May 20, 2025 14:52
Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
@jgehrcke jgehrcke force-pushed the jp/fix-cross-platform-build branch from 454c79e to 6b77458 Compare May 20, 2025 15:02

PUSH_ON_BUILD ?= false

# Override ARCH in env to set the target build platform
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

target build platform

surprised about myself here. :)

@jgehrcke jgehrcke merged commit 5a36b01 into NVIDIA:main May 21, 2025
13 checks passed
@klueska klueska added this to the v25.3.0 milestone Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants