codegen tool: resolves GOBIN via go env
#132378
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Welcome @bartoszmajsak! |
|
Hi @bartoszmajsak. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bartoszmajsak The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
Using `go env GOBIN` to locate where codegen binaries should be installed provides a more reliable and flexible solution. - Picks up a user's custom `GOBIN` whether it's set as an environment variable or only in `go env` - Falls back to `$GOPATH/bin` if no `GOBIN` is defined This handles setups (e.g. using tools like [`mise`](https://mise.jdx.dev/lang/go.html)) where `GOBIN` is configured internally but not exported. The old lookup missed those internally-configured values and could invoke the wrong codegen binary. Signed-off-by: Bartosz Majsak <[email protected]>
There was a problem hiding this comment.
/hold
We expect GOBIN to be setup by kube::golang::setup_env, not sure why that isn't being used here
/assign @thockin
| @@ -116,7 +116,8 @@ function kube::codegen::gen_helpers() { | |||
| # shellcheck disable=2046 # printf word-splitting is intentional | |||
| GO111MODULE=on go install $(printf "k8s.io/code-generator/cmd/%s " "${BINS[@]}") | |||
| ) | |||
| # Go installs in $GOBIN if defined, and $GOPATH/bin otherwise | |||
|
|
|||
| GOBIN=$(go env GOBIN) # exported $GOBIN variable takes precedence, otherwise the local go env is taken | |||
There was a problem hiding this comment.
I am not at a computer to test, but doesn't this mean GOBIN is always defined, making the next line a no-op?
There was a problem hiding this comment.
That was my original assumption as well, after reading go help install
Executables are installed in the directory named by the GOBIN environment
variable, which defaults to $GOPATH/bin or $HOME/go/bin if the GOPATH
environment variable is not set. Executables in $GOROOT
are installed in $GOROOT/bin or $GOTOOLDIR instead of $GOBIN.
but it failed kubernetes-verify job. I think the problem boils down to the fact how we execute installed binaries.
The problem is that when unset GOBIN and go env -w GOBIN= are invoked, go install will perform this fallback logic, but it is not exposed in a way we could use it (at least, I do not know how to get it easily, as it's not set back to go env GOBIN).
There was a problem hiding this comment.
What I meant is this:
$ set -o nounset
$ unset GOBIN
$ declare -p GOBIN
bash: declare: GOBIN: not found
$ echo "${GOBIN}"
bash: GOBIN: unbound variable
It is unambiguously NOT set. But:
$ GOBIN=$(go env GOBIN)
$ declare -p GOBIN
declare -- GOBIN=""
$ echo "${GOBIN:-unset}"
unset
$ echo "${GOBIN}"
$ set -o nounset
$ unset GOBIN
$ declare -p GOBIN
bash: declare: GOBIN: not found
$ echo "${GOBIN}"
bash: GOBIN: unbound variable
$ GOBIN=$(go env GOBIN)
$ declare -p GOBIN
declare -- GOBIN=""
$ echo "${GOBIN:-unset}"
unset
$ echo "${GOBIN}"
It is now set (as per the shell) to the value "". The next line still works because `${VAR:-alt}` handles unset or null ("" counts as null, as defined by bash).
Still it makes me uncomfortable because it is too subtle. What if we go even more explicit. I tried this quickly, can you cross-check?
```diff
diff --git a/staging/src/k8s.io/code-generator/kube_codegen.sh b/staging/src/k8s.io/code-generator/kube_codegen.sh
index 478ddde11a6..f7907594c94 100755
--- a/staging/src/k8s.io/code-generator/kube_codegen.sh
+++ b/staging/src/k8s.io/code-generator/kube_codegen.sh
@@ -32,6 +32,19 @@ KUBE_CODEGEN_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)"
# before sourcing this file.
CODEGEN_VERSION_SPEC="${KUBE_CODEGEN_TAG:+"@${KUBE_CODEGEN_TAG}"}"
+# Go installs in $GOBIN if defined, and $GOPATH/bin otherwise. We want to know
+# which one it is, so we can use it later.
+function get_gobin() {
+ local from_env
+ from_env="$(go env GOBIN)"
+ if [[ -z "${from_env}" ]]; then
+ echo "${from_env}"
+ fi
+ echo "$(go env GOPATH)/bin"
+}
+GOBIN="$(get_gobin)"
+export GOBIN
+
function kube::codegen::internal::findz() {
# We use `find` rather than `git ls-files` because sometimes external
# projects use this across repos. This is an imperfect wrapper of find,
@@ -116,8 +129,6 @@ function kube::codegen::gen_helpers() {
# shellcheck disable=2046 # printf word-splitting is intentional
GO111MODULE=on go install $(printf "k8s.io/code-generator/cmd/%s " "${BINS[@]}")
)
- # Go installs in $GOBIN if defined, and $GOPATH/bin otherwise
- gobin="${GOBIN:-$(go env GOPATH)/bin}"
# Deepcopy
#
@@ -144,7 +155,7 @@ function kube::codegen::gen_helpers() {
-name zz_generated.deepcopy.go \
| xargs -0 rm -f
- "${gobin}/deepcopy-gen" \
+ "${GOBIN}/deepcopy-gen" \
-v "${v}" \
--output-file zz_generated.deepcopy.go \
--go-header-file "${boilerplate}" \
@@ -176,7 +187,7 @@ function kube::codegen::gen_helpers() {
-name zz_generated.validations.go \
| xargs -0 rm -f
- "${gobin}/validation-gen" \
+ "${GOBIN}/validation-gen" \
-v "${v}" \
--output-file zz_generated.validations.go \
--go-header-file "${boilerplate}" \
@@ -208,7 +219,7 @@ function kube::codegen::gen_helpers() {
-name zz_generated.defaults.go \
| xargs -0 rm -f
- "${gobin}/defaulter-gen" \
+ "${GOBIN}/defaulter-gen" \
-v "${v}" \
--output-file zz_generated.defaults.go \
--go-header-file "${boilerplate}" \
@@ -244,7 +255,7 @@ function kube::codegen::gen_helpers() {
for arg in "${extra_peers[@]:+"${extra_peers[@]}"}"; do
extra_peer_args+=("--extra-peer-dirs" "$arg")
done
- "${gobin}/conversion-gen" \
+ "${GOBIN}/conversion-gen" \
-v "${v}" \
--output-file zz_generated.conversion.go \
--go-header-file "${boilerplate}" \
@@ -367,8 +378,6 @@ function kube::codegen::gen_openapi() {
# shellcheck disable=2046 # printf word-splitting is intentional
GO111MODULE=on go install $(printf "k8s.io/kube-openapi/cmd/%s " "${BINS[@]}")
)
- # Go installs in $GOBIN if defined, and $GOPATH/bin otherwise
- gobin="${GOBIN:-$(go env GOPATH)/bin}"
local input_pkgs=( "${extra_pkgs[@]:+"${extra_pkgs[@]}"}")
while read -r dir; do
@@ -393,7 +402,7 @@ function kube::codegen::gen_openapi() {
-name zz_generated.openapi.go \
| xargs -0 rm -f
- "${gobin}/openapi-gen" \
+ "${GOBIN}/openapi-gen" \
-v "${v}" \
--output-file zz_generated.openapi.go \
--go-header-file "${boilerplate}" \
@@ -600,8 +609,6 @@ function kube::codegen::gen_client() {
# shellcheck disable=2046 # printf word-splitting is intentional
GO111MODULE=on go install $(printf "k8s.io/code-generator/cmd/%s " "${BINS[@]}")
)
- # Go installs in $GOBIN if defined, and $GOPATH/bin otherwise
- gobin="${GOBIN:-$(go env GOPATH)/bin}"
local group_versions=()
local input_pkgs=()
@@ -642,7 +649,7 @@ function kube::codegen::gen_client() {
|| true \
) | xargs -0 rm -f
- "${gobin}/applyconfiguration-gen" \
+ "${GOBIN}/applyconfiguration-gen" \
-v "${v}" \
--go-header-file "${boilerplate}" \
--output-dir "${out_dir}/${applyconfig_subdir}" \
@@ -665,7 +672,7 @@ function kube::codegen::gen_client() {
for arg in "${group_versions[@]}"; do
inputs+=("--input" "$arg")
done
- "${gobin}/client-gen" \
+ "${GOBIN}/client-gen" \
-v "${v}" \
--go-header-file "${boilerplate}" \
--output-dir "${out_dir}/${clientset_subdir}" \
@@ -687,7 +694,7 @@ function kube::codegen::gen_client() {
|| true \
) | xargs -0 rm -f
- "${gobin}/lister-gen" \
+ "${GOBIN}/lister-gen" \
-v "${v}" \
--go-header-file "${boilerplate}" \
--output-dir "${out_dir}/${listers_subdir}" \
@@ -704,7 +711,7 @@ function kube::codegen::gen_client() {
|| true \
) | xargs -0 rm -f
- "${gobin}/informer-gen" \
+ "${GOBIN}/informer-gen" \
-v "${v}" \
--go-header-file "${boilerplate}" \
--output-dir "${out_dir}/${informers_subdir}" \
@@ -772,8 +779,6 @@ function kube::codegen::gen_register() {
# shellcheck disable=2046 # printf word-splitting is intentional
GO111MODULE=on go install $(printf "k8s.io/code-generator/cmd/%s " "${BINS[@]}")
)
- # Go installs in $GOBIN if defined, and $GOPATH/bin otherwise
- gobin="${GOBIN:-$(go env GOPATH)/bin}"
# Register
#
@@ -800,7 +805,7 @@ function kube::codegen::gen_register() {
-name zz_generated.register.go \
| xargs -0 rm -f
- "${gobin}/register-gen" \
+ "${GOBIN}/register-gen" \
-v "${v}" \
--output-file zz_generated.register.go \
--go-header-file "${boilerplate}" \
What type of PR is this?
/kind bug
What this PR does / why we need it:
Using
go env GOBINto locate where codegen binaries should be installed provides a more reliable and flexible solution.GOBINwhether it's set as an environment variable or only ingo env$GOPATH/binif noGOBINis definedThis handles setups (e.g. using tools like
mise) whereGOBINis configured internally but not exported.The old lookup missed those internally configured values and could fail if the binary isn't present in the assumed location (or worse invoke the wrong codegen binary :)).
Which issue(s) this PR is related to:
Fixes #132377
Does this PR introduce a user-facing change?