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.

Sign up for GitHub

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

✨ Makefile: handle $GOPATH with a trailing slash #4768

Merged
merged 1 commit into from
Feb 7, 2024
Merged

✨ Makefile: handle $GOPATH with a trailing slash #4768

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, this seems ok given that go env GOPATH enforces absolute path

Copy link
Contributor Author

@stevekuznetsov stevekuznetsov Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

GEN_OUTPUT_BASE := --output-base=$(REPO_ROOT)
else
export GOPATH := $(shell go env GOPATH)
Expand Down