Further improve GOPATH detection in Makefile #24387
Conversation
|
This change is |
In recent versions of go, GOPATH need not be explicitly set (and `go env GOPATH` will return nothing) if using the default $HOME/go as $GOPATH. Additionally, it is necessary to use `readlink -f` to get the real paths of both $GOPATH and $CWD, as symlinks may be in play. For instance, on FreeBSD `/home` is symlinked to `/usr/home/`, but the former is often used. If `readlink` isn't used, the string-based prefix matching will incorrectly claim that CWD is not a child of GOPATH. This patch sets GOPATH to `$HOME/go` if GOPATH is not exported and `$HOME/go` exists (since `readlink -f $HOME/go` will return an empty string if its target is not found). No change in the existing behavior is expected if the environment variable GOPATH is not blank or if it is both blank and $HOME/go is not the GOPATH. Release note: None
Makefile
Outdated
| # GOPATH need not be explicitly set if it is the default $HOME/go | ||
| ifeq ($(strip $(GOPATH)),) | ||
| GOPATH := $(shell readlink -f $$HOME/go) | ||
| endif |
There was a problem hiding this comment.
Hmm, I can't reproduce the need for this. go env GOPATH does the right thing for me when GOPATH is not set in the environment—and in fact that's why we don't use GOPATH directly. Does the following return a different result for you?
$ unset GOPATH && go env GOPATH
/home/benesch/go
There was a problem hiding this comment.
mqudsi@mysql ~/g/s/g/c/cockroach> go version
go version go1.10.1 freebsd/amd64
mqudsi@mysql ~/g/s/g/c/cockroach> set -e GOPATH; go env GOPATH
mqudsi@mysql ~/g/s/g/c/cockroach> env GOPATH="" go env GOPATHThere was a problem hiding this comment.
However, on another machine:
mqudsi@freebsd ~/g/s/g/c/cockroach> go version
go version go1.10.1 freebsd/amd64
mqudsi@freebsd ~/g/s/g/c/cockroach> env GOPATH="" go env GOPATH
/home/mqudsi/goNot sure what to make of this. A golang bug, perhaps?
There was a problem hiding this comment.
Check this out:
$ HOME= GOPATH= go env GOPATH
$
Seems like maybe $HOME is unset? Or os/user.Current is otherwise bricked on that machine?
There was a problem hiding this comment.
Nope, $HOME is correct. This was a new deployment of both go and cockroach, just installed. cockroach is the first go installation on this machine. Perhaps the clue lies therein somewhere?
There was a problem hiding this comment.
Ok, I've come up with another way of causing this situation:
$ GOROOT=$HOME/go GOPATH= go env GOPATH
$
Does that perhaps describe your situation? If not, could you post the full output of go env?
There was a problem hiding this comment.
You nailed it. GOPATH and GOROOT can't both be the same, so that breaks the auto-generation of GOPATH.
I guess the best thing here is to error out if go env GOPATH returns an empty result.
Makefile
Outdated
| @@ -201,7 +201,11 @@ UI_ROOT := $(PKG_ROOT)/ui | |||
| SQLPARSER_ROOT := $(PKG_ROOT)/sql/parser | |||
|
|
|||
| # Ensure we have an unambiguous GOPATH. | |||
| GOPATH := $(shell $(GO) env GOPATH) | |||
| GOPATH := $(shell readlink -f $(shell $(GO) env GOPATH)) | |||
There was a problem hiding this comment.
I'm afraid this won't work on macOS.
$ readlink -f
readlink: illegal option -- f
usage: readlink [-n] [file ...]
There was a problem hiding this comment.
Make's built-in realpath function might do what you need, though: https://www.gnu.org/software/make/manual/html_node/File-Name-Functions.html
There was a problem hiding this comment.
I'm a bmake guy, so thanks for pointing me in the right direction.
There was a problem hiding this comment.
@benesch regarding CURDIR, I meant that the value of the two variables compared here:
ifeq "$(filter $(GOPATH)%,$(CURDIR))" ""may differ in "symlinkage" while pointing to the same (with possibly either one using the symlink'd $HOME value).
There was a problem hiding this comment.
Ah, I see. It appears that $(CURDIR) has internally been set to the equivalent of $(realpath $(shell pwd)).
For compatibility with macOS. Thanks to @benesch. Release note: None
| @@ -201,7 +201,11 @@ UI_ROOT := $(PKG_ROOT)/ui | |||
| SQLPARSER_ROOT := $(PKG_ROOT)/sql/parser | |||
|
|
|||
| # Ensure we have an unambiguous GOPATH. | |||
| GOPATH := $(shell $(GO) env GOPATH) | |||
| GOPATH := $(realpath $(shell $(GO) env GOPATH)) | |||
There was a problem hiding this comment.
This fix looks good. Thanks!
Makefile
Outdated
| # GOPATH need not be explicitly set if it is the default $HOME/go | ||
| ifeq ($(strip $(GOPATH)),) | ||
| GOPATH := $(realpath $(shell echo $$HOME/go)) | ||
| endif |
There was a problem hiding this comment.
I'd prefer not to commit this workaround yet. I think it's papering over something more serious.
Makefile
Outdated
| @@ -201,7 +201,11 @@ UI_ROOT := $(PKG_ROOT)/ui | |||
| SQLPARSER_ROOT := $(PKG_ROOT)/sql/parser | |||
|
|
|||
| # Ensure we have an unambiguous GOPATH. | |||
| GOPATH := $(shell $(GO) env GOPATH) | |||
| GOPATH := $(shell readlink -f $(shell $(GO) env GOPATH)) | |||
There was a problem hiding this comment.
Ah, I see. It appears that $(CURDIR) has internally been set to the equivalent of $(realpath $(shell pwd)).
|
Perfect, that sounds good.
…On Sat, Mar 31, 2018 at 5:01 PM Mahmoud Al-Qudsi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Makefile
<#24387 (comment)>
:
> @@ -201,7 +201,11 @@ UI_ROOT := $(PKG_ROOT)/ui
SQLPARSER_ROOT := $(PKG_ROOT)/sql/parser
# Ensure we have an unambiguous GOPATH.
-GOPATH := $(shell $(GO) env GOPATH)
+GOPATH := $(shell readlink -f $(shell $(GO) env GOPATH))
+# GOPATH need not be explicitly set if it is the default $HOME/go
+ifeq ($(strip $(GOPATH)),)
+GOPATH := $(shell readlink -f $$HOME/go)
+endif
You nailed it. GOPATH and GOROOT can't both be the same, so that breaks
the auto-generation of GOPATH.
I guess the best thing here is to error out if go env GOPATH returns an
empty result.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24387 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA15IPWFkfJWFrLpdvXlGT-i9FsOmzBhks5tj-6dgaJpZM4TCkIF>
.
|
If `go env GOPATH` does not return a workable value, that means there is something wrong with the golang deployment and it is unsafe to assume the value of GOPATH and attempt to proceed. Error out with a message about GOPATH resolution if `go env GOPATH` does not come back with a good value. Release note: None
|
Updated per the review above. Net patch: realpath + error on blank GOPATH. |
|
Great, thanks! I filed golang/go#24623 upstream for the issue we observed with GOROOT=$HOME/go. Merging on green. |
24391: Makefile: further improve GOPATH detection r=RaduBerinde a=benesch Squash of #24387. Closes #24387. --- It is necessary to use `realpath` to get the real GOPATH, as symlinks may be in play. For instance, on FreeBSD `/home` is symlinked to `/usr/home/`, but the former is often used. If `realpath` isn't used, the string-based prefix matching will incorrectly claim that CWD is not a child of GOPATH. Additionally, if `go env GOPATH` does not return a workable value, that means there is something wrong with the golang deployment and it is unsafe to assume the value of GOPATH and attempt to proceed. Error out with a message about GOPATH resolution if `go env GOPATH` does not come back with a good value. Release note (build change): None
In recent versions of go, GOPATH need not be explicitly set (and
go env GOPATHwill return nothing) if using the default $HOME/go as $GOPATH.Additionally, it is necessary to use
readlink -fto get the real pathsof both $GOPATH and $CWD, as symlinks may be in play. For instance, on
FreeBSD
/homeis symlinked to/usr/home/, but the former is oftenused. If
readlinkisn't used, the string-based prefix matching willincorrectly claim that CWD is not a child of GOPATH.
This patch sets GOPATH to
$HOME/goif GOPATH is not exported and$HOME/goexists (sincereadlink -f $HOME/gowill return an emptystring if its target is not found). No change in the existing behavior
is expected if the environment variable GOPATH is not blank or if it is
both blank and $HOME/go is not the GOPATH.
Release note: None