-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
luaPackages.luv: cleanup build #80528
Conversation
Use github source tarball and use cmakeFlags instead of hacky sed to CMakeLists.txt. Mark TODO for darwin checks I can't perform myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the effort!
It works as expected in a Linux environment.
@GrahamcOfBorg build neovim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The darwin build is broken: libluv.dylib
not found.
Note: Although #79870 was closed by @bjornfor , I doubt that it builds on Darwin as expected.
Just as before. Hence I think this PR is still relevant. It's just that I can't fiddle around with the darwin specific build changes we need to set in order for Neovim to build on Darwin. Help will be appreciated. |
This PR fixed the build for me on NixOS using latest |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/why-does-gconf-depends-on-python2/5995/5 |
use neovim from stable channel until NixOS/nixpkgs#80528 is merged
@thefloweringash Thanks a lot for your help! Everything should be ready now for a Darwin build test by @GrahamcOfBorg . |
Just to sum up the bigpicture issues:
Previously we had a luv lua module that contained an alternative luv built via cmake, here we just have a luv. |
@GrahamcOfBorg build neovim |
Remove luv from lua-packages.csv
pkgs/top-level/lua-packages.nix
Outdated
@@ -99,6 +99,51 @@ with self; { | |||
|
|||
luarocks-nix = callPackage ../development/tools/misc/luarocks/luarocks-nix.nix { }; | |||
|
|||
luv = pkgs.stdenv.mkDerivation rec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't appear as a lua module anymore, thus installation of luaPackages.nvim-client fails. You can add toLuaModule() but it triggers another issue. Also it doesn't seem to install lua files anymore does it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw I've created this luvit/luv#457
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need luarocks 3.3.1 ? I've reverted the luarocks bump in my fork, maybe we could do likewise until we find a better fix like the one mentioned in my luv issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for catching that @teto (neovim
builds fine because doCheck = false
by default). After a while trying to debug this - I've finally realized that the BUILD_MODULE
cmake flag is what's so problematic - they either install it as a module without headers (which is good only for native Lua packages) or they install everything (which is suitable for neovim). This very issue is what created the original motivation for the libluv
and luv
different derivations ! I've failed to understand that earlier..
Let's wait for their response for my luvit/luv#458 (comment) .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair, there is also the problem that the current nixpkgs lua infrastructure doesn't work with cmake based project, I've worked quite a bit on it yesterday and found some issues with luarocks . I think I could solve it via creating a cmake wrapper but meanwhile I've proceeded to upload some unrelated changes I had in my fork #81067 . I don't have the space to run a nixpkgs-review on it (trying to buy a new SSD) but it should be ok overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
luv should make sure that SHAREDLIBS_INSTALL_INC_DIR is not empty, it should be possible to define it with the existing variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I'm working on a fix for luv's CMakeLists right now, feel free to open an issue first there and explain the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teto while investigating the issue with luv's CMakeLists, I've come to the conclusion that luarocks make
sets too many cmake variables, and I don't think we can stop it from doing it. Here's the list of them extracted out of a nix-build -A luajit.pkgs.luv
while NIX_DEBUG = 2;
was set in luv's derivation:
-G"Unix Makefiles"
-DCMAKE_MODULE_LINKER_FLAGS="-shared"
-DLIBDIR="/nix/store/5wf8zrgpp199pmbbgyvkp7hc484pzrbs-luajit-2.1.0-beta3-luv-1.34.2-0/luv-1.34.2-0-rocks/luv/1.34.2-0/lib"
-DLUA_INCDIR="/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/include/luajit-2.1"
-DLUA_LIBDIR=""
-DLUA="/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/bin/luajit"
-DCMAKE_LIBRARY_PATH="/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/lib:/nix/store/w89ndqsb7ig22mw5hk7k7zfvxmhx44xb-libuv-1.34.2/lib:/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/lib:/nix/store/w89ndqsb7ig22mw5hk7k7zfvxmhx44xb-libuv-1.34.2/lib"
-DCMAKE_INCLUDE_PATH="/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/include:/nix/store/w89ndqsb7ig22mw5hk7k7zfvxmhx44xb-libuv-1.34.2/include:/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/include:/nix/store/w89ndqsb7ig22mw5hk7k7zfvxmhx44xb-libuv-1.34.2/include"
-DLUADIR="/nix/store/5wf8zrgpp199pmbbgyvkp7hc484pzrbs-luajit-2.1.0-beta3-luv-1.34.2-0/luv-1.34"
There are a couple of troubles with this:
LIBDIR
is set to some distant path inside$out
and not just$out/lib
.LUA_LIBDIR
is empty.
In my opinion, (1) is severe and unfixable - it's hard-coded inside luarocks make
. I think luarocks make
can't even imagine itself running a cmake command that will imply installing files in a $prefix/lib
instead of $prefix/${rockname-version}-rocks/${rockname}/${version}/lib
. Even if we'll fix the original issue cp
error this PR was written for, I doubt that neovim
will be able to find pkg-config files for luv inside the standard ${luv}/lib
path.
I'm not sure what's the effects of (2) but I don't like it either.
This is very messy indeed, most likely because luarocks
doesn't expect packages that provide headers and shared libraries someone (or a project like neovim) would expect to find in the standard locations. And I think I realize now that according to this behavior, the original design of luv's CMakeLists files chose to make the BUILD_MODULE and BUILD_SHARED_LIBS colliding.
While writing this, I think perhaps we should provide a libluv
package in lua-packages.nix
that will use stdenv.mkDerivation
and that will be used by neovim along with another luv
package that we'll only override, just as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried my idea and I think it's the most elegant solution to the issue. Especially considering luarocks
lack of support for installing arbitrary files to a $prefix
, as explained above. I've pushed the final fix for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having to maintain the same package twice because its build system cant find where to install files does not seem elegant + it will keep being a headache; better fix it now that we have everything in mind. As for the cmake call via luarocks; it s not a problem; you can override the parameters via environment variables or patch the luv rockspec. We better open an issue on luv. I am busy tomorrow but will try to do at least saturday.
HI, I just tried to install neovim with nix-darwin and I am getting the same build error:
As neovim is one of the first packages a user might install after installing nix, shouldn't this be fixed ASAP? |
That has been fixed quite some time ago by the reverts. |
This is evidence that a Darwin user is still encountering this issue, which is troubling but I have no idea why is that. @amckinlay I'm sorry to hear about your experience, but unfortunately this issue is a nasty one which I think most of the involved consider it "upstream's fault". The discussions and comments above should explain it more thoroughly, but basically, we are stuck because of: luarocks/luarocks#509 and/or luarocks/luarocks#339 . Note that we are also stuck with Luarocks 3.2.1 because updating it will break Neovim for all of us. See luarocks/luarocks#1155 . Even if we'd try to solve this without waiting for upstream, a cleaner fix (then this PR) is orthogonal to some other big changes to Nixpkgs:
And more... 😞 |
@doronbehar No my revert was for the darwin side. https://hydra.nixos.org/job/nixpkgs/trunk/neovim.x86_64-darwin @amckinlay I would suggest ask a question on irc or discourse, I suspect you've not updated your channel (eg. multi-user) but this is not the right place for debugging that. |
@LnL7 @doronbehar Okay, it was fixed. For some reason all my channels were missing so my |
I marked this as stale due to inactivity. → More info |
rev = version; | ||
sha256 = "0iq272n7p0wkll4a7d880qyhdp65582cwc3b2zzrirpli93x3v87"; | ||
}; | ||
disabled = (luaOlder "5.1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disabled = (luaOlder "5.1"); | |
disabled = luaOlder "5.1"; |
# So we can be sure no internal dependency is used from the repo and that | ||
# everything is provided by us | ||
postUnpack = '' | ||
rm -rf deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm -rf deps | |
rm -r deps |
NIX_LDFLAGS = pkgs.lib.optionalString pkgs.stdenv.isDarwin | ||
(if isLuaJIT then "-lluajit-${lua.luaversion}" else "-llua"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIX_LDFLAGS = pkgs.lib.optionalString pkgs.stdenv.isDarwin | |
(if isLuaJIT then "-lluajit-${lua.luaversion}" else "-llua"); | |
NIX_LDFLAGS = pkgs.lib.optionalString pkgs.stdenv.isDarwin (if isLuaJIT then "-lluajit-${lua.luaversion}" else "-llua"); |
(if isLuaJIT then "-lluajit-${lua.luaversion}" else "-llua"); | ||
propagatedBuildInputs = [ lua ]; | ||
|
||
meta = with stdenv.lib; { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meta = with stdenv.lib; { | |
meta = with lib; { |
license = { | ||
fullName = "Apache 2.0"; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
license = { | |
fullName = "Apache 2.0"; | |
}; | |
license = licenses.asl20; |
NIX_LDFLAGS = pkgs.lib.optionalString pkgs.stdenv.isDarwin | ||
(if isLuaJIT then "-lluajit-${lua.luaversion}" else "-llua"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIX_LDFLAGS = pkgs.lib.optionalString pkgs.stdenv.isDarwin | |
(if isLuaJIT then "-lluajit-${lua.luaversion}" else "-llua"); | |
NIX_LDFLAGS = pkgs.lib.optionalString pkgs.stdenv.isDarwin (if isLuaJIT then "-lluajit-${lua.luaversion}" else "-llua"); |
rm -rf deps | ||
'' | ||
# See the following issues: | ||
# - https://github.com/luarocks/luarocks/issues/1160 | ||
# - https://github.com/luarocks/luarocks/issues/509 | ||
# - https://github.com/luarocks/luarocks/issues/339 | ||
+ '' | ||
sed -i 's,\(option(WITH_SHARED_LIBUV.*\)OFF,\1ON,' CMakeLists.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm -rf deps | |
'' | |
# See the following issues: | |
# - https://github.com/luarocks/luarocks/issues/1160 | |
# - https://github.com/luarocks/luarocks/issues/509 | |
# - https://github.com/luarocks/luarocks/issues/339 | |
+ '' | |
sed -i 's,\(option(WITH_SHARED_LIBUV.*\)OFF,\1ON,' CMakeLists.txt | |
rm -r deps | |
# See the following issues: | |
# - https://github.com/luarocks/luarocks/issues/1160 | |
# - https://github.com/luarocks/luarocks/issues/509 | |
# - https://github.com/luarocks/luarocks/issues/339 | |
sed -i 's,\(option(WITH_SHARED_LIBUV.*\)OFF,\1ON,' CMakeLists.txt |
This reverts commit eec90bc. See discussion in: NixOS#141718 NixOS#80528
This reverts commit eec90bc. See discussion in: NixOS#141718 NixOS#80528
Motivation for this change
Things done
passthru
oflibluv
which was always too confusing IMO - I think I understand the original motivation behind that but I don't think it's worth the maintenance / confusion burden it introduced.luaPackages.compat53
install it's c-api files luv needs in order to build cleanly.sandbox
innix.conf
on non-NixOS linux)TODO
s I've left.nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)ccing people involved in #79870 :