-
-
Notifications
You must be signed in to change notification settings - Fork 42
feat(cli,nvim): add nix flake and option to specify cli_cmds in nvim #144
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
=======================================
Coverage 99.27% 99.27%
=======================================
Files 22 22
Lines 1519 1523 +4
=======================================
+ Hits 1508 1512 +4
Misses 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dae5a09 to
2fe9869
Compare
|
Hi, thanks for this PR! I've never personally used Nix, so I wouldn't be able to maintain the Nix-related code. I have some questions related to the maintenance of the Nix stuff:
I'll finish reviewing the rest of the PR soon. Thanks again for doing this! |
|
you should definitely try it 😄 it's the same rabbit hole as neovim 😄
If I have the time I try to upgrade chromadb in nixpkgs also so I can create the package for vectorcode. Do you know if vectorcode also supports chromadb 1.x? |
Does this mean the current nix package builds chroma 0.6.3 from source? I see there are references to cargo, is that used for building chroma-hnswlib (this used to cause problems for ppl without the build deps so I want to know whether this is taken care of)?
The Chromadb problem is a bit complicated. I've mostly done the migration to chromadb 1.0.x in #36, and on a clean installation (for both chroma and vectorcode) it works, but if the database files were created by chroma 0.6.x it doesn't work, and it's an upstream bug. Therefore, I won't be bumping the chroma version to 1.0.x any time soon (unless they fix the incompatibility issue), and when that happens it'll be marked as a breaking change (most likely with VectorCode 1.0.0). |
Davidyz
left a 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.
Sorry about delaying this for so long. Since this PR contains quite some changes, I thought I'd make a dedicated release for it so that if anything goes wrong, people can easily revert to previous versions without losing other features/fixes.
The changes are mostly good to me, except that I don't think it's necessary to make the LSP executable configurable through setup(). Although your approach is technically ok, it adds an extra source of confusion over "which option takes precedence", so I think it's better to keep it to one place and just use vim.lsp.config.
Another thing that is missing from your PR is the content in lua/vectorcode/init.lua, which also contains quite a few hardcoded vectorcode executable. I'll address this in a separate PR (replace them with jobrunner) and merge it before this one, so that all cmd calls correctly use the configured executable.
| require("vectorcode").setup({ | ||
| cli_cmds = { | ||
| vectorcode = "vectorcode", | ||
| vectorcode_server = "vectorcode-server", |
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.
We can get rid of vectorcode_server here because the cmd is loaded from vim.lsp.config. In other words, it's already possible to overload the executable for the LSP runner.
f678e3e to
9968537
Compare
|
I've rebased changes to Next, I'll try to set up Nix and do some final tests. |
|
btw I'd like to add a formatter for the nix flake (either pre-commit or github action). Do you have any recommendations? |
alejandra is a popular one. Also check out treefmt. The nixpkgs repo itself uses nixfmt-rfc-style but I personally find its style less appealing. Not that anyone asked for my opinion but I would also recommend against hardcoding the executable paths like this. IMO it adds unnecessary complexity (see: the majority of code changes in this PR) when people can just as easily manipulate their PATH. Conversely, one might even say that |
9968537 to
13d44d6
Compare
e3aa67b to
868a1d4
Compare
|
@Davidyz when you say "run ... in nix shell" what do you mean? As it stands on this branch, I'm not sure the nix-shell will "do" anything besides setup some environment variables ( Perhaps a better test is to Note that I have experimented with this branch in all sorts of way 2 and haven't been able to reproduce this error. Footnotes
|
Thanks for the suggestions. After some more research on Nix, I'm pretty sure it was because I didn't RTFM enough. I'm now able to work with the CLI with no problems. There are still some things to fix before I merge this, but I now have a pretty good idea about what to do next. |
|
@chrishrb as a fellow nix user, could the flake also contain an overlay? that would make consumption even easier. |
| }; | ||
| }; | ||
|
|
||
| pkgVersion = "0.5.6"; |
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 is kinda confusing as it shows up in vectorcode --help as its version.
i don't have a better idea from the top of my head tho.
maybe just nightly ?
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 plan to change this to the most recent version just before merging. As far as I understand, it's not recommended in Nix to use a non-deterministic/non-reproducible version number, so we'll probably need to bump pkgVersion for every release.
|
If you needed chromadb to be at 0.6.x, why not file an upgrade ticket at NixOS/nixpkgs? P.S. As long as I'm bearing bad news, that one commit really should be multiple logical commits (doc update, add flake, adding cli_cmds for nvim, etc.). |
868a1d4 to
fec8d6d
Compare
I've been thinking about this too. I plan to checkout from this PR, strip all the nix-related stuff and merge it in a new PR. This'll address the hardcoded |
FYI, |
| vimPlugin = pkgs.vimUtils.buildVimPlugin { | ||
| pname = "VectorCode"; | ||
| version = pkgVersion; | ||
| src = self; | ||
| dependencies = [ | ||
| pkgs.vimPlugins.plenary-nvim | ||
| ]; | ||
| patches = pkgs.replaceVars ./nix/vim-plugin.patch { | ||
| inherit vectorcode; | ||
| }; |
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.
Wouldn't this make more sense as its own derivation in nixpkgs? There's already over 2000 vim plugins avalilable.
This should be an easy PR.
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.
Done!
|
Added the Neovim plugin to Nixpkgs for you: NixOS/nixpkgs#413395 It gets around the |
I'm an active maintainer, so I'll stay on top of this. |
|
@chrisrb I have nixpkgs PRs open to bring chromadb up to 0.6.3 (NixOS/nixpkgs#412528) and to add the neovim plugin (NixOS/nixpkgs#413395). Will those two changes meet your needs? |
|
@sarahec thanks a lot! this is an even better solution. for me this PR isn't necessary anymore now 👍 |
Excellent! Now all I have to do is get the chromadb update (to 0.6.3) merged at nixpkgs and you'll be all set. |
This PR introduces a Nix flake. To ensure Nix works correctly, it also updates the Neovim plugin to support specifying
cli_cmds, which are later patched by Nix to point directly to the Nix store. Both changes are included in this PR.