ko stops building containers after Golang updates

[scothis]

After each minor Golang version bump (e.g. 1.13.2 -> 1.13.3), ko apply no longer builds container images but happily applies the yaml untransformed, which leads with image pull errors on the cluster. After reinstalling ko, it works again as expected.

Ideally ko would continue to work after a Golang update, but if not, it should fail loudly and demand to be reinstalled.

[jonjohnsonjr]

That doesn't seem right. Do you have any idea why?

[scothis]

That doesn't seem right. Do you have any idea why?

I don't, but it has hit me on each minor Golang 1.13.x update, and always goes away immediately after reinstalling.

I'm on a Mac using brew to install/update golang. I have seen some binaries hold on to the GOROOT used to build them, but they fail loudly as the previous GOROOT is removed on update.

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/scothis/Library/Caches/go-build"
GOENV="/Users/scothis/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/scothis/Development/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.4/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.4/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="<snip>"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/sp/sr4fwz_j565dltczdm2tvgvw0000gn/T/go-build589270524=/tmp/go-build -gno-record-gcc-switches -fno-common"

[jonjohnsonjr]

We pull the GOPATH from here which might explain that?

Default is the default Context for builds. It uses the GOARCH, GOOS, GOROOT, and GOPATH environment variables if set, or else the compiled code's GOARCH, GOOS, and GOROOT.

Happens here:

https://github.com/google/ko/blob/4833bb4a3e931c1bd9238578cdabc65c6ab6e87b/pkg/build/gobuild.go#L136

Not sure if there's a better way to do it...

[scothis]

Interesting.

My GOPATH isn't changing between golang releases. The GOROOT is not set as a env, but defined by go env GOROOT. The target I'm trying to build is a go module (running from inside the module), so perhaps that detection is failing and falling back to GOPATH.

[jonjohnsonjr]

The target I'm trying to build is a go module (running from inside the module), so perhaps that detection is failing and falling back to GOPATH.

Ah, weird. That check is just shelling out to:

$ go list -mod=readonly -m -json

I can't imagine why reinstalling would fix that. We call go/build.Import to check if a package is build-able. I wonder if the output of go list and the go/build.Import implementation are somehow coupled, so using different versions wouldn't work? It would be interesting to see if the go list output is different between two versions.

[jonjohnsonjr]

rsc's comment here hints at the problem:

A very real problem we've had with go/build is that tools built with one Go version are used with another Go version. Concretely, if I go get github.com/magefile/mage, we want that one binary to work no matter which version of Go I am using. I should be able to flip back and forth between, say, Go 1.10 and Go 1.11beta2, without having to maintain "mage for Go 1.10" vs "mage for Go 1.11beta2". Same for golint, etc. The fundamental mistake here is to allow tools to have a copy of the logic for where you get source code from. The last source layout change was the introduction of vendor directories and we ran into the same problem. There should be one canonical implementation - inside the go command. Other tools should invoke the go command to access this implementation. If you update to a new Go distribution, you get a new go command, and all the tools that invoke it automatically adapt to whatever has changed.

We seem to be hitting the bolded part, but I'm still not sure what is actually breaking.

We're completely swallowing any go list errors here because we still want to work without go modules, and writing warnings to stderr when there's nothing actually wrong seems aggressive. It might make sense to change the logging behavior based on the values of GO111MODULE and $PWD? I'd have to look through how the go command behaves here.

[jonjohnsonjr]

From the go 1.14 release notes:

The Context type has a new field Dir which may be used to set the working directory for the build. The default is the current directory of the running process. In module mode, this is used to locate the main module.

This might be something we can use.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[scothis]

/remove-lifecycle stale

[jonjohnsonjr]

I think we've improved this situation by surfacing better errors and requiring ko://, but I'm not sure if the underlying problem is actually fixed. This reminds me of a discussion on slack where the issue involved homebrew, and the resolution was to:

$ export GOROOT=/usr/local/go

Which previously returned:

$ go env GOROOT
/usr/local/Cellar/go/1.15.5/libexec

My theory is that there are multiple go installations, and we're getting confused about which one to use in here somewhere.

@scothis are you still running into this? Does this sound plausible?

---

Edit: Nevermind! I see that this has been already discussed in #218

I wonder how problematic it would be to just set GOROOT to $(go env GOROOT)?

Looking at it:

{GOARCH:amd64 GOOS:linux GOROOT:/nix/store/1drmsvcyf0mlzhs3yzwblrwhv4hvqkkx-go-1.15.5/share/go GOPATH:/home/jonjohnson/go Dir: CgoEnabled:true UseAllFiles:false Compiler:gc BuildTags:[] ReleaseTags:[go1.1 go1.2 go1.3 go1.4 go1.5 go1.6 go1.7 go1.8 go1.9 go1.10 go1.11 go1.12 go1.13 go1.14 go1.15] InstallSuffix: JoinPath:<nil> SplitPathList:<nil> IsAbsPath:<nil> IsDir:<nil> HasSubdir:<nil> ReadDir:<nil> OpenFile:<nil>}

It seems pretty reasonable to do this. Everything else seems pretty static, though I wonder how we'd handle BuildTags and ReleaseTags?

[scothis]

are you still running into this?

Not sure. I've gotten into the habit of reinstalling ko automatically after upgrading golang

[jonjohnsonjr]

We could do something stupid like:

// somewhere we don't expect the build to fail (perhaps even checking the error)
if err != nil {
  if goroot, execErr := exec("go env GOROOT"); execErr == nil {
    if us, them := gb.buildContext.GOROOT, goroot; us != them {
      return fmt.Errorf("ko was installed with GOROOT=%q, but `go env GOROOT`=%q, see %s to fix this; build failed: %v", us, them, issue106URL, err)
    }
  }
}

And suggest export GOROOT=$(go env GOROOT) or reinstalling ko at the top of this issue.

We also currently shell out to get module info: https://github.com/google/ko/blob/522c37c4e0ec9537068f16e8f1652f969377b62e/pkg/build/gobuild.go#L104-L147

We could concurrently shell out to go env GOROOT and just log a warning at the beginning of execution that these are different. Or we could set gb.buildContext.GOROOT to the result and log a warning?

[scothis]

This also bites the version of ko distributed via brew. Setting GOROOT explicitly helps GOROOT=$(go env GOROOT).

Annoyingly, despite build paths being prefixed with ko://, it fails silently passing the unresolved ko:// value.

[jonjohnsonjr]

Annoyingly, despite build paths being prefixed with ko://, it fails silently passing the unresolved ko:// value.

This shouldn't be happening anymore... with #281 and #275 I thought we would fail loudly...

Looks like 281 didn't get into the v0.7.0 cut. Let me send a PR to "fix" this, then we can cut v0.8.0 that includes a bunch of fixes.

Edit: in fact, @scothis can you test that HEAD doesn't fail silently?

[scothis]

@jonjohnsonjr sorry I missed this. With 0.8 I see the new warning with the proper fallback by default. 👍