Changes to auto-download of tools #37
Conversation
|
Mostly looking for comments as to whether this can ever be accepted. If it can, I will add docs and make any requested changes. I like the choices that this vim plugin has made but I did like, in vim-go, being able to use :GoInstallBinaries when I wanted. I have found the auto-download of tools a mixed bag. I'd like to move :GoForceSetup to plugin/go.vim so that it is global and may be used like so: |
|
Hey, thanks for your PR :-) What is the use case for using a different $GOBIN? The intention behind using a our own $GOBIN is that I know which versions of which tools work, and that I want those exact versions to prevent upstream updates from breaking stuff, similar to e.g. the vendor directory in Go. This happened several times when I maintained vim-go, and then we had to scramble to fix stuff, which wasn't great. Just want to understand the problem so we can add the best fix :-) As a general comment, I tried to prevent having a sprawl of different So I'd prefer something like I wouldn't change this just yet, btw, since we may decide solving your problem in a different way and I'd hate for you to waste your time. |
|
I like the :GoSetup! idea. I thought of doing that, but decided against it for this PR. I'm neutral as to the use of global settings. Fewer is probably better for the reason you give. I mentioned in my earlier comment that one thing I would really like is for :GoSetup[!] to be available immediately. This allows for automation such as:
to f.e. be run from scripts when initializing or updating an environment. One way this can be achieved is by moving the command definition to plugin/go.vim. I understand the motivation for downloading the tools to a separate and controlled directory. I want to be able to use the same tools I use within vim when I am outside of vim. There are ways to do this without downloading to go's default $GOBIN. I can set $PATH to include this special directory. I can create symlinks from $GOBIN to the files in the special directory. These solutions are less appealing than having all tools (both tools downloaded by gopher and other tools downloaded separately) in a single, well-known place. |
|
Right, so it's just a personal preference. That's fair enough, and don't mind supporting that. Just wondered if there's a problem you ran in to. As for the setup, I think you should already be able to set up the environment with: The trick is to make sure the filetype is set to I'm not sure if there are any differences in the |
autoload/gopher/system.vim
Outdated
| " By default autodownload of tools happens. However, if | ||
| " we want a 'local' install we can no longer have | ||
| " autodownload. | ||
| let s:tool_autodownload = s:use_gotools is 1 ? 1 : 0 |
There was a problem hiding this comment.
Did you run in to a problem with the autodownload and the default GOBIN/local_install logic, or is it just something you'd prefer not to happen?
I think this could perhaps be two different settings: "use default GOBIN" and "don't download tools automatically"? I could imagine some people might prefer to use the default GOBIN but still keep the auto-download stuff (or use gopher.vim's GOBIN but never automatically download stuff).
There was a problem hiding this comment.
This is all true. The amount of refactoring that would be required to do that seemed greater than I was willing to devote when I was working on this PR. This PR was primarily a "here is an idea I have, will you accept it?" If you are okay with the concept behind the PR I can work on it. Or you or anyone else can. I think allowing auto-download while also placing tools in default $GOBIN sounds fine. The problem you run in to is when $GOBIN is unset. I think go uses $GOPATH and looks for a bin directory. But $GOPATH can be more than one path. So determining where exactly go would download tools to can be complicated. Unless there is a way to programatically determine this. I tried
go env GOBIN
but this doesn't return the default value if $GOBIN is unset. It just returns nothing.
Perhaps the solution is more obvious than I think it is.
There was a problem hiding this comment.
Thinking about the $GOPATH issue sent me down a rabbit hole. This seems to be the best answer:
golang/go#23439
Hopefully go 1.15 will make it easier to determine where tools are installed when $GOBIN is unset.
There was a problem hiding this comment.
So I mentioned all of the above because s:tool() performs an existence check on the tool using filereadable(). It requires the full path to the tool. This can be re-written using something like exepath() and there is no longer a need for the path at all.
|
I think there is an advantage to calling :GoSetup over calling gopher#system#setup() directly. It is not guaranteed that gopher#system#setup() will always be the only thing called by :GoSetup. Or maybe the setup() function moves to a new name. These are minor concerns but :GoSetup is the umbrella over the setup process. For "future proofing" it seems better to call :GoSetup. Setting the filetype is actually what I am doing now in my automation scripts. It's one of those things that can be done, but, perhaps, should not need to be done. gopher#system#forcedownload is similar to s:tool. s:tool only downloads a single tool, only downloads to the tools/bin dir and will short-circuit for various reasons. forcedownload does no short-circuiting, will download either to tools/bin or default $GOBIN, and downloads all tools listed in the go.mod. There may be a way to refactor such that there is less duplication of the call to 'go install'. I briefly spent some time looking at how to change s:tool to do this, but it wasn't immediately clear that this would be all that straightforward. This is something to look at if this PR is to move forward. Thanks for looking at this PR. I appreciate it. |
|
Okay, I added a new commit with documentation on the new behaviour (but didn't really implement anything yet); I think this would work for you? I added a way to print some more info as well, because to be honest I was rather confused on what it is and isn't doing myself as there's quite a lot of logic in there now. This also includes a basic prototype fix for the filereadable()/exepath() thing mention in the other thread. I'll look some more at the entire thing next week or so.
That's fair enough, but on the other hand, making Aside: the API is quite stable (and documented) and should be guaranteed to be stable once 1.0 is released so |
* Allow *no* auto-download of tools (default is to auto-download) * Allow to install tools in the default $GOBIN (default is to install tools in <plugin>/tools/bin * Create command that forces download of all tools
Also remove the verbose flag from :GoSetup as I don't feel like implementing that correctly now (and it's not really related to this PR anyway).
|
Okay, I did some more work on this and I think it should be done now; the new behaviour is documented at: https://github.com/arp242/gopher.vim/pull/37/files#diff-ddb7fdffdda30755d89d0c488e77e51dR255 I think this should work for you? Let me know and I'll merge it. |
|
Looks good to me. Thank you. |
|
Coolio, let me know if you run in to any issues 👍 Sorry it took a while; I'm going to try and spend at least half a day a week on this so I can finally get the v1.0 out that I've been planning for almost 2 years now 😅 So hopefully things should move in a faster pace for the next while. |
tools in /tools/bin