vimPluginsUpdater: rewrite to use nupd #380691
Open
+40,341
−23,181
Conversation
This file contains hidden or 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
|
Now update of all plugins is faster than my git status! Also, please don't merge any new plugins until this is merged |
51984bc to
1287ee2
Compare
1287ee2 to
3dffe72
Compare
|
Weird, eval works for me |
khaneliman
reviewed
Feb 9, 2025
|
|
||
| ``` | ||
| nix-shell -p vimPluginsUpdater --run vim-plugins-updater -i vim-plugin-names -o generated.nix --no-commit | ||
| nix-shell -p vimPluginsUpdater --run 'vim-plugins-updater update "https://github.com/folke/lazy.nvim"' |
There was a problem hiding this comment.
Does this no longer support by plugin name? Seems more appropriate than having to enter an entire url everytime
ie:
vim-plugins-updater update lazy-nvim
There was a problem hiding this comment.
It uses the same system as add command. I plan to remove https://github.com/ that is required for every plugin in the near future
There was a problem hiding this comment.
Hmm... alright :S Not a fun regression, but we rarely do individual updates anyways, I suppose.
Do you plan to leave a single commit for this ? |
khaneliman
reviewed
Feb 9, 2025
khaneliman
reviewed
Feb 9, 2025
This comment was marked as outdated.
This comment was marked as outdated.
fe7934a to
f7d2070
Compare
13 tasks
acb7dd4 to
8b90296
Compare
8b90296 to
4e020cb
Compare
FWIW this is no longer a blocker since we don't have |
4e020cb to
c456a70
Compare
PerchunPak
commented
Aug 6, 2025
There was a problem hiding this comment.
Thanks everyone for their feedback! I was improving nupd for the last half a year and finally put a better version.
Now there is documentation and I wrote a detailed PR description
I will resolve conflicts once this will get multiple approves and will be ready for merging, as those conflicts are trivial to solve using script, but they happen too often.
|
|
||
| @t.override | ||
| def minify(self) -> VimMiniEntry: | ||
| assert self.fetched.commit is not None, "This plugin was not prefetched!" |
There was a problem hiding this comment.
No, it is displayed in the traceback
Comment on lines
133
to
150
| if self.nvim_treesitter_updated: | ||
| print("updating nvim-treesitter grammars") | ||
| cmd = [ | ||
| "nix", | ||
| "build", | ||
| "vimPlugins.nvim-treesitter.src", | ||
| "-f", | ||
| self.nixpkgs, | ||
| "--print-out-paths", | ||
| ] | ||
| log.debug("Running command: %s", " ".join(cmd)) | ||
| nvim_treesitter_dir = subprocess.check_output( | ||
| cmd, text=True, timeout=90 | ||
| ).strip() | ||
|
|
||
| generated = treesitter.update_grammars(nvim_treesitter_dir) | ||
| treesitter_generated_nix_path = os.path.join( | ||
| NIXPKGS_NVIMTREESITTER_FOLDER, "generated.nix" |
There was a problem hiding this comment.
Does this block this PR? If so maybe an interim solution is needed and/or the follow up PR should be worked on before this is merged?
The tree-sitter updater can be run independently, so it is not blocking. Actually, since recently, we no longer run the nvim-treesitter updater automatically with the Vim plugins updater, because they use different branches of nvim-treesitter
Or is the issue that the new
generated.jsonfile itself doesn't know how to represent treesitter grammars? They could always be moved to their own json file if that's simpler.
The nvim-treesitter updater already stores grammars in a separate lockfile
|
|
||
| treesitter = importlib.import_module("nvim-treesitter.update") | ||
| # Always assunme CWD is the root of nixpkgs, later will figure out how to use the CLI flag | ||
| ROOT = Path.cwd() / "pkgs/applications/editors/vim/plugins" |
There was a problem hiding this comment.
I fixed it by adding support for --nixpkgs-path flag, now the user can specify nixpkgs dir using the CLI
| } | ||
| writers.writePython3Bin "vim-plugins-updater" { | ||
| libraries = [ python3Packages.nixpkgs-updaters-library ]; | ||
| doCheck = false; |
There was a problem hiding this comment.
It would be, but it is not really necessary when we can just rerun the script, and it will trigger every single line of code
The underlying nupd library has extensive testing
|
c456a70 to
4c5a067
Compare
MattSturgeon
reviewed
Aug 6, 2025
pkgs/applications/editors/vim/plugins/utils/parse-generated.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/editors/vim/plugins/utils/parse-generated.nix
Outdated
Show resolved
Hide resolved
pkgs/development/python-modules/nixpkgs-updaters-library/default.nix
Outdated
Show resolved
Hide resolved
khaneliman
reviewed
Aug 6, 2025
pkgs/development/python-modules/nixpkgs-updaters-library/default.nix
Outdated
Show resolved
Hide resolved
pkgs/development/python-modules/nixpkgs-updaters-library/default.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/editors/vim/plugins/utils/parse-generated.nix
Outdated
Show resolved
Hide resolved
Comment on lines
+7
to
+9
| # Note that this updater does not support anything but GitHub, because we want | ||
| # to support metadata fetching and it is problematic to do automatically for all | ||
| # platforms. If you want to package a non-GitHub plugin - place it to non-generated folder |
There was a problem hiding this comment.
I'd like for this not not be the case long-term.
There was a problem hiding this comment.
Is this a regression when compared with the current update script?
There was a problem hiding this comment.
Yeah, I believe @PerchunPak had a previous PR for pulling out the non github plugins into non-generated in anticipation for this update. #376366 It was only a handful of plugins so we just moved them out, for now. But, for a long-term solution... we should be flexible with the sources, eventually.
There was a problem hiding this comment.
It's frustrating that we can't lean on more generic package update tooling, such as https://github.com/NixOS/nixpkgs/tree/master/pkgs/common-updater and https://github.com/Mic92/nix-update
There was a problem hiding this comment.
Yeah.. I really wanted to shift towards nix-update... Artturin had started on something Mic92/nix-update#202 to previously help with updating packages not adhering to the standard derivation style.
There was a problem hiding this comment.
I agree that losing support for non-GitHub plugins is controversial, but it is a trade-off that is worth making. I added this information to "features we lose" in the PR description, but also duplicated it here
There is only 26 non-GitHub Vim plugins in nixpkgs, and adding support for every possible Git hosting platform is too big a maintenance burden. Remember that this rewrite also fetches all metadata for the plugin. It is much easier and better to package those plugins in
non-generatedand update it usingnix-update. It's either we lose metadata info for non-GitHub plugins or just package them manually
There was a problem hiding this comment.
This seems reasonable. However if that's the direction we want to go in, I'd like to see an effort towards:
- renaming
generated.jsonto indicate it only relates to GitHub packages - updating packaging documentation to cover adding/maintaining non-github packages
- adding an update-script wrapper that runs all vimPlugins update scripts, including this one
None of these feel like things that need to be done in this PR, especially not the wrapper script.
But this PR could pivot from "TODO no GitHub support for now" to "this script only handles GitHub, other sources should define per-package update scripts".
There was a problem hiding this comment.
updating packaging documentation to cover adding/maintaining non-github packages
adding an update-script wrapper that runs all vimPlugins update scripts, including this one
@r-ryantm handles those updates. Also, such a script would probably be avoided as there is a bunch of Rust plugins in non-generated that are costly to build.
But this PR could pivot from "TODO no GitHub support for now" to "this script only handles GitHub, other sources should define per-package update scripts".
Could you send a link where I have that TODO comment? I could not find it
4c5a067 to
3bb988d
Compare
|
Applied some straightforward review suggestions, thanks for the fast reviews! Will work on slicing this into separate PRs now |
This was referenced Aug 6, 2025
3bb988d to
c32ed73
Compare
|
See (Sorry, still haven't reviewed the python, yet... just cursory stuff since it's end of my work day and my eyes are getting tired.) "NotebookNavigator-nvim": {
"description": "A neovim plugin to navigate and execute code cells",
"homepage": null,
"info": {
"alias": null,
"branch": null,
"repo": {
"name": "NotebookNavigator.nvim",
"owner": "GCBallesteros"
}
},
"license": null,
"nurl_result": {
"args": {
"hash": "sha256-+FLHJ1tfIplXhO/6kajdcDBTIsNYN9kCOR9IdhXL6d4=",
"owner": "GCBallesteros",
"repo": "NotebookNavigator.nvim",
"rev": "20cb6f72939194e32eb3060578b445e5f2e7ae8b"
},
"fetcher": "fetchFromGitHub"
},
"version": "0-unstable-2024-05-23"
}, "NrrwRgn": {
"description": "A Narrow Region Plugin for vim (like Emacs Narrow Region)",
"homepage": "http://www.vim.org/scripts/script.php?script_id=3075",
"info": {
"alias": null,
"branch": null,
"repo": {
"name": "NrrwRgn",
"owner": "chrisbra"
}
},
"license": null,
"nurl_result": {
"args": {
"hash": "sha256-ULlstztsQxzDicj1OWexNWwwhcomaUJ4MagK5hb2nFU=",
"owner": "chrisbra",
"repo": "NrrwRgn",
"rev": "e027db9d94f94947153cd7b5ac9abd04371ab2b0"
},
"fetcher": "fetchFromGitHub"
},
"version": "0-unstable-2022-02-13"
}, |
That is a homepage that is explicitly set on GitHub, not sure if setting it to the repo URL by default makes sense
Np, I also just returned from work and got some time to work on this |
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.
This rewrites the vim plugins updater to use nixpkgs-updaters-library, which makes the code much better and more extensible.
Command to run:
nix-build -A vimPluginsUpdater; ./result/bin/vim-plugins-updater --helpOld implementation problems
The
pluginupdate.pyis used for sharing some updater logic between the Vim plugins updater, Luarocks updater, and Kakoune plugins updater. Because of bad design, it is extremely hard to add new features and there are a ton of bugs. As I can see from the code structure, it was designed as a simple 100-line script: load CSV table, fetch last commit and hash, write it togenerated.nix. But as time passed, the number of Vim plugins in nixpkgs got to thousands and the script got extremely slow.Thus, someone improved it, added parallelization, and some other features, probably. As a result, we now have Frankenstein. To avoid the same thing, the library is designed with extensibility in mind and learns from mistakes made in
pluginsupdate.py.Here is a list of main major issues with
pluginsupdate.py(some things are fixed in #336137, some are impossible to fix without a complete rewrite):pluginsupdate.pyonly implements parallelization of fetchers for Git & GitHub, you cannot properly modify some steps of execution without rewriting a lot of code in the core logic.PluginandPluginDesc: the second class contains info that we got from the input file, the first contains already fetched information. You cannot getPluginDescfromPluginand vice versa without making network calls or assuming some values are the same (because of redirects, they may be different).nix-prefetch-...are repeated if you want to resolve multiple different attributes. This makes adding features that require new data (e.g., adding a proper link to the homepage) painful, because we cannot reuse the same request we just did. We would have to rewrite the entire caching system to implement it in the old script. And the more such features we want to add, the more rate limits become a thing.None. So it tries to sort by an alias, which is usually an empty string (it is set only for a small number of plugins, where using the repository name would cause a conflict). This was a bug for years, and it was really difficult to debugSee also #336137.
Design decisions
The updater script has to implement only a few classes; the rest is handled by the library.
VimEntryInfo: information about a plugin that is read directly from CSV (includes GitHub URL, branch, and alias)VimEntry: already prefetched information (infoattribute links back toEntryInfoinstance of this plugin, hash, commit, etc)VimMiniEntry: minified information that is stored in the result file. If we don't minify anything, the output file is multiple times largerVimImpl: implements some general methods likeget_all_entriesDividing plugin information to
EntryInfoandEntrywas inspired by pluginupdate'sPluginDescandPlugin, the rest just logically followed. The classes are namedEntry*instead ofPlugin*because the library tries to be agnostic to what the user fetches. Entry can be a git repo or even a GNOME plugin.Feature comparison
We get:
--helpcommand and see what I am talking about)0-unstable-{date}We lose:
masterbranch (which we use) stopped updating grammars in-tree, all updates have moved to themainbranch. We no longer rely on that feature, and instead nvim-treesitter grammar updater works perfectly fine by itself.non-generatedand update it usingnix-update. It's either we lose metadata info for non-GitHub plugins or just package them manuallyThings done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)CC @NixOS/neovim
Add a 👍 reaction to pull requests you find important.