Neovim: 0.3.8 -> 0.4.2 #68882
Merged
Neovim: 0.3.8 -> 0.4.2 #68882
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@GrahamcOfBorg build neovim |
ofborg
bot
added
11.by: package-maintainer
teto
reviewed
Sep 16, 2019
| preBuild = '' | ||
| sed -i 's,\(option(WITH_SHARED_LIBUV.*\)OFF,\1ON,' CMakeLists.txt | ||
| sed -i 's,\(option(BUILD_MODULE.*\)ON,\1OFF,' CMakeLists.txt | ||
| sed -i 's,$''+''{INSTALL_INC_DIR},${placeholder "out"}/include/luv,' CMakeLists.txt |
There was a problem hiding this comment.
can't we use substituteAll instead ? better use nix abstraction tools.
There was a problem hiding this comment.
sed seems more consistent with the rest of the file
teto
reviewed
Sep 16, 2019
| @@ -273,9 +276,21 @@ with super; | |||
| sed -i 's,\(option(WITH_SHARED_LIBUV.*\)OFF,\1ON,' CMakeLists.txt | |||
| rm -rf deps/libuv | |||
| ''; | |||
| }); | |||
|
|
|||
| luv-dev = super.luv.override({ | |||
There was a problem hiding this comment.
could you add a comment explaining the difference between luv and luv-dev ? why it's not luv.dev output for instance
There was a problem hiding this comment.
cc @gloaming
I assume this is not a luv.dev output, since a different build is necessary here
There was a problem hiding this comment.
It was the case when I had tried building luv but maybe they improved it by now.
If it's a temporary package to work around luv "faulty" build system, it would be good to mention it
teto
reviewed
Sep 16, 2019
|
super happy to see this PR. I made a few comments for the sake of it :) |
rvolosatovs
force-pushed
the
update/neovim
branch
from
a8d35c2 to
bca38e1
Compare
|
@GrahamcOfBorg build neovim |
|
Hey, thanks for this, but you used an older version of my work. We can use the shared library instead of the static one, and I put that output under the I rebased your package update onto my newer branch, please have a look: master...gloaming:neovim/0.4.2 |
|
@gloaming I'm getting a failure on your branch when attempting |
|
Not sure if this helps, but I got neovim-nightly to work just 3 days ago. For that I packaged libluv the following way: https://termbin.com/k75a7 neovim worked fine with just putting that derivation into the buildinputs. |
Co-authored-by: Roman Volosatovs <rvolosatovs@riseup.net>
rvolosatovs
force-pushed
the
update/neovim
branch
from
bca38e1 to
824869c
Compare
|
@GrahamcOfBorg build neovim |
|
the passtru thing looks kinda ugly but I am confident libluv system will improve so this might be a temporary wart. |
|
@rvolosatovs that's odd, it works for me 🤔 I was going to ask if you had some kind of overlay, but It's the same hash(!), so there's some kind of reproducibility issue I guess. Your version works because you're using ((824869c3fc0...))$ git diff neovim/0.4.2 -- pkgs/applications/editors/neovim/default.nix
diff --git a/pkgs/applications/editors/neovim/default.nix b/pkgs/applications/editors/neovim/default.nix
index d573a83a73d..6808d7115c5 100644
--- a/pkgs/applications/editors/neovim/default.nix
+++ b/pkgs/applications/editors/neovim/default.nix
@@ -79,7 +79,7 @@ in
cmakeFlags = [
"-DGPERF_PRG=${gperf}/bin/gperf"
- "-DLIBLUV_LIBRARY=${lua.pkgs.luv.libluv}/lib/lua/${lua.luaversion}/libluv.so"
+ "-DLIBLUV_LIBRARY=${lua.pkgs.luv}/lib/lua/${lua.luaversion}/luv.so"
"-DLUA_PRG=${neovimLuaEnv.interpreter}"
]
++ optional doCheck "-DBUSTED_PRG=${neovimLuaEnv}/bin/busted"If I change that line back, I get this: Which is why I put the I guess since Neovim works with |
|
So what still needs to be done to merge this? |
|
let's merge this. The luv thing is awkward but I am confident it will be improved in the future. |
|
This doesn't build for me on macOS + The error I get when building shared libraries with this PR as-is: |
|
and if you change the .so by .dylib instead ? wouldn't that solve the problem ? |
|
Tried various combinations of looking in The only one I've found to work (not ideal, I know) is It all works fine on Linux (using |
|
Just wanted to mention that this PR has now made it into the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
Upstream update
Refs #64400
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @gloaming @manveru