From 26800952441a31875d497acc91dbaa2a9ecbc49a Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Thu, 20 Aug 2026 10:27:37 +0000 Subject: [PATCH 1/3] Key go-installed tool binaries on the vendored Go version - Tools built with "go install" are cached at $(DOWNLOAD_DIR)/tools/@__, a path which says nothing about the Go toolchain that built them. - CI persists that download directory between runs, so after a VENDORED_GO_VERSION bump the stale binary is restored and reused indefinitely, even when it can no longer parse the new standard library. - Include the Go version in the path of Go-built tools, so that a Go upgrade forces a rebuild, and depend on the VENDORED_GO_VERSION stamp file so the unversioned symlink is re-pointed. Refs: https://github.com/cert-manager/cert-manager/pull/9174 Signed-off-by: Richard Wall --- modules/tools/00_mod.mk | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/modules/tools/00_mod.mk b/modules/tools/00_mod.mk index c48fda1d..75e8ef09 100644 --- a/modules/tools/00_mod.mk +++ b/modules/tools/00_mod.mk @@ -287,22 +287,21 @@ tool_names := # the absolute path should be used when executing the binary # in targets or in scripts, because it is agnostic to the # working directory +# - a $(XXX_DOWNLOAD_PATH) variable is generated +# -> this variable contains the path of the versioned binary in +# $(DOWNLOAD_DIR), which the unversioned target links to. Tools +# that are built from source override it in the go_dependency +# template below # - an unversioned target $(bin_dir)/tools/xxx is generated that # creates a link to the corresponding versioned target: -# $(DOWNLOAD_DIR)/tools/xxx@$(XXX_VERSION)_$(HOST_OS)_$(HOST_ARCH) +# $(XXX_DOWNLOAD_PATH) define tool_defs tool_names += $1 $(call uc,$1)_VERSION ?= $2 NEEDS_$(call uc,$1) := $$(bin_dir)/tools/$1 $(call uc,$1) := $$(CURDIR)/$$(bin_dir)/tools/$1 - -# Create symlink from $(bin_dir)/tools/$1 to the versioned binary in $(DOWNLOAD_DIR) -$$(bin_dir)/tools/$1: $$(bin_dir)/scratch/$(call uc,$1)_VERSION | $$(DOWNLOAD_DIR)/tools/$1@$$($(call uc,$1)_VERSION)_$$(HOST_OS)_$$(HOST_ARCH) $$(bin_dir)/tools - @# cd into tools dir and create relative symlink (e.g., ../downloaded/tools/helm@v4.0.1_darwin_arm64) - @# patsubst converts absolute path to relative by replacing $(bin_dir) with .. - @cd $$(dir $$@) && $$(LN) $$(patsubst $$(bin_dir)/%,../%,$$(word 1,$$|)) $$(notdir $$@) - @touch $$@ # making sure the target of the symlink is newer than *_VERSION +$(call uc,$1)_DOWNLOAD_PATH := $$(DOWNLOAD_DIR)/tools/$1@$$($(call uc,$1)_VERSION)_$$(HOST_OS)_$$(HOST_ARCH) endef # For each tool in the tools list (e.g., "helm=v4.0.1"), split on "=" and call tool_defs @@ -459,7 +458,15 @@ go_tool_names := # Template for building Go-based tools from source using "go install" define go_dependency go_tool_names += $1 -$$(DOWNLOAD_DIR)/tools/$1@$($(call uc,$1)_VERSION)_$(HOST_OS)_$(HOST_ARCH): | $$(NEEDS_GO) $$(DOWNLOAD_DIR)/tools + +# The binary is keyed on the Go version as well as the tool version, because a +# tool built by an older Go cannot always parse a newer standard library. Without +# this, a cached binary is never rebuilt after a Go upgrade: the download +# directory is persisted between CI runs, so the stale binary is restored and +# reused indefinitely. +$(call uc,$1)_DOWNLOAD_PATH := $$(DOWNLOAD_DIR)/tools/$1@$($(call uc,$1)_VERSION)_go$$(VENDORED_GO_VERSION)_$(HOST_OS)_$(HOST_ARCH) + +$$($(call uc,$1)_DOWNLOAD_PATH): | $$(NEEDS_GO) $$(DOWNLOAD_DIR)/tools @# 1. Use lock script to prevent concurrent builds of the same tool @# 2. Install to temp dir using GOBIN, with GOWORK=off to ignore workspace files @# 3. Move the binary to final location @@ -471,6 +478,18 @@ $$(DOWNLOAD_DIR)/tools/$1@$($(call uc,$1)_VERSION)_$(HOST_OS)_$(HOST_ARCH): | $$ endef $(call for_each_kv,go_dependency,$(go_dependencies)) +# Create the symlink from $(bin_dir)/tools/xxx to the versioned binary in +# $(DOWNLOAD_DIR). This runs after the go_dependency template above, so that the +# tools built from source link to their Go-version-specific binary. +define tool_link_defs +$$(bin_dir)/tools/$1: $$(bin_dir)/scratch/$(call uc,$1)_VERSION $(if $(filter $1,$(go_tool_names)),$$(bin_dir)/scratch/VENDORED_GO_VERSION) | $$($(call uc,$1)_DOWNLOAD_PATH) $$(bin_dir)/tools + @# cd into tools dir and create relative symlink (e.g., ../downloaded/tools/helm@v4.0.1_darwin_arm64) + @# patsubst converts absolute path to relative by replacing $(bin_dir) with .. + @cd $$(dir $$@) && $$(LN) $$(patsubst $$(bin_dir)/%,../%,$$(word 1,$$|)) $$(notdir $$@) + @touch $$@ # making sure the target of the symlink is newer than *_VERSION +endef +$(foreach tool_name,$(tool_names),$(eval $(call tool_link_defs,$(tool_name)))) + ################## # File downloads # ################## From 9e650c647ebfd1b61dad7c3539f42b76b7ebb689 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Thu, 20 Aug 2026 14:14:50 +0100 Subject: [PATCH 2/3] Key go-installed tools on the toolchain that builds them - When vendoring is disabled, tools are built with the system Go, so keying the download path on VENDORED_GO_VERSION mislabels binaries in the shared cache and a system Go upgrade never invalidates them. Key the path and the stamp file on the Go version actually used: "go env GOVERSION" for the system Go, go$(VENDORED_GO_VERSION) when vendoring. - Make the versioned binary a normal prerequisite of the unversioned symlink, so a rebuilt binary always re-points the symlink. Previously an existing symlink newer than the stamp files caused the binary to be rebuilt at the new path while the symlink kept pointing at the old one. In the steady state the symlink resolves to the same file as the prerequisite, so nothing is remade. - Update the LN comment for the new path format. Co-Authored-By: Claude Fable 5 Signed-off-by: Richard Wall --- modules/tools/00_mod.mk | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/modules/tools/00_mod.mk b/modules/tools/00_mod.mk index 75e8ef09..8214f741 100644 --- a/modules/tools/00_mod.mk +++ b/modules/tools/00_mod.mk @@ -250,9 +250,9 @@ $(bin_dir)/scratch/%_VERSION: FORCE | $(bin_dir)/scratch CURL := curl --silent --show-error --fail --location --retry 10 --retry-connrefused # LN is expected to be an atomic action, meaning that two Make processes -# can run the "link $(DOWNLOAD_DIR)/tools/xxx@$(XXX_VERSION)_$(HOST_OS)_$(HOST_ARCH) -# to $(bin_dir)/tools/xxx" operation simultaneously without issues (both -# will perform the action and the second time the link will be overwritten). +# can run the "link $(XXX_DOWNLOAD_PATH) to $(bin_dir)/tools/xxx" operation +# simultaneously without issues (both will perform the action and the second +# time the link will be overwritten). # # -s = Create a symbolic link # -f = Force the creation of the link (replace existing links) @@ -337,12 +337,17 @@ __require-go: endif GO := go NEEDS_GO = __require-go +# The version of the Go toolchain that builds the go_dependencies tools, e.g. +# "go1.27.0". When vendoring is disabled this is the system Go, which may +# differ from VENDORED_GO_VERSION. +GO_TOOLCHAIN_VERSION := $(shell go env GOVERSION 2>/dev/null) else export GOROOT := $(CURDIR)/$(bin_dir)/tools/goroot export PATH := $(CURDIR)/$(bin_dir)/tools/goroot/bin:$(PATH) GO := $(CURDIR)/$(bin_dir)/tools/go NEEDS_GO := $(bin_dir)/tools/go MAKE := $(MAKE) vendor-go +GO_TOOLCHAIN_VERSION := go$(VENDORED_GO_VERSION) endif .PHONY: vendor-go @@ -459,12 +464,12 @@ go_tool_names := define go_dependency go_tool_names += $1 -# The binary is keyed on the Go version as well as the tool version, because a -# tool built by an older Go cannot always parse a newer standard library. Without -# this, a cached binary is never rebuilt after a Go upgrade: the download -# directory is persisted between CI runs, so the stale binary is restored and -# reused indefinitely. -$(call uc,$1)_DOWNLOAD_PATH := $$(DOWNLOAD_DIR)/tools/$1@$($(call uc,$1)_VERSION)_go$$(VENDORED_GO_VERSION)_$(HOST_OS)_$(HOST_ARCH) +# The binary is keyed on the Go toolchain version as well as the tool version, +# because a tool built by an older Go cannot always parse a newer standard +# library. Without this, a cached binary is never rebuilt after a Go upgrade: +# the download directory is persisted between CI runs, so the stale binary is +# restored and reused indefinitely. +$(call uc,$1)_DOWNLOAD_PATH := $$(DOWNLOAD_DIR)/tools/$1@$($(call uc,$1)_VERSION)_$$(GO_TOOLCHAIN_VERSION)_$(HOST_OS)_$(HOST_ARCH) $$($(call uc,$1)_DOWNLOAD_PATH): | $$(NEEDS_GO) $$(DOWNLOAD_DIR)/tools @# 1. Use lock script to prevent concurrent builds of the same tool @@ -481,11 +486,18 @@ $(call for_each_kv,go_dependency,$(go_dependencies)) # Create the symlink from $(bin_dir)/tools/xxx to the versioned binary in # $(DOWNLOAD_DIR). This runs after the go_dependency template above, so that the # tools built from source link to their Go-version-specific binary. +# +# The versioned binary is a normal (not order-only) prerequisite: rebuilding it +# makes it newer than the symlink, which forces the symlink to be re-pointed. +# In the steady state the symlink resolves to that same binary, so their +# modification times are equal and nothing is remade. The stamp files catch +# version changes that mtimes cannot, e.g. reverting to an older, already-cached +# tool or Go version. define tool_link_defs -$$(bin_dir)/tools/$1: $$(bin_dir)/scratch/$(call uc,$1)_VERSION $(if $(filter $1,$(go_tool_names)),$$(bin_dir)/scratch/VENDORED_GO_VERSION) | $$($(call uc,$1)_DOWNLOAD_PATH) $$(bin_dir)/tools +$$(bin_dir)/tools/$1: $$(bin_dir)/scratch/$(call uc,$1)_VERSION $(if $(filter $1,$(go_tool_names)),$$(bin_dir)/scratch/GO_TOOLCHAIN_VERSION) $$($(call uc,$1)_DOWNLOAD_PATH) | $$(bin_dir)/tools @# cd into tools dir and create relative symlink (e.g., ../downloaded/tools/helm@v4.0.1_darwin_arm64) @# patsubst converts absolute path to relative by replacing $(bin_dir) with .. - @cd $$(dir $$@) && $$(LN) $$(patsubst $$(bin_dir)/%,../%,$$(word 1,$$|)) $$(notdir $$@) + @cd $$(dir $$@) && $$(LN) $$(patsubst $$(bin_dir)/%,../%,$$($(call uc,$1)_DOWNLOAD_PATH)) $$(notdir $$@) @touch $$@ # making sure the target of the symlink is newer than *_VERSION endef $(foreach tool_name,$(tool_names),$(eval $(call tool_link_defs,$(tool_name)))) From c5ed50b4226b24d86d239d65a4d3e1c05ce0faa7 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Fri, 21 Aug 2026 19:36:20 +0100 Subject: [PATCH 3/3] Address review: harden the Go toolchain version key and test invalidation Query the system Go with GOTOOLCHAIN=local so that computing the key can never trigger a toolchain download at makefile parse time, nor fail silently to an empty key when the download is impossible. Sanitise the value because a devel toolchain reports a multi-word GOVERSION which would word-split the generated rules, and fall back to a non-fatal "unknown" so that make help still works with no Go installed. Align the escaping of the two _DOWNLOAD_PATH assignments, correct the symlink recipe comments (the link is absolute in practice), and note where the GO_TOOLCHAIN_VERSION stamp file comes from. Assert the invalidation contract in the e2e test: a toolchain version change rebuilds and re-links a go_dependency tool, and reverting re-links the cached binary without rebuilding. Inodes, not mtimes, are compared because the relink recipe touches through the symlink. Co-Authored-By: Claude Fable 5 Signed-off-by: Richard Wall --- modules/tools/00_mod.mk | 23 +++++++++++++---- .../e2e-projects/test-project/test-config.sh | 1 + tests/test_e2e.sh | 25 +++++++++++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/modules/tools/00_mod.mk b/modules/tools/00_mod.mk index 8214f741..7c19bd7e 100644 --- a/modules/tools/00_mod.mk +++ b/modules/tools/00_mod.mk @@ -340,7 +340,17 @@ NEEDS_GO = __require-go # The version of the Go toolchain that builds the go_dependencies tools, e.g. # "go1.27.0". When vendoring is disabled this is the system Go, which may # differ from VENDORED_GO_VERSION. -GO_TOOLCHAIN_VERSION := $(shell go env GOVERSION 2>/dev/null) +# GOTOOLCHAIN=local: never trigger a toolchain download while parsing this +# file, and match the go$(VENDORED_GO_VERSION) form used when Go is vendored. +# The awk pass keeps the value safe to embed in a target name: a devel +# toolchain reports a multi-word GOVERSION, which would word-split the +# generated rules. +GO_TOOLCHAIN_VERSION := $(shell GOTOOLCHAIN=local go env GOVERSION 2>/dev/null | awk '{gsub(/[^A-Za-z0-9._-]/,"-"); print}') +ifeq ($(GO_TOOLCHAIN_VERSION),) +# Non-fatal so that targets which need no Go, e.g. "make help", still work +# with no Go installed. Nothing can be built in that state anyway. +GO_TOOLCHAIN_VERSION := unknown +endif else export GOROOT := $(CURDIR)/$(bin_dir)/tools/goroot export PATH := $(CURDIR)/$(bin_dir)/tools/goroot/bin:$(PATH) @@ -469,7 +479,7 @@ go_tool_names += $1 # library. Without this, a cached binary is never rebuilt after a Go upgrade: # the download directory is persisted between CI runs, so the stale binary is # restored and reused indefinitely. -$(call uc,$1)_DOWNLOAD_PATH := $$(DOWNLOAD_DIR)/tools/$1@$($(call uc,$1)_VERSION)_$$(GO_TOOLCHAIN_VERSION)_$(HOST_OS)_$(HOST_ARCH) +$(call uc,$1)_DOWNLOAD_PATH := $$(DOWNLOAD_DIR)/tools/$1@$$($(call uc,$1)_VERSION)_$$(GO_TOOLCHAIN_VERSION)_$$(HOST_OS)_$$(HOST_ARCH) $$($(call uc,$1)_DOWNLOAD_PATH): | $$(NEEDS_GO) $$(DOWNLOAD_DIR)/tools @# 1. Use lock script to prevent concurrent builds of the same tool @@ -492,11 +502,14 @@ $(call for_each_kv,go_dependency,$(go_dependencies)) # In the steady state the symlink resolves to that same binary, so their # modification times are equal and nothing is remade. The stamp files catch # version changes that mtimes cannot, e.g. reverting to an older, already-cached -# tool or Go version. +# tool or Go version. The GO_TOOLCHAIN_VERSION stamp is produced by the generic +# %_VERSION pattern rule above, which stamps the value of the make variable of +# the same name. define tool_link_defs $$(bin_dir)/tools/$1: $$(bin_dir)/scratch/$(call uc,$1)_VERSION $(if $(filter $1,$(go_tool_names)),$$(bin_dir)/scratch/GO_TOOLCHAIN_VERSION) $$($(call uc,$1)_DOWNLOAD_PATH) | $$(bin_dir)/tools - @# cd into tools dir and create relative symlink (e.g., ../downloaded/tools/helm@v4.0.1_darwin_arm64) - @# patsubst converts absolute path to relative by replacing $(bin_dir) with .. + @# The link is absolute in practice: DOWNLOAD_DIR defaults to a path outside + @# $(bin_dir). The patsubst makes it relative only when DOWNLOAD_DIR is + @# overridden to live under $(bin_dir). @cd $$(dir $$@) && $$(LN) $$(patsubst $$(bin_dir)/%,../%,$$($(call uc,$1)_DOWNLOAD_PATH)) $$(notdir $$@) @touch $$@ # making sure the target of the symlink is newer than *_VERSION endef diff --git a/tests/e2e-projects/test-project/test-config.sh b/tests/e2e-projects/test-project/test-config.sh index 22be252f..5219f862 100755 --- a/tests/e2e-projects/test-project/test-config.sh +++ b/tests/e2e-projects/test-project/test-config.sh @@ -36,5 +36,6 @@ targets_to_run+=( "_bin/tools/kind" "_bin/tools/kubectl" "_bin/tools/etcd" + "_bin/tools/gojq" "vendor-go" ) diff --git a/tests/test_e2e.sh b/tests/test_e2e.sh index 2d723346..6e813335 100755 --- a/tests/test_e2e.sh +++ b/tests/test_e2e.sh @@ -74,5 +74,30 @@ for project in "${script_dir}"/e2e-projects/*; do echo "> Running make ${target}" make "${target}" done + + # Assert that a Go toolchain version change rebuilds and re-links a + # go_dependency tool, and that reverting re-links the cached binary + # without rebuilding it. An isolated DOWNLOAD_DIR keeps the fake + # toolchain key out of the shared cache. + if printf '%s\n' "${modules_to_copy[@]}" | grep -qx "tools"; then + echo "> Asserting Go toolchain cache invalidation for gojq" + dl_dir="${tmp_dir}/e2e_download" + + make DOWNLOAD_DIR="${dl_dir}" _bin/tools/gojq + original_target=$(readlink _bin/tools/gojq) + + make DOWNLOAD_DIR="${dl_dir}" GO_TOOLCHAIN_VERSION=go0.0.0-test _bin/tools/gojq + fake_target=$(readlink _bin/tools/gojq) + [[ "${fake_target}" == *"_go0.0.0-test_"* ]] + [[ "${fake_target}" != "${original_target}" ]] + + # A rebuild produces a new file (new inode); the relink recipe + # touches the existing one, so compare inodes, not mtimes. + inode_before=$(stat -c %i "${original_target}" 2>/dev/null || stat -f %i "${original_target}") + make DOWNLOAD_DIR="${dl_dir}" _bin/tools/gojq + [[ "$(readlink _bin/tools/gojq)" == "${original_target}" ]] + inode_after=$(stat -c %i "${original_target}" 2>/dev/null || stat -f %i "${original_target}") + [[ "${inode_before}" == "${inode_after}" ]] + fi popd > /dev/null done