This repository has been archived by the owner on Jul 30, 2021. It is now read-only.
Makefile: Respect GOBIN #949
Merged
Conversation
This file contains 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
|
Can one of the admins verify this patch? |
3 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
k8s-ci-robot
added
cncf-cla: yes
|
Should we just call |
That sounds good to me, but I ran into golang/go#18981 attempting it. That issue was fixed by golang/go@8f70e1f8a91 in Go 1.10. Are we comfortable bumping our minimum version? Would you prefer on of the other workarounds mentioned in the upstream image? Or are you ok with this PR as it stands until we are comfortable making Go 1.10 the minimum version? |
|
Yes. I think it's fine to bump to 1.10
…On Tue, Apr 3, 2018 at 9:45 AM W. Trevor King ***@***.***> wrote:
Should we just call go install instead?
That sounds good to me, but I ran into golang/go#18981
<golang/go#18981> attempting it. That issue was
fixed by ***@***.***
<golang/go@8f70e1f8a91> in Go 1.10. Are we
comfortable bumping our minimum version? Would you prefer on of the other
workarounds mentioned in the upstream image? Or are you ok with this PR as
it stands until we are comfortable making Go 1.10 the minimum version?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#949 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACO_XRix5bqCbHinKCMvgxMavGbscdW1ks5tk6bfgaJpZM4S_vOG>
.
|
GOBIN (documented in [1]) defaults to DIR/bin, but it's a configurable variable in its own right. The old logic was just looking at GOPATH, though, and not respecting GOBIN. This commit updates our install target to lean on Go's build caching (instead of using Make's dependency trees) to ensure a fresh-enough build lands in the appropriate directory. This approach relies on Go 1.10+ to avoid [2], but we've required Go 1.10+ since b859ebf (Documentation/development: Bump minimum Go version to 1.10, 2018-04-03, kubernetes-retired#955). For the build documentation, I've switched to 'go env GOPATH' to get the (platform-specific [1]) default value when the environment variable is not set. And I've used cut [3] (instead of the awk [4] the Makefile used to use) to pull out the first component of GOPATH. Both are in POSIX, but cut is a simpler tool for this particular problem. [1]: https://golang.org/cmd/go/#hdr-GOPATH_environment_variable [2]: golang/go#18981 [3]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cut.html [4]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/awk.html
|
Rebased around #955 and converted to |
|
ok to test |
rphillips
approved these changes
Apr 3, 2018
|
Hmm, I seem to have missed the |
wking
added a commit
to wking/bootkube
that referenced
this pull request
Apr 3, 2018
This was my intention with dbf0b6a (Makefile: Use 'go install ...' for the install target, 2018-04-03, kubernetes-retired#949), but I seem to have missed the actual code :/. Instead, that command landed an earlier implementation which we intended to drop based on kubernetes-retired#949 review [1]. This commit has the intended implementation. For detailed motivation, see the description in dbf0b6a. [1]: kubernetes-retired#949 (comment)
wking
added a commit
to wking/bootkube
that referenced
this pull request
Apr 3, 2018
This was my intention with dbf0b6a (Makefile: Use 'go install ...' for the install target, 2018-04-03, kubernetes-retired#949), but I seem to have missed the actual code :/. Instead, that commit landed an earlier implementation which we intended to drop based on kubernetes-retired#949 review [1]. This commit has the intended implementation. For detailed motivation, see the description in dbf0b6a. [1]: kubernetes-retired#949 (comment)
rphillips
pushed a commit
that referenced
this pull request
Apr 3, 2018
This was my intention with dbf0b6a (Makefile: Use 'go install ...' for the install target, 2018-04-03, #949), but I seem to have missed the actual code :/. Instead, that commit landed an earlier implementation which we intended to drop based on #949 review [1]. This commit has the intended implementation. For detailed motivation, see the description in dbf0b6a. [1]: #949 (comment)
This was referenced Apr 3, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
GOBIN(documented here) defaults toDIR/bin, but it's a configurable variable in its own right. This command updates our calculations, using?=(documented here) to avoid expensive shell operations when we don't actually need the result (because a different target was called or because the user provided their own value).I've also used
go env GOPATHto get the (platform-specific) default value when the environment variable is not set.And I've used
cutinstead ofawkto pull out the first component ofGOPATH. Both are in POSIX, butcutis a simpler tool for this particular problem.This will have trivial context conflicts with the in-flight #948. I'm happy to rebase the other if/when one of them lands.