Vimjas / vim-testbed

Docker image for testing Vim plugins
42 stars 6 forks source link

Add Neovim support #9

Closed blueyed closed 7 years ago

blueyed commented 8 years ago

Paging @tweekmonster and @mhinz for review.

Fixes https://github.com/tweekmonster/vim-testbed/issues/1.

TODO:

blueyed commented 8 years ago

[amended, fixed by https://github.com/tweekmonster/vim-testbed/issues/10]

blueyed commented 8 years ago

This should be good for now, but I'd like to merge it with install_vim.sh. That could be done later though, and then adding support to install specific tags of Neovim also.

blueyed commented 7 years ago

Rebased for now.

The next time I come here without stopping feedback in the form of comments, I'll merge it (nearly) as-is.

blueyed commented 7 years ago

Merged it with install_vim.sh now.. seems to work good.

So only the docs need to be updated now.

blueyed commented 7 years ago

I still need to revisit the README properly, but apart from that it seems very good now.

blueyed commented 7 years ago

@tweekmonster Please review.

blueyed commented 7 years ago

I think -flavor is still relevant, even when we would add better support through -tag, since it's possible to parse -tag tweekmonster/neovim:wip-branch into FLAVOR=NEOVIM, but not for -tag tweekmonster/myneovim:wip-branch. We could require to use -tag neovim:tweekmonster/myneovim:wip-branch in that case though.

It is constructed, but yeah..

tweekmonster commented 7 years ago

Good point. FLAVOR=nvim is more of an instruction on how to build. I'm in support for keeping the script dumb. vim/* and neovim/* should be the only "officially" recognized prefixes. It shouldn't have to guess that tweekmonster/neovim is Neovim.

blueyed commented 7 years ago

So now support for -tag tweekmonster/neovim:wip-branch at all? We might just want to resolve the prefix before : to a GitHub URL, using neovim/neovim or vim/vim as the default depending on -flavor, where neovim enables the Neovim build, and uses neovim/neovim as the prefix then.

tweekmonster commented 7 years ago

So now support for -tag tweekmonster/neovim:wip-branch at all?

Up to you. It was just an idea. TBH, I can't think of a good use case for it yet since the goal is to create semi-permanent and reusable images.

We might just want to resolve the prefix before : to a GitHub URL

Yeah. I was thinking it'd be simply user/repo:branch-or-tag, and as you've said, -flavor would be like a build directive for the install script. -tag XXX would imply vim/vim:XXX, -tag neovim:XXX would imply neovim/neovim:XXX. This would at least make it future-proofed if a fork needs to be targeted for any reason.

BTW, did you get an email about joining https://github.com/testbed ?

blueyed commented 7 years ago

did you get an email about joining https://github.com/testbed ?

Yes, joined. I think you could move this repo over there then.

I've pushed some updates, also for the better -tag support.

blueyed commented 7 years ago

I've broken it on Travis again. Seems to (hopefully) just don't like https://github.com/tweekmonster/vim-testbed/pull/9/commits/f74a3c0d1f94e126ba16b74f8e12ad1ecd7fdee6#diff-e8849b97cf69febe9876e8ab47d44460R16.

blueyed commented 7 years ago

Any idea about getting a better version for Neovim builds? From the TODO above:

since we use curl to get the tarball, no .git information is available, and the Git information in missing in nvim --version etc. We can use -DNVIM_VERSION_MEDIUM=$tag in `CMAKE_EXTRA_FLAGS, but that is not ideal either: would just say "master" there then for example.

/cc @mhinz

blueyed commented 7 years ago

\o/

blueyed commented 7 years ago

Regarding the version: should we use git after all to fetch the sources? It should preferably use --depth=1, which then would not help with git-describe anymore though.

Since we are assuming to use GitHub here still, we might want to use their API to get the commit hash:

% curl https://api.github.com/repos/neovim/neovim/git/refs/heads/master
{
  "ref": "refs/heads/master",
  "url": "https://api.github.com/repos/neovim/neovim/git/refs/heads/master",
  "object": {
    "sha": "c35420558bed0bfa9938ecd1facec88f1df392a5",
    "type": "commit",
    "url": "https://api.github.com/repos/neovim/neovim/git/commits/c35420558bed0bfa9938ecd1facec88f1df392a5"
  }
}

This would allow us to use $tag-$sha at least.

blueyed commented 7 years ago

Squashed, waiting for feedback on the version issue with Neovim.

tweekmonster commented 7 years ago

If you want to tackle the GitHub API, I'm all for it since it should speed up the build time. But, the /repos/:owner/:repo/tags might be more useful: https://api.github.com/repos/neovim/neovim/git/refs/tags

Only issue I can think of with this is the API rate limit without an API token. If the user is allowed to supply the API token, wouldn't it be exposed in the built image if it's pushed to docker hub?

blueyed commented 7 years ago

Why does using the GitHub API (to get something for the version) speed up the build?!

tweekmonster commented 7 years ago

Why does using the GitHub API (to get something for the version) speed up the build?!

I was thinking that the release archives could be downloaded instead of cloning the repos. A single binary blob is faster to transmit over a network, and the archive's compression ratio should be fairly high since it's all text. Installing git could also be skipped.

blueyed commented 7 years ago

@tweekmonster We are downloading tarballs already. Git is used for something else though IIRC.

blueyed commented 7 years ago

Improved the version output for Neovim (https://github.com/tweekmonster/vim-testbed/pull/9/commits/3f379afc6a6f31882b07635d6e321c6392e1cfc2). Do you like it that way?