zquestz / s

Open a web search in your terminal.
MIT License
2.32k stars 114 forks source link

Add support for multiple GOPATHs in `make uninstall` #105

Closed mewmew closed 8 years ago

mewmew commented 8 years ago

Currently the uninstall rule of the Makefile assumes that GOPATH contains a single path. This assumption breaks for multiple GOPATHs. See go/build.Context.SrcDir and path/filepath.SplitList for more details.

Relevant section of the Makefile:

uninstall:
    rm $(GOPATH)/bin/$(APPNAME)
zquestz commented 8 years ago

Thanks for reporting this @mewmew. I can look at it tonight and come up with a fix.

zquestz commented 8 years ago

This is now resolved.

mewmew commented 8 years ago

Hi @zquestz!

Thanks for taking a look at this. The solution in #106 indeed works for common use cases, and may be enough. However, it does break for more complex use cases. Imagine a scenario where a user go gets s to a GOPATH and later on adds another GOPATH, as illustrated by the commands below.

[/tmp]$ export GOPATH=/tmp/go1
[/tmp]$ go get github.com/zquestz/s
[/tmp]$ cd /tmp/go1/src/github.com/zquestz/s
[/tmp/go1/src/github.com/zquestz/s]$ make
[/tmp/go1/src/github.com/zquestz/s]$ make install
[/tmp/go1/src/github.com/zquestz/s]$ export GOPATH=/tmp/go2:/tmp/go1
[/tmp/go1/src/github.com/zquestz/s]$ make uninstall
rm: cannot remove '/tmp/go2/bin/s': No such file or directory
Makefile:23: recipe for target 'uninstall' failed
make: *** [uninstall] Error 1
[/tmp/go1/src/github.com/zquestz/s]$ ls -l /tmp/go1/bin/s
-rwxr-xr-x 1 u users 9209184 Mar 29 14:15 /tmp/go1/bin/s*
zquestz commented 8 years ago

While I would accept a PR to improve this behavior, I think the current solution is just fine.

Also since Go 1.5 we have real vendoring support, which alleviates the need to modify your GOPATH constantly to sandbox projects.

Lastly, most users will uninstall shortly after an install if they are not fond of the tool, and in those cases it is very unlikely their Go environment changed in the few minutes since they installed.

If you do want to improve this, an ideal flow would be:

This still has the side effect of not working if they completely change their GOPATH, but is probably the best solution.

mewmew commented 8 years ago

While I would accept a PR to improve this behavior, I think the current solution is just fine.

I definitely agree, the current behaviour is good enough and covers most use cases. Just wanted to point it out for potential future users, who may discover this discussion when searching the s repo issues.

Keep up the happy work :)

zquestz commented 8 years ago

Thanks for the support! =)