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

Convert maintainer file entries to attributes, add github handles #36119

Merged
merged 5 commits into from
Mar 4, 2018

Conversation

Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Feb 28, 2018

Based on #34842, the
nix-instantiate output was pretty-printed and the validity of the github handles
manually verified, by automatically checking whether the user handles exist on
github (https://github.com/userhandle, status 200 or 404).
Each handle under 5 characters was manually checked (because the collision
probability with non-maintainer accounts is high), each missing entry was
manually researched.

The script used is kept in maintainers/scripts as an example of how to work
with the mainainers list through nix’ JSON interface.

Hydra might need to be patched.

cc @FRidh @vcunat

@FRidh
Copy link
Member

FRidh commented Feb 28, 2018

Thanks for the effort. Note the merge conflict though.

};
dizfer = {
email = "[email protected]";
githu
Copy link
Member

Choose a reason for hiding this comment

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

My github id is cillianderoiste, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@Profpatsch
Copy link
Member Author

Note the merge conflict though.

Yeah, I expect this to take a while to merge, so there might be multiple changes to maintainers nix by then.

@FRidh
Copy link
Member

FRidh commented Feb 28, 2018

Right. That's why I suggested merging #34842 first, then fixing Hydra, and when that is done convert to what you have here.

};
dizfer = {
email = "[email protected]";
githu
Copy link
Member

Choose a reason for hiding this comment

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

Should be @gavinrogers, see #35714

Copy link
Member Author

Choose a reason for hiding this comment

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

Something is broken with these markers, every one points to dizfer somehow?

Copy link
Member

Choose a reason for hiding this comment

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

gavingavinrogers

};
dizfer = {
email = "[email protected]";
githu
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

genesisbignaux

Copy link

Choose a reason for hiding this comment

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

ok

};
dizfer = {
email = "[email protected]";
githu
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@jtojnar jtojnar Feb 28, 2018

Choose a reason for hiding this comment

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

eelcoedolstra

@Profpatsch
Copy link
Member Author

It looks like cross-checking entries <5 chars is not enough, normal dictionary words will most likely also overlap.

@Profpatsch
Copy link
Member Author

That's why I suggested merging #34842 first

The problem with that commit is that lots of github handles are simply wrong, because the maintainer names don’t translate. So we are going to mention lots of non-nixos people.

@jtojnar
Copy link
Member

jtojnar commented Feb 28, 2018

How hard would it be to check if given handle contributed to the nixpkgs repo and (manually) fix those that did not?

Edit: There is https://github.com/NixOS/nixpkgs/commits?author=ttuegel

@7c6f434c
Copy link
Member

Well, may vanity counter does build a correspondence of names, emails and github names

};
dizfer = {
email = "[email protected]";
githu
Copy link
Member

Choose a reason for hiding this comment

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

That's actually me, 7c6f434c

Copy link
Member

Choose a reason for hiding this comment

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

raskin7c6f434c

Copy link
Member

Choose a reason for hiding this comment

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

GitHub is really weird today.

@Profpatsch
Copy link
Member Author

How hard would it be to check if given handle contributed to the nixpkgs repo and (manually) fix those that did not?

Probably not very, might be sensible to use the Github API for that and rewrite the script (in the commit) a bit.

@jtojnar
Copy link
Member

jtojnar commented Feb 28, 2018

curl "https://github.com/NixOS/nixpkgs/commits?author=${user}" | grep 'No commits found' should be good enough.

@Profpatsch
Copy link
Member Author

Checking for commits right now, this finds quite a few accounts that are not commiters. curling the github repo is pretty slow, since github has very hard limits on the amount of requests possible (and returns HTTP 429 in that case).

@Profpatsch
Copy link
Member Author

Okay, here we go. Now every handle that had no nixpkgs commits is corrected!

@Profpatsch Profpatsch force-pushed the maintainer-reformat branch from cbfe828 to 53df522 Compare March 3, 2018 15:50
@Profpatsch
Copy link
Member Author

Thanks to @aszlig’s input, I added a shortName property, which is used by hydra to send mails. @FRidh this should supersede #34842, minus the need for parsing.

@Profpatsch Profpatsch force-pushed the maintainer-reformat branch 2 times, most recently from 08f2720 to 53df522 Compare March 3, 2018 21:20
@Profpatsch
Copy link
Member Author

Removed the shortName again, @grahamc said we should fix hydra instead. According to @aszlig’s and my research, this will only lead to hydra not sending any mails for the time being.

@oxij
Copy link
Member

oxij commented Mar 3, 2018 via email

Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Maybe also run curl "https://github.com/NixOS/nixpkgs/pulls?utf8=%E2%9C%93&q=author%3A${user}" | grep 'No results matched your search' for users that commit without matching e-mail address.

@@ -1319,7 +1313,6 @@
};
fuzzy-id = {
email = "[email protected]";
github = "fuzzy-id";
Copy link
Member

Choose a reason for hiding this comment

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

@@ -1150,7 +1145,6 @@
};
epitrochoid = {
email = "[email protected]";
github = "epitrochoid";
Copy link
Member

Choose a reason for hiding this comment

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

@@ -1020,7 +1017,6 @@
};
drewkett = {
email = "[email protected]";
github = "drewkett";
Copy link
Member

Choose a reason for hiding this comment

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

Based on NixOS#34842, the
nix-instantiate output was pretty-printed and the validity of the github handles
manually verified, by automatically checking whether the user handles exist on
github (https://github.com/userhandle, status 200 or 404).
Each handle under 5 characters was manually checked (because the collision
probability with non-maintainer accounts is high), each missing entry was
manually researched.

The script used is kept in `maintainers/scripts` as an example of how to work
with the mainainers list through nix’ JSON interface.
Now also finds name-clashes for github handles who never contributed to nixpkgs
before. Also deals with too many request errors.
Corrected every handle that had no commits to nixpkgs, manually researched the
correct handles by looking at maintained packages & blames/history on Github.
Add new maintainers that were added before merging the new `maintainers.nix`
file format.
@Profpatsch Profpatsch force-pushed the maintainer-reformat branch from 53df522 to 050abd0 Compare March 4, 2018 02:09
@Profpatsch
Copy link
Member Author

@jtojnar I don’t understand, why is this a problem?

@oxij
Copy link
Member

oxij commented Mar 4, 2018

See #36275.

@Profpatsch
Copy link
Member Author

@GrahamcOfBorg eval

@jtojnar
Copy link
Member

jtojnar commented Mar 4, 2018

@Profpatsch the problem is, you are removing GitHub handles that have commits in nixpkgs (but GitHub cannot pair them).

@Profpatsch
Copy link
Member Author

I removed every handle where Github couldn’t resolve to a user page, yes, since those are either commits from non-Github users (via normal git commits) or Github accounts that don’t exist anymore.

@jtojnar
Copy link
Member

jtojnar commented Mar 4, 2018

At least the three I mentioned do resolve to a user page, are commits by GitHub users, and the accounts do still exist, only GitHub cannot pair them correctly (due to e-mail address mismatch). Checking the pull requests should fix these unpaired accounts.

Then only renamed accounts will remain. They need to be checked manually by checking the git log for expressions by the maintainer.

@Profpatsch
Copy link
Member Author

Ah, I see. Can you open a new issue (or maybe even pull request) for those cases? Thanks for the scrutiny!

oxij added a commit to oxij/nixpkgs that referenced this pull request Mar 6, 2018
oxij added a commit to oxij/nixpkgs that referenced this pull request Mar 6, 2018
Many commits unrelated to `lib` touch that file, this will make `git log ./lib` much saner.

This is what I meant in NixOS#36119 (comment).
infinisil added a commit to infinisil/nixpkgs that referenced this pull request Jul 11, 2018
Was inconveniently sorted since NixOS#36119
eadwu pushed a commit to eadwu/nixpkgs that referenced this pull request Jul 11, 2018
Was inconveniently sorted since NixOS#36119
@infinisil infinisil mentioned this pull request Mar 15, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants