neovim 0.2.0 (new formula) #14485

javian · 2017-06-11T16:39:14Z

Have you followed the guidelines for contributing?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

This is slightly different from the Formula that currently sits in the neovim/neovim tap since the linux specific code has been removed since brew audit complains. The issue in the Neovim tap requesting the move to homebrew-core is here neovim/homebrew-neovim#115 which also contains a few question marks related to inclusion of code from non brew formula. Let me know if you have any feedback.

ilovezfs · 2017-06-11T17:07:08Z

Formula/neovim.rb

+    url "https://github.com/neovim/neovim/archive/v0.2.0.tar.gz"
+    sha256 "72e263f9d23fe60403d53a52d4c95026b0be428c1b9c02b80ab55166ea3f62b5"
+    resource "luarocks" do
+      url "https://github.com/luarocks/luarocks/archive/5d8a16526573b36d5b22aa74866120c998466697.tar.gz"


url "https://github.com/luarocks/luarocks.git", :revision => "5d8a16526573b36d5b22aa74866120c998466697"

Why not building from the luarocks tag?

ilovezfs · 2017-06-11T17:09:09Z

Formula/neovim.rb

+  head do
+    url "https://github.com/neovim/neovim.git", :shallow => false
+    resource "luarocks" do
+      url "https://github.com/luarocks/luarocks/archive/2.4.2.tar.gz"


seems odd the tag is here not in stable

ilovezfs · 2017-06-11T17:09:29Z

Formula/neovim.rb

+
+  depends_on "cmake" => :build
+  depends_on "libtool" => :build
+  depends_on "automake" => :build


does this really require both cmake and Autotools?

I will try and find out. This has been in use for the past 2 years and no changes has been made making a case for it in the neovim tap so I don't have the answer at the moment.

@justinmk can you shine a light on this ?

ilovezfs · 2017-06-11T17:09:36Z

Formula/neovim.rb

+  depends_on "automake" => :build
+  depends_on "autoconf" => :build
+  depends_on "pkg-config" => :build
+  depends_on "jemalloc" => :recommended


why recommended?

Not sure why recommended was chosen but its linked to this issue neovim/neovim#5681 . I'm uncertain if the root cause was found.

Interesting. The last comment there (neovim/neovim#5681 (comment)) says the original issue was resolved. I'm not sure if that implies it's fixed even if jemalloc is not used but it would be good to clarify if the jemalloc dependency is still actually required or just a vestige of that old bug and no longer necessary.

@ilovezfs The reason this was still "recommended" in our brew formula was IIRC because we hadn't released the fixed version yet. I am 90% sure this can be "required" now, since we released 0.2 which includes the fix.

@justinmk I'm a little confused. If the bug is fixed, wouldn't that mean we can just drop the dependency?

We want to use jemalloc. It was temporarily changed to "recommended" instead of "required" because there was an issue related to jemalloc.

Ah, OK! Yes, let's just require it then. No :recommended.

I've removed it completely now require jemalloc for all builds.

ilovezfs · 2017-06-11T17:09:57Z

Formula/neovim.rb

+    sha256 "620fa4eb12375021bef6e4f237cbd2dd5d49e56beb414bee052c746beef1807d"
+  end
+
+  resource "luarocks" do


this is a duplicate resource

ilovezfs · 2017-06-11T17:16:53Z

Formula/neovim.rb

+      cmake_args = std_cmake_args + ["-DDEPS_PREFIX=../deps-build/usr", "-DCMAKE_BUILD_TYPE=#{build_type}"]
+      cmake_args += ["-DENABLE_JEMALLOC=OFF"] if build.without?("jemalloc")
+      cmake_args += ["-DJEMALLOC_LIBRARY=#{Formula["jemalloc"].opt_lib}/libjemalloc.a"] if build.with?("jemalloc")
+      cmake_args += ["-DMSGPACK_LIBRARY=#{Formula["msgpack"].opt_lib}/libmsgpackc.2.dylib"]


Is there a way to do this without hard coding a library number?

ilovezfs · 2017-06-11T17:17:29Z

Formula/neovim.rb

+      cmake_args += ["-DENABLE_JEMALLOC=OFF"] if build.without?("jemalloc")
+      cmake_args += ["-DJEMALLOC_LIBRARY=#{Formula["jemalloc"].opt_lib}/libjemalloc.a"] if build.with?("jemalloc")
+      cmake_args += ["-DMSGPACK_LIBRARY=#{Formula["msgpack"].opt_lib}/libmsgpackc.2.dylib"]
+      cmake_args += ["-DIconv_INCLUDE_DIRS:PATH=/usr/include", "-DIconv_LIBRARIES:PATH=/usr/lib/libiconv.dylib"]


You'll need to use #{MacOS.sdk_path}/usr/include here since /usr/include only exists if the CLT is installed these days.

ilovezfs · 2017-06-11T17:17:44Z

Formula/neovim.rb

+    end
+  end
+
+  def caveats; <<-EOS.undent


I don't think we need any of the caveats.

ilovezfs · 2017-06-11T17:18:51Z

Formula/neovim.rb

+  test do
+    (testpath/"test.txt").write("Hello World from Vim!!")
+    system bin/"nvim", "--headless", "-i", "NONE", "-u", "NONE", "+s/Vim/Neovim/g", "+wq", "test.txt"
+    assert_equal "Hello World from Neovim!!", File.read("test.txt").strip


.chomp instead of .strip

ilovezfs · 2017-06-11T17:19:18Z

Formula/neovim.rb

+  end
+
+  head do
+    url "https://github.com/neovim/neovim.git", :shallow => false


Is shallow false actually required?

This was the issue that triggered the addition of the deep clone neovim/homebrew-neovim#135 . I can check if its still a valid argument.

It would be great to check if that's still a problem but :shallow => false is fine if it is.

ilovezfs · 2017-06-11T17:52:24Z

Formula/neovim.rb

+    cd "deps-build" do
+      ohai "Building Neovim third-party dependencies."
+      system "cmake", "../third-party",
+            "-DUSE_BUNDLED_BUSTED=OFF",


can the big list be replaced with the generic -DUSE_BUNDLED=OFF?

I'm assuming the reason its not done like that now is because of luarocks, luajit and luv since they need to be ON as they are downloaded as separate resources and don't exist as a Formula in homebrew.

javian · 2017-06-11T20:25:39Z

I've made an initial commit tackling the easy corrections and suggestions you made.

ilovezfs · 2017-06-12T02:23:01Z

Formula/neovim.rb

+    end
+
+    mkdir "build" do
+      cmake_args = std_cmake_args


args = std_cmake_args + %W[ -DDEPS_PREFIX=../deps-build/usr -DMSGPACK_LIBRARY=#{Formula["msgpack"].opt_lib}/libmsgpackc.2.dylib -DIconv_INCLUDE_DIRS:PATH=#{MacOS.sdk_path}/usr/include -DIconv_LIBRARIES:PATH=/usr/lib/libiconv.dylib ] if build.with? "jemalloc" args << "-DJEMALLOC_LIBRARY=#{Formula["jemalloc"].opt_lib}/libjemalloc.a" else args << "-DENABLE_JEMALLOC=OFF" end system "cmake", "..", *args

ilovezfs · 2017-06-12T02:31:04Z

I'm assuming the reason its not done like that now is because of luarocks, luajit and luv since they need to be ON as they are downloaded as separate resources and don't exist as a Formula in homebrew.

We'll need to get those vendored as well.

javian · 2017-06-12T19:56:47Z

@ilovezfs what do you mean by that ? (get them vendored)

javian

Made additional updates based on your feedback.

javian · 2017-06-12T20:31:44Z

Formula/neovim.rb

+            "-DUSE_EXISTING_SRC_DIR=ON",
+            *std_cmake_args
+
+      # Deparallelize required since LuaRocks 2.4.1 for reasons unknown


Reported where ?

javian · 2017-06-12T20:32:45Z

Formula/neovim.rb

+    mkdir "build" do
+      args = std_cmake_args + %W[
+        -DDEPS_PREFIX=../deps-build/usr
+        -DCMAKE_BUILD_TYPE=RelWithDebInfo


@justinmk should we use "Release" here instead ?

Release is the default from std_cmake_args so: yes, I think so.=

Great then I'll just remove the line and it will use the default.

We tend to ship RelWithDebInfo so that users have stacktraces should a crash occur. But if homebrew prefers to omit debug info that's ok. We have a RelWithDebInfo macOS binary available in our downloads page.

Are there any agreements that would govern this choice @ilovezfs ?

ilovezfs · 2017-06-12T23:27:52Z

@ilovezfs what do you mean by that ? (get them vendored)

I mean let's use -DUSE_BUNDLED=OFF and see what needs to be done so that the formula doesn't break with that option being passed.

javian · 2017-06-13T04:39:46Z

So it seems to work if the options are reversed which looks better. If there are other ways to include (vendor) the formula resources then they can be removed as we move along.

ilovezfs · 2017-06-13T07:06:53Z

Formula/neovim.rb

+      system "cmake", "../third-party",
+            "-DUSE_BUNDLED=OFF",
+            "-DUSE_BUNDLED_LUAROCKS=ON",
+            "-DUSE_BUNDLED_LUAJIT=ON",


we have a luajit formula

Will research this

If I include the luajit formula and disable the above line I get this error when compiling

Last 15 lines from /Users/biomass/Library/Logs/Homebrew/neovim/02.make: [ 54%] Performing configure step for 'luarocks' cd /tmp/neovim-20170613-23390-1sbuqi4/neovim-0.2.0/deps-build/build/src/luarocks && /tmp/neovim-20170613-23390-1sbuqi4/neovim-0.2.0/deps-build/build/src/luarocks/configure --prefix=/tmp/neovim-20170613-23390-1sbuqi4/neovim-0.2.0/deps-build/usr --force-config --lua-suffix=jit Looking for Lua... luajit found in $PATH: /usr/local/opt/luajit/bin Checking Lua interpreter... luajit found in /usr/local/opt/luajit/bin Checking Lua includes... lua.h not found (looked in /usr/local/opt/luajit/include, /usr/local/opt/luajit/include/lua/5.1, /usr/local/opt/luajit/include/lua5.1)

The lua.h file is located in /usr/local/opt/luajit/include/luajit-2.0 - can I force it somehow to search there ?

This is the code in neovim responsible for the luarocks configuration https://github.com/neovim/neovim/blob/master/third-party/cmake/BuildLuarocks.cmake#L56-L67 . It doesn't seem to accept any variables so maybe the only way is to monkey patch the cmake file ?

So it is possible to do

inreplace "../third-party/cmake/BuildLuarocks.cmake", "${LUAROCKS_OPTS}", "${LUAROCKS_OPTS} --with-lua-include=#{Formula["luajit"].include}/luajit-2.0 --with-lua-bin=#{Formula["luajit"].bin}"

but then it doesn't seem to find the luarocks modules that has been installed during the luarocks install.

Can't LUAROCKS_OPTS be passed to cmake somehow? We use list(APPEND ... so it should have the same effect.

Hmm, maybe it would work just to do -DLUAROCK_OPTS... I'll check.

Also have to handle neovim/homebrew-neovim#149 when LUA_PATH and LUA_CPATH are set in the user's environment for use with Homebrew's Lua.

Other formulas seems to have solved it like this https://github.com/Homebrew/homebrew-core/pull/12326/files

ilovezfs · 2017-06-13T07:09:23Z

Formula/neovim.rb

+            "-DUSE_BUNDLED=OFF",
+            "-DUSE_BUNDLED_LUAROCKS=ON",
+            "-DUSE_BUNDLED_LUAJIT=ON",
+            "-DUSE_BUNDLED_LUV=ON",


we have a luvit formula

luv and luvit are different afaik . https://github.com/luvit/luv

Yes, luv is a minimal alternative to luvit (which is much more complicated). Is luv not in brew core?

No, in core there's only luvit.

ilovezfs · 2017-06-13T07:14:36Z

Formula/neovim.rb

+      args = std_cmake_args + %W[
+        -DDEPS_PREFIX=../deps-build/usr
+        -DCMAKE_BUILD_TYPE=RelWithDebInfo
+        -DMSGPACK_LIBRARY=#{Formula["msgpack"].opt_lib}/libmsgpackc.2.dylib


Is it not able to find this automatically and if not, did you find a way not to have to hard code the library version number?

ilovezfs · 2017-06-13T07:14:55Z

Formula/neovim.rb

+        -DDEPS_PREFIX=../deps-build/usr
+        -DCMAKE_BUILD_TYPE=RelWithDebInfo
+        -DMSGPACK_LIBRARY=#{Formula["msgpack"].opt_lib}/libmsgpackc.2.dylib
+        -DIconv_INCLUDE_DIRS:PATH=#{MacOS.sdk_path}/usr/include


Is it not able to find this automatically? And same question for libiconv.dylib

IIRC random iconv in the include path of some users, caused problems.

The formula as written is using the the macOS libiconv and headers...

random iconvs

Ah. That shouldn't be an issue since superenv filters such things out.

@ilovezfs Too fast. I updated my comment after actually reading the code.

So we should be able to just remove these 2 lines ?

see neovim/homebrew-neovim@f0469ed for why this was added. Is superenv something that didn't exist around December 2015?

I think that in the homebrew(-core) world stray libraries are a nono and brew doctor is supposed to pick up things like that and detect any potential conflicts. It can therefore be assumed that the brewed formulas will pick up the intended libraries.

If that's the case, then it should be safe to remove this line.

ilovezfs · 2017-06-13T07:22:02Z

Formula/neovim.rb

+
+    resource "luarocks" do
+      url "https://github.com/luarocks/luarocks.git",
+        :revision => "5d8a16526573b36d5b22aa74866120c998466697"


: should be directly under the "

I just used the regular identation. Does this means that consecutive lines should use two "tab" stops ? The two tab stop spaces and the " align so both answers result in the same thing.

The "rule" we standardized on is alignment directly under " so for example

url "https://github.com/facebook/osquery.git", :tag => "2.4.6", :revision => "f9cb7149a9ac53c4aa563886d3bd37955876753f"

go_resource "github.com/mitchellh/gox" do url "https://github.com/mitchellh/gox.git", :revision => "c9740af9c6574448fd48eb30a71f964014c7a837" end

and for commands alignment under the " of the first or second argument depending on the phase of the moon. For example,

system "go-bindata", "-pkg", "docker", "-nocompress", "-nomemcopy", "-nometadata", "-o", "#{dir}/executors/docker/bindata.go", "prebuilt-x86_64.tar.xz", "prebuilt-arm.tar.xz"

and

system "go", "build", "-ldflags", "-X github.com/mholt/caddy/caddy/caddymain.gitTag=#{version}", "-o", bin/"caddy", "github.com/mholt/caddy/caddy"

ilovezfs · 2017-06-14T11:50:48Z

Formula/neovim.rb

+      ]
+
+      if build.with? "jemalloc"
+        args << "-DJEMALLOC_LIBRARY=#{Formula["jemalloc"].opt_lib}/libjemalloc.a"


Can this use the dylib? Is it not able to find it on its own?

That's on my todo list to check - I don't think it will be an issue.

Yeah it works without this line.

javian · 2017-06-14T21:44:21Z

So just a thought. The cmake script for the third party dependencies should automatically download any dependency it needs to complete the compilation. We could therefore remove the resources and rely on the cmake process to do its thing - would that be an acceptable solution ?

ilovezfs · 2017-06-15T01:59:43Z

Actually, the typical standard for inclusion in core is that the build should not be downloading anything except via resource blocks, which would include the luarocks, etc.

javian · 2017-06-15T20:46:19Z

Right so that means we need to add all the deps that are installed via luarocks in the cmake script which there are currently 8 + 8 dependencies. This will take a little while to put together and as far as I can tell not all the deps are versioned in the cmake script which I would assume means that the latest one should be used.

ilovezfs · 2017-06-15T20:51:51Z

Ideally upstream would specify what versions are to be used, but if they don't we should pin to whatever is current at the time of a given neovim release.

javian · 2017-06-19T23:24:59Z

I somewhat solved the issue with the installation of the rocks. However the way it works now is that it actually downloads off a remote site since the .rock file resource is unzipped at download from what I can tell. I need to be able to tell brew not to unzip the .rock file but currently don't know how.

I'm also not sure how it deals with the other dependencies since the compile seems to be working.

ilovezfs · 2017-06-19T23:33:26Z

I'm also not sure how it deals with the other dependencies since the compile seems to be working.

All the cases of -DUSE_BUNDLED_FOO=ON need to be removed.

ilovezfs · 2017-06-19T23:39:07Z

the way it works now is that it actually downloads off a remote site since the .rock file resource is unzipped at download from what I can tell.

These need to be downloaded via resource blocks.

javian · 2017-06-20T04:24:30Z

@justinmk it seems that neovim build just fine with only lpeg and mpack rocks - do you know if the other rocks are only required to run the tests ? The ones that are currently missing are

inspect
penlight
busted
luacheck
luv
nvim-client

justinmk · 2017-06-20T23:19:16Z

@javian All of those are for tests only. luv might be required in the future, but not currently.

javian · 2017-06-21T04:36:37Z

So I've removed luv and testing removing the autotools dependency. Not gotten to the shallow clone yet but it is moving in the right direction.

ilovezfs · 2017-06-21T04:38:11Z

@javian nice work!

ilovezfs · 2017-06-21T04:39:53Z

Formula/neovim.rb

+  homepage "https://neovim.io"
+
+  stable do
+    url "https://github.com/neovim/neovim/archive/v0.2.0.tar.gz"


If there aren't resources or dependencies specific to stable or head, you can make this top section

url "https://github.com/neovim/neovim/archive/v0.2.0.tar.gz" sha256 "72e263f9d23fe60403d53a52d4c95026b0be428c1b9c02b80ab55166ea3f62b5" head "https://github.com/neovim/neovim.git", :shallow => false

ilovezfs · 2017-06-21T04:42:51Z

Formula/neovim.rb

+      r.stage(buildpath/"deps-build/build/src/#{r.name}")
+    end
+
+    ENV.prepend "LUA_PATH", "#{buildpath}/deps-build/share/lua/5.1/?.lua"


Technically this should probably be ENV.prepend_path here and for LUA_CPATH.

ilovezfs · 2017-06-21T04:44:18Z

Formula/neovim.rb

+      system "luarocks-5.1", "build", "build/src/mpack/mpack-1.0.6-0.src.rock", "--tree=."
+      system "luarocks-5.1", "build", "build/src/lpeg/lpeg-1.0.1-1.src.rock", "--tree=."
+      system "cmake", "../third-party",
+            "-DUSE_BUNDLED=OFF",


This and *std_cmake_args can be on the same line as system "cmake", "../third-party",

ilovezfs · 2017-06-21T17:35:54Z

Formula/neovim.rb

+  url "https://github.com/neovim/neovim/archive/v0.2.0.tar.gz"
+  sha256 "72e263f9d23fe60403d53a52d4c95026b0be428c1b9c02b80ab55166ea3f62b5"
+
+  head do


should be

head "https://github.com/neovim/neovim.git"

no do and end

ilovezfs · 2017-06-21T17:36:52Z

Formula/neovim.rb

+      system "luarocks-5.1", "build", "build/src/mpack/mpack-1.0.6-0.src.rock", "--tree=."
+
+      system "cmake", "../third-party", "-DUSE_BUNDLED=OFF", *std_cmake_args
+


this blank line can be deleted

javian · 2017-06-21T17:37:20Z

Should I remove the VERBOSE=1 flag on the make lines ?

ilovezfs · 2017-06-21T17:39:06Z

Yeah, I think the "-DCMAKE_VERBOSE_MAKEFILE=ON" in std_cmake_args takes care of that for us.

ilovezfs · 2017-06-21T17:41:15Z

Formula/neovim.rb

+  depends_on "luajit"
+  depends_on "msgpack"
+  depends_on "unibilium"
+  depends_on :python => :recommended if MacOS.version <= :snow_leopard


why recommended?

@justinmk can you elaborate on the Python dependency ?

I think the stock python on snow leopard was too old.

ah, yes: neovim/homebrew-neovim#20

@justinmk right ... the question is why isn't it simply required? When would someone pass --without-python?

python isn't required unless user wants to run python plugins.

Vim Formula uses something similar https://github.com/Homebrew/homebrew-core/blob/master/Formula/vim.rb#L37-L38 but also "supports" python 3.

I think I would just remove the => :recommended because there is no logic in the formula for the if build.without? "python" case, and we're unlikely to get any complaints.

Indeed. So is the logic that anything post snow leopard will build with python automatically since its shipped with macOS ?

I couldn't see that there was any linkage to python in brew linkage neovim but perhaps it doesn't show up there. If that isn't the case there is no option to build --with-python for anything post snow leopard which I would assume would be the intention if you wanted to offer python script support =).

If you did see explicit Python framework links in the linkage output, it would actually be a "bug" as we prefer -undefined dynamic_lookup for Python.

So is the logic that anything post snow leopard will build with python automatically since its shipped with macOS ?

Yes, I assume so, though you'd have to check the CMake output to find out if the Python stuff is getting built or not currently.

(Note Snow Leopard also has Python, but it's just too old.)

I'll have a look and see what I can find, in the meantime I will remove the recommended and commit that later today.

javian · 2017-06-21T17:41:26Z

➜  Formula git:(neovim020) otool -L /usr/local/bin/nvim
/usr/local/bin/nvim:
	/usr/local/opt/gettext/lib/libintl.8.dylib (compatibility version 10.0.0, current version 10.5.0)
	/usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
	/usr/local/opt/libuv/lib/libuv.1.dylib (compatibility version 2.0.0, current version 2.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.50.2)
	/usr/local/opt/msgpack/lib/libmsgpackc.2.dylib (compatibility version 2.0.0, current version 2.0.0)
	/usr/local/opt/libvterm/lib/libvterm.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/local/opt/libtermkey/lib/libtermkey.1.dylib (compatibility version 15.0.0, current version 15.0.0)
	/usr/local/opt/unibilium/lib/libunibilium.0.dylib (compatibility version 4.0.0, current version 4.0.0)
	/usr/lib/libutil.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/local/opt/luajit/lib/libluajit-5.1.2.dylib (compatibility version 2.0.0, current version 2.0.5)
	/usr/local/opt/jemalloc/lib/libjemalloc.2.dylib (compatibility version 0.0.0, current version 0.0.0)

I wonder if the lua formula should be => :build - I'll give it a try. Luarocks is the dependency which is bundled with lua so I'm not certain lua itself is actually used and since luajit is linked then that should cover any lua requirements during runtime no ?

ilovezfs · 2017-06-21T17:43:54Z

I'm not sure about that, but we don't have any depends_on "lua" => :build currently, so I'm skeptical. First thing would be to check brew linkage neovim.

javian · 2017-06-21T17:50:32Z

➜  Formula git:(neovim020) brew linkage neovim
System libraries:
  /usr/lib/libSystem.B.dylib
  /usr/lib/libiconv.2.dylib
  /usr/lib/libutil.dylib
Homebrew libraries:
  /usr/local/opt/gettext/lib/libintl.8.dylib (gettext)
  /usr/local/opt/jemalloc/lib/libjemalloc.2.dylib (jemalloc)
  /usr/local/opt/libtermkey/lib/libtermkey.1.dylib (libtermkey)
  /usr/local/opt/libuv/lib/libuv.1.dylib (libuv)
  /usr/local/opt/libvterm/lib/libvterm.0.dylib (libvterm)
  /usr/local/opt/luajit/lib/libluajit-5.1.2.dylib (luajit)
  /usr/local/opt/msgpack/lib/libmsgpackc.2.dylib (msgpack)
  /usr/local/opt/unibilium/lib/libunibilium.0.dylib (unibilium)

It doesn't appear there. Technically we wouldn't require LUA if Luarocks had a formula of its own.

ilovezfs · 2017-06-22T04:13:01Z

@justinmk any idea why the MacPorts port depends on luabitop? https://github.com/macports/macports-ports/blob/master/editors/neovim/Portfile#L38

ilovezfs · 2017-06-22T04:14:24Z

Formula/neovim.rb

+  head "https://github.com/neovim/neovim.git"
+
+  depends_on "cmake" => :build
+  depends_on "libtool" => :build


Are you sure this is needed?

No, will test without.

ilovezfs · 2017-06-22T04:46:33Z

Formula/neovim.rb

+      system "luarocks-5.1", "build", "build/src/lpeg/lpeg-1.0.1-1.src.rock", "--tree=."
+      system "luarocks-5.1", "build", "build/src/mpack/mpack-1.0.6-0.src.rock", "--tree=."
+      system "cmake", "../third-party", "-DUSE_BUNDLED=OFF", *std_cmake_args
+


This blank line can be removed

ilovezfs · 2017-06-22T04:49:20Z

@javian yeah let's make it depends_on "lua@5.1" => :build and see how it goes.

ilovezfs · 2017-06-22T04:59:14Z

Formula/neovim.rb

+  depends_on "libtermkey"
+  depends_on "libuv"
+  depends_on "libvterm"
+  depends_on "lua@5.1"


Does 5.2 not work?

5.1 is our target, since that is mostly what luajit tracks.

I believe the reason I put this down is that it would not build with 5.2 and that I read somewhere that luajit was not 5.2 compatible which is why I changed it.

Even the homebrew formula requires you to add an additional option to enable 5.2 compat https://github.com/Homebrew/homebrew-core/blob/master/Formula/luajit.rb#L23

5.1 is our target, since that is mostly what luajit tracks.

OK, this seems fine then.

justinmk · 2017-06-22T06:47:15Z

any idea why the MacPorts port depends on luabitop?

some of our build-time generator scripts use lua bitwise operations. This is builtin to luajit but not lua 5.1.

ilovezfs · 2017-06-22T06:52:21Z

@justinmk OK, so because we have depends_on "luajit", it's unneeded?

justinmk · 2017-06-22T07:18:39Z

Ah yes, if luajit is not optional then bitop is implied.

javian · 2017-06-22T16:43:25Z

Since lua is not linked in anyway to the binary and the downloaded luarocks are not copied into the Formula in any way and deleted once the install is complete the user wouldn't be able to access the compiled rocks in any case. If accessing the rocks from the runtime is a requirement then we need to do this differently. The initial test of running the runtime works but I haven't tested the lua functions so something might be hidden underneath the surface.

justinmk · 2017-06-22T23:16:54Z

FYI Lua or luajit is required and compiled into 0.2.1

ilovezfs · 2017-06-23T08:26:41Z

@justinmk So this may be a stupid question, but if we were to require the main lua formula, which is currently version 5.2.4, as a run-time dependency, would that mean that we could drop both the luajit and lua@5.1 dependencies?

ilovezfs · 2017-06-23T08:29:37Z

Formula/neovim.rb

+
+  test do
+    (testpath/"test.txt").write("Hello World from Vim!!")
+    system bin/"nvim", "--headless", "-i", "NONE", "-u", "NONE", "+s/Vim/Neovim/g", "+wq", "test.txt"


system bin/"nvim", "--headless", "-i", "NONE", "-u", "NONE", "+s/Vim/Neovim/g", "+wq", "test.txt"

justinmk · 2017-06-25T15:12:17Z

if we were to require the main lua formula, which is currently version 5.2.4, as a run-time dependency, would that mean that we could drop both the luajit and lua@5.1 dependencies?

We aren't prepared to support Lua 5.2. Maybe we accidentally do, but no one has looked closely at it, and it could break in the future if we write some Lua 5.1 code that is broken by 5.2.

What's the problem with lua@5.1 ? Lua 5.1 is like C99: yes, C11 exists, but no, C99 is not going away. Lua 5.1 cannot be regarded as "deprecated". Not to mention luajit...

ilovezfs · 2017-06-25T15:15:03Z

@justinmk we like to avoid dependencies on versioned @ formulae, especially in a new formula like neovim, but in this particular case I'm happy with luajit+lua@5.1 if upstream is.

ilovezfs · 2017-06-25T15:25:02Z

@javian Thanks for enduring the review and the new formula! 💯 🆕 🚀

@justinmk thanks for the upstream help! It's really appreciated :)

ilovezfs suggested changes Jun 11, 2017

View reviewed changes

ilovezfs added the new formula PR adds a new formula to Homebrew/homebrew-core label Jun 11, 2017

ilovezfs reviewed Jun 11, 2017

View reviewed changes

ilovezfs reviewed Jun 12, 2017

View reviewed changes

javian commented Jun 12, 2017

View reviewed changes

ilovezfs reviewed Jun 13, 2017

View reviewed changes

ilovezfs reviewed Jun 14, 2017

View reviewed changes

ilovezfs reviewed Jun 21, 2017

View reviewed changes

ilovezfs reviewed Jun 22, 2017

View reviewed changes

ilovezfs reviewed Jun 23, 2017

View reviewed changes

javian force-pushed the neovim020 branch from 2faff76 to a0c98a8 Compare June 25, 2017 14:48

neovim 0.2.0 (new formula)

19a8cc2

javian force-pushed the neovim020 branch from a0c98a8 to 19a8cc2 Compare June 25, 2017 15:06

ilovezfs approved these changes Jun 25, 2017

View reviewed changes

ilovezfs closed this in e290aa0 Jun 25, 2017

petobens mentioned this pull request Jul 3, 2017

macOS: hangs/freezes/crash with jemalloc 5.0.0 neovim/neovim#6950

Closed

Homebrew locked and limited conversation to collaborators May 4, 2018

		system "luarocks-5.1", "build", "build/src/mpack/mpack-1.0.6-0.src.rock", "--tree=."

		system "cmake", "../third-party", "-DUSE_BUNDLED=OFF", *std_cmake_args

neovim 0.2.0 (new formula) #14485

neovim 0.2.0 (new formula) #14485

Conversation

javian commented Jun 11, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk Jun 13, 2017 • 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

Choose a reason for hiding this comment

ilovezfs Jun 11, 2017 • 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

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

javian commented Jun 11, 2017

Choose a reason for hiding this comment

ilovezfs commented Jun 12, 2017

javian commented Jun 12, 2017

javian 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

ilovezfs commented Jun 12, 2017

javian commented Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javian Jun 13, 2017 • 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

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

justinmk Jun 13, 2017 • 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

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

javian commented Jun 14, 2017 • edited Loading

ilovezfs commented Jun 15, 2017

javian commented Jun 11, 2017 •

edited

Loading

justinmk Jun 13, 2017 •

edited

Loading

ilovezfs Jun 11, 2017 •

edited

Loading

javian commented Jun 13, 2017 •

edited

Loading

javian Jun 13, 2017 •

edited

Loading

justinmk Jun 13, 2017 •

edited

Loading

javian commented Jun 14, 2017 •

edited

Loading