pluginupdate.py: add support for adding/updating individual plugins #336137

PerchunPak · 2024-08-20T19:10:35Z

Description of changes

This PRs adds long-awaited support for updating individual plugins to Vim and Kakoune (LuaEditor is a third class that uses pluginupdate.py, but they patch too much in pluginupdate, so I leave it to someone else).

As a bonus (though it was my main goal), I implemented adding an entry to generated.nix when adding a plugin using the script. I also implemented a whole bunch of bugfixes and I will document some things as comments here.

Feel free to correct grammar, I am not a native, so will gladly accept any grammar nitpicks.

Fixes #218584.

Things done

Things tested:

Vim
- Full update (result file)
- Individual update
- Adding package adds an entry to generated.nix
Kakoune - seems broken on master, so leaving it on maintainer (@philiptaron)
Lua
- Full update - ratelimits even with token and one worker, assuming works

CC @teto (author of #218696), @figsoda (maintainer of Vim), @philiptaron (maintainer of Kakoune)

Add a 👍 reaction to pull requests you find important.

PerchunPak · 2024-08-20T19:13:50Z

maintainers/scripts/pluginupdate.py

+        Returns:
+            True if we should update plugin and False if not.
+        """
+        return plugin.name.replace(".", "-") in to_update


Here you can see how optimistic I was for this PR (26 lines of docs to one line of code)

PerchunPak · 2024-08-20T19:16:19Z

maintainers/scripts/pluginupdate.py

@@ -559,6 +657,7 @@ def run(
        parser = self.create_parser()
        args = parser.parse_args()
        command = args.command or "update"
+        logging.basicConfig()


Without this line, any logging using log variable didn't work. Also, why default log level is warning? It is not annoying on info level, should I change it too?

Logging at info level is a good idea.

PerchunPak · 2024-08-20T19:17:20Z

maintainers/scripts/pluginupdate.py

-            log.debug("using plugin_line", plugin_line)
+            log.debug("using plugin_line %s", plugin_line)
            pdesc = PluginDesc.load_from_string(fetch_config, plugin_line)
-            log.debug("loaded as pdesc", pdesc)
+            log.debug("loaded as pdesc %s", pdesc)


It raised an error where complained at the lack of %s

PerchunPak · 2024-08-20T19:18:20Z

maintainers/scripts/pluginupdate.py

+        current: list[Tuple[PluginDesc, Plugin]],
+        fetched: List[Tuple[PluginDesc, Union[Exception, Plugin], Optional[Repo]]],
+    ) -> List[Tuple[PluginDesc, Union[Exception, Plugin], Optional[Repo]]]:
+        result: Dict[str, Tuple[PluginDesc, Union[Exception, Plugin], Optional[Repo]]] = dict(


Those types are copied from check_results func, and result of this function will be passed to it

PerchunPak · 2024-08-20T19:20:07Z

maintainers/scripts/pluginupdate.py

+            # remove plugin from index file, so we won't add it
+            # to deprecations again
+            for i, plugin in enumerate(plugins):
+                if plugin.name == pdesc.name:


As you could see at the description of third commit, someone just forgot about deleting plugins from index for years!

Time for some cleaning 🧹

PerchunPak · 2024-08-20T19:22:48Z

maintainers/scripts/pluginupdate.py

@@ -772,7 +875,7 @@ def rewrite_input(
        fieldnames = ["repo", "branch", "alias"]
        writer = csv.DictWriter(f, fieldnames, dialect="unix", quoting=csv.QUOTE_NONE)
        writer.writeheader()
-        for plugin in sorted(plugins):
+        for plugin in sorted(plugins, key=lambda x: x.name):


interesting fact: plugin's URI is not unique, but name is. It is because there can be multiple plugins in one repo but on different branches (e.g. different versions, see harpoon) and name is used as a key in the final result

pkgs/applications/editors/kakoune/plugins/update.py

PerchunPak · 2024-08-20T19:26:32Z

pkgs/applications/editors/vim/plugins/update.py

-sh = logging.StreamHandler()
-formatter = logging.Formatter("%(name)s:%(levelname)s: %(message)s")
-sh.setFormatter(formatter)
-log.addHandler(sh)


This is unnecessary with logging.basicConf in the main file

mweinelt · 2024-08-20T19:47:16Z

@ofborg eval

philiptaron

Some initial feedback. This looks great -- thanks for your work here @PerchunPak.

What remains:

I need to validate each URL update
You mentioned that I have some work to do for kakoune. Could you make it more clear for me what's broken and what I ought to pay attention to?
This deprecated.json thing is a total mess. What would cleanup look like?

Thank you!

philiptaron · 2024-08-20T19:35:15Z

pkgs/development/lua-modules/updater/updater.py

+        outfile: str,
+        config: FetchConfig,
+        # TODO: implement support for adding/updating individual plugins
+        to_update: Optional[List[str]],


Want to raise an error if this is non-None so that it's not frustrating that it's not working?

philiptaron · 2024-08-21T00:29:04Z

maintainers/scripts/pluginupdate.py

+            repo,
+            branch.strip(),
+            # alias is usually an empty string
+            # I wasted ~5 hours to find out this...


😢

Sometimes it's that way...

philiptaron · 2024-08-21T00:29:45Z

maintainers/scripts/pluginupdate.py


-    def get_current_plugins(self, nixpkgs) -> List[Plugin]:
+    def get_current_plugins(
+            self, config: FetchConfig, nixpkgs: str


Mind pushing a commit that does ruff format on this file? Getting a consistent style would be great.

philiptaron · 2024-08-21T00:33:24Z

maintainers/scripts/pluginupdate.py

+            version = version_split[1] if len(version_split) > 1 else version_split[0]
+            date = datetime.strptime(version, "%Y-%m-%d")
+
+            pdesc = PluginDesc.load_from_string(config, attr["homePage"] + " as " + name)


I usually do f strings instead of +, but it's only as preference.

Suggested change

pdesc = PluginDesc.load_from_string(config, attr["homePage"] + " as " + name)

pdesc = PluginDesc.load_from_string(config, f'{attr["homePage"]} as {name}')

philiptaron · 2024-08-21T00:34:25Z

maintainers/scripts/pluginupdate.py

-    def get_update(self, input_file: str, outfile: str, config: FetchConfig):
-        cache: Cache = Cache(self.get_current_plugins(self.nixpkgs), self.cache_file)
+    def filter_plugins_to_update(self, plugin: PluginDesc, to_update: List[str]) -> bool:
+        """Function for filtering out plugins, that user doesn't want to update.


Suggested change

"""Function for filtering out plugins, that user doesn't want to update.

"""filter_plugins_to_update exists to filter out plugins that user doesn't want to update.

I am pretty sure this breaks the Google style I used in this docstring. Though the closest rule I could find is https://docs.astral.sh/ruff/rules/no-signature/

philiptaron · 2024-08-21T00:45:48Z

maintainers/scripts/pluginupdate.py

+        filter_func = functools.partial(self.filter_plugins_to_update, to_update=to_update)
+        plugins_to_update = list(
+            filter(filter_func, current_plugin_specs)
+            if to_update
+            else current_plugin_specs
+        )


I'm big on list comprehensions instead of functools.partial or map/filter calls, mostly because they seem to my eyes to read better.

What do you think about this suggestion?

Suggested change

filter_func = functools.partial(self.filter_plugins_to_update, to_update=to_update)

plugins_to_update = list(

filter(filter_func, current_plugin_specs)

if to_update

else current_plugin_specs

)

plugins_to_update = current_plugin_specs if len(to_update) == 0 else [

description

for description in current_plugin_specs

if self.filter_plugins_to_update(description, to_update)

)

philiptaron · 2024-08-21T01:23:45Z

maintainers/scripts/pluginupdate.py

@@ -559,6 +657,7 @@ def run(
        parser = self.create_parser()
        args = parser.parse_args()
        command = args.command or "update"
+        logging.basicConfig()


Logging at info level is a good idea.

philiptaron · 2024-08-21T01:24:18Z

maintainers/scripts/pluginupdate.py

+            # remove plugin from index file, so we won't add it
+            # to deprecations again
+            for i, plugin in enumerate(plugins):
+                if plugin.name == pdesc.name:


Time for some cleaning 🧹

philiptaron · 2024-08-21T01:26:31Z

maintainers/scripts/pluginupdate.py

+        args.input_file,
+        args.outfile,
+        fetch_config,
+        getattr(args, "update_only", None),  # if script was called without arguments


I recommend using names for the parameters here.

Suggested change

args.input_file,

args.outfile,

fetch_config,

getattr(args, "update_only", None), # if script was called without arguments

input_file=args.input_file,

output_file=args.outfile,

config=fetch_config,

to_update=getattr(args, "update_only", None), # if script was called without arguments

philiptaron · 2024-08-21T01:27:15Z

pkgs/applications/editors/kakoune/plugins/update.py

@@ -21,30 +21,40 @@
 )
 import pluginupdate

-GET_PLUGINS = f"""(with import <localpkgs> {{}};
+GET_PLUGINS = f"""with import <localpkgs> {{ }};


This is the broken updater?

Yes, if you run

cd pkgs/applications/editors/kakoune/plugins nix-shell update-shell.nix python update.py

It outputs (on master and here is the same error)

error: … <borked> at «none»:0: (source not available) … from call site at «string»:1:6: 1| with import <localpkgs> { }; | ^ 2| let error: function 'anonymous lambda' called without required argument 'kakouneUtils' at /home/perchun/dev/nixpkgs/pkgs/applications/editors/kakoune/plugins/default.nix:1:1: 1| { callPackage, config, kakouneUtils, lib }: | ^ 2|

@philiptaron ^^^

Thanks! It's on the list to go fix. I'm camping this weekend but will attend to it when back.

PerchunPak · 2024-08-21T08:37:33Z

Pushed changes from review and formatting changes in different force pushes for simpler reviewing. I didn't push it as a new commit, as it would create a lot of conflicts if there will be other changes.

Thanks for the review!

PerchunPak · 2024-08-21T08:55:26Z

I need to validate each URL update

This should be easy with nixpkgs-review:

Result of nixpkgs-review pr 336137 run on x86_64-linux 1

3 packages built:

luarocks-packages-updater
nixpkgs-manual
vimPluginsUpdater

Which means no Vim plugins were changed/removed/added. I also quickly glanced over the diff on generated.nix after the full plugin update and didn't find any plugins that were moved/removed/added (before I created the PR there were a few bugs with sorting).

This deprecated.json thing is a total mess. What would cleanup look like?

What do you mean? The file deprecated.json or the code for it? If the code, it is in a good state compared to other parts of the code I've seen in this file.

teto · 2024-08-21T10:37:07Z

maintainers/scripts/pluginupdate.py

    :param expr nix expression to fetch current plugins
    :param nixpkgs Path towards a nixpkgs checkout
-    '''
+    """


aws this changed by the black formatter ?

I ran ruff format which is 99.9% compatible with black (just because it was installed on my system), so whatever?

teto · 2024-08-21T10:53:04Z

doc/languages-frameworks/vim.section.md

+If you want to update only certain plugins, you can specify their names to `--update` flag. Note that those name must be the same as in `pkgs/applications/editors/vim/plugins/vim-plugin-names` file.
+
+```sh
+nix-shell -p vimPluginsUpdater --run 'vim-plugins-updater --update "nvim-treesitter" "LazyVim"'


apparently it's not --update but update:

nix run .#vimPluginsUpdater -- update nvim-treesitter LazyVim WARNING:root:You have enabled parallel updates but haven't set a github token. You may be hit with `HTTP Error 429: too many requests` as a consequence.Either set --proc=1 or --github-token=YOUR_TOKEN. path is '/nix/store/rgwip825v0m4nhsvqyabf0mb2as81x0q-0b8b78f9d08dc338a146eb4cd4bcbed8dd36a783.tar.gz' 2 of 1471 were checked

Fixed here, thanks!

teto · 2024-08-21T12:36:34Z

thanks for the great work. It's a very exciting feature. I had thought of a treesitter-based solution but this seems to work as well. Not having the feature in all updaters is no problem, that's part of the design.

I am a bit annoyed that you have mixed different things in the PR (logging level, formatting, sorting, new feature). Most of it I want to merge but some of it (the update filtering) I would like to test & experiment with myself more. Would it be too much to ask for the sorting fix in a separate PR ?
For the logging part, the approach is to print user-facing messages and logging is a separate thing, which is why the level is set to WARN (since the logger is verbose, which is ok, it's targeted at developers first).
As for the formatting, it's supposed to be "black"-ified but maybe some parts were not done correctly.
If you could open separate PRs for the different changes, it would help with review work.

PerchunPak · 2024-08-22T09:29:56Z

I am a bit annoyed that you have mixed different things in the PR (logging level, formatting, sorting, new feature). ... Would it be too much to ask for the sorting fix in a separate PR ?

Unfortunately, most of those changes are QOL and usually they fix some weird bug, that prevents from properly implementing individual updates. For example, before sorting was completely broken (it sorted by PluginDesc.name, PluginDesc.name implementation was:

if self.alias is not None:
    return self.alias
else:
    return self.repo.name

BUT self.alias, if unset, was an empty string (because in CSV if something is unset, it is an empty string), instead of None, so it sorted by what request finished first, instead of proper name). This resulted in vim-updater-script add ... resorts half a file, if ran right after vim-updater-script update. Implementing proper sorting prevents merge conflicts. And I don't see much sense in individual updates, if those generate huge diffs that are hard to review.

For the logging part, the approach is to print user-facing messages and logging is a separate thing, which is why the level is set to WARN (since the logger is verbose, which is ok, it's targeted at developers first).

Ok, I will revert INFO by default.

PerchunPak · 2024-08-23T07:33:04Z

I am reviewing (and merging with the help of GaetanLepage) all vimPlugins.* PRs, so conflicts here are probably to stay until merge (they are not worth solving, if next merged PR will break them again + I will need to ensure sorting is correct right before merge)

I fixed a lot of hidden bugs and did some small improvements here and there. The main reason this was separated from NixOS#336137 is for fixing the sorting issue. Before this commit, sorting for Vim plugins was broken, and worked by what was fetched first. This is because sorting was done by empty string (default value in CSV is not None, but empty string). This PR fixes it and also moves sorting from user to the library (from `vim/plugins/update.py` to `pluginupdate.py`) to prevent such weird issues and duplication of code.

I fixed many hidden bugs and made some small improvements. The main reason this was separated from NixOS#336137 is to fix the sorting issue. Before this commit, sorting for Vim plugins was broken and worked by what was fetched first. This is because the sorting was done by empty strings (the default value in CSV is not None but an empty string). This PR fixes it and also moves sorting from the user to the library (from `vim/plugins/update.py` to `pluginupdate.py`) to prevent such weird issues and duplication of code.

PerchunPak · 2024-11-05T10:04:16Z

Blocked by #353786, will resolve conflicts after it is merged.

I fixed many hidden bugs and made some small improvements. The main reason this was separated from NixOS#336137 is to fix the sorting issue. Before this commit, sorting for Vim plugins was broken and worked by what was fetched first. This is because the sorting was done by empty strings (the default value in CSV is not None but an empty string). This PR fixes it and also moves sorting from the user to the library (from `vim/plugins/update.py` to `pluginupdate.py`) to prevent such weird issues and duplication of code.

doc/languages-frameworks/vim.section.md

GaetanLepage · 2024-11-10T13:50:33Z

maintainers/scripts/pluginupdate-py/pluginupdate.py

+        current: list[Tuple[PluginDesc, Plugin]],
+        fetched: List[Tuple[PluginDesc, Union[Exception, Plugin], Optional[Repo]]],


Suggested change

current: list[Tuple[PluginDesc, Plugin]],

fetched: List[Tuple[PluginDesc, Union[Exception, Plugin], Optional[Repo]]],

current: list[tuple[PluginDesc, Plugin]],

fetched: list[tuple[PluginDesc, Exception | Plugin, Repo | None]],

maintainers/scripts/pluginupdate-py/pluginupdate.py

GaetanLepage

The implementation looks good overall. Have you been able to successfully test it ?

PerchunPak · 2024-11-16T10:36:47Z

The implementation looks good overall. Have you been able to successfully test it ?

Yep, I ran it here

#355724 (comment)

teto · 2024-11-21T19:25:48Z

haven't tested but I feel confident that any potential problem will be solved swiftly :)

Introduced by #336137

PerchunPak requested review from figsoda and philiptaron as code owners August 20, 2024 19:10

github-actions bot added 8.has: documentation This PR adds or changes documentation 6.topic: vim 6.topic: lua labels Aug 20, 2024

PerchunPak force-pushed the updater-one-plugin branch from 9c05ff4 to 65d9fa3 Compare August 20, 2024 19:26

PerchunPak commented Aug 20, 2024

View reviewed changes

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 20, 2024

philiptaron reviewed Aug 21, 2024

View reviewed changes

PerchunPak force-pushed the updater-one-plugin branch 2 times, most recently from 03d3245 to 16419ba Compare August 21, 2024 08:36

teto reviewed Aug 21, 2024

View reviewed changes

PerchunPak force-pushed the updater-one-plugin branch from 16419ba to 0c32241 Compare August 21, 2024 17:00

PerchunPak force-pushed the updater-one-plugin branch 3 times, most recently from 9b7f93d to 644404a Compare August 22, 2024 12:07

PerchunPak mentioned this pull request Aug 23, 2024

vimPlugins.telescope-emoji-nvim: init at 2022-12-08 #313731

Closed

13 tasks

PerchunPak mentioned this pull request Sep 10, 2024

[RFC 0109] Nixpkgs Generated Code Policy NixOS/rfcs#109

Closed

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024

This was referenced Sep 11, 2024

vimPlugins: update on 2024-09-11 #341146

Merged

vimPlugins.gina-vim: rename plugin to fix alias #341309

Merged

PerchunPak force-pushed the updater-one-plugin branch from 644404a to 3a767d0 Compare September 12, 2024 14:18

PerchunPak mentioned this pull request Nov 5, 2024

pluginupdate.py: fix bugs and add improvements; vimPlugins: sort properly #353786

Merged

13 tasks

PerchunPak marked this pull request as draft November 5, 2024 10:04

pluginupdate.py: add support for adding/updating individual plugins

ecee72e

PerchunPak force-pushed the updater-one-plugin branch from 8b8cfef to ecee72e Compare November 10, 2024 00:43

PerchunPak marked this pull request as ready for review November 10, 2024 00:43

nix-owners bot requested review from GaetanLepage, khaneliman, manveru, mrcjkb and rvolosatovs November 10, 2024 00:44

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 10, 2024

GaetanLepage requested changes Nov 10, 2024

View reviewed changes

pluginupdate.py: use newer syntax for types

3c2cc30

MattSturgeon requested a review from GaetanLepage November 13, 2024 21:37

GaetanLepage approved these changes Nov 15, 2024

View reviewed changes

wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 15, 2024

GaetanLepage requested a review from teto November 16, 2024 15:48

PerchunPak mentioned this pull request Nov 21, 2024

vimPlugins.tsc-nvim: init at 2024-08-14 #357769

Merged

13 tasks

teto merged commit d6c5afd into NixOS:master Nov 21, 2024
26 checks passed

veehaitch mentioned this pull request Dec 13, 2024

luarocks-packages-updater: fix keyword argument name #364834

Merged

13 tasks

teto pushed a commit that referenced this pull request Dec 16, 2024

luarocks-packages-updater: fix keyword argument name (#364834)

2efaa5c

Introduced by #336137

	pdesc = PluginDesc.load_from_string(config, attr["homePage"] + " as " + name)
	pdesc = PluginDesc.load_from_string(config, f'{attr["homePage"]} as {name}')

	"""Function for filtering out plugins, that user doesn't want to update.
	"""filter_plugins_to_update exists to filter out plugins that user doesn't want to update.

		current: list[Tuple[PluginDesc, Plugin]],
		fetched: List[Tuple[PluginDesc, Union[Exception, Plugin], Optional[Repo]]],

pluginupdate.py: add support for adding/updating individual plugins #336137

pluginupdate.py: add support for adding/updating individual plugins #336137

Conversation

PerchunPak commented Aug 20, 2024 • edited Loading

Description of changes

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mweinelt commented Aug 20, 2024

philiptaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

😢

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PerchunPak commented Aug 21, 2024 • edited Loading

PerchunPak commented Aug 21, 2024

teto Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teto commented Aug 21, 2024

PerchunPak commented Aug 22, 2024

PerchunPak commented Aug 23, 2024

PerchunPak commented Nov 5, 2024

Choose a reason for hiding this comment

GaetanLepage left a comment

Choose a reason for hiding this comment

PerchunPak commented Nov 16, 2024

teto commented Nov 21, 2024

PerchunPak commented Aug 20, 2024 •

edited

Loading

PerchunPak commented Aug 21, 2024 •

edited

Loading

teto Aug 21, 2024 •

edited

Loading