Makefile: Respect GOBIN #774
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
I am fine with this change, as soon as you get the tests to pass. :^( |
Contributor
Author
|
Looks like And this is because Travis is explicitly setting |
wking
force-pushed
the
gobin
branch
2 times, most recently
from
efaed01 to
12ab406
Compare
Member
|
Tests look to be passing. LGTM. |
Contributor
Author
|
On Tue, May 15, 2018 at 04:35:31PM -0700, Matthew Heon wrote:
Tests look to be passing. LGTM.
My commit message was stale, since I'd shifted some of the ?= around.
I've just pushed 12ab406 -> 44111b7 updating the commit message (and
that's all it does, so I expect it to pass as well ;).
|
Member
Collaborator
|
📌 Commit 44111b7 has been approved by |
Collaborator
|
⌛ Testing commit 44111b7 with merge 5fbe959... |
Collaborator
|
💔 Test failed - status-papr |
Member
|
Homu is a known flake |
Collaborator
|
🔒 Merge conflict |
Collaborator
|
☔ The latest upstream changes (presumably 1aaf8df) made this pull request unmergeable. Please resolve the merge conflicts. |
Member
|
Sorry @wking Needs rebase, but it was probably caused by another one of your PRs. :^) |
And use 'go env GOBIN' to detect the user's existing preference. From [1]: > The bin directory holds compiled commands. Each command is named > for its source directory, but only the final element, not the entire > path. That is, the command with source in DIR/src/foo/quux is > installed into DIR/bin/quux, not DIR/bin/foo/quux. The "foo/" > prefix is stripped so that you can add DIR/bin to your PATH to get > at the installed commands. If the GOBIN environment variable is > set, commands are installed to the directory it names instead of > DIR/bin. GOBIN must be an absolute path. > ... > Go searches each directory listed in GOPATH to find source code, but > new packages are always downloaded into the first directory in the > list. So if GOBIN is set, it will be non-empty, and we can use $(GOBIN)/... If GOBIN is unset, 'go env GOBIN' will return an empty string (as it does on Travis [2]). In that case, I'm assuming that the package in question is in the first directory in GOPATH and using the new FIRST_GOPATH (firstword and subst are documented in [3]). That's probably fairly safe, since our previous GOPATH handling assumed it only contained a single path, and nobody was complaining about that. Using ?= allows us to skip the 'dirname' call if we end up not needing GOPKGBASEDIR [4] (e.g. for the 'help' target). The recursive expansion could cause an issue if the result of the shell expansions included a '$', but those seem unlikely in GOPKGBASEDIR, GOMD2MAN, or the manpage paths. I haven't used ?= for GOBIN, because we'll always need the expanded value for the if check. Using GOMD2MAN allows us to collapse old ||-based recipe into a less confusing invocation. And using a static pattern rule [5] for $(MANPAGES) lets us write a single rule to handle both section 1 and section 5. While I was updating the GOPATH handling, I moved .gopathok from the possibly-shared $(GOPATH)/.gopathok to the definitely-specific-to-this-project .gopathok. That may cause some issues if you rebuild after changing your GOPATH without calling 'clean', but I don't expect folks to change their GOPATH frequently. And the old approach would fail if different consumers were also using the same flag path to mean something else (as CRI-O does [6]). As part of cleaning up .gopathok, I've also collapsed clean's rm calls into a single invocation. That will give us the same results with less process setup/teardown penalties. [1]: https://golang.org/cmd/go/#hdr-GOPATH_environment_variable [2]: https://travis-ci.org/projectatomic/libpod/jobs/379345071#L459 [3]: https://www.gnu.org/software/make/manual/html_node/Text-Functions.html [4]: https://www.gnu.org/software/make/manual/html_node/Setting.html [5]: https://www.gnu.org/software/make/manual/html_node/Static-Usage.html [6]: https://github.com/kubernetes-incubator/cri-o/blob/v1.10.1/Makefile#L62 Signed-off-by: W. Trevor King <wking@tremily.us>
Contributor
Author
Member
Collaborator
|
📌 Commit e63f3f3 has been approved by |
Collaborator
Collaborator
|
☀️ Test successful - status-papr |
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
And use
go env GOBINto detect the user's existing preference.Using
?=allows us to skip thego envcall if we already haveGOBINin the environment, or if we don't needGOBINat all (e.g. for thehelptarget). The recursive expansion could cause an issue if the result of the shell expansions included a$, but those seem unlikely inGOBIN,GOMD2MAN, or the manpage paths.Using
GOMD2MANallows us to collapse old||-based recipe into a less confusing invocation. And using a static pattern rule for$(MANPAGES)lets us write a single rule to handle both section 1 and section 5.