Skip to content
This repository has been archived by the owner on May 29, 2018. It is now read-only.

Rely on a locally installed copy of godep binary. #47

Merged
merged 1 commit into from
Aug 12, 2015

Conversation

dmitshur
Copy link
Contributor

Install godep into a local .bin folder. That way, it will always be accessible even if the user does not have $GOPATH/bin added to their $PATH.

This is an implementation of the alternative idea described in #46 (comment). Review and thoughts are welcome.

Closes #46.

Install godep into a local .bin folder. That way, it will always be accessible even if the user does not have $GOPATH/bin added to their $PATH.

Closes #46.
@dmitshur
Copy link
Contributor Author

Note, I have not tested this Makefile yet, it is an idea up for review first. If it looks good, I'll test before merging. I do not have enough confidence in Makefile semantics to make this change blindly (perhaps the way I am setting the env variable that way is not supported, etc.).

@sqs
Copy link
Member

sqs commented Aug 12, 2015

You pointed out the complexity of figuring out the right cwd in the Makefile in the other PR. But does godep handle it properly? I mean, if a Makefile recipe executes godep go xyz and you run the Makefile with make -C some/other/dir, does Godep properly set GOPATH to /abspath/to/some/other/dir/Godeps/_workspace:$GOPATH or does it set it to the Godeps dir that's the nearest parent of $PWD?

@dmitshur
Copy link
Contributor Author

I think the processes that are started via the listed commands have their current working directory set where the Makefile lives. If that weren't the case, the -o .bin/srclib-go part of go build step would not work as expected.

The problem is figuring out the current working directory as an absolute path string which can be passed as a string parameter. Namely, $GOPATH needs to be be made of absolute paths only; that was the difficulty.

I think it's best to test this and confirm it works as expected.

@dmitshur
Copy link
Contributor Author

Ok, I've tested it (on a fresh Ubuntu live VM), it worked as expected.

I was able to reproduce the failure when $GOPATH/bin was not in my $PATH, and switching to this branch allowed the following command to succeed.

Before:

Building Go toolchain program
Running  [make -C /home/ubuntu/.srclib/sourcegraph.com/sourcegraph/srclib-go]
make: Entering directory '/home/ubuntu/.srclib/sourcegraph.com/sourcegraph/srclib-go'
go get github.com/tools/godep
godep go build -o .bin/srclib-go
make: godep: Command not found
Makefile:7: recipe for target 'install' failed
make: *** [install] Error 127
make: Leaving directory '/home/ubuntu/.srclib/sourcegraph.com/sourcegraph/srclib-go'
failed to install/upgrade Go (sourcegraph.com/sourcegraph/srclib-go) toolchain: command ["make" "-C" "/home/ubuntu/.srclib/sourcegraph.com/sourcegraph/srclib-go"] failed: exit status 2
The following toolchains were not installed:
Go (sourcegraph.com/sourcegraph/srclib-go)

After:

Building Go toolchain program
Running  [make -C /home/ubuntu/.srclib/sourcegraph.com/sourcegraph/srclib-go]
make: Entering directory '/home/ubuntu/.srclib/sourcegraph.com/sourcegraph/srclib-go'
GOBIN=.bin go get github.com/tools/godep
.bin/godep go build -o .bin/srclib-go
make: Leaving directory '/home/ubuntu/.srclib/sourcegraph.com/sourcegraph/srclib-go'
OK! Installed/upgraded Go (sourcegraph.com/sourcegraph/srclib-go) toolchain
================================================================================

@sqs
Copy link
Member

sqs commented Aug 12, 2015

LGTM. And wanna just remove the unused var decls at the top of the Makefile after merge?

@dmitshur dmitshur merged commit 5ae3b41 into master Aug 12, 2015
@dmitshur dmitshur deleted the use-local-godep-during-installation branch August 12, 2015 19:43
dmitshur added a commit that referenced this pull request Aug 12, 2015
beyang pushed a commit that referenced this pull request Mar 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants