···
<title>Reviewing contributions</title>
10
-
<para>The following section is a draft and reviewing policy is still being
10
+
<para>The following section is a draft and reviewing policy is still being
14
-
<para>The nixpkgs projects receives a fairly high number of contributions via
15
-
GitHub pull-requests. Reviewing and approving these is an important task and a
14
+
<para>The nixpkgs projects receives a fairly high number of contributions via
15
+
GitHub pull-requests. Reviewing and approving these is an important task and a
way to contribute to the project.</para>
18
-
<para>The high change rate of nixpkgs make any pull request that is open for
19
-
long enough subject to conflicts that will require extra work from the
20
-
submitter or the merger. Reviewing pull requests in a timely manner and being
21
-
responsive to the comments is the key to avoid these. GitHub provides sort
22
-
filters that can be used to see the <link
23
-
xlink:href="https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc">most
24
-
recently</link> and the <link
25
-
xlink:href="https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-asc">least
18
+
<para>The high change rate of nixpkgs make any pull request that is open for
19
+
long enough subject to conflicts that will require extra work from the
20
+
submitter or the merger. Reviewing pull requests in a timely manner and being
21
+
responsive to the comments is the key to avoid these. GitHub provides sort
22
+
filters that can be used to see the <link
23
+
xlink:href="https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc">most
24
+
recently</link> and the <link
25
+
xlink:href="https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-asc">least
recently</link> updated pull-requests.</para>
28
-
<para>When reviewing a pull request, please always be nice and polite.
29
-
Controversial changes can lead to controversial opinions, but it is important
28
+
<para>When reviewing a pull request, please always be nice and polite.
29
+
Controversial changes can lead to controversial opinions, but it is important
to respect every community members and their work.</para>
32
-
<para>GitHub provides reactions, they are a simple and quick way to provide
33
-
feedback to pull-requests or any comments. The thumb-down reaction should be
34
-
used with care and if possible accompanied with some explanations so the
32
+
<para>GitHub provides reactions, they are a simple and quick way to provide
33
+
feedback to pull-requests or any comments. The thumb-down reaction should be
34
+
used with care and if possible accompanied with some explanations so the
submitter has directions to improve his contribution.</para>
37
-
<para>Pull-requests reviews should include a list of what has been reviewed in a
38
-
comment, so other reviewers and mergers can know the state of the
37
+
<para>Pull-requests reviews should include a list of what has been reviewed in a
38
+
comment, so other reviewers and mergers can know the state of the
41
-
<para>All the review template samples provided in this section are generic and
42
-
meant as examples. Their usage is optional and the reviewer is free to adapt
41
+
<para>All the review template samples provided in this section are generic and
42
+
meant as examples. Their usage is optional and the reviewer is free to adapt
them to his liking.</para>
<section><title>Package updates</title>
47
-
<para>A package update is the most trivial and common type of pull-request.
48
-
These pull-requests mainly consist in updating the version part of the package
47
+
<para>A package update is the most trivial and common type of pull-request.
48
+
These pull-requests mainly consist in updating the version part of the package
name and the source hash.</para>
50
-
<para>It can happen that non trivial updates include patches or more complex
50
+
<para>It can happen that non trivial updates include patches or more complex
<para>Reviewing process:</para>
56
-
<listitem><para>Add labels to the pull-request. (Requires commit
56
+
<listitem><para>Add labels to the pull-request. (Requires commit
59
-
<listitem><para><literal>8.has: package (update)</literal> and any topic
59
+
<listitem><para><literal>8.has: package (update)</literal> and any topic
label that fit the updated package.</para></listitem>
63
-
<listitem><para>Ensure that the package versioning is fitting the
63
+
<listitem><para>Ensure that the package versioning is fitting the
guidelines.</para></listitem>
65
-
<listitem><para>Ensure that the commit text is fitting the
65
+
<listitem><para>Ensure that the commit text is fitting the
guidelines.</para></listitem>
<listitem><para>Ensure that the package maintainers are notified.</para>
69
-
<listitem><para>mention-bot usually notify GitHub users based on the
70
-
submitted changes, but it can happen that it misses some of the
69
+
<listitem><para>mention-bot usually notify GitHub users based on the
70
+
submitted changes, but it can happen that it misses some of the
package maintainers.</para></listitem>
74
-
<listitem><para>Ensure that the meta field contains correct
74
+
<listitem><para>Ensure that the meta field contains correct
77
-
<listitem><para>License can change with version updates, so it should be
77
+
<listitem><para>License can change with version updates, so it should be
checked to be fitting upstream license.</para></listitem>
79
-
<listitem><para>If the package has no maintainer, a maintainer must be
80
-
set. This can be the update submitter or a community member that
79
+
<listitem><para>If the package has no maintainer, a maintainer must be
80
+
set. This can be the update submitter or a community member that
accepts to take maintainership of the package.</para></listitem>
<listitem><para>Ensure that the code contains no typos.</para></listitem>
<listitem><para>Building the package locally.</para>
87
-
<listitem><para>Pull-requests are often targeted to the master or staging
88
-
branch so building the pull-request locally as it is submitted can
87
+
<listitem><para>Pull-requests are often targeted to the master or staging
88
+
branch so building the pull-request locally as it is submitted can
trigger a large amount of source builds.</para>
90
-
<para>It is possible to rebase the changes on nixos-unstable or
91
-
nixpkgs-unstable for easier review by running the following commands
90
+
<para>It is possible to rebase the changes on nixos-unstable or
91
+
nixpkgs-unstable for easier review by running the following commands
94
-
$ git remote add channels https://github.com/NixOS/nixpkgs-channels.git <co
94
+
$ git remote add channels https://github.com/NixOS/nixpkgs-channels.git <co
xml:id='reviewing-rebase-1' />
$ git fetch channels nixos-unstable <co xml:id='reviewing-rebase-2' />
$ git fetch origin pull/PRNUMBER/head <co xml:id='reviewing-rebase-3' />
98
-
$ git rebase --onto nixos-unstable BASEBRANCH FETCH_HEAD <co
98
+
$ git rebase --onto nixos-unstable BASEBRANCH FETCH_HEAD <co
xml:id='reviewing-rebase-4' />
<callout arearefs='reviewing-rebase-1'>
103
-
<para>This should be done only once to be able to fetch channel
103
+
<para>This should be done only once to be able to fetch channel
branches from the nixpkgs-channels repository.</para>
<callout arearefs='reviewing-rebase-2'>
<para>Fetching the nixos-unstable branch.</para>
<callout arearefs='reviewing-rebase-3'>
110
-
<para>Fetching the pull-request changes, <varname>PRNUMBER</varname>
111
-
is the number at the end of the pull-request title and
112
-
<varname>BASEBRANCH</varname> the base branch of the
110
+
<para>Fetching the pull-request changes, <varname>PRNUMBER</varname>
111
+
is the number at the end of the pull-request title and
112
+
<varname>BASEBRANCH</varname> the base branch of the
<callout arearefs='reviewing-rebase-3'>
116
-
<para>Rebasing the pull-request changes to the nixos-unstable
116
+
<para>Rebasing the pull-request changes to the nixos-unstable
123
-
<para>The <link xlink:href="https://github.com/madjar/nox">nox</link>
124
-
tool can be used to review a pull-request content in a single command.
125
-
It doesn't rebase on a channel branch so it might trigger multiple
126
-
source builds. <varname>PRNUMBER</varname> should be replaced by the
123
+
<para>The <link xlink:href="https://github.com/madjar/nox">nox</link>
124
+
tool can be used to review a pull-request content in a single command.
125
+
It doesn't rebase on a channel branch so it might trigger multiple
126
+
source builds. <varname>PRNUMBER</varname> should be replaced by the
number at the end of the pull-request title.</para>
$ nix-shell -p nox --run "nox-review -k pr PRNUMBER"
···
<section><title>New packages</title>
156
-
<para>New packages are a common type of pull-requests. These pull requests
156
+
<para>New packages are a common type of pull-requests. These pull requests
consists in adding a new nix-expression for a package.</para>
<para>Reviewing process:</para>
162
-
<listitem><para>Add labels to the pull-request. (Requires commit
162
+
<listitem><para>Add labels to the pull-request. (Requires commit
165
-
<listitem><para><literal>8.has: package (new)</literal> and any topic
165
+
<listitem><para><literal>8.has: package (new)</literal> and any topic
label that fit the new package.</para></listitem>
169
-
<listitem><para>Ensure that the package versioning is fitting the
169
+
<listitem><para>Ensure that the package versioning is fitting the
guidelines.</para></listitem>
171
-
<listitem><para>Ensure that the commit name is fitting the
171
+
<listitem><para>Ensure that the commit name is fitting the
guidelines.</para></listitem>
173
-
<listitem><para>Ensure that the meta field contains correct
173
+
<listitem><para>Ensure that the meta field contains correct
176
-
<listitem><para>License must be checked to be fitting upstream
176
+
<listitem><para>License must be checked to be fitting upstream
license.</para></listitem>
178
-
<listitem><para>Platforms should be set or the package will not get binary
178
+
<listitem><para>Platforms should be set or the package will not get binary
substitutes.</para></listitem>
180
-
<listitem><para>A maintainer must be set, this can be the package
181
-
submitter or a community member that accepts to take maintainership of
180
+
<listitem><para>A maintainer must be set, this can be the package
181
+
submitter or a community member that accepts to take maintainership of
the package.</para></listitem>
<listitem><para>Ensure that the code contains no typos.</para></listitem>
<listitem><para>Ensure the package source.</para>
188
-
<listitem><para>Mirrors urls should be used when
188
+
<listitem><para>Mirrors urls should be used when
available.</para></listitem>
190
-
<listitem><para>The most appropriate function should be used (e.g.
191
-
packages from GitHub should use
190
+
<listitem><para>The most appropriate function should be used (e.g.
191
+
packages from GitHub should use
<literal>fetchFromGitHub</literal>).</para></listitem>
···
<section><title>Module updates</title>
226
-
<para>Module updates are submissions changing modules in some ways. These often
226
+
<para>Module updates are submissions changing modules in some ways. These often
contains changes to the options or introduce new options.</para>
<para>Reviewing process</para>
232
-
<listitem><para>Add labels to the pull-request. (Requires commit
232
+
<listitem><para>Add labels to the pull-request. (Requires commit
235
-
<listitem><para><literal>8.has: module (update)</literal> and any topic
235
+
<listitem><para><literal>8.has: module (update)</literal> and any topic
label that fit the module.</para></listitem>
<listitem><para>Ensure that the module maintainers are notified.</para>
241
-
<listitem><para>Mention-bot notify GitHub users based on the submitted
242
-
changes, but it can happen that it miss some of the package
241
+
<listitem><para>Mention-bot notify GitHub users based on the submitted
242
+
changes, but it can happen that it miss some of the package
maintainers.</para></listitem>
246
-
<listitem><para>Ensure that the module tests, if any, are
246
+
<listitem><para>Ensure that the module tests, if any, are
succeeding.</para></listitem>
<listitem><para>Ensure that the introduced options are correct.</para>
250
-
<listitem><para>Type should be appropriate (string related types differs
251
-
in their merging capabilities, <literal>optionSet</literal> and
250
+
<listitem><para>Type should be appropriate (string related types differs
251
+
in their merging capabilities, <literal>optionSet</literal> and
<literal>string</literal> types are deprecated).</para></listitem>
253
-
<listitem><para>Description, default and example should be
253
+
<listitem><para>Description, default and example should be
provided.</para></listitem>
<listitem><para>Ensure that option changes are backward compatible.</para>
259
-
<listitem><para><literal>mkRenamedOptionModule</literal> and
260
-
<literal>mkAliasOptionModule</literal> functions provide way to make
259
+
<listitem><para><literal>mkRenamedOptionModule</literal> and
260
+
<literal>mkAliasOptionModule</literal> functions provide way to make
option changes backward compatible.</para></listitem>
264
-
<listitem><para>Ensure that removed options are declared with
264
+
<listitem><para>Ensure that removed options are declared with
<literal>mkRemovedOptionModule</literal></para></listitem>
266
-
<listitem><para>Ensure that changes that are not backward compatible are
266
+
<listitem><para>Ensure that changes that are not backward compatible are
mentioned in release notes.</para></listitem>
268
-
<listitem><para>Ensure that documentations affected by the change is
268
+
<listitem><para>Ensure that documentations affected by the change is
updated.</para></listitem>
···
<para>New modules submissions introduce a new module to NixOS.</para>
297
-
<listitem><para>Add labels to the pull-request. (Requires commit
297
+
<listitem><para>Add labels to the pull-request. (Requires commit
300
-
<listitem><para><literal>8.has: module (new)</literal> and any topic label
300
+
<listitem><para><literal>8.has: module (new)</literal> and any topic label
that fit the module.</para></listitem>
304
-
<listitem><para>Ensure that the module tests, if any, are
304
+
<listitem><para>Ensure that the module tests, if any, are
succeeding.</para></listitem>
<listitem><para>Ensure that the introduced options are correct.</para>
308
-
<listitem><para>Type should be appropriate (string related types differs
309
-
in their merging capabilities, <literal>optionSet</literal> and
308
+
<listitem><para>Type should be appropriate (string related types differs
309
+
in their merging capabilities, <literal>optionSet</literal> and
<literal>string</literal> types are deprecated).</para></listitem>
311
-
<listitem><para>Description, default and example should be
311
+
<listitem><para>Description, default and example should be
provided.</para></listitem>
315
-
<listitem><para>Ensure that module <literal>meta</literal> field is
315
+
<listitem><para>Ensure that module <literal>meta</literal> field is
318
-
<listitem><para>Maintainers should be declared in
318
+
<listitem><para>Maintainers should be declared in
<literal>meta.maintainers</literal>.</para></listitem>
320
-
<listitem><para>Module documentation should be declared with
320
+
<listitem><para>Module documentation should be declared with
<literal>meta.doc</literal>.</para></listitem>
324
-
<listitem><para>Ensure that the module respect other modules
324
+
<listitem><para>Ensure that the module respect other modules
327
-
<listitem><para>For example, enabling a module should not open firewall
327
+
<listitem><para>For example, enabling a module should not open firewall
ports by default.</para></listitem>
···
- [ ] options have default
- [ ] options have example
- [ ] options have descriptions
343
-
- [ ] No unneeded package is added to system.environmentPackages
343
+
- [ ] No unneeded package is added to environment.systemPackages
- [ ] meta.maintainers is set
- [ ] module documentation is declared in meta.doc
···
<para>Other type of submissions requires different reviewing steps.</para>
358
-
<para>If you consider having enough knowledge and experience in a topic and
359
-
would like to be a long-term reviewer for related submissions, please contact
360
-
the current reviewers for that topic. They will give you information about the
358
+
<para>If you consider having enough knowledge and experience in a topic and
359
+
would like to be a long-term reviewer for related submissions, please contact
360
+
the current reviewers for that topic. They will give you information about the
362
-
The main reviewers for a topic can be hard to find as there is no list, but
363
-
checking past pull-requests to see who reviewed or git-blaming the code to see
362
+
The main reviewers for a topic can be hard to find as there is no list, but
363
+
checking past pull-requests to see who reviewed or git-blaming the code to see
who committed to that topic can give some hints.</para>
366
-
<para>Container system, boot system and library changes are some examples of the
366
+
<para>Container system, boot system and library changes are some examples of the
pull requests fitting this category.</para>
<section><title>Merging pull-requests</title>
373
-
<para>It is possible for community members that have enough knowledge and
373
+
<para>It is possible for community members that have enough knowledge and
experience on a special topic to contribute by merging pull requests.</para>
<para>TODO: add the procedure to request merging rights.</para>
···
proposition and should be modified to what the community agrees to be the right
383
-
<para>Please note that contributors with commit rights unactive for more than
383
+
<para>Please note that contributors with commit rights unactive for more than
three months will have their commit rights revoked.</para>
387
-
<para>In a case a contributor leaves definitively the Nix community, he should
388
-
create an issue or notify the mailing list with references of packages and
389
-
modules he maintains so the maintainership can be taken over by other
387
+
<para>In a case a contributor leaves definitively the Nix community, he should
388
+
create an issue or notify the mailing list with references of packages and
389
+
modules he maintains so the maintainership can be taken over by other