Skip to content

neovimUtils.makeNeovimConfig: add pathDependencies#208029

Closed
figsoda wants to merge 5 commits intoNixOS:masterfrom
figsoda:path-deps
Closed

neovimUtils.makeNeovimConfig: add pathDependencies#208029
figsoda wants to merge 5 commits intoNixOS:masterfrom
figsoda:path-deps

Conversation

@figsoda
Copy link
Copy Markdown
Member

@figsoda figsoda commented Dec 27, 2022

Description of changes

I couldn't figure how to do this for vim, so we can only adopt this for neovim plugins for now

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: vim Advanced text editor label Dec 27, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Dec 27, 2022
@teto
Copy link
Copy Markdown
Member

teto commented Dec 28, 2022

I dont see the point of the MR (which could have used a better description). What problem does it solve ? introducing a pathDependencies seems redundant with what we have already aka buildInputs etc.

@figsoda
Copy link
Copy Markdown
Member Author

figsoda commented Dec 28, 2022

this adds things to PATH instead, buildInputs doesn't work for this purpose: #207759 (comment)

this is more similar to runtimeInputs for writeShellApplication, but I named it differently to not confuse it with vim runtimepath. open for suggestions for the name

@teto
Copy link
Copy Markdown
Member

teto commented Dec 28, 2022

this could become confusing very fast, better not rely on PATH too much. Furthermore the nvim-teal-maker seems to work here so why is the change needed ?
Another issue that could be solved by this MR approach and for which we dont have a solution at the moment is #172538

@figsoda
Copy link
Copy Markdown
Member Author

figsoda commented Dec 28, 2022

this could become confusing very fast

could you elaborate?

Furthermore the nvim-teal-maker seems to work here so why is the change needed?

This change is not needed, but I do think it is nice to not have to patch things as it is hard to maintain, and generated packages like vim plugins are usually not given the most thorough reviews and tests

Another issue that could be solved by this MR approach and for which we dont have a solution at the moment is #172538

I do like the idea of #172538, but I don't think it solves the exact problem this one solves. not all plugins provide options that allow you to specify path to binaries with configuration, and nvim-teal-maker is an example of that

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@itslychee
Copy link
Copy Markdown

Are there any updates to this PR, it would be an extremely useful addition!

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@Frontear
Copy link
Copy Markdown
Member

Frontear commented Nov 1, 2024

+1 to this PR, having a wrapper-esque PATH extension with derivations is very useful for neovim, particularly for consolidating dependencies inside the derivation and not requiring them more globally.

I think a great accompanying PR (not necessarily by author of this PR) to declutter and clarify wrapNeovimUnstable would be great. As it is, it's really obscure to use and honestly a little inflexible.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 1, 2024
substituteInPlace lua/tealmaker/init.lua \
--replace cyan ${luaPackages.cyan}/bin/cyan
'';
passthru.pathDependencies = [ luaPackages.cyan ];
Copy link
Copy Markdown
Member

@Frontear Frontear Nov 1, 2024

Choose a reason for hiding this comment

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

Can you elaborate on this change. How does it replicate the substituteInPlace from the previous diff?

Ah I see, this passes the derivation directly into makeWrapper, very cool, but a little bit indirect. I think some form of propagated deps may be better here.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 1, 2025
@teto
Copy link
Copy Markdown
Member

teto commented Jan 8, 2026

this was made achievable via plugins runtimeDeps in another PR #358587 (was not available when this PR was created). wrapNeovimUnstable aggregates plugin runtime dependencies only, if a user wants to add new ones, it should add it to wrapperArgs of wrapNeovimUnstable.

@Frontear wrapNeovimUnstable is now documented at https://nixos.org/manual/nixpkgs/unstable/#neovim-custom-configuration . There are still a few problems to solve such as lua transitive dependencies (soon), how to expose lua config. There are many usecases (wrapped vs unwrapped in home-manager) and lack of typing in nix makes exposing/evoling the API hard. We try to maintain backwards compatibility as well since neovim is crucial to many people. We are getting there though.

honestly a little inflexible.

What do you have in mind ?

@teto teto closed this Jan 8, 2026
@figsoda figsoda deleted the path-deps branch January 8, 2026 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: vim Advanced text editor 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants