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

Further improve GOPATH detection in Makefile #24387

Closed
wants to merge 3 commits into from
Closed

Further improve GOPATH detection in Makefile #24387

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c596fdd
Further improve GOPATH detection in Makefile
mqudsi Mar 31, 2018
0b149a2
Use `$(realpath ...)` instead of `$(shell readlink -f ...) in Makefile
mqudsi Mar 31, 2018
62c6d6f
Error out if GOPATH cannot be resolved
mqudsi Mar 31, 2018
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
6 changes: 5 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this won't work on macOS.

$ readlink -f 
readlink: illegal option -- f
usage: readlink [-n] [file ...]

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bmake guy, so thanks for pointing me in the right direction.

Copy link
Contributor Author

@mqudsi mqudsi Mar 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. It appears that $(CURDIR) has internally been set to the equivalent of $(realpath $(shell pwd)).

# GOPATH need not be explicitly set if it is the default $HOME/go
ifeq ($(strip $(GOPATH)),)
GOPATH := $(shell readlink -f $$HOME/go)
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 GOPATH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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/go

Not sure what to make of this. A golang bug, perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

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

Check this out:

$ HOME= GOPATH= go env GOPATH

$

Seems like maybe $HOME is unset? Or os/user.Current is otherwise bricked on that machine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer not to commit this workaround yet. I think it's papering over something more serious.


ifneq "$(or $(findstring :,$(GOPATH)),$(findstring ;,$(GOPATH)))" ""
$(error GOPATHs with multiple entries are not supported)
Expand Down