Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

neovim: fix build on Darwin #95364

Merged
merged 1 commit into from
Aug 17, 2020
Merged

neovim: fix build on Darwin #95364

merged 1 commit into from
Aug 17, 2020

Conversation

siriobalmelli
Copy link
Contributor

libluv path passed to -DLIBLUV_LIBRARY broken by change in libluv:
libluv.dylib -> libluv.1.30.0.dylib

Signed-off-by: Sirio Balmelli sirio@b-ad.ch

Motivation for this change

I daily-drive neovim on Darwin

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@thefloweringash
Copy link
Member

thefloweringash commented Aug 13, 2020

I think this is luarocks still breaking the symlinks. It’s filesystem enumeration order dependent, so breaks at random. Longer comment at #81206 (comment)

Notice the symlinks aren’t alongside the library in the lib/lua/5.1 directory.

$ tree -l $(nix-build -A luajit.pkgs.luv.libluv) 
/nix/store/dfllmqilrddan563d953ph6xy6xhl2sz-luajit-2.1.0-2020-08-05-luv-1.30.0-0
├── include
│   └── luv
│       ├── lhandle.h
│       ├── lreq.h
│       ├── luv.h
│       └── util.h
├── lib
│   └── lua
│       └── 5.1
│           └── libluv.1.30.0.dylib
├── luv-1.30.0-0-rocks
│   ├── luv
│   │   └── 1.30.0-0
│   │       ├── doc
│   │       │   ├── LICENSE.txt
│   │       │   ├── README.md
│   │       │   └── docs.md
│   │       ├── lib
│   │       │   ├── libluv.1.dylib -> libluv.1.30.0.dylib
│   │       │   └── libluv.dylib -> libluv.1.dylib
│   │       ├── luv-1.30.0-0.rockspec
│   │       └── rock_manifest
│   └── manifest
└── nix-support
    └── propagated-build-inputs

11 directories, 14 files

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Aug 13, 2020
@ofborg ofborg bot requested review from manveru and Ma27 August 13, 2020 20:25
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Aug 13, 2020
@siriobalmelli
Copy link
Contributor Author

I think this is luarocks still breaking the symlinks. It’s filesystem enumeration order dependent, so breaks at random. Longer comment at #81206 (comment)

Notice the symlinks aren’t alongside the library in the lib/lua/5.1 directory.

$ tree -l $(nix-build -A luajit.pkgs.luv.libluv) 
/nix/store/dfllmqilrddan563d953ph6xy6xhl2sz-luajit-2.1.0-2020-08-05-luv-1.30.0-0
├── include
│   └── luv
│       ├── lhandle.h
│       ├── lreq.h
│       ├── luv.h
│       └── util.h
├── lib
│   └── lua
│       └── 5.1
│           └── libluv.1.30.0.dylib
├── luv-1.30.0-0-rocks
│   ├── luv
│   │   └── 1.30.0-0
│   │       ├── doc
│   │       │   ├── LICENSE.txt
│   │       │   ├── README.md
│   │       │   └── docs.md
│   │       ├── lib
│   │       │   ├── libluv.1.dylib -> libluv.1.30.0.dylib
│   │       │   └── libluv.dylib -> libluv.1.dylib
│   │       ├── luv-1.30.0-0.rockspec
│   │       └── rock_manifest
│   └── manifest
└── nix-support
    └── propagated-build-inputs

11 directories, 14 files

Agreed, that's what I'm seeing.

My current idea is to work around this by not relying on symlinks but instead computing what the library name should be:

  luv = lua.pkgs.luv;
  luvpath = with builtins ; if stdenv.isDarwin
    then "${luv.libluv}/lib/lua/${lua.luaversion}/libluv.${head (match "([0-9.]+).*" luv.version)}.dylib"
    else "${luv}/lib/lua/${lua.luaversion}/luv.so";

On Darwin 10.14.6 this gives:

/nix/store/jpzxw0ahvrg7yl6gf68k41i3lvaybmj7-lua5.2-luv-1.30.0-0/lib/lua/5.2/libluv.1.30.0.dylib

... which is referencing the non-symlinked portion of the tree:

/nix/store/jpzxw0ahvrg7yl6gf68k41i3lvaybmj7-lua5.2-luv-1.30.0-0
...
├── lib
│   └── lua
│       └── 5.2
│           └── libluv.1.30.0.dylib
...

My guess is that this would make this less fragile in the long-run?

Also, I am uncertain of the difference between:

  • luajit.pkgs.luv.libluv
  • lua.pkgs.luv.libluv

... they don't evaluate to the same derivation on my machine

@doronbehar
Copy link
Contributor

That's because lua = lua52 not luajit.

@doronbehar
Copy link
Contributor

@siriobalmelli libluv is almost broken ever since Luarocks 3.3.1. See:

The best attempt to fix this was #80528 but it got stale. Bottom lines: #80528 (comment) & #80528 (comment) & #80528 (comment).

I'm sorry to hear that Neovim is now broken on Darwin, but I'm not sure what can be done. I think the best way to go is to help Luarocks play better with cmake based rocks builds, as @teto said in #80528 (comment) .

@siriobalmelli
Copy link
Contributor Author

That's because lua = lua52 not luajit.

I'm tracking ... but (from someone who is not in Lua ecosystem at all), which one should we be using for neovim?

@siriobalmelli
Copy link
Contributor Author

@siriobalmelli libluv is almost broken ever since Luarocks 3.3.1. See:

* [luarocks/luarocks#1155](https://github.com/luarocks/luarocks/issues/1155)

* #79870

The best attempt to fix this was #80528 but it got stale. Bottom lines: #80528 (comment) & #80528 (comment) & #80528 (comment).

I'm sorry to hear that Neovim is now broken on Darwin, but I'm not sure what can be done. I think the best way to go is to help Luarocks play better with cmake based rocks builds, as @teto said in #80528 (comment) .

Wow, I see you and @thefloweringash already spent quite a bit of effort trying to get this fixed ... but I'm also seeing the upstream issue luarocks/luarocks#1160 is still open 😮

From my understanding, we can avoid immediate breakage with this PR by referencing libluv.1.30.0.dylib directly, instead of libluv.dylib which has this flaky symlink issue ... while upstream Lua gets their house in order.

@doronbehar
Copy link
Contributor

which one should we be using for neovim?

luajit.

but I'm also seeing the upstream issue luarocks/luarocks#1160 is still open 😮

Yea, this was sort of already requested at luarocks/luarocks#339 which is even more ancient.

From my understanding, we can avoid immediate breakage with this PR by referencing libluv.1.30.0.dylib directly, instead of libluv.dylib which has this flaky symlink issue ... while upstream Lua gets their house in order.

Sounds good to me. Please use ${libluv.version} in place of 1.30.0.

@doronbehar
Copy link
Contributor

@GrahamcOfBorg build neovim

@doronbehar
Copy link
Contributor

@GrahamcOfBorg eval

@siriobalmelli
Copy link
Contributor Author

which one should we be using for neovim?

luajit.

but I'm also seeing the upstream issue luarocks/luarocks#1160 is still open 😮

Yea, this was sort of already requested at luarocks/luarocks#339 which is even more ancient.

From my understanding, we can avoid immediate breakage with this PR by referencing libluv.1.30.0.dylib directly, instead of libluv.dylib which has this flaky symlink issue ... while upstream Lua gets their house in order.

Sounds good to me. Please use ${libluv.version} in place of 1.30.0.

Thank you, appreciated. A couple notes:

Standardized on lua.pkgs.luv.libluv (some stanzas referenced lua.pkgs.luv instead).

Both luajit.pkgs.luv.version and luajit.pkgs.luv.libluv.version give "1.30.0-0", in order to derive 1.30.0 the trailing -0 must be stripped, which I'm currently doing with with builtins; head (match "([0-9.]+).*" luv.version) (suggestions for improvement welcome).

@doronbehar
Copy link
Contributor

Damn, ofborg detected this as a no changed output for Linux, yet it still failed to compile!

[100%] Building C object src/nvim/CMakeFiles/nvim.dir/xdiff/xhistogram.c.o
[100%] Building C object src/nvim/CMakeFiles/nvim.dir/xdiff/xprepare.c.o
make[2]: *** No rule to make target '/nix/store/5dvf6r6qm5rx5ng02gfcm79fjbrg1ph0-luajit-2.1.0-2020-08-05-luv-1.30.0-0/lib/lua/5.1/luv.so', needed by 'bin/nvim'.  Stop.
make[2]: *** Waiting for unfinished jobs....

trailing -0 must be stripped, which I'm currently doing with

I can't think of a better way to do it, well done.

@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Aug 14, 2020
@siriobalmelli
Copy link
Contributor Author

Damn, ofborg detected this as a no changed output for Linux, yet it still failed to compile!

[100%] Building C object src/nvim/CMakeFiles/nvim.dir/xdiff/xhistogram.c.o
[100%] Building C object src/nvim/CMakeFiles/nvim.dir/xdiff/xprepare.c.o
make[2]: *** No rule to make target '/nix/store/5dvf6r6qm5rx5ng02gfcm79fjbrg1ph0-luajit-2.1.0-2020-08-05-luv-1.30.0-0/lib/lua/5.1/luv.so', needed by 'bin/nvim'.  Stop.
make[2]: *** Waiting for unfinished jobs....

trailing -0 must be stripped, which I'm currently doing with

I can't think of a better way to do it, well done.

I'm on it - will have a fix up shortly

libluv path passed to -DLIBLUV_LIBRARY broken by change in libluv, eg:
libluv.dylib -> libluv.1.30.0.dylib

Signed-off-by: Sirio Balmelli <sirio@b-ad.ch>
@siriobalmelli
Copy link
Contributor Author

ok, looks like Linux needs to reference lua.pkgs.luv while Darwin needs lua.pkgs.luv.libluv ... neither system will work with the other path.

My efforts to standardize on lua.pkgs.luv.libluv were what broke Linux ... even though it is a valid derivation (?)

Scratching my head head here a bit, but it works and that's good enough for the interim, while Lua upstream straightens out.

@siriobalmelli
Copy link
Contributor Author

@GrahamcOfBorg build neovim

@siriobalmelli
Copy link
Contributor Author

@GrahamcOfBorg eval

@thefloweringash
Copy link
Member

In that OfBorg failure, the requested path file was luv.so but the directory contains libluv.so. Does it work if you add lib to linux as well?

@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-linux: 1-10 labels Aug 14, 2020
@siriobalmelli
Copy link
Contributor Author

siriobalmelli commented Aug 14, 2020

In that OfBorg failure, the requested path file was luv.so but the directory contains libluv.so. Does it work if you add lib to linux as well?

Unfortunately, that was the change I tried that broke the build on Linux:

  luv = lua.pkgs.luv;
  luversion = with builtins; head (match "([0-9.]+).*" luv.version);
  luvpath =  if stdenv.isDarwin
    then "${luv.libluv}/lib/lua/${lua.luaversion}/libluv.${luversion}.dylib"
    else "${luv.libluv}/lib/lua/${lua.luaversion}/libluv.so.${luversion}";

This gives:

nix-repl> :l ./default.nix
Added 12234 variables.

nix-repl> luv = lua.pkgs.luv

nix-repl> luversion = with builtins; head (match "([0-9.]+).*" luv.version)  

nix-repl> luvpath3 = "${luv.libluv}/lib/lua/${lua.luaversion}/libluv.so.${luversion}"

nix-repl> luvpath3                                                                    
"/nix/store/bwbh13d3s22izh8vy5kwa5f7qpxb4bpz-lua5.2-luv-1.30.0-0/lib/lua/5.2/libluv.so.1.30.0"

... and the derivation looks like:

$ tree /nix/store/bwbh13d3s22izh8vy5kwa5f7qpxb4bpz-lua5.2-luv-1.30.0-0
/nix/store/bwbh13d3s22izh8vy5kwa5f7qpxb4bpz-lua5.2-luv-1.30.0-0
|-- include
|   `-- luv
|       |-- lhandle.h
|       |-- lreq.h
|       |-- luv.h
|       `-- util.h
|-- lib
|   `-- lua
|       `-- 5.2
|           `-- libluv.so.1.30.0
|-- luv-1.30.0-0-rocks
|   |-- luv
|   |   `-- 1.30.0-0
|   |       |-- doc
|   |       |   |-- LICENSE.txt
|   |       |   |-- README.md
|   |       |   `-- docs.md
|   |       |-- lib
|   |       |   |-- libluv.so -> libluv.so.1
|   |       |   `-- libluv.so.1 -> libluv.so.1.30.0
|   |       |-- luv-1.30.0-0.rockspec
|   |       `-- rock_manifest
|   `-- manifest
`-- nix-support
    `-- propagated-build-inputs

11 directories, 14 files

But unfortunately the build fails with:

[100%] Linking C executable ../../bin/nvim
[100%] Built target nvim
Scanning dependencies of target runtime
[100%] Generating syntax/vim/generated.vim
[100%] Generating doc/tags
[100%] Generating pack/dist/opt/matchit/doc/tags
[100%] Generating pack/dist/opt/vimball/doc/tags
/tmp/nix-build-neovim-unwrapped-0.4.4.drv-0/source/build/bin/nvim: error while loading shared libraries: libluv.so.1: cannot open shared object file: No such file or directory
/tmp/nix-build-neovim-unwrapped-0.4.4.drv-0/source/build/bin/nvimmake[2]: *** [runtime/CMakeFiles/runtime.dir/build.make:99: runtime/pack/dist/opt/matchit/doc/tags] Error 127
: error while loading shared libraries: libluv.so.1: cannot open shared object file: No such file or directory
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [runtime/CMakeFiles/runtime.dir/build.make:104: runtime/pack/dist/opt/vimball/doc/tags] Error 127
/tmp/nix-build-neovim-unwrapped-0.4.4.drv-0/source/build/bin/nvim: error while loading shared libraries: libluv.so.1: cannot open shared object file: No such file or directory
make[2]: *** [runtime/CMakeFiles/runtime.dir/build.make:94: runtime/doc/tags] Error 127
make[1]: *** [CMakeFiles/Makefile2:6896: runtime/CMakeFiles/runtime.dir/all] Error 2
make: *** [Makefile:171: all] Error 2
builder for '/nix/store/smzq5ll4yib9bkfj81239zpl1ljl8fcm-neovim-unwrapped-0.4.4.drv' failed with exit code 2
cannot build derivation '/nix/store/lr0hlx9zvj2liz5y6nafbja5krc96h06-neovim-0.4.4.drv': 1 dependencies couldn't be built
error: build of '/nix/store/lr0hlx9zvj2liz5y6nafbja5krc96h06-neovim-0.4.4.drv' failed

Going back to "${luv}/lib/lua/${lua.luaversion}/luv.so" builds cleanly again

@thefloweringash
Copy link
Member

Oh, I see why. The linker succeeds but the SONAME from the shared library file is libluv.so.1 so it fails at load time since it's not present on the rpath. The luv.so library doesn't have a SONAME and the resulting nvim binary has the absolute path. Thanks for checking, sorry for the derail.

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR label rebuild-linux:0 is set and the build of it for Linux and darwin succeed, so I conclude it's good to go. But yes, we do need to fix this properly at some point. Thanks for adding the link to #80528 in a comment.

@siriobalmelli
Copy link
Contributor Author

Oh, I see why. The linker succeeds but the SONAME from the shared library file is libluv.so.1 so it fails at load time since it's not present on the rpath. The luv.so library doesn't have a SONAME and the resulting nvim binary has the absolute path. Thanks for checking, sorry for the derail.

Wow, I learned something new today ... was mystified as to what was happening 😅

@LnL7 LnL7 merged commit ad6d9ad into NixOS:master Aug 17, 2020
@siriobalmelli siriobalmelli deleted the fix/neovim branch August 17, 2020 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants