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

adopt go1.13 #82531

Open
liggitt opened this issue Sep 10, 2019 · 16 comments

Comments

@liggitt
Copy link
Member

commented Sep 10, 2019

Tasks to move to go1.13:

Things we should do after moving to go1.13 (in 1.17-only after fundamental build changes required for go1.13 are picked back to 1.16):

  • fix gengo vendor assumptions (exploratory hacky POC in https://github.com/liggitt/kubernetes/commits/go-1.13)
  • fix excessive go list use in build scripts causing extremely long build times in go1.13 (go list is fast in go1.12, slow in go1.13)
  • remove $GOPATH requirements and fake _output $GOPATH setup from build code (build with modules+vendor)

/cc @fejta @BenTheElder @cblecker
/sig release

@aanm

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

fix excessive go list use in build scripts causing extremely long build times in go1.13 (go list is fast in go1.12, slow in go1.13)

@liggitt The only way I found out to keep using go list without long build times was to run with GO111MODULE=off for go lint commands.

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

I think @thockin was looking into running the command once and caching/reusing the results for the remainder of the make invocation

@carlosedp

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

I'd like to bring to attention that beginning with Go 1.13, the command go mod vendor will not include files tagged with // +build ignore so some CI validation might fail with vendor dirs created with this version.
More info on golang/go#33353 and golang/go#31088.

@bcmills

This comment has been minimized.

Copy link

commented Sep 13, 2019

go list is fast in go1.12, slow in go1.13

That may be a side-effect of the fix for golang/go#29773, since it seems that you do have a lot of dependencies through older versions from the main module (due to heketi/heketi#1648).

If the go list performance regression you're seeing is not attributable to that, then please do file an issue — I'm not aware of anything else that should have slowed it down at all.

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

Hmm, maybe it wasn't that go list was slower, it was that it started having side effects, and the side-effect-free alternatives were slower. xref golang/go#31087

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

heketi/heketi issue resolved in heketi/heketi#1649, vendored copy bumped in #82805

@thockin

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

The things that call go list have to be totally rewritten.

golang/go#31087

@thockin

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

compiling with 1.13 gives me:

$ make kubectl
go/build: importGo k8s.io/kubernetes: exit status 1
can't load package: package k8s.io/kubernetes: cannot find module providing package k8s.io/kubernetes


+++ [0917 16:03:22] Building go targets for linux/amd64:
    ./vendor/k8s.io/code-generator/cmd/deepcopy-gen
go: finding k8s.io/kubernetes/vendor latest
go: finding k8s.io/kubernetes/vendor/k8s.io/code-generator/cmd/deepcopy-gen latest
go: finding k8s.io/kubernetes/vendor/k8s.io latest
go: finding k8s.io/kubernetes/vendor/k8s.io/code-generator latest
go: finding k8s.io/kubernetes/vendor/k8s.io/code-generator/cmd latest
can't load package: package k8s.io/kubernetes/vendor/k8s.io/code-generator/cmd/deepcopy-gen: no matching versions for query "latest"
!!! [0917 16:03:24] Call tree:
!!! [0917 16:03:24]  1: /usr/local/google/home/thockin/src/k/kubernetes/hack/lib/golang.sh:714 kube::golang::build_some_binaries(...)
!!! [0917 16:03:24]  2: /usr/local/google/home/thockin/src/k/kubernetes/hack/lib/golang.sh:853 kube::golang::build_binaries_for_platform(...)
!!! [0917 16:03:24]  3: hack/make-rules/build.sh:27 kube::golang::build_binaries(...)
!!! [0917 16:03:24] Call tree:
!!! [0917 16:03:24]  1: hack/make-rules/build.sh:27 kube::golang::build_binaries(...)
!!! [0917 16:03:24] Call tree:
!!! [0917 16:03:24]  1: hack/make-rules/build.sh:27 kube::golang::build_binaries(...)
make[1]: *** [Makefile.generated_files:200: _output/bin/deepcopy-gen] Error 1
make: *** [Makefile:559: generated_files] Error 2

At the bottom it calls go install k8s.io/kubernetes/./vendor/k8s.io/code-generator/cmd/deepcopy-gen. I forget exactly why that prefix was needed but it was. If I remove it, it passes.

@thockin

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

...but the build fails later:

$ make kubectl
go/build: importGo k8s.io/kubernetes: exit status 1
can't load package: package k8s.io/kubernetes: cannot find module providing package k8s.io/kubernetes


+++ [0917 16:05:28] Building go targets for linux/amd64:
    ./vendor/k8s.io/code-generator/cmd/deepcopy-gen
go: downloading k8s.io/klog v0.4.0
go: downloading k8s.io/gengo v0.0.0-20190822140433-26a664648505
go: extracting k8s.io/klog v0.4.0
go: extracting k8s.io/gengo v0.0.0-20190822140433-26a664648505
go: downloading golang.org/x/tools v0.0.0-20190621195816-6e04913cbbac
go: extracting golang.org/x/tools v0.0.0-20190621195816-6e04913cbbac
go: finding k8s.io/gengo v0.0.0-20190822140433-26a664648505
go: finding k8s.io/klog v0.4.0
go: finding golang.org/x/tools v0.0.0-20190621195816-6e04913cbbac
F0917 16:06:34.716074  242494 main.go:82] Error: Failed making a parser: unable to add directory "k8s.io/kubernetes/vendor/k8s.io/api/admission/v1": unable to import "k8s.io/kubernetes/vendor/k8s.io/api/admission/v1": go/build: importGo k8s.io/kubernetes/vendor/k8s.io/api/admission/v1: exit status 1
go: finding k8s.io/kubernetes/vendor/k8s.io latest
go: finding k8s.io/kubernetes/vendor latest
go: finding k8s.io/kubernetes/vendor/k8s.io/api latest
go: finding k8s.io/kubernetes/vendor/k8s.io/api/admission latest
can't load package: package k8s.io/kubernetes/vendor/k8s.io/api/admission/v1: no matching versions for query "latest"

make[1]: *** [Makefile.generated_files:152: gen_deepcopy] Error 1
make: *** [Makefile:559: generated_files] Error 2
@fejta

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

We use modules, golang1.13 (and bazel) in test-infra. The way we found to get the generators to work is to temporarily disable modules:

https://github.com/kubernetes/test-infra/blob/master/hack/update-codegen.sh#L119

Here's how kind does the same thing without bazel's help:

https://github.com/kubernetes-sigs/kind/blob/master/hack/update/generated.sh

Does GO111MODULE=off change anything?

Are we sure we need to generate code for people on the fly? We could instead ask people to call go generate explicitly when they want to regenerate things.

@thockin

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

if we depend on people calling go generate they WILL run tests without having regenerated code. This is why we did on-the-fly in the first place

@fejta

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

We can have presubmits catch this though, right? AKA a presubmit can regenerate and fail if anything changes.

This is how test-infra handles things. We don't generate for people, but presubmits will fail if they forget:

It seems like go is pretty committed to making people use this pattern? It seems hard to reconcile doing anything else with the expectation that you can go get code.

For example historically we generated protos on the fly with bazel, but it caused enough annoyance that we abandoned it in favor of an update-protos.sh (and verify-protos.sh) script and checking in the .pb.go files... Paradoxically people seem a lot happier since this change, despite the need to update manually.

@thockin

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

What a miserable experience, but I guess, maybe?

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

At the bottom it calls go install k8s.io/kubernetes/./vendor/k8s.io/code-generator/cmd/deepcopy-gen. I forget exactly why that prefix was needed but it was. If I remove it, it passes.

That's where I ended up in https://github.com/liggitt/kubernetes/commits/go-1.13 as well

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2019

The immediate motivation to get to go1.13 is for support reasons. Supported go versions last ~1 year, we're six months into go1.12, and we're about to ship k8s 1.16 and support it for ~9 months. My ideal scenario is to get a small set of changes in build/generation scripts we can pick to release-1.16 to get it into a go version supported for its lifetime.

Long term, I'm hopeful the move to go1.13 and to using modules in build scripts will let us clean out a lot of GOPATH cruft we've accumulated, though I'd expect that cleanup to be more invasive and hopefully limited to k8s 1.17 only.

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2019

looks like simply setting GO111MODULE=off in init.sh works around the build issues... that could be a reasonable first step to validate go1.13, and consider picking back to 1.16 (not urgently, just sometime in the next few months)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.