✨ Makefile: handle $GOPATH with a trailing slash #4768
Conversation
When a $GOPATH contains a trailing slash, the current check for string equality between the repo root and the computed path does not pass, even though it should. Passing both sides of the check through `abspath` makes this check more robust. Signed-off-by: Steve Kuznetsov <[email protected]>
k8s-ci-robot
added
the
do-not-merge/release-note-label-needed
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
lgtm
| @@ -76,7 +76,7 @@ DOCKER_BUILDKIT=1 | |||
| export ACK_GINKGO_DEPRECATIONS := 1.16.4 | |||
|
|
|||
| # Set --output-base for conversion-gen if we are not within GOPATH | |||
| ifneq ($(abspath $(REPO_ROOT)),$(shell go env GOPATH)/src/sigs.k8s.io/cluster-api-provider-aws) | |||
| ifneq ($(abspath $(REPO_ROOT)),$(abspath $(shell go env GOPATH)/src/sigs.k8s.io/cluster-api-provider-aws)) | |||
There was a problem hiding this comment.
To clarify, this seems ok given that go env GOPATH enforces absolute path
There was a problem hiding this comment.
abspath is an idempotent function.
Your concern is that, for some user, this logic would have worked before but will be broken after this change. Let's investigate that.
If the workflow was functional for the user before, we know that this ifneq evaluated to false, or, that $( abspath $(REPO_ROOT)) was equal to the string on the right hand side of the expression.
Therefore, we know that the value on the right hand side of the expression must already be in the output set of abspath.
My change is to run that value through abspath again. Given that abspath is idempotent (running the function on an absolute path returns the input, since it's already an absolute path), it's provably correct here to apply abspath once more - given some user for whom this logic works before my change, it is not possible for the addition of the abspath call to change that.
So, I'd re-word your comment: it does not seem ok, it must logically be ok, and it does not rely on go env GOPATH outputting an absolute path. It is provable regardless of the output that if this check passed before passing the second argument through abspath it must pass after.
|
/retest |
|
/lgtm |
|
/lgtm cc @vincepri for final approval based on open comment |
|
thanks @Ankitasw |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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
the
approved
When a $GOPATH contains a trailing slash, the current check for string equality between the repo root and the computed path does not pass, even though it should. Passing both sides of the check through
abspathmakes this check more robust.What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist:
Release note: