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.

Sign up for GitHub

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

Jump to bottom

Structured maintainers #31739

Closed
wants to merge 3 commits into from
Closed

Structured maintainers #31739

wants to merge 3 commits into from

Conversation

moretea
Copy link
Contributor

@moretea moretea commented Nov 16, 2017 •
edited

Motivation for this change

Enables having proper metadata about maintainers, which is useful for NixOS/rfcs#19 (maintainers file) / NixOS/rfcs#21 (PR's on Hydra).

Example
nix-instantiate --strict --eval . -A pkgs.keen4.meta.maintainers
[ { GitHub = "edolstra"; IRC = "niksnut"; email = "eelco.dolstra@logicblox.com"; name = "Eelco Dolstra"; } ]
Things done

Verified that not everything is completely broken by running:

nix-build nixos/tests/boot.nix 
nix-build nixos/tests/firefox.nix
Why the manipulation of the data structure at the bottom of the file?

If addGitHub would not be applied to the attrset, then keys of the attrset cannot be observed when accessing them via pkgs.my_pkg.meta.maintainers.

Remaining questions
  • Is setting IRC to null if not defined desired behavior?

edolstra
edolstra reviewed Nov 16, 2017
View reviewed changes
lib/attrsets.nix Outdated
@@ -1,4 +1,4 @@
{ lib }:
{ lib, ... }:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? ... considered harmful.

@edolstra
Copy link
Member

This seems overly complicated to me. Why do we need that all that funky _structuredMaintainers and ... business? I would just change lib/maintainers.nix to return a attrset of attrsets.

@moretea
Copy link
Contributor Author

moretea commented Nov 16, 2017

I would very much prefer to only have that!

My reasoning for implementing a flag is based on:

  1. This structured data is very useful for the RFCs 19&21 to ping the correct maintainers.
  2. I assumed that hydra depends on pkgs.my_pkg.meta.maintainers to be a list of email addresses.

Having a flag will allows us to get to use this data before support is added to Hydra.

@moretea
Copy link
Contributor Author

moretea commented Nov 16, 2017

We discussed this on IRC, and we'll just move to the new format, after which @edolstra will fix Hydra.

@orivej orivej mentioned this pull request Nov 26, 2017
Closed
8 tasks
@grahamc grahamc mentioned this pull request Feb 11, 2018
Closed
8 tasks
@FRidh
Copy link
Member

FRidh commented Feb 11, 2018

I would like to take this further (see #34842 (comment)). @moretea did you use a script to generate the attr sets?

@7c6f434c
Copy link
Member

I think the feature has been implemented somehow, so any PRs about maintainer list structure should be discussed relative to what we have now.

@7c6f434c 7c6f434c closed this Jun 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra edolstra left review comments

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@moretea @edolstra @FRidh @7c6f434c @GrahamcOfBorg