Add Neovim support #9

blueyed · 2016-10-08T01:30:42Z

Paging @tweekmonster and @mhinz for review.

Fixes #1.

TODO:

update README
sync with updates, e.g. install_vim.sh: allow to pass through configure flags #22 (when not refactoring into a single script)
get Git version info; 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.

blueyed · 2016-10-08T02:16:50Z

[amended, fixed by https://github.com//issues/10]

This fixes `nvim` being executed from `/usr/local/bin`, although it is installed/linked in `/vim-build/bin`. Ref: Vimjas#9 (comment)

blueyed · 2016-10-08T03:44:58Z

scripts/install_neovim.sh

+# > make[3]: Leaving directory '/neovim/build'
+# > make[3]: Entering directory '/neovim/build'
+# > Bus error (core dumped)
+# > runtime/CMakeFiles/vimball-tags.dir/build.make:57: recipe for target 'runtime/CMakeFiles/vimball-tags' failed


It would be nice to install Neovim into a specific location, but is not required currently, since we're not using tags etc.
But still..

Woah, bus error? That's hardware telling software that something is seriously broken. No idea why that would happen...

Yeah, but it's reproducible.
Just tried with make CMAKE_EXTRA_FLAGS="-DCMAKE_INSTALL_PREFIX=/nvim/nvim-master" again.

Oh, I was overwriting the export CMAKE_EXTRA_FLAGS=-DENABLE_JEMALLOC=OFF from the top with that, which is likely the issue.

mhinz · 2016-10-08T10:22:24Z

scripts/install_neovim.sh

+  rm -r /root/.cache || exit 1
+
+# Install Neovim Python support.
+pip install neovim pep8


A few lines above we upgrade pip3 and here we use pip(2). Intended?

This is the pip that is installed through pip3 install --upgrade pip setuptools.
By default there is no pip in the image. Will change it for clarity though.

Just for my understanding: That's pip3 in pip disguise?

This fixes `nvim` being executed from `/usr/local/bin`, although it is installed/linked in `/vim-build/bin`. Ref: #9 (comment)

blueyed · 2016-10-08T22:15:32Z

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 · 2016-11-22T01:34:08Z

Rebased for now.

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

mhinz

The first post mentions updating the README. What about that?

blueyed · 2017-03-24T23:50:18Z

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

So only the docs need to be updated now.

blueyed · 2017-03-25T21:04:47Z

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

blueyed · 2017-03-25T21:04:54Z

@tweekmonster
Please review.

tweekmonster

Just a code review. I haven't tried building an image yet.

tweekmonster · 2017-03-25T21:21:27Z

README.md

@@ -44,16 +45,16 @@ The following flags can be used for each build:

 Flag | Description
 ---- | -----------
-`-tag` | The Vim release.  It must match the tags on Vim's [releases page](https://github.com/vim/vim/releases).
-`-name` | The name to use for the binary's symlink.  If omitted, the name will default to `vim-$TAG`.
+`-vim` | The Vim variant.  Either `vim` (default) or `nvim`.


Why not a boolean -nvim? -vim nvim could be confusing to understand at 2AM and -vim vim is redundant.

Yes, the name -vim is weird after all.
There might be other variants in the future though, so -nvim 1 might not be sufficient..?!

But I am +0 on changing it like that.

How hard would it be to have -tag support a flavor prefix? Like -tag nvim:0.1.7 or -tag vim:7.4.123

Actually, like -tag neovim/neovim:v0.1.7 so it could be any git repo. Like a fork -tag tweekmonster/neovim:wip-branch or something.

Sounds good.
With vim/vim being the default prefix then?

tweekmonster · 2017-03-25T21:23:03Z

example/Dockerfile

-                -name vim-xenial -tag v7.4.963 -build
+                -name vim-xenial -tag v7.4.963 -build \
+                -vim neovim -tag v0.1.7 -build \
+                -vim neovim -tag master -build


Looks like a mistake. Should these be -vim nvim?

tweekmonster · 2017-03-25T21:26:34Z

scripts/install_vim.sh

@@ -1,88 +1,186 @@
 #!/bin/bash

 set -e
+set -x


Is this added so the builds are verbose or is this left in by mistake? Maybe this should be enabled if a VERBOSE env variable exists.

I think it's quite nice in general, although too verbose with echos for sure.
We could have a VERBOSE var for this, but I think the default should be 1 then maybe after all: it shows up nicely in the output in red lines, so you see what is being executed currently.

I don't feel strongly about it. I think it can stay as is. It just stuck out as something that might've been left behind from testing.

tweekmonster · 2017-03-25T21:28:52Z

scripts/install_vim.sh


 bail() {
  echo "$@"
  exit 1
 }

 init_vars() {
+  VIM=vim


I think this should be FLAVOR or something. It seems a little easy to lose track of the variable because the characters "vim" are all over. WDYT?

FLAVOR sounds good, and could be used as -flavor then, too.

This comment is probably irrelevant if a change is made to the -vim flag.

blueyed · 2017-03-25T22:19:54Z

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 · 2017-03-25T22:51:49Z

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 · 2017-03-25T23:20:46Z

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 · 2017-03-26T00:05:18Z

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 · 2017-03-26T14:46:36Z

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.

tweekmonster

make test passes for me! This seems to work. Only blocking issue I saw was a typo.

tweekmonster · 2017-03-27T15:06:20Z

README.md

@@ -33,27 +32,28 @@ FROM testbed/vim:latest

 RUN install_vim -tag v7.3 -name vim73 -build \
                -tag v7.4.052 -name vim74 -build \
-                -tag master -build
+                -tag master -build \
+                -vim neovim -tag master -py2 -py3 -build


Should this be changed to -tag neovim:master ?

I will slightly enhance the examples in example/Dockerfile, and use them here. I've thought about using those in the tests then (automatically), and did it now.. :)

tweekmonster · 2017-03-27T15:11:32Z

scripts/install_vim.sh

+      luajit \
+      luajit-dev \
+      m4 \
+      make \


if [ "$FLAVOR" = vim ]; only installs ncurses-dev, so does this mean some of these are already installed by this point? It doesn't affect this PR, but might be good to clean this up in the future to reduce the noise.

Neovim certainly has more deps, and these here are pretty fine-grained (and I see it as to see what is required really, if you want to create a Dockerfile yourself).

Oh, I was referring to things like make. It's not installed by the vim flavor, but it still uses make, right?

Yes, there is a more global one at the top: apk add --virtual vim-build curl gcc libc-dev make.

Required for luarocks etc already also.

blueyed · 2017-03-27T20:35:47Z

I've broken it on Travis again.
Seems to (hopefully) just don't like f74a3c0#diff-e8849b97cf69febe9876e8ab47d44460R16.

blueyed · 2017-03-27T20:36:56Z

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 · 2017-03-27T21:00:52Z

\o/

blueyed · 2017-03-29T20:59:17Z

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 · 2017-03-29T22:12:07Z

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

tweekmonster · 2017-03-30T04:11:13Z

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 · 2017-03-30T21:25:05Z

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

tweekmonster · 2017-04-03T03:19:08Z

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 · 2017-04-03T06:51:07Z

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

blueyed · 2017-04-21T14:37:39Z

Improved the version output for Neovim (3f379af).
Do you like it that way?

blueyed added a commit to blueyed/vim-testbed that referenced this pull request Oct 8, 2016

run_vim.sh: exec everything that's not in /vim-build/bin

191a46d

This fixes `nvim` being executed from `/usr/local/bin`, although it is installed/linked in `/vim-build/bin`. Ref: Vimjas#9 (comment)

blueyed mentioned this pull request Oct 8, 2016

run_vim.sh: exec everything that's not in /vim-build/bin #10

Merged

blueyed commented Oct 8, 2016

View reviewed changes

mhinz reviewed Oct 8, 2016

View reviewed changes

blueyed added a commit that referenced this pull request Oct 8, 2016

run_vim.sh: exec everything that's not in /vim-build/bin (#10)

5f8091d

This fixes `nvim` being executed from `/usr/local/bin`, although it is installed/linked in `/vim-build/bin`. Ref: #9 (comment)

blueyed force-pushed the install-neovim branch from c983662 to 76782f0 Compare October 8, 2016 22:12

blueyed force-pushed the install-neovim branch from 3f27e10 to 0c1a0ba Compare October 9, 2016 21:01

blueyed changed the title ~~Add install_neovim script~~ [WIP] Add install_neovim script Oct 10, 2016

blueyed force-pushed the install-neovim branch from 0c1a0ba to 8289dad Compare November 22, 2016 01:32

mhinz reviewed Nov 22, 2016

View reviewed changes

blueyed force-pushed the install-neovim branch from 30f9928 to c6435b3 Compare March 24, 2017 20:43

blueyed requested a review from tweekmonster March 24, 2017 23:50

blueyed force-pushed the install-neovim branch from 1746db1 to 0a56713 Compare March 24, 2017 23:54

blueyed changed the title ~~[WIP] Add install_neovim script~~ [WIP] Add Neovim support Mar 25, 2017

This was referenced Mar 25, 2017

Add Neovim to Docker targets neomake/neomake#1161

Merged

install_vim: "apk add ruby" for -ruby #23

Closed

blueyed changed the title ~~[WIP] Add Neovim support~~ [RFC] Add Neovim support Mar 25, 2017

tweekmonster reviewed Mar 25, 2017

View reviewed changes

blueyed force-pushed the install-neovim branch from d484cfb to 84d4b84 Compare March 25, 2017 21:56

blueyed requested a review from tweekmonster March 26, 2017 19:24

tweekmonster approved these changes Mar 27, 2017

View reviewed changes

blueyed force-pushed the install-neovim branch 2 times, most recently from 61370cc to b25c8ec Compare March 27, 2017 21:19

blueyed mentioned this pull request Mar 27, 2017

build: undefined reference to `termkey_hook_terminfo_getstr' on Alpine Linux neovim/neovim#6356

Closed

blueyed force-pushed the install-neovim branch from 76bc8fc to ffa0f72 Compare March 29, 2017 22:11

blueyed mentioned this pull request Apr 2, 2017

[WIP] Integration tests: Docker, tmux, Neovim junegunn/vader.vim#124

Open

blueyed force-pushed the install-neovim branch 2 times, most recently from 24f53ff to 1fcf0e5 Compare April 21, 2017 14:27

blueyed changed the title ~~[RFC] Add Neovim support~~ Add Neovim support Apr 21, 2017

blueyed force-pushed the install-neovim branch 3 times, most recently from e0e0b34 to abfa18e Compare April 22, 2017 14:21

Add support for installing Neovim and other fixes

e622442

blueyed force-pushed the install-neovim branch from abfa18e to e622442 Compare April 22, 2017 14:21

blueyed merged commit c6f1b84 into Vimjas:master Apr 22, 2017

blueyed deleted the install-neovim branch April 22, 2017 22:38

Add Neovim support #9

Add Neovim support #9

Conversation

blueyed commented Oct 8, 2016 • edited Loading

blueyed commented Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhinz Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueyed commented Oct 8, 2016

blueyed commented Nov 22, 2016

mhinz left a comment

Choose a reason for hiding this comment

blueyed commented Mar 24, 2017

blueyed commented Mar 25, 2017

blueyed commented Mar 25, 2017

tweekmonster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueyed commented Mar 25, 2017

tweekmonster commented Mar 25, 2017

blueyed commented Mar 25, 2017

tweekmonster commented Mar 26, 2017

blueyed commented Mar 26, 2017

tweekmonster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueyed commented Mar 27, 2017

blueyed commented Mar 27, 2017 • edited Loading

blueyed commented Mar 27, 2017

blueyed commented Mar 29, 2017

blueyed commented Mar 29, 2017 • edited Loading

tweekmonster commented Mar 30, 2017

blueyed commented Mar 30, 2017

tweekmonster commented Apr 3, 2017

blueyed commented Apr 3, 2017 • edited Loading

blueyed commented Apr 21, 2017

blueyed commented Oct 8, 2016 •

edited

Loading

blueyed commented Oct 8, 2016 •

edited

Loading

mhinz Oct 8, 2016 •

edited

Loading

blueyed commented Mar 27, 2017 •

edited

Loading

blueyed commented Mar 29, 2017 •

edited

Loading

blueyed commented Apr 3, 2017 •

edited

Loading