cert-manager / makefile-modules

Reusable Makefile modules that can be kloned into your project
Apache License 2.0
1 stars 10 forks source link

Makefile Modules, Go Versions and Vendoring #202

Open SgtCoDFish opened 1 month ago

SgtCoDFish commented 1 month ago

This issue is to capture the battling I had with makefile-modules when trying to ensure a specific Go version was used (for testing something unrelated). This might inform how we handle vendoring in the future (since GOTOOLCHAIN can kinda do vendoring).

Important reading for background: https://go.dev/doc/toolchain - nothing is happening in makefile-modules which isn't documented there, but it can be surprising.

I'll list some scenarios below and the behaviour that happens with makefile modules today

Scenario 1: High go.mod Version, Default Configuration

The Go toolchain docs are clear that the go values in a go.mod file can take precedence over the actual version fo Go being used:

In the standard configuration, the go command uses its own bundled toolchain when that toolchain is at least as new as the go or toolchain lines in the main module or workspace. For example, when using the go command bundled with Go 1.21.3 in a main module that says go 1.21.0, the go command uses Go 1.21.3. When the go or toolchain line is newer than the bundled toolchain, the go command runs the newer toolchain instead. For example, when using the go command bundled with Go 1.21.3 in a main module that says go 1.21.9, the go command finds and runs Go 1.21.9 instead.

The "standard configuration" is GOTOOLCHAIN=auto which is the default (see footnotes).

This means that make vendor-go is mostly irrelevant to the actual version of Go being run - VENDORED_GO_VERSION sets the floor of how low the Go version can be, but if used on a project whose go.mod requests a version higher than VENDORED_GO_VERSION, the vendored go toolchain will just download that new Go version and run that higher version, sidestepping vendoring in makefile-modules almost entirely.

In this scenario, makefile-modules is simply a bootstrapper for Go as long as VENDORED_GO_VERSION is higher than 1.21 (when toolchains were first introduced).

Scenario 2: High go.mod Version, GOTOOLCHAIN=local

We can set GOTOOLCHAIN=local to force the version of Go to match the actual version in the binary. So if VENDORED_GO_VERSION=1.23.1, that version will always be used. If the go.mod file says go.1.23.2 in this scenario, the project will fail to build:

go: go.mod requires go >= 1.23.2 (running go 1.23.1; GOTOOLCHAIN=local)

Scenario 3: Explicit GOTOOLCHAIN Version

As long as a version of Go 1.21+ is available - either on the system or via make vendor-go - GOTOOLCHAIN can be used to intentionally download a specific version of Go, which is similar how make vendor-go works.

GOTOOLCHAIN Playground

Create the following go.mod:

module github.com/sgtcodfish/goversions-makefile-modules

go 1.21.0

And the following main.go:

package main

import (
    "fmt"
    "runtime"
)

func main() {
    fmt.Println(runtime.Version())
}

Then the following will be the output:

$ go env GOTOOLCHAIN
local

$ go run main.go
go1.23.2

$ GOTOOLCHAIN=go1.21.1 go run main.go                                                                                    
go: downloading go1.21.1 (darwin/arm64)
go1.21.1

$ GOTOOLCHAIN=go1.23.2 go run main.go
go1.23.2

What's the problem?

In makefile-modules today, VENDORED_GO_VERSION is sometimes not respected because toolchain logic can override it.

With GOTOOLCHAIN=auto (which again is going to be the default - see footnotes), the best makefile-modules can do today is guarantee that the lowest version which will be used for building is either the value in go.mod or the value in VENDORED_GO_VERSION, whichever is higher.

Since we update VENDORED_GO_VERSION regularly and automatically, changing the GOTOOLCHAIN variable's value might seem redundant - most projects are going to be released having been built with the latest value of VENDORED_GO_VERSION.

The issues are that:

1) It's not immediately clear which version will be used from looking at the project. You can't rely on VENDORED_GO_VERSION or go.mod individually - you have to check both and know the logic that's used. 2) We're reimplementing logic for downloading Go in makefile-modules with complicated symlink magic 3) Go binaries downloaded through GOTOOLCHAIN are cached alongside Go modules, which might be more efficient in scenarios where we don't cache downloaded assets but do cache go modules. (see ls $(go env GOPATH)/pkg/mod/golang.org/ to see cached Go versions)

What should we do?

We have three options.

1. Set GOTOOLCHAIN=local

Knowing exactly what Go version will be used to build is valuable. It means that a quick glance at a commit will provide a single source of truth of what version will be used.

If we unconditionally set GOTOOLCHAIN=local when using make vendor-go we'll ensure that the Go version used is exactly VENDORED_GO_VERSION, and if go.mod is updated to require a newer version of Go the build will fail until the VENDORED_GO_VERSION is updated to the same version or newer.

2. Switch to using GOTOOLCHAIN to vendor Go

If make vendor-go were changed to set export GOTOOLCHAIN=go$(VENDORED_GO_VERSION) we'd have very similar behaviour to what we currently have except Go versions would be cached alongside Go modules, and we could remove our symlink logic.

Or, if we keep the symlink logic (or something like it) we'll download a Go toolchain using make vendor-go which will then be mostly ignored in favour of GOTOOLCHAIN logic. This will help in environments with Go not installed (e.g. some CI jobs).

One advantage of this is that it would be quite easy to allow projects to vary the Go version they use, so we could keep e.g. cert-manager 1.16 on go1.23.x but use go1.24.x for cert-manager 1.17

3. Do Nothing

It won't be obvious at a glance which version of Go will be used to build, but maybe we accept that.

Footnotes

Default value for GOTOOLCHAIN: Downloaded go tarballs (i.e. the ones we download with make vendor-go) contain a go.env file in the GOROOT which sets GOTOOLCHAIN=auto. Your local value of GOTOOLCHAIN may have a different value, either because you set it with go env -w or because it defaulted to local or some other value.

ThatsMrTalbot commented 1 month ago

I don't like the option of GOTOOLCHAIN=auto.

We should explicitly what version of Go is used to compile and IMO in the context of CI I think GOTOOLCHAIN=local makes the most sense, why download one version just to have it download another.

I don't like the idea of varying the go version per project as it means we have more to track in terms of ensuring we are up to date, but if that is really a feature people want we could change VENDORED_GO_VERSION from := to ?=.

SgtCoDFish commented 1 month ago

I don't like the option of GOTOOLCHAIN=auto

Yeah I tend to agree. I don't like option 3 and I think we should change something.

We should explicitly what version of Go is used to compile and IMO in the context of CI I think GOTOOLCHAIN=local makes the most sense, why download one version just to have it download another.

Yeah that's fair. For option 2, I think in CI terms it would make some some sense as long as we ensure all of our base images include some version of Go >= 1.21 - then we'd no longer have to download a tarball with vendor-go in CI, and we'd still just be downloading one version of Go in most CI jobs (just we'd be using GOTOOLCHAIN to do it rather than make vendor-go).

I don't like the idea of varying the go version per project as it means we have more to track in terms of ensuring we are up to date, but if that is really a feature people want we could change VENDORED_GO_VERSION from := to ?=.

Good point, agreed. I like that we update everywhere at once, automatically.

I do like the idea of changing VENDORED_GO_VERSION to ?= - it would help with debugging in some cases but those cases are pretty niche!

ThatsMrTalbot commented 1 month ago

For option 2, I think in CI terms it would make some some sense as long as we ensure all of our base images include some version of Go >= 1.21

If we did this we could consider removing the make vendor-go tooling entirely

IMO the smallest change we can do while solving the problem is changing it to GOTOOLCHAIN=local, but removing the complexities of the vendor-go stuff probably has value. So its more about how much effort we want to throw at this problem.