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

build: try creating the go bin directory #19862

Merged
merged 4 commits into from
Apr 17, 2023
Merged

Conversation

robmonte
Copy link
Member

@robmonte robmonte commented Mar 30, 2023

Currently if the $GOPATH/bin directory does not exist, the binary will fail to be copied to the location.

I don't believe an if statement is needed to check for directory existence before the mkdir command, it simply does nothing when the directory already exists.

Closes #7170

@robmonte robmonte requested review from a team and ncabatoff March 30, 2023 17:41
Copy link
Contributor

@raymonstah raymonstah left a comment

Choose a reason for hiding this comment

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

Curious if this stemmed from an open issue.

@robmonte
Copy link
Member Author

robmonte commented Mar 31, 2023

Curious if this stemmed from an open issue.

@raymonstah Not that I've seen, no. I encountered this myself when setting up a fresh EC2 instance to build and run Vault, and thought it'd be a good idea. However I came across this old issue: #7170

It kinda makes sense to me that some users with GOBIN set may want it in that location. Though I think this location is usually meant for Go packages when using go install and not technically for build artifacts? At least that's my understanding if someone can correct me.

 

Curious what others would think of something like this for line 53:
IFS=: FIRST=($GOPATH) BIN_PATH=${GOBIN:-${FIRST}/bin}

This sets BIN_PATH to the values in this order:

  1. If GOBIN is set, always set BIN_PATH to GOBIN (ignore GOPATH)
  2. If only GOPATH is set, set BIN_PATH to FIRST, which itself is set to the first element of GOPATH with /bin appended
  3. If neither are set, BIN_PATH remains unset (uses Go's default of $HOME/go)

Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

This might warrant a changelog entry, not so much for vault users as for vault packagers; conceivably this might break someone's build script.

@robmonte robmonte enabled auto-merge (squash) April 17, 2023 22:35
@robmonte robmonte merged commit aae6c65 into main Apr 17, 2023
@robmonte robmonte deleted the check-build-output-path branch April 17, 2023 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make dev should default to GOBIN
4 participants