-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
VimPlugins: update redirected repos #83119
VimPlugins: update redirected repos #83119
Conversation
Many of the plugins in vim-plugin-names are out of date and redirect to their new github repos. This commit adds a flag to automatically update these and defines a process for committing these updates into nixpkgs.
d0db312
to
dada36b
Compare
dada36b
to
6601c16
Compare
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.
diff LGTM
commits LGTM
going to give some time for others to commit, ping me in 2-3 days if nothing happends
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.
Great idea! I've left some feedback in the review. Also, did you check this with mypy
?
Instead of adding aliases I would kind of prefer to just throw
instead, but that would be a policy change and is definitely out of scope of this PR.
urllib.parse.urlsplit(response_url).path.strip("/").split("/")[:2] | ||
) | ||
end_line = f" as {self.alias}\n" if self.alias else "\n" | ||
plugin_line = "{owner}/{name}" + end_line |
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.
The "\n"
should probably be separate since it is always appended. For example:
alias_spec = f" as {self.alias}\n" if self.alias else ""
plugin_line = "{owner}/{name}" + alias_spec + "\n"
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.
In your example if self.alias
is truthy the resulting plugin_line
would result in an extra newline: "{owner}/{name}\n\n".
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.
Oh yes, I didn't mean to include the \n
in the alias_spec
. Also now that you mention it, I would rather not rely on things being "truthy" but instead check explicitly (is not None
).
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.
alias
is actually an empty string in this case, the truthiness of which is relied on in a couple other alias or
clauses in this script. Do you prefer if self.alias != ""
?
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.
Yes, I definitely prefer explicit comparisons. "Truthiness" seems to directly contradict the python ethos of explicitness. It reduces readability, especially for people not familiar with python ("truthiness" differs between languages).
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
end_line = f" as {self.alias}\n" if self.alias else "\n" | ||
plugin_line = "{owner}/{name}" + end_line | ||
|
||
old_plugin = plugin_line.format(owner=self.owner, name=self.name) |
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 seems very brittle to me, can't we use the original line instead? This breaks on any whitespace change.
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.
Or alternatively we could just rewrite the whole file and not bother replacing anything.
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'm not sure what you mean by a "whitespace change". The vim-plugin-names
file is rewritten in the same run of update.py
as the redirections are detected here so why would the file change?
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 mean that this would break as soon as we'd have a line
owner/repo as alias
(note the double space). The simplified solution would be to just remember the new lines (regardless of whether there was a redirect or not) and completely re-create the file instead of updating it. That would also simplify the sorting.
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 pushed a commit to normalize whitespace to deal with this particular issue. I recognize that this is still a bit flimsy but I would think it acceptable for a helper script, the results of which will be manually reviewed.
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 pushed a commit to normalize whitespace to deal with this particular issue.
Why didn't you go for the complete rewrite? Seems more robust, easier and you get a sorted result "for free".
I recognize that this is still a bit flimsy but I would think it acceptable for a helper script, the results of which will be manually reviewed.
I'd be careful with the "manual review" statement. These big automated PRs tend to be skimmed for malicious intent, but details tend to be skipped. At least that's how I do it. They are simply too big and too repetitive for detailed review.
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.
The file is rewritten every time now but I'm not sure if it's what you had in mind. It's not any more robust with respect to parsing the existing file.
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.
Its not quite what I had in mind. Now that we rewrite the file every time, there's not really a need to take the old file into consideration at all in rewrite_input
. We could just re-generate an plugin-names
file from scratch / from the data that was originally parsed. There would be no need to replace individual lines and worry about whitespace.
That's just a nitpick though, no reason to hold this off any longer.
pkgs/misc/vim-plugins/update.py
Outdated
2. If any of the plugin names were changed, add the old names as aliases in | ||
aliases.nix | ||
3. Make sure the updated {input_file} is still correctly sorted: | ||
sort -udf ./vim-plugin-names > sorted && mv sorted vim-plugin-names |
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 step would also be unnecessary if we'd just completely rewrite the file.
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 agree it would be good if update.py
automatically sorted vim-plugin-names
, though that might be out of scope for this 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.
Wouldn't that actually reduce complexity?
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 agree that it would be nice, but I think it's out of scope of the PR, and can be done later.
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.
Now that the --update-rewrite
flag is gone and we're modifying the input file without user input we might as well sort it while we're at it. As @timokau points out it is basically free.
sort -udf ./vim-plugin-names > sorted && mv sorted vim-plugin-names | ||
4. Run this script again so these changes will be reflected in the | ||
generated expressions (no need to use the --update-redirects flag again): | ||
./update.py |
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.
It would be nicer to just generate the nice expressions in the first place. I suspect this step would be easy to miss and even easier to miss in review.
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.
It would be nice if multiple invocations were not necessary but I think this script would then need to automatically commit the update before updating the redirects. It seems like if this was all done at once in one commit it would be harder hard to review than if the updates were separated from the redirect updates as I suggest.
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.
Yes, committing both changes at the same time is not ideal. But I think it is better than accidentally having the two files go out of sync, which I think is likely with the current model.
pkgs/misc/vim-plugins/update.py
Outdated
@@ -407,6 +462,12 @@ def parse_args(): | |||
default=DEFAULT_OUT, | |||
help="Filename to save generated nix code", | |||
) | |||
parser.add_argument( |
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.
If we suggest to do this anyway, why not default to true
?
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 don't know exactly what the use case for the --input-file
argument of this script is so I was cautious. It seems designed so one could maintain their own plugin list and it would be unexpected for this script to magically rewrite their file.
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 agree. We don't need an option here. Just make it the default. If someone complains about the new behavior with a good reason, we can still add it back.
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.
Your point is valid, but this is a nixpkgs-internal tool and I don't think we need to prioritize backwards compatibility. If someone else is using this outside of nixpkgs, its their responsibility to keep up with upstream changes. If we wanted to advertise this outside of nixpkgs we'd make it into its own package (with semver).
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.
The flag is removed.
pkgs/misc/vim-plugins/update.py
Outdated
git add {output_file} | ||
git commit -m "vimPlugins: Update" | ||
2. If any of the plugin names were changed, add the old names as aliases in | ||
aliases.nix |
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.
It would be amazing if we could automate this as well, though that's not a requirement for merge in my view (but a very nice to have).
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'd rather invest effort into eliminating aliases.nix if possible.
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 would propose to keep that file (or an equivalent file) but just add entries in the form of
old_name = throw "${old_name} was renamed to ${new_name}. Please use that instead" # added on yyyy-dd-mm
Since just doing the rename without any notification would be confusing to end users. We could then drop the throw lines once they're "old enough" (maybe 2 releases or something, see NixOS/rfcs#33).
So either way we'd still need a file that keeps track of name changes.
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 added a commit to throw errors like this rather than silently aliasing.
I did indeed check with mypy.
Definitely! The aliases largely defeat my motive to fix the redirections in the first place. One of the most troubling aspects of the vim plugin ecosystem is that there's very little attention when the owner/org changes. |
Feel free to do create a follow-up PR/issue for that once this is done. Though owner/org changes would still remain unnoticed since the names are not namespaced (something which I also kind of wish wasn't the case). |
In response to @timokau's review here are a couple changes: - Decrease the fragility of the replacement code by normalizing whitespace on each line. - Throw an error when plugins are renamed rather than silently aliasing to the new name.
Per review, the input file will now be rewritten automatically if redirects are found.
I believe the last remaining issue is simplifying the update process i proscribe, how to automate it, and whether this is in scope or better done in another PR. Here's what is printed currently: print(
f"""\
Redirects have been detected and {input_file} has been updated. Please take the
following steps:
1. Go ahead and commit just the updated expressions as you intended to do:
git add {output_file}
git commit -m "vimPlugins: Update"
2. If any of the plugin names were changed, throw an error in aliases.nix:
<oldName> = deprecateName "<oldName>" "<newName>"; # backwards compat, added YYYY-MM-DD
3. Run this script again so these changes will be reflected in the
generated expressions:
./update.py
4. Commit {input_file} along with aliases and generated expressions:
git add {output_file} {input_file} aliases.nix
git commit -m "vimPlugins: Update redirects"
"""
) Thoughts:
|
That sounds like an excellent idea. Instead of a nix file, it would be
I think that would be fine. I implemented that in the spotify updater
That would be slightly less ideal, but also okay in my opinion. One Another (probably more complex) possibility would be to keep printing |
@timokau Can we hold off on deprecation automation and git automation for a separate PR? If so, can you approve this PR? |
Personal feelings
If we had it as part of some CI, then I think it would be important to create a git commit, however, I think it's less surprising to an individual if the changes are put into unstaged instead of a commit. If we did want a git commit, then I think it should be opted-in, such as
We could do this as another PR
Agreed I think it's a big improvement to have the updated repos, I'm fine with merging just those changes, and tackling the automation/git history in another PR |
The advantages of git integration are (1) separate commits for updates and redirections and (2) automating standardized commit messages. I don't see the point without committing. |
I think I was coming from the perspective of these would be opt-in options (E.g `update.py --update-redirects) and not trying to both an update and redirect update at the same time. But yes, if the plugins and redirects were being updated, it would make sense to differentiate them in commits. If it is the case that the git commits happen as part of the script, then please also update docs at https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/vim.section.md#adding-new-plugins-to-nixpkgs to reflect the new git behavior |
f47bae5
to
1cb7fb9
Compare
It's working fine as long as I don't use any of the deprecated names, but if I add one of the deprecated names to my plugins I get:
I must have made a dumb mistake in |
This actually works perfectly for me with
Which plugin where you trying to add? The error occurs because the plugin mechanism expects vim plugins to have an |
It would be nice to store the date of the deprecation. That way, when and if we decide on a strategy, we can prune them after some time. This should work: diff --git a/pkgs/misc/vim-plugins/aliases.nix b/pkgs/misc/vim-plugins/aliases.nix
index ac6ffc4c14d..194433fed7f 100644
--- a/pkgs/misc/vim-plugins/aliases.nix
+++ b/pkgs/misc/vim-plugins/aliases.nix
@@ -31,8 +31,8 @@ let
(checkInPkgs n alias)))
aliases;
- deprecations = lib.mapAttrs (old: new: {
- old = throw "${old} was renamed to ${new}. Please update to ${new}.";
+ deprecations = lib.mapAttrs (old: info: {
+ old = throw "${old} was renamed to ${info.new} on ${info.date}. Please update to ${info.new}.";
}) (builtins.fromJSON (builtins.readFile ./deprecated.json));
in
diff --git a/pkgs/misc/vim-plugins/deprecated.json b/pkgs/misc/vim-plugins/deprecated.json
index bb4ad18c3ec..197d6806fd7 100644
--- a/pkgs/misc/vim-plugins/deprecated.json
+++ b/pkgs/misc/vim-plugins/deprecated.json
@@ -1,6 +1,18 @@
{
- "gist-vim": "vim-gist",
- "vim-jade": "vim-pug",
- "vundle": "Vundle.vim",
- "youcompleteme": "YouCompleteMe"
-}
\ No newline at end of file
+ "gist-vim": {
+ "new": "vim-gist",
+ "date": "2020-03-26"
+ },
+ "vim-jade": {
+ "new": "vim-pug",
+ "date": "2020-03-26"
+ },
+ "vundle": {
+ "new": "Vundle.vim",
+ "date": "2020-03-26"
+ },
+ "youcompleteme": {
+ "new": "YouCompleteMe",
+ "date": "2020-03-26"
+ }
+}
diff --git a/pkgs/misc/vim-plugins/update.py b/pkgs/misc/vim-plugins/update.py
index d513669d0f3..bbeef0889f4 100755
--- a/pkgs/misc/vim-plugins/update.py
+++ b/pkgs/misc/vim-plugins/update.py
@@ -420,13 +420,17 @@ def rewrite_input(input_file: Path, output_file: Path, redirects: dict):
if redirects:
lines = [redirects.get(line, line) for line in lines]
+ cur_date_iso = datetime.now().strftime("%Y-%m-%d")
with open(DEPRECATED, "r") as f:
deprecations = json.load(f)
for old, new in redirects.items():
old_name = old.split("/")[1].split(" ")[0].strip("\n")
new_name = new.split("/")[1].split(" ")[0].strip("\n")
if old_name != new_name:
- deprecations[old_name] = new_name
+ deprecations[old_name] = {
+ "new": new_name,
+ "date": cur_date_iso,
+ }
with open(DEPRECATED, "w") as f:
json.dump(deprecations, f, indent=4, sort_keys=True)
|
On my latest commit 1cb7fb91166aeb657cbc6a54e8f9473f30738d14 I get the rtp error for gist-vim though the previous commit b886ad9 does yield the correct deprecation error. I agree on your analysis -- I don't see how that change is causing it to be parsed as a plugin when it should be a literal I can't reproduce your example: $ nix-build -E 'with import ./default.nix {}; neovim.override { configure = { packages.myVimPackage = with vimPlugins; { start = [ gist-vim ]; }; }; }'
error: anonymous function at /home/ryne/projects/nixpkgs/pkgs/misc/vim-plugins/default.nix:2:1 called without required argument 'callPackage', at (string):1:6 I'd considered your approach of adding the date but couldn't think of any reason to bother pruning deprecations unless/until conflicts with new packages using the deprecated names arise. However I don't object and appreciate the additional structure imposed on |
I don't have any issue at 1cb7fb9. You need to execute that command from the root of your nixpkgs clone (or adapt the
|
Thanks. I can reproduce and indeed have no problem with either commit using vim packages as you suggest. However, the latest commit does break the vim-plug implementation: $ git checkout b886ad9 &>/dev/null && nix-build -E 'with import ./default.nix {}; neovim.override { configure = { plug.plugins = with vimPlugins; [ gist-vim ]; }; }'
error: vim-gist was renamed to gist-vim. Please update to gist-vim.
(use '--show-trace' to show detailed location information)
$ git checkout 1cb7fb9 &>/dev/null && nix-build -E 'with import ./default.nix {}; neovim.override { configure = { plug.plugins = with vimPlugins; [ gist-vim ]; }; }'
error: attribute 'rtp' missing, at /home/ryne/projects/nixpkgs/pkgs/misc/vim-plugins/vim-utils.nix:222:59
(use '--show-trace' to show detailed location information) |
Ah, I've found the issue(s).
The current implementation results in an attribute set like this
I guess the vim packages implementation evaluates all attributes of the plugins for some reason, which is why the So in summary, to fix it you need to replace deprecations = lib.mapAttrs (
old: new: throw "${old} was renamed to ${new}. Please update to ${new}."
)(
builtins.fromJSON (builtins.readFile ./deprecated.json)
) |
Out of curiosity I also figured out why it works with vim packages. The implementation uses nixpkgs/pkgs/misc/vim-plugins/vim-utils.nix Line 318 in 4a3f9ac
To remove duplicates from the list, pairwise comparison is necessary. For comparison you need to evaluate the whole thing, which leads to evaluation of the throw expression.
|
When an updated redirect results in a package name change, not just an owner change, throw an error to warn users the package names has changed.
96ef669
to
fc78a89
Compare
fc78a89
to
25fea45
Compare
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.
diff LGTM
let's users know what the new attr is
[1 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/83119
17 package built:
vimPlugins.Vundle-vim vimPlugins.YouCompleteMe vimPlugins.denite-nvim vimPlugins.deoplete-fish vimPlugins.deoplete-khard vimPlugins.deoplete-nvim vimPlugins.neoformat vimPlugins.nvim-lsp vimPlugins.vim-codefmt vimPlugins.vim-fugitive vimPlugins.vim-gist vimPlugins.vim-go vimPlugins.vim-lsc vimPlugins.vim-pug vimPlugins.vim-test vimPlugins.vimtex vimPlugins.yats-vim
I'll let @timokau click merge as he has more context on this 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.
Looks good to me! Thanks for sticking with this so long and putting up with all my requests. This is a bit improvement, much appreciated 👍
Thanks for all the great review @timokau!
Is there a roadmap for this? (Perhaps open a separate issue to track roadmap?) I'm wondering if we could use nixpkgs-update even though most of the packages probably aren't tracked by Repology. I would think it would be sufficient to just update the whole package set each time one of the tracked packages (for example, YouCompleteMe) is updated. |
seeing as this is just a script that needs to be executed, we could just make a simple bot that runs the script on some interval |
No roadmap, since nobody has been motivated enough to implement this yet I think. There are related issues though: nix-community/nixpkgs-update#117, nix-community/nixpkgs-update#12. There are some exciting developments recently in nixpkgs-update: https://discourse.nixos.org/t/nixpkgs-update-automation-and-new-sources/5645
It would be great to reuse the already trusted r-ryantm hardware, but that would also be a good alternative. |
nixpkgs-update IIRC only scrapes repology for version information, this script just directly scrapes from the repo's. So I don't think they have alignment on how they determine the need for a PR. @ryantm @bhipple correct me if I'm wrong. The question being: can nixpkgs-update also do vimplugins (I'm assuming it can, but it would be gathering the metadata off repology rather than github) |
I don't have an ideological opposition to nixpkgs-update doing things like running a script and making a PR that way. Though, it would be a very different branch of the code. |
If someone made a regular script that used hub (to make the PR) and added it as a systemd service here https://github.com/nix-community/infra/blob/master/build01/nixpkgs-update.nix We could consider including it under things that @r-ryantm does, but are not specifically using the nixpkgs-update Haskell code. |
I would be very okay with this, and in the PR description, just make a mention that it was generated using the script |
Motivation for this change
Many of the plugins in vim-plugin-names are out of date and redirect to their new github repos. This adds a flag to automatically update these, defines a process for committing those updates into nixpkgs, and performs the update.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)