Skip to content
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.

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

proposal: set GOTOOLCHAIN=local (or =path) in our image #472

Closed
tianon opened this issue Jun 27, 2023 · 6 comments · Fixed by #491
Closed

proposal: set GOTOOLCHAIN=local (or =path) in our image #472

tianon opened this issue Jun 27, 2023 · 6 comments · Fixed by #491

Comments

@tianon
Copy link
Member

tianon commented Jun 27, 2023

Per https://go.dev/doc/toolchain, it might make sense for us to set GOTOOLCHAIN=local (or even GOTOOLCHAIN=path) by default in our images. I'll follow this up with a comment describing more long-form why I think this might be appropriate here, but wanted to get an issue open where it can be discussed properly.

@tianon
Copy link
Member Author

tianon commented Jun 27, 2023

@rsc I really hope you don't mind being pinged for your opinion (or help directing to the right folks for an opinion) on how we can best represent Go here; I've tried to make sure this has enough information for you help guide us without having to be totally well versed in how Go is commonly used in containers (just in case you aren't) ❤️ 😅 🙇

One of our highest goals in maintaining this image is to appropriately represent Go upstream in the way we believe they might represent themselves (https://github.com/docker-library/official-images/tree/78bad2f7ead1688af35d768fa1a23939fff77f89#what-are-official-images, bullet point 5). We're obviously not perfect at this (or balancing this with end user expectations/desires), but we do try very hard to do so unobtrusively.

This has come up most recently in the context of GOTOOLCHAIN (https://go.dev/doc/toolchain), with some users expecting/requesting that we set it explicitly to local in our images for 1.21+, and us hesitating to do so because the default was chosen explicitly and intentionally upstream (after some very long debates on the subject).

We do not want or intend to reopen those debates (and any unproductive comments attempting to do so here will be hidden as off topic), but we do think the way that users interact with this image currently perhaps does warrant a change in that default behavior.

Typically, users of this image will have a Dockerfile which looks something like this:

FROM golang:X.Y
# or
FROM golang:X.Y.Z
# ...
COPY my-code /some/path
RUN go build ...

As you can see, there is usually a very clear user intent that they desire Go version X.Y (or X.Y.Z), and the argument here is that it would be very surprising if a go.mod that contains a newer go X.Y+N were to not error, but rather download a newer toolchain (again, given the very clear user intent in the Dockerfile).

In short, the proposal is that we should set GOTOOLCHAIN=local (or perhaps GOTOOLCHAIN=path so that go get go@xxx still works appropriately and is used correctly, for users who might be mixing and matching multiple Go versions in a single image), but we want to make sure we're doing so in a way that is respectful to the origins of the default choice. ❤️

Any thoughts or opinions you might be willing to share would be deeply appreciated!! 🙇

@yosifkit
Copy link
Member

I am in favor of at least setting local but leaning more toward path to force users to know that they need a newer image (or to override the GOTOOLCHAIN that we will set). Auto-upgrading the go toolchain does not make sense for the container images that we have.

@rsc
Copy link

rsc commented Sep 15, 2023

It seems to me that it would be best to leave it at the default GOTOOLCHAIN=auto.
The upgrade only happens when the Go toolchain already running is older than what is required,
and if it does happen there will be a download line printed to standard error that will explain what's going on.

If you set GOTOOLCHAIN=local, then in the cases where a download would have happened,
the build will completely fail instead. That seems strictly worse and less helpful.

@neersighted
Copy link

neersighted commented Sep 15, 2023

I strongly object to GOTOOLCHAIN=auto because it can cause a relatively simple operation (e.g. go env) to take an unexpectedly long time/require network access, introduce a lengthy timeout, etc.

As the base image is rebuilt regularly, a FROM golang:1.y.z expresses a very strong intent to use a 1.y.z toolchain (as opposed to "the base image as it existed when 1.y.z was latest," which is what I am inferring your mental model is). It strikes me as very expected that I would get an error upon encountering a go.mod that requested a newer toolchain, and unexpected that I could (silently from the perspective of many automated usages of containers) download an entire toolchain independent of my intended version as expressed by my FROM line.

Especially in CI/semi-automated environments [scripts] (a large part of the use case for this base image), automatically upgrading the toolchain instead of failing early and informing the user what is wrong severely violates the principle of least surprise for me.

@rsc
Copy link

rsc commented Sep 15, 2023

In that case, setting GOTOOLCHAIN=local makes sense.
Setting GOTOOLCHAIN=path does not really sense given that rationale.

thaJeztah added a commit to thaJeztah/docker that referenced this issue Sep 22, 2023
Related discussion in docker-library/golang#472

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/docker that referenced this issue Sep 27, 2023
Related discussion in docker-library/golang#472

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/docker that referenced this issue Sep 28, 2023
Related discussion in docker-library/golang#472

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit aa28297)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@tianon
Copy link
Member Author

tianon commented Dec 9, 2023

Many belated thanks for your time/thoughts, Russ -- it's really appreciated (as is your upstream work) ❤️

matthewhughes934 added a commit to matthewhughes934/setup-go that referenced this issue Mar 3, 2024
Force `go` to always use the local toolchain (i.e. the one the one that
shipped with the go command being run) via setting the `GOTOOLCHAIN`
environment variable to `local`[1]:

> When GOTOOLCHAIN is set to local, the go command always runs the
bundled Go toolchain.

This is how things are setup in the official Docker images (e.g.[2], see
also the discussion around that change[3]). The motivation behind this
is to:

* Reduce duplicate work, the action will install a version of Go, a
  toolchain will be detected, the toolchain will be detected and then
  another version of Go installed
* Avoid Unexpected behaviour: if you specify this action runs with some Go
  version (e.g. `1.21.0`) but your go.mod contains a `toolchain` or `go`
  directive for a newer version (e.g. `1.22.0`) then, without any other
  configuration/environment setup, any go commands will be run using go
  `1.22.0`
* TODO: link image

This will be a **breaking change** for some workflows. Given a `go.mod`
like:

    module proj

    go 1.22.0

Then running any `go` command, e.g. `go mod tidy`, in an environment
where only go versions before `1.22.0` were installed would previously
trigger a toolchain download of Go `1.22.0` and that version being used
to execute the command. With this change the above would error out with
something like:

> go: go.mod requires go >= 1.22.0 (running go 1.21.7;
GOTOOLCHAIN=local)

[1] https://go.dev/doc/toolchain#select
[2] https://github.com/docker-library/golang/blob/dae3405a325073e8ad7c8c378ebdf2540d8565c4/Dockerfile-linux.template#L163
[3] docker-library/golang#472
codyoss added a commit to googleapis/testing-infra-docker that referenced this issue Aug 14, 2024
Note: we need to set toolchain to auto just for installing tools. We want to keep the image default of local for CI and fail if a dependency tries to update the go version.

Ref: docker-library/golang#472
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants