Skip to content
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: Add description attribute from GitHub metadata. #90257

Closed
wants to merge 2 commits into from

Conversation

progval
Copy link
Member

@progval progval commented Jun 13, 2020

Motivation for this change

Adding the missing metadata attribute (for searches, etc.)

Things done

Changed the update.py to output it, and recreated generated.nix.

There are no changes to the derivations.

@jonringer
Copy link
Contributor

@progval thanks for opening your first nixpkgs PR :)

@jonringer
Copy link
Contributor

not entirely sure why, but the description line is missing a space:

meta.description ="Vim configuration for Zig";

should be

meta.description = "Vim configuration for Zig";

@progval
Copy link
Member Author

progval commented Jun 13, 2020

oh yeah I committed an older version of the file. It's fixed now

@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-linux: 1 10.rebuild-linux: 1-10 labels Jun 13, 2020
@jonringer jonringer requested review from timokau and evanjs June 13, 2020 22:45
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff LGTM
1 package is getting updated, but I think that's acceptable.

just want to hear from some others

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Great idea, this would be nice to have. I do have concerns about the rate limit though.

# https://github.com/settings/tokens
# and paste it, to get a rate-limit high enough for the script to complete
# in reasonable time:
OAUTH_TOKEN = None
Copy link
Member

Choose a reason for hiding this comment

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

The script previously explicitly avoided the GitHub API because of the rate limiting. Requiring every contributor to generate & set an oauth token is a significant change. I'm not entirely sure its worth the benefit.

Copy link
Member

Choose a reason for hiding this comment

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

Also if we do decide to require an OAUTH token, it should be passed in through the environment instead of hard-coded into the script (os.environ.get("GH_TOKEN") with some appropriate error message when it is not specified).

Copy link
Member

Choose a reason for hiding this comment

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

The script previously explicitly avoided the GitHub API because of the rate limiting. Requiring every contributor to generate & set an oauth token is a significant change. I'm not entirely sure its worth the benefit.

You needed to provide one already anyway or otherwise you would get most likely rate limited.

pkgs/misc/vim-plugins/update.py Outdated Show resolved Hide resolved
@progval
Copy link
Member Author

progval commented Jun 14, 2020

Yeah, that's a concern.
But it shouldn't be too much of an issue unless generated.nix is recreated from scratch.

Btw, when generating it from scratch, I also got rate-limited on commits.atom, and needed to change the user-agent to get around it

@timokau
Copy link
Member

timokau commented Jun 14, 2020

But it shouldn't be too much of an issue unless generated.nix is recreated from scratch.

That happens a lot though. Every time we update the vim plugins, which often happens when someone adds a new plugin. Requiring an oauth token increases the friction, meaning it will happen less often.

I also got rate-limited on commits.atom

I added the backoff-retry mechanism at some point to work around that. Doesn't that work anymore?

The advantage is that the rate limit is much softer than the one for the API. A backoff of ~10 seconds may already help. The API has a strict hourly rate limit (60 unauthenticated requests / hour), so just waiting it out is not really an option.

I think the best way forward would be to teach nixpkgs-update about vim plugins (#83119 (comment)). Then individual contributors would only be concerned with new additions, which should fit into the 60 request limits.

As long as vim plugin updates are manual, I'd prefer to keep the friction as low as possible so that people will keep doing it.

@progval
Copy link
Member Author

progval commented Jun 14, 2020

I added the backoff-retry mechanism at some point to work around that. Doesn't that work anymore?

not with the default value of --proc (and probably only when git repositories are already cached locally), I had to lower it and try a couple of times.
Or maybe it's because I already did a bunch of requests before doing a full run, or GitHub doesn't like my IP range, who knows

As long as vim plugin updates are manual, I'd prefer to keep the friction as low as possible so that people will keep doing it.

Understandable. Should I close this PR?

@timokau
Copy link
Member

timokau commented Jun 14, 2020

I added the backoff-retry mechanism at some point to work around that. Doesn't that work anymore?

not with the default value of --proc (and probably only when git repositories are already cached locally), I had to lower it and try a couple of times.
Or maybe it's because I already did a bunch of requests before doing a full run, or GitHub doesn't like my IP range, who knows

That's a shame. The way we currently do things definitely isn't idea. Once we have nixpkgs-update do the package set updates, we could also start using the proper API for everything else the script does.

As long as vim plugin updates are manual, I'd prefer to keep the friction as low as possible so that people will keep doing it.

Understandable. Should I close this PR?

I have my concerns, but I don't want to block this PR alone. I'm curious what @jonringer, @teto or @evanjs think.

Another approach would be to conditionally only fetch descriptions if the oauth token is set. That would mean that the descriptions would sometimes be available, sometimes not (depending on who last made the update). Might be confusing.

If you do want to invest some time into improving the vim plugin infrastructure on nix, consider working on the nixpkgs-update integration. @ryantm has previously said that he would be open to contributions there.

@progval
Copy link
Member Author

progval commented Jun 14, 2020

I don't really have the time to work on that, sorry :/

@Mic92
Copy link
Member

Mic92 commented Jun 14, 2020

I added the backoff-retry mechanism at some point to work around that. Doesn't that work anymore?

not with the default value of --proc (and probably only when git repositories are already cached locally), I had to lower it and try a couple of times.
Or maybe it's because I already did a bunch of requests before doing a full run, or GitHub doesn't like my IP range, who knows

That's a shame. The way we currently do things definitely isn't idea. Once we have nixpkgs-update do the package set updates, we could also start using the proper API for everything else the script does.

As long as vim plugin updates are manual, I'd prefer to keep the friction as low as possible so that people will keep doing it.

Understandable. Should I close this PR?

I have my concerns, but I don't want to block this PR alone. I'm curious what @jonringer, @teto or @evanjs think.

Another approach would be to conditionally only fetch descriptions if the oauth token is set. That would mean that the descriptions would sometimes be available, sometimes not (depending on who last made the update). Might be confusing.

If you do want to invest some time into improving the vim plugin infrastructure on nix, consider working on the nixpkgs-update integration. @ryantm has previously said that he would be open to contributions there.

I think we actually need to switch to GITHUB_TOKEN because github also sets tight rate limits when cloning repositories or fetching rss feeds, which breaks parallel downloads. If we get descriptions that's another plus making having to generate a token.

@timokau
Copy link
Member

timokau commented Jun 14, 2020

We probably need to switch at some point, but I think currently at least without any parallelism the update still works right?

Being able to use the oauth token would be much nicer, but I do think it would decrease the amount of people willing to do an update.

@Mic92
Copy link
Member

Mic92 commented Jun 15, 2020

We probably need to switch at some point, but I think currently at least without any parallelism the update still works right?

Being able to use the oauth token would be much nicer, but I do think it would decrease the amount of people willing to do an update.

It might be bearable if it was easy to setup i.e. having to run a one liner. After all everyone has a github account anyway when contributing to nixpkgs.

@timokau
Copy link
Member

timokau commented Jun 15, 2020

Yeah my concern is less that people cannot do it, its just that it adds just enough friction that they won't. Now you either have to store your oauth token somewhere (and do that securely) or regenerate it every time.

Still I'm not completely opposed to this, since the status quo is far from perfect too. I don't know. If you want to move this forward, go ahead :)

@ryantm ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020
@stale
Copy link

stale bot commented Jun 7, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 7, 2021
@teto
Copy link
Member

teto commented Jun 7, 2021

an interesting thing to have. Maybe without token, it could retreive the existing nix description or leave the field empty.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 7, 2021
@stale
Copy link

stale bot commented Apr 19, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 19, 2022
@progval progval closed this Oct 1, 2023
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 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants