From d406d1dc9806606f4d913534addd4ea832b7a39b Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Wed, 12 Aug 2026 11:18:52 -0400 Subject: [PATCH 1/2] Fix ENVTESTPATH arch decided at Makefile-parse time on cold bin/ ifeq evaluates its $(shell ...) condition at parse time, before any target's prerequisites run. On a cold bin/ (setup-envtest not yet installed), `$(ENVTEST) list` fails silently, the grep finds nothing, and ENVTESTPATH gets permanently redefined to force --arch=amd64 -- regardless of host arch -- even though setup-envtest is correctly installed for the host's native arch by the time the `test` recipe actually runs. Move the fallback entirely into the shell expression itself (still a recursively-expanded variable, so it's only evaluated when referenced in the `test` recipe, after `envtest` has installed the right arch). Fixes #2377 Signed-off-by: Tiger Kaovilai --- Makefile | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 2c546c1c7d3..3ccc7433d38 100644 --- a/Makefile +++ b/Makefile @@ -300,15 +300,24 @@ vet: check-go ## Run go vet against code. go vet -mod=mod ./... ENVTEST := $(shell pwd)/bin/setup-envtest -ENVTESTPATH = $(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path) -ifeq ($(shell $(ENVTEST) list | grep $(ENVTEST_K8S_VERSION)),) - ENVTESTPATH = $(shell $(ENVTEST) --arch=amd64 use $(ENVTEST_K8S_VERSION) -p path) -endif +# Native-arch resolution first, falling back to amd64 only if that fails (e.g. no +# native-arch envtest assets published for this k8s version). Done as a single shell +# expression (not a make ifeq) so it's decided when ENVTESTPATH is actually referenced +# in a recipe, after $(ENVTEST) is guaranteed installed for the host's real arch -- +# not at Makefile-parse time against a possibly-cold bin/ (see issue #2377). +ENVTESTPATH = $(shell out=$$($(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path 2>/dev/null); if [ -z "$$out" ]; then out=$$($(ENVTEST) --arch=amd64 use $(ENVTEST_K8S_VERSION) -p path 2>/dev/null); fi; echo "$$out") +.PHONY: check-envtest-arch +check-envtest-arch: + @if [ -f $(ENVTEST) ] && ! $(ENVTEST) --help >/dev/null 2>&1; then \ + echo "$(ENVTEST) is not executable on this platform, removing and re-downloading"; \ + rm -f $(ENVTEST); \ + fi + # Uses go-install-tool-versioned (see its doc comment above) for both the version and -# architecture check, rather than a bespoke arch-only check here. +# architecture check, in addition to check-envtest-arch's not-executable safety net above. .PHONY: envtest $(ENVTEST) envtest: $(ENVTEST) ## Download envtest-setup locally if necessary. -$(ENVTEST): $(LOCALBIN) +$(ENVTEST): check-envtest-arch $(LOCALBIN) $(call go-install-tool-versioned,$(ENVTEST),sigs.k8s.io/controller-runtime/tools/setup-envtest@v0.0.0-20250308055145-5fe7bb3edc86,v0.0.0-20250308055145-5fe7bb3edc86) # If test results in prow are different, it is because the environment used. From eccd2e8a6331d683e691f7537a179fd648d00d5c Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Fri, 25 Sep 2026 12:45:10 -0400 Subject: [PATCH 2/2] Address Copilot review: inherit --bin-dir envtest resolution from oadp-1.5/1.6/dev Copilot flagged two real bugs in the previous ENVTESTPATH/check-envtest-arch approach: - The native-use command substitution's own nonzero exit (under this Makefile's .SHELLFLAGS = -ec) aborted the recipe before the amd64 fallback check ran. - check-envtest-arch treated setup-envtest's documented --help exit code 2 as a broken binary, deleting and reinstalling a perfectly good cached binary on every invocation. Rather than patching around both, adopt the simpler mechanism oadp-1.5/oadp-1.6/oadp-dev already use: resolve KUBEBUILDER_ASSETS inline in the test recipe via '$(ENVTEST) use ... --bin-dir $(LOCALBIN) -p path', after $(ENVTEST) is guaranteed installed for the host's real arch. This drops the separate ENVTESTPATH variable and check-envtest-arch target entirely, fixing #2377 without introducing either of Copilot's flagged issues. Verified locally on arm64: ENVTESTPATH resolves to bin/k8s/-darwin-arm64 (native), and go build ./... passes clean. > [!Note] > Responses generated with Hermes Agent --- Makefile | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/Makefile b/Makefile index 3ccc7433d38..8b2960ce6b4 100644 --- a/Makefile +++ b/Makefile @@ -300,24 +300,18 @@ vet: check-go ## Run go vet against code. go vet -mod=mod ./... ENVTEST := $(shell pwd)/bin/setup-envtest -# Native-arch resolution first, falling back to amd64 only if that fails (e.g. no -# native-arch envtest assets published for this k8s version). Done as a single shell -# expression (not a make ifeq) so it's decided when ENVTESTPATH is actually referenced -# in a recipe, after $(ENVTEST) is guaranteed installed for the host's real arch -- -# not at Makefile-parse time against a possibly-cold bin/ (see issue #2377). -ENVTESTPATH = $(shell out=$$($(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path 2>/dev/null); if [ -z "$$out" ]; then out=$$($(ENVTEST) --arch=amd64 use $(ENVTEST_K8S_VERSION) -p path 2>/dev/null); fi; echo "$$out") -.PHONY: check-envtest-arch -check-envtest-arch: - @if [ -f $(ENVTEST) ] && ! $(ENVTEST) --help >/dev/null 2>&1; then \ - echo "$(ENVTEST) is not executable on this platform, removing and re-downloading"; \ - rm -f $(ENVTEST); \ - fi - # Uses go-install-tool-versioned (see its doc comment above) for both the version and -# architecture check, in addition to check-envtest-arch's not-executable safety net above. +# architecture check. ENVTESTPATH itself is resolved inline in the `test` recipe below +# via `--bin-dir`, the same mechanism oadp-1.5/oadp-1.6/oadp-dev use -- it lets +# setup-envtest resolve its own native-arch assets directly against $(LOCALBIN) rather +# than this Makefile maintaining a separate arch-fallback variable/probe (see #2377: the +# old separate-variable approach got its arch decision wrong on a cold bin/, and a probe +# added to fix it tripped over setup-envtest's own documented `--help` exit-2 behavior +# under this Makefile's `.SHELLFLAGS = -ec` -- inheriting the already-correct mechanism +# from the newer branches avoids both failure modes rather than patching around them). .PHONY: envtest $(ENVTEST) envtest: $(ENVTEST) ## Download envtest-setup locally if necessary. -$(ENVTEST): check-envtest-arch $(LOCALBIN) +$(ENVTEST): $(LOCALBIN) $(call go-install-tool-versioned,$(ENVTEST),sigs.k8s.io/controller-runtime/tools/setup-envtest@v0.0.0-20250308055145-5fe7bb3edc86,v0.0.0-20250308055145-5fe7bb3edc86) # If test results in prow are different, it is because the environment used. @@ -328,7 +322,7 @@ $(ENVTEST): check-envtest-arch $(LOCALBIN) # If bin/ contains binaries of different arch, you may remove them so the container can install their arch. .PHONY: test test: check-go vet envtest ## Run Go linter and unit tests and check Go code format and if api and bundle folders are up to date. - KUBEBUILDER_ASSETS="$(ENVTESTPATH)" go test -mod=mod $(shell go list -mod=mod ./... | grep -v /tests/e2e) -coverprofile cover.out + KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test -mod=mod $(shell go list -mod=mod ./... | grep -v /tests/e2e) -coverprofile cover.out @make fmt-isupdated @make api-isupdated @make bundle-isupdated