Skip to content

chore: parameterize BUILD_ARCH/BUILD_PLATFORM in Makefiles #2257

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.

Sign up for GitHub

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

Merged
jakubno merged 3 commits into from
Mar 30, 2026
+32 −11
Merged

chore: parameterize BUILD_ARCH/BUILD_PLATFORM in Makefiles #2257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff8ed13
chore: parameterize BUILD_ARCH/BUILD_PLATFORM in Makefiles
Mar 29, 2026
d5173be
fix: remove unused BUILD_ARCH from api Makefile, fix multi-arch comments
Mar 29, 2026
7318e8f
fix: use BUILD_PLATFORM in test-docker echo message
Mar 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/client-proxy/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,13 @@ PREFIX := $(strip $(subst ",,$(PREFIX)))
HOSTNAME := $(shell hostname 2> /dev/null || hostnamectl hostname 2> /dev/null)
$(if $(HOSTNAME),,$(error Failed to determine hostname: both 'hostname' and 'hostnamectl' failed))

# Architecture for builds. Defaults to local arch; override for cross-compilation
# (e.g., BUILD_ARCH=amd64 make build-and-upload from an ARM64 host).
BUILD_ARCH ?= $(shell go env GOARCH)
# Docker platform string. Override for multi-arch builds:
# BUILD_PLATFORM=linux/amd64,linux/arm64 make build-and-upload
BUILD_PLATFORM ?= linux/$(BUILD_ARCH)

ifeq ($(PROVIDER),aws)
IMAGE_REGISTRY := $(AWS_ACCOUNT_ID).dkr.ecr.$(AWS_REGION).amazonaws.com/$(PREFIX)core/client-proxy
else
Expand All @@ -16,7 +23,7 @@ endif
build:
# Allow for passing commit sha directly for docker builds
$(eval COMMIT_SHA ?= $(shell git rev-parse --short HEAD))
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o bin/client-proxy -ldflags "-X=main.commitSHA=$(COMMIT_SHA)" .
CGO_ENABLED=0 GOOS=linux GOARCH=$(BUILD_ARCH) go build -o bin/client-proxy -ldflags "-X=main.commitSHA=$(COMMIT_SHA)" .

.PHONY: build-debug
build-debug:
Expand All @@ -26,7 +33,7 @@ build-debug:
.PHONY: build-and-upload
build-and-upload:
$(eval COMMIT_SHA := $(shell git rev-parse --short HEAD))
@docker buildx build --platform linux/amd64 --tag $(IMAGE_REGISTRY) --push --build-arg COMMIT_SHA="$(COMMIT_SHA)" -f ./Dockerfile ..
@docker buildx build --platform $(BUILD_PLATFORM) --tag $(IMAGE_REGISTRY) --push --build-arg COMMIT_SHA="$(COMMIT_SHA)" -f ./Dockerfile ..

.PHONY: run
run:
Expand Down
13 changes: 10 additions & 3 deletions packages/envd/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ LDFLAGS=-ldflags "-X=main.commitSHA=$(BUILD)"
AWS_BUCKET_PREFIX ?= $(PREFIX)$(AWS_ACCOUNT_ID)-
GCP_BUCKET_PREFIX ?= $(GCP_PROJECT_ID)-

# Architecture for builds. Defaults to local arch; override for cross-compilation
# (e.g., BUILD_ARCH=amd64 make build from an ARM64 host).
BUILD_ARCH ?= $(shell go env GOARCH)
# Docker platform string. Override for cross-platform builds:
# BUILD_PLATFORM=linux/arm64 make start-docker
BUILD_PLATFORM ?= linux/$(BUILD_ARCH)

.PHONY: init
init:
brew install protobuf
Expand All @@ -20,17 +27,17 @@ else
endif

build:
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o bin/envd ${LDFLAGS}
CGO_ENABLED=0 GOOS=linux GOARCH=$(BUILD_ARCH) go build -a -o bin/envd ${LDFLAGS}

build-debug:
CGO_ENABLED=1 go build -race -gcflags=all="-N -l" -o bin/debug/envd ${LDFLAGS}

start-docker:
make build
DOCKER_BUILDKIT=1 docker build --platform linux/amd64 -t envd-debug . -f debug.Dockerfile
DOCKER_BUILDKIT=1 docker build --platform $(BUILD_PLATFORM) -t envd-debug . -f debug.Dockerfile
docker run \
--name envd \
--platform linux/amd64 \
--platform $(BUILD_PLATFORM) \
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 29, 2026

Choose a reason for hiding this comment

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

P2 Badge Keep BUILD_PLATFORM single-valued for start-docker

start-docker now passes $(BUILD_PLATFORM) to docker run --platform, but this Makefile also documents BUILD_PLATFORM=linux/amd64,linux/arm64 as a supported override. docker run --platform expects one platform value, so a comma-separated list will make start-docker fail before envd starts. This regression is specific to users following the new multi-arch override pattern.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

@tomassrnka tomassrnka Mar 30, 2026

Choose a reason for hiding this comment

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

Fixed — comment updated to single-arch example.

Copy link
Copy Markdown

@cursor cursor bot Mar 29, 2026

Choose a reason for hiding this comment

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

Envd start-docker mixes local binary arch with Docker platform

Medium Severity

The start-docker target calls make build (which uses GOARCH=$(BUILD_ARCH)) then docker build/run --platform $(BUILD_PLATFORM). Since debug.Dockerfile copies the locally-built binary (COPY bin/envd), overriding only BUILD_PLATFORM (as the comment on line 14 documents: BUILD_PLATFORM=linux/arm64 make start-docker) creates an architecture mismatch — the Go binary is built for the host arch while the container expects the overridden platform. This causes an exec format error at runtime.

Additional Locations (1)
Fix in Cursor Fix in Web

Copy link
Copy Markdown
Member Author

@tomassrnka tomassrnka Mar 30, 2026

Choose a reason for hiding this comment

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

Won't fix — BUILD_ARCH and BUILD_PLATFORM both default to host arch via go env GOARCH. They only diverge if a user overrides one but not the other, which is a user error. Adding coupling would add complexity for no practical benefit.

-p 49983:49983 \
-p 2345:2345 \
-p 9999:9999 \
Comment on lines 35 to 43
Copy link
Copy Markdown

@claude claude bot Mar 29, 2026

Choose a reason for hiding this comment

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

🔴 The comment for BUILD_PLATFORM in packages/envd/Makefile explicitly documents BUILD_PLATFORM=linux/amd64,linux/arm64 make start-docker as a valid example, but the start-docker target passes $(BUILD_PLATFORM) to docker run --platform, which only accepts a single platform string and will fail with a comma-separated value. The comment should either reference a build-only target or the start-docker target should use a separate single-platform variable for the docker run step.

Extended reasoning...

What the bug is and how it manifests

The BUILD_PLATFORM variable comment in packages/envd/Makefile explicitly documents BUILD_PLATFORM=linux/amd64,linux/arm64 make start-docker as a usage example (lines 12-13). The start-docker target then passes this variable to both docker build --platform $(BUILD_PLATFORM) and docker run --platform $(BUILD_PLATFORM). While docker build (via BuildKit) can accept a comma-separated multi-arch platform list, docker run --platform strictly requires a single platform string.

The specific code path that triggers it

When a user runs BUILD_PLATFORM=linux/amd64,linux/arm64 make start-docker, execution proceeds as follows:

  1. make build — succeeds (plain Go cross-compilation, unaffected by BUILD_PLATFORM)
  2. DOCKER_BUILDKIT=1 docker build --platform linux/amd64,linux/arm64 ... — succeeds, creates a multi-arch manifest
  3. docker run --platform linux/amd64,linux/arm64 ... — fails with an error such as invalid platform linux/amd64,linux/arm64

Why existing code doesn't prevent it

Make has no mechanism to validate that a variable is appropriate for a specific Docker subcommand. The same $(BUILD_PLATFORM) token is interpolated into both the build and run invocations without any guard or split.

What the impact would be

Any developer following the documented example for start-docker will encounter a hard failure at the docker run step. Even if they work around the comma-separated format by passing a single arch (the default behavior), building a multi-arch manifest and then attempting to run it locally is not a meaningful workflow—multi-arch manifests need to be pushed to a registry first. The comment actively misleads users toward a broken invocation.

How to fix it

Two options:

  1. Change the comment to reference only make build (or a push/upload target) instead of make start-docker, since multi-arch builds are only useful when pushing to a registry.
  2. Split the variable in start-docker: use BUILD_PLATFORM for the docker build step and derive a single-arch run platform from BUILD_ARCH (e.g., --platform linux/$(BUILD_ARCH)) for docker run.

Step-by-step proof

  1. User reads the comment: # BUILD_PLATFORM=linux/amd64,linux/arm64 make start-docker
  2. User runs: BUILD_PLATFORM=linux/amd64,linux/arm64 make start-docker
  3. Make expands start-docker → runs docker run --platform linux/amd64,linux/arm64 ...
  4. Docker rejects the value: docker: invalid platform linux/amd64,linux/arm64: unknown os or arch or variant
  5. The container never starts; the command fails.

Copy link
Copy Markdown
Member Author

@tomassrnka tomassrnka Mar 30, 2026

Choose a reason for hiding this comment

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

Fixed — comment now shows single-arch example only.

Expand Down
19 changes: 13 additions & 6 deletions packages/orchestrator/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ GCP_BUCKET_PREFIX ?= $(GCP_PROJECT_ID)-
HOSTNAME := $(shell hostname 2> /dev/null || hostnamectl hostname 2> /dev/null)
$(if $(HOSTNAME),,$(error Failed to determine hostname: both 'hostname' and 'hostnamectl' failed))

# Architecture for builds. Defaults to local arch; override for cross-compilation
# (e.g., BUILD_ARCH=amd64 make build from an ARM64 host).
BUILD_ARCH ?= $(shell go env GOARCH)
# Docker platform string. Override for cross-platform builds:
# BUILD_PLATFORM=linux/arm64 make build
BUILD_PLATFORM ?= linux/$(BUILD_ARCH)

.PHONY: init
init:
brew install protobuf
Expand All @@ -18,18 +25,18 @@ generate:
.PHONY: build
build:
$(eval COMMIT_SHA := $(shell git rev-parse --short HEAD))
@docker build --platform linux/amd64 --output=bin --build-arg COMMIT_SHA="$(COMMIT_SHA)" -f ./Dockerfile ..
@docker build --platform $(BUILD_PLATFORM) --output=bin --build-arg COMMIT_SHA="$(COMMIT_SHA)" -f ./Dockerfile ..

.PHONY: build-local
build-local:
# Allow for passing commit sha directly for docker builds
$(eval COMMIT_SHA ?= $(shell git rev-parse --short HEAD))
CGO_ENABLED=1 GOOS=linux GOARCH=amd64 go build -o bin/orchestrator -ldflags "-X=main.commitSHA=$(COMMIT_SHA)" .
CGO_ENABLED=1 GOOS=linux GOARCH=amd64 go build -o bin/clean-nfs-cache -ldflags "-X=main.commitSHA=$(COMMIT_SHA)" ./cmd/clean-nfs-cache
CGO_ENABLED=1 GOOS=linux GOARCH=$(BUILD_ARCH) go build -o bin/orchestrator -ldflags "-X=main.commitSHA=$(COMMIT_SHA)" .
CGO_ENABLED=1 GOOS=linux GOARCH=$(BUILD_ARCH) go build -o bin/clean-nfs-cache -ldflags "-X=main.commitSHA=$(COMMIT_SHA)" ./cmd/clean-nfs-cache

.PHONY: build-debug
build-debug:
CGO_ENABLED=1 GOOS=linux GOARCH=amd64 go build -race -gcflags=all="-N -l" -o bin/orchestrator .
CGO_ENABLED=1 GOOS=linux GOARCH=$(BUILD_ARCH) go build -race -gcflags=all="-N -l" -o bin/orchestrator .

.PHONY: run-debug
run-debug:
Expand Down Expand Up @@ -100,12 +107,12 @@ test:

.PHONY: test-docker
test-docker:
@echo "Running orchestrator tests in Docker (AMD64 Linux)..."
@echo "Running orchestrator tests in Docker ($(BUILD_PLATFORM))..."
@rm -rf .shared/
@cp -r ../shared .shared/
@rm -rf .clickhouse/
@cp -r ../clickhouse .clickhouse/
@docker build --platform linux/amd64 -f test.Dockerfile --no-cache-filter runner --progress=plain -t orchestrator-test .
@docker build --platform $(BUILD_PLATFORM) -f test.Dockerfile --no-cache-filter runner --progress=plain -t orchestrator-test .
@rm -rf .shared/
@rm -rf .clickhouse/
@echo "Done"
Comment on lines 112 to 118
Copy link
Copy Markdown

@claude claude bot Mar 29, 2026

Choose a reason for hiding this comment

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

🟡 The test-docker target still echoes Running orchestrator tests in Docker (AMD64 Linux)... even though the platform is now controlled by $(BUILD_PLATFORM). When a user overrides the platform (e.g., BUILD_PLATFORM=linux/arm64 make test-docker), the log output will incorrectly claim AMD64 Linux. Update the echo to reference $(BUILD_PLATFORM) so it reflects the actual platform being used.

Extended reasoning...

What the bug is and how it manifests

In packages/orchestrator/Makefile, the test-docker target contains two lines that are now inconsistent after this PR's parameterization refactor. Line 110 (the echo) still hardcodes "AMD64 Linux", while line 115 (the actual docker build command) correctly uses $(BUILD_PLATFORM):

@echo "Running orchestrator tests in Docker (AMD64 Linux)..."
...
@docker build --platform $(BUILD_PLATFORM) -f test.Dockerfile ...

The specific code path that triggers it

Any invocation of make test-docker where BUILD_PLATFORM is set to a non-amd64 value will trigger the mismatch. For example: BUILD_PLATFORM=linux/arm64 make test-docker will print "Running orchestrator tests in Docker (AMD64 Linux)..." while Docker actually builds for linux/arm64.

Why existing code doesn't prevent it

The PR correctly updated every functional reference from linux/amd64 to $(BUILD_PLATFORM) but missed updating this informational echo statement. There is no mechanism in make to enforce consistency between echo strings and actual command flags.

What the impact would be

The impact is cosmetic/informational only — the actual docker build uses the correct $(BUILD_PLATFORM) variable, so builds are functionally correct. However, developers relying on the log output to confirm which platform is being targeted will be misled when using non-default platforms, potentially causing confusion during debugging or CI output review.

How to fix it

Change the echo to dynamically include the platform value:

@echo "Running orchestrator tests in Docker ($(BUILD_PLATFORM))..."

Step-by-step proof

  1. Default case (amd64 host): make test-docker → BUILD_PLATFORM resolves to linux/amd64 → echo says "AMD64 Linux" → happens to be accurate.
  2. Non-default case: BUILD_PLATFORM=linux/arm64 make test-docker → echo still prints "Running orchestrator tests in Docker (AMD64 Linux)..." → docker actually runs --platform linux/arm64 → the log output contradicts the actual behavior, misleading the developer.

Copy link
Copy Markdown
Member Author

@tomassrnka tomassrnka Mar 30, 2026

Choose a reason for hiding this comment

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

Fixed — echo now uses $(BUILD_PLATFORM) instead of hardcoded "AMD64 Linux".

Expand Down
Loading