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

feat: move neovim flake into the overlay itself #483

Closed
wants to merge 1 commit into from

Conversation

ribru17
Copy link
Contributor

@ribru17 ribru17 commented Apr 18, 2024

Attempts to move the Neovim nix flake into this overlay. See neovim/neovim#28305 (comment). I'm marking this as draft because I am not confident enough in my Nix knowledge, but in my local testing this seemed to work well. The sub-flake importing followed solutions discussed in NixOS/nix#3978, though it has its caveats as mentioned over there. Still, I think this may be a fine solution, and I will try to be quick to amend the PR if anything needs a change 👍

EDIT: I think this is ready for review. It seems to work fine in my configuration. And I believe the drawbacks mentioned above are not applicable here.

Note that the flake copied over here differs slightly from the original Neovim flake in that it pulls in

{
    neovim-src.url = "github:neovim/neovim";
    neovim-src.flake = false;
}

because it obviously can no longer use relative paths to access the files in the repo.

@ribru17 ribru17 marked this pull request as ready for review April 18, 2024 16:45
@@ -6,7 +6,7 @@
flake-parts = { url = "github:hercules-ci/flake-parts"; inputs.nixpkgs-lib.follows = "nixpkgs"; };
hercules-ci-effects = { url = "github:hercules-ci/hercules-ci-effects"; inputs.nixpkgs.follows = "nixpkgs"; };
flake-compat = { url = "github:edolstra/flake-compat"; flake = false; };
neovim-flake = { url = "github:neovim/neovim?dir=contrib"; inputs.nixpkgs.follows = "nixpkgs"; };
neovim-flake = { url = "path:./neovim"; inputs.nixpkgs.follows = "nixpkgs"; };
Copy link
Contributor

Choose a reason for hiding this comment

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

I get a `cannot fetch path because it uses a relative path. I believe we should just keep this flake and merge neovim's one into it (whcih could also import *.nix files)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Man I tested this when I put it up and swear I didn't get this error 😆 Thanks for pointing this out, I will merge the two 👍

@teto
Copy link
Contributor

teto commented May 1, 2024

thanks for doing this ! It's very much appreciated.

@clason
Copy link

clason commented May 17, 2024

@ribru17 You need to bump the tree-sitter dependency to 0.22.6 for nightly/0.11

@teto can we merge this ASAP (and remove the flake from neovim/neovim)? We're getting increasing noise from the outdated flake.

@ribru17
Copy link
Contributor Author

ribru17 commented May 17, 2024

Apologies: @teto I might need some guidance from you on this. When trying to merge the two flakes my lack of nix knowledge began showing and I was unable to resolve the errors after quite some trying. I'll keep on it today but maybe in the meantime maybe I can just make a PR to update the dependency in the neovim repo?

EDIT: I think the current unstable version of nixpkgs also has the out-of-date version of tree-sitter (0.22.5) so it will need to be bumped there first so we can update the flake input here (or in the neovim repo)

dundargoc added a commit to dundargoc/neovim that referenced this pull request May 20, 2024
It does not work and we don't plan on maintaining these anymore.

The flake files are being moved to
nix-community/neovim-nightly-overlay#483
instead.
@willruggiano
Copy link
Contributor

@ribru17 I just moved off of neovim/neovim?dir=contrib so if you need a reference you can check out: willruggiano/neovim.drv@f16c01d#diff-e126406cafda4a612d447ad20677962966b4f61764ea252c7c932b244e7d5cd3

Note that this also uses the versions specified in deps.txt for a few other dependencies: gettext, libiconv, libuv, libvterm-neovim, msgpack-c and tree-sitter. So your comment above about the outdated tree-sitter version is resolved by this method.

@ribru17
Copy link
Contributor Author

ribru17 commented May 20, 2024

Thank you, this is amazing!! I will try and integrate a minimal version of it here.

@willruggiano
Copy link
Contributor

Thank you, this is amazing!! I will try and integrate a minimal version of it here.

No problem. Feel free to @ me if you need any help.

@teto
Copy link
Contributor

teto commented May 21, 2024

@ribru17 I will start doing it myself in a few hours if you dont beat me to it. Neovim core contributors are (rightfully) tired of dealing with nix issues and I've made them wait too long. I've got a window of opportunity in a few hours.

@ribru17
Copy link
Contributor Author

ribru17 commented May 21, 2024

Thank you @teto, I appreciate your work. I've definitely bitten off more than I can chew with this one :/ Apologies if I led anyone on with this one, I definitely had higher confidence in myself in the beginning stages of this PR...

@teto
Copy link
Contributor

teto commented May 23, 2024

finally done in #516 . Sry for the trouble

@teto teto closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants