Skip to content

Key go-installed tool binaries on the Go toolchain that builds them #708

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

Open
wallrj-cyberark wants to merge 2 commits into
base: main
Choose a base branch
from
+43 −12
Open

Key go-installed tool binaries on the Go toolchain that builds them #708

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2680095
Key go-installed tool binaries on the vendored Go version
wallrj-cyberark Aug 20, 2026
9e650c6
Key go-installed tools on the toolchain that builds them
wallrj Aug 20, 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
55 changes: 43 additions & 12 deletions modules/tools/00_mod.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -338,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)

@SgtCoDFish SgtCoDFish Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two problems with this line, both silent, and both fixable together.

1. go env GOVERSION can download a toolchain at parse time, and its failure is swallowed

This is a := at the top level of the non-vendored branch, so it runs in $(CURDIR) — a Go module directory — on every make invocation in every downstream repo that doesn't vendor Go, including make help. With the default GOTOOLCHAIN=auto, if the repo's go.mod go/toolchain directive is newer than the installed Go, this triggers a toolchain download during makefile parsing:

$ cat go.mod          # module x / go 1.27.0, installed go is 1.26.7
$ GOPROXY=off go env GOVERSION
go: downloading go1.27.0 (darwin/arm64)
go: download go1.27.0 for darwin/arm64: toolchain not available
exit=1

When that fails, 2>/dev/null hides the message and $(shell) discards the exit status, so GO_TOOLCHAIN_VERSION is silently empty and every toolchain collapses onto one key:

$ make -n GO_TOOLCHAIN_VERSION= _bin/tools/gojq
... ln -fsn /tmp/mmtest708/dl/tools/gojq@v0.12.19__darwin_arm64 gojq

That is exactly the stale-binary bug this PR fixes, reintroduced — but now invisible, because the path looks keyed. Air-gapped CI, GOPROXY=off, a restricted-egress Prow job, or a bad GOFLAGS all land here.

GOTOOLCHAIN=local fixes both halves, and is the exact analogue of the vendored branch's go$(VENDORED_GO_VERSION) (also a local-toolchain label, not a switched one):

$ GOTOOLCHAIN=local go env GOVERSION     # in the same too-new module dir
go1.26.7                                 # never downloads, never fails

You already note under "Known limitations" that GOTOOLCHAIN=local would make the label exact (#202) — worth pulling that in here, since it buys robustness too, not just accuracy.

2. A whitespace-bearing GOVERSION silently corrupts the generated rules

runtime.Version() for a gotip/devel toolchain is multi-word. Because the value lands unquoted in target names, make word-splits it and the file quietly produces nonsense:

$ make -n 'GO_TOOLCHAIN_VERSION=devel go1.28-abc123 Wed Aug 20' _bin/tools/gojq
source tools//util/lock.sh 20_darwin_arm64; ... go install ...pinact/v4/cmd/pinact@v4.1.1 ...
cd _bin/tools/ && ln -fsn /tmp/.../gojq@v0.12.19_devel go1.28-abc123 Wed Aug 20_darwin_arm64 gojq

Note it built pinact while asked for gojq, and ln got four arguments. No error, no warning. GOTOOLCHAIN=local doesn't help here — a devel toolchain is the local one.

Suggested fix

# 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),)
GO_TOOLCHAIN_VERSION := unknown
endif

I'd keep the empty case non-fatal rather than $(error ...): make help and make non-go-tools currently work with no Go installed and shouldn't regress. unknown is at least self-describing in a filename, and nothing can be built in that state anyway.

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
Expand Down Expand Up @@ -459,7 +463,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 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)

@SgtCoDFish SgtCoDFish Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: the two _DOWNLOAD_PATH assignments escape differently for the same intent. This one uses $(HOST_OS)/$($(call uc,$1)_VERSION) but $$(GO_TOOLCHAIN_VERSION), while the one in tool_defs uses $$(HOST_OS)/$$($(call uc,$1)_VERSION). Both are := so the results are identical, but a reader has to work that out — and these two lines are precisely the pair you want read side by side. Worth making them consistent.


$$($(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
Expand All @@ -471,6 +483,25 @@ $$(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.
#
# 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/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 ..
Comment on lines +497 to +499

@SgtCoDFish SgtCoDFish Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two comment nits in this block:

  • $(bin_dir)/scratch/GO_TOOLCHAIN_VERSION only works because the %_VERSION pattern rule ~250 lines up sets $* to GO_TOOLCHAIN and there happens to be a GO_TOOLCHAIN_VERSION variable. That's correct and rather neat, but non-obvious enough at this distance to earn half a sentence in the block comment above.

  • The two @# comment lines moved verbatim, but they're misleading and this PR is the natural place to fix them. DOWNLOAD_DIR defaults to $(HOME)/.cache/makefile-modules/downloaded (or $(CURDIR)/$(bin_dir)/downloaded under CI), neither of which matches $(bin_dir)/%, so the patsubst is a no-op and the link is absolute in practice:

    $ ls -l _bin/tools/gojq
_bin/tools/gojq -> /Users/…/.cache/makefile-modules/downloaded/tools/gojq@v0.12.19_go1.26.7_darwin_arm64

    The example ../downloaded/tools/helm@v4.0.1_darwin_arm64 never happens.

@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))))

##################
# File downloads #
##################
Expand Down
Loading