replace go_container.sh with gimme #1769
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| PATH:=$(shell . hack/build/setup-go.sh && echo "$${PATH}") | ||
| export PATH | ||
| # work around broken PATH export | ||
| SHELL:=env PATH=$(PATH) $(SHELL) |
There was a problem hiding this comment.
this is the trickiest bit, at least on macOS default make, without this you fail to actually set path in trivial commands.
https://stackoverflow.com/questions/8941110/how-i-could-add-dir-to-path-in-makefile put me on the right track
| all: build | ||
| # build flags for the kind binary | ||
| # - reproducible builds: -trimpath and -ldlflags=-buildid= | ||
| # - smaller binaries: -w (trim debugger data, but not panics) |
There was a problem hiding this comment.
forgot to mention in the PR body, 9.1M => 7M on macOS
| # ========================= Setup Go With Gimme ================================ | ||
| # go version to use for build etc. | ||
| # setup correct go version with gimme | ||
| PATH:=$(shell . hack/build/setup-go.sh && echo "$${PATH}") |
There was a problem hiding this comment.
- GOROOT is not required since go.19, go will look next to the go binary on it's own
- I don't think GOOS / GOARCH are relevant here
- PATH is how we inject the go version we want
This part is also not striclty necessary after a later commit, where scripts try to ensure this themselves
There was a problem hiding this comment.
@amwat pointed out that gvm sets this (and probably other tools, including gimme when used natively), and when set to a different value in the host environment that causes us to potentially use the gimme go binary against some other goroot. we mitigate that below now by clearing GOROOT so it will be auto-detected instead.
|
@BenTheElder why are you adding the gimme script? |
to version it and avoid having to download it from the internet each time |
|
not really related to this PR I think, but amwat discovered some issues with go1.15 and trying to use vendor if vendor is present. |
|
I prefer this to the docker build honestly, defer to @amwat the final lgtm |
|
/retest |
|
verify now has an inexplicable go.sum issue after da5b190 |
Makefile
Outdated
| clean: clean-output clean-cache | ||
| clean: | ||
| rm -rf $(OUT_DIR)/ | ||
| # TODO: remove this in the future. We no longer populat vendor |
There was a problem hiding this comment.
nit: dedent
this currently prints out as a shell command
|
|
||
| # only setup go if we haven't set FORCE_HOST_GO, or `go version` doesn't match | ||
| # go version output looks like: | ||
| # go version go1.14.5 darwin/amd64 |
There was a problem hiding this comment.
something somewhere is outputting the go version, can't seem to pinpoint where :/
There was a problem hiding this comment.
it's related to gimme, I've failed to silence it properly
gimme has an env to disable this, but it only works if used during the first time setting up that version, they don't regenerate the .evn file. not sure what the best fix is yet.
There was a problem hiding this comment.
it is mostly cosmetic though
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/retest |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/retest |
|
what's one more flake |
|
(I speedread this, but I just wanted to drop by and say I'm excited to see it!) |
|
this is allowing us to upgrade go versions immediately, instead of waiting a fairly long time for the |
|
#1771 is within 30m of the release announcement (just when I got around to it) instead of checking back repeatedly for the image to be live |
speeds up builds. streamlines a lot of the tools (they can just invoke go without containerization hoops).
still should be reproducible with go1.13+
also the .go-version file should be goenv compatible