chore: parameterize BUILD_ARCH/BUILD_PLATFORM in Makefiles#2257
chore: parameterize BUILD_ARCH/BUILD_PLATFORM in Makefiles#2257
Conversation
Replace hardcoded amd64/linux/amd64 with BUILD_ARCH and BUILD_PLATFORM variables across all service Makefiles. Defaults to host architecture via `go env GOARCH`, preserving existing behavior. Enables cross-compilation by setting BUILD_ARCH=arm64 or multi-arch Docker builds via BUILD_PLATFORM=linux/amd64,linux/arm64. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tomassrnka
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
PR SummaryLow Risk Overview Written by Cursor Bugbot for commit 7318e8f. This will update automatically on new commits. Configure here. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
packages/api/Makefile
Outdated
| 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) |
There was a problem hiding this comment.
BUILD_PLATFORM is defined here but never actually used. The build-and-upload target calls docker buildx bake without passing this variable, and api/docker-bake.hcl still hardcodes platforms = ["linux/amd64"] for both api and db-migrator targets. Running BUILD_PLATFORM=linux/arm64 make build-and-upload from this package will silently produce an amd64 image.
To wire it up, either pass --set "*.platforms=$(BUILD_PLATFORM)" to docker buildx bake, or add a BUILD_PLATFORM variable to docker-bake.hcl and pass it as an environment variable.
There was a problem hiding this comment.
Fixed — removed BUILD_ARCH/BUILD_PLATFORM from api Makefile entirely.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Unused
BUILD_ARCH/BUILD_PLATFORMvariables in api Makefile- Removed the unused BUILD_ARCH and BUILD_PLATFORM variables since they were never referenced by any target in the api Makefile.
- ✅ Fixed:
docker runbreaks with multi-platformBUILD_PLATFORMvalue- Updated the comment to document single-platform usage only, since docker build and docker run don't support comma-separated multi-platform values.
- ✅ Fixed: Non-buildx
docker buildbreaks with multi-platform value- Updated the comment to document single-platform usage only, since non-buildx docker build doesn't support comma-separated multi-platform values.
Or push these changes by commenting:
@cursor push df1c556b27
Preview (df1c556b27)
diff --git a/packages/api/Makefile b/packages/api/Makefile
--- a/packages/api/Makefile
+++ b/packages/api/Makefile
@@ -6,12 +6,6 @@
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)
expectedMigration := $(shell ./../../scripts/get-latest-migration.sh)
diff --git a/packages/envd/Makefile b/packages/envd/Makefile
--- a/packages/envd/Makefile
+++ b/packages/envd/Makefile
@@ -10,8 +10,8 @@
# 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 multi-arch builds:
-# BUILD_PLATFORM=linux/amd64,linux/arm64 make start-docker
+# Docker platform string (single platform only for docker build/run).
+# Example: BUILD_PLATFORM=linux/amd64 make start-docker
BUILD_PLATFORM ?= linux/$(BUILD_ARCH)
.PHONY: init
diff --git a/packages/orchestrator/Makefile b/packages/orchestrator/Makefile
--- a/packages/orchestrator/Makefile
+++ b/packages/orchestrator/Makefile
@@ -10,8 +10,8 @@
# 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 multi-arch builds:
-# BUILD_PLATFORM=linux/amd64,linux/arm64 make build
+# Docker platform string (single platform only for docker build).
+# Example: BUILD_PLATFORM=linux/amd64 make build
BUILD_PLATFORM ?= linux/$(BUILD_ARCH)
.PHONY: initThis Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff8ed13fa0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| docker run \ | ||
| --name envd \ | ||
| --platform linux/amd64 \ | ||
| --platform $(BUILD_PLATFORM) \ |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Fixed — comment updated to single-arch example.
packages/api/Makefile
Outdated
| 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) |
There was a problem hiding this comment.
BUILD_PLATFORM into build-and-upload flow
The new BUILD_PLATFORM knob in the API Makefile is not consumed by build-and-upload: that target still calls docker buildx bake with api/docker-bake.hcl, and the bake file remains pinned to platforms = ["linux/amd64"] for both targets. As a result, setting BUILD_PLATFORM (as the new comment suggests) has no effect, so API builds are still fixed to amd64.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — removed from api Makefile.
packages/api/Makefile
Outdated
| # 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) | ||
|
|
There was a problem hiding this comment.
🔴 In packages/api/Makefile, BUILD_ARCH and BUILD_PLATFORM are declared but never referenced in any build target, so cross-compilation via BUILD_ARCH=arm64 or BUILD_PLATFORM=linux/arm64 silently has no effect. The fix requires adding GOARCH=$(BUILD_ARCH) to the build/build-debug targets and either passing BUILD_PLATFORM through to the docker buildx bake invocation or updating api/docker-bake.hcl to accept it as a variable.
Extended reasoning...
Bug: BUILD_ARCH and BUILD_PLATFORM are declared but unused in packages/api/Makefile
The PR's stated goal is to parameterize BUILD_ARCH/BUILD_PLATFORM across all four service Makefiles so that cross-compilation and multi-arch Docker builds can be triggered uniformly. Three of the four packages (client-proxy, envd, orchestrator) correctly wire the new variables into their build commands. The api package declares the variables but fails to use them anywhere.
Specific code paths that are broken:
-
buildandbuild-debugtargets (packages/api/Makefile lines 30–38): Both targets invokego buildasCGO_ENABLED=0 go build ...andCGO_ENABLED=1 go build ...respectively — without anyGOARCHenv var. Every other package in this PR setsGOARCH=$(BUILD_ARCH)explicitly (e.g.,client-proxyline 26:CGO_ENABLED=0 GOOS=linux GOARCH=$(BUILD_ARCH) go build ...). Theapipackage omits this, so runningBUILD_ARCH=arm64 make buildinpackages/apicompiles for the host architecture, not arm64. -
build-and-uploadtarget (packages/api/Makefile lines ~75–80): This target callsdocker buildx bake -f api/docker-bake.hcl --pushand passes onlyREGISTRY_PREFIX,COMMIT_SHA, andEXPECTED_MIGRATION_TIMESTAMPas env vars. It does not passBUILD_PLATFORM(or any platform-related variable) to the bake command. Furthermore,api/docker-bake.hclhardcodesplatforms = ["linux/amd64"]in both theapianddb-migratortargets and defines noBUILD_PLATFORMvariable at all. As a result,BUILD_PLATFORM=linux/arm64 make build-and-uploadsilently produces an amd64 image.
Why existing code doesn't prevent it:
Make variable declarations with ?= only set defaults; they don't enforce usage. There is no mechanism in GNU Make that would warn or error when a declared variable is never referenced in a recipe. The variables evaluate correctly (defaulting to the host arch via go env GOARCH), but because neither the go build commands nor the bake invocation reference them, they are effectively dead declarations.
Impact:
Any developer or CI job attempting to cross-compile the api package (e.g., for ARM64 support as described in the linked issue #1875) will get a silently incorrect binary or image. There is no error message — the build succeeds but targets the wrong architecture. This directly contradicts the PR's test plan item: "Verify BUILD_PLATFORM=linux/arm64 make build produces arm64 Docker images" — it will not for the api package.
Step-by-step proof:
- Developer runs
BUILD_ARCH=arm64 make buildinpackages/api - Make evaluates
BUILD_ARCH ?= $(shell go env GOARCH)— but sinceBUILD_ARCH=arm64is set in the environment,BUILD_ARCHisarm64. ✓ - Make executes the
buildtarget:CGO_ENABLED=0 go build -o bin/api ...— noGOARCHis set, so Go uses the host architecture (e.g.,amd64). - The resulting
bin/apibinary is amd64, not arm64. TheBUILD_ARCH=arm64override was ignored. - Similarly,
BUILD_PLATFORM=linux/arm64 make build-and-uploadcallsdocker buildx bake -f api/docker-bake.hclwhich reads its ownplatforms = ["linux/amd64"]from the HCL file and ignores the Makefile variable entirely.
How to fix:
- Add
GOARCH=$(BUILD_ARCH)to thebuildtarget:CGO_ENABLED=0 GOOS=linux GOARCH=$(BUILD_ARCH) go build ... - Either: add a
variable "BUILD_PLATFORM"block toapi/docker-bake.hcland pass it via an env var in the bake invocation, or switchbuild-and-uploadfromdocker buildx baketodocker buildx build --platform $(BUILD_PLATFORM)(asclient-proxydoes).
There was a problem hiding this comment.
Fixed — BUILD_ARCH/BUILD_PLATFORM removed from api Makefile entirely.
| 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) \ | ||
| -p 49983:49983 \ | ||
| -p 2345:2345 \ | ||
| -p 9999:9999 \ |
There was a problem hiding this comment.
🔴 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:
make build— succeeds (plain Go cross-compilation, unaffected by BUILD_PLATFORM)DOCKER_BUILDKIT=1 docker build --platform linux/amd64,linux/arm64 ...— succeeds, creates a multi-arch manifestdocker run --platform linux/amd64,linux/arm64 ...— fails with an error such asinvalid 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:
- Change the comment to reference only
make build(or a push/upload target) instead ofmake start-docker, since multi-arch builds are only useful when pushing to a registry. - Split the variable in
start-docker: useBUILD_PLATFORMfor thedocker buildstep and derive a single-arch run platform fromBUILD_ARCH(e.g.,--platform linux/$(BUILD_ARCH)) fordocker run.
Step-by-step proof
- User reads the comment:
# BUILD_PLATFORM=linux/amd64,linux/arm64 make start-docker - User runs:
BUILD_PLATFORM=linux/amd64,linux/arm64 make start-docker - Make expands
start-docker→ runsdocker run --platform linux/amd64,linux/arm64 ... - Docker rejects the value:
docker: invalid platform linux/amd64,linux/arm64: unknown os or arch or variant - The container never starts; the command fails.
There was a problem hiding this comment.
Fixed — comment now shows single-arch example only.
| @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" |
There was a problem hiding this comment.
🟡 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
- Default case (amd64 host):
make test-docker→BUILD_PLATFORMresolves tolinux/amd64→ echo says "AMD64 Linux" → happens to be accurate. - 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.
There was a problem hiding this comment.
Fixed — echo now uses $(BUILD_PLATFORM) instead of hardcoded "AMD64 Linux".
- Remove BUILD_ARCH/BUILD_PLATFORM from api Makefile (uses docker-bake.hcl) - Fix misleading multi-arch BUILD_PLATFORM examples in envd and orchestrator Makefiles (these use plain docker build, not buildx) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| docker run \ | ||
| --name envd \ | ||
| --platform linux/amd64 \ | ||
| --platform $(BUILD_PLATFORM) \ |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
Replace hardcoded 'AMD64 Linux' with $(BUILD_PLATFORM) so the log output reflects the actual platform being tested. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
BUILD_ARCHandBUILD_PLATFORMvariables to all service Makefiles (api,client-proxy,envd,orchestrator)amd64/linux/amd64references with the new variablesgo env GOARCH, preserving existing behavior on amd64 hostsBUILD_ARCH=arm64 make build) and multi-arch Docker builds (e.g.,BUILD_PLATFORM=linux/amd64,linux/arm64 make build)Part of #1875 (ARM64 support), split out as a standalone change since the Makefile parameterization is safe and backwards-compatible.
Test plan
make buildin each package still defaults to amd64 on an amd64 hostBUILD_ARCH=arm64 make build-localcross-compiles correctly inpackages/orchestratorBUILD_PLATFORM=linux/arm64 make buildproduces arm64 Docker images🤖 Generated with Claude Code