manual: reviewing contributions nixos -> nixpkgs (#20626)

2 files changed, 394 insertions, 0 deletions

diff --git a/doc/manual.xml b/doc/manual.xml
index 32e94e8e59c..6ad66d48652 100644
--- a/doc/manual.xml
+++ b/doc/manual.xml
@@ -20,6 +20,7 @@
   <xi:include href="package-notes.xml" />
   <xi:include href="coding-conventions.xml" />
   <xi:include href="submitting-changes.xml" />
+  <xi:include href="reviewing-contributions.xml" />
   <xi:include href="contributing.xml" />
 
 </book>
diff --git a/doc/reviewing-contributions.xml b/doc/reviewing-contributions.xml
new file mode 100644
index 00000000000..f86928bcd5d
--- /dev/null
+++ b/doc/reviewing-contributions.xml
@@ -0,0 +1,393 @@
+<chapter xmlns="http://docbook.org/ns/docbook"
+        xmlns:xlink="http://www.w3.org/1999/xlink"
+        xmlns:xi="http://www.w3.org/2001/XInclude"
+        version="5.0"
+        xml:id="sec-reviewing-contributions">
+
+<title>Reviewing contributions</title>
+
+<warning>
+  <para>The following section is a draft and reviewing policy is still being 
+    discussed.</para>
+</warning>
+
+<para>The nixpkgs projects receives a fairly high number of contributions via 
+  GitHub pull-requests. Reviewing and approving these is an important task and a 
+  way to contribute to the project.</para>
+
+<para>The high change rate of nixpkgs make any pull request that is open for 
+  long enough subject to conflicts that will require extra work from the 
+  submitter or the merger. Reviewing pull requests in a timely manner and being 
+  responsive to the comments is the key to avoid these. Github provides sort 
+  filters that can be used to see the <link 
+    xlink:href="https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc">most 
+    recently</link> and the <link 
+    xlink:href="https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-asc">least 
+    recently</link> updated pull-requests.</para>
+
+<para>When reviewing a pull request, please always be nice and polite. 
+  Controversial changes can lead to controversial opinions, but it is important 
+  to respect every community members and their work.</para>
+
+<para>GitHub provides reactions, they are a simple and quick way to provide 
+  feedback to pull-requests or any comments. The thumb-down reaction should be 
+  used with care and if possible accompanied with some explanations so the 
+  submitter has directions to improve his contribution.</para>
+
+<para>Pull-requests reviews should include a list of what has been reviewed in a 
+  comment, so other reviewers and mergers can know the state of the 
+  review.</para>
+
+<para>All the review template samples provided in this section are generic and 
+  meant as examples. Their usage is optional and the reviewer is free to adapt 
+  them to his liking.</para>
+
+<section><title>Package updates</title>
+
+<para>A package update is the most trivial and common type of pull-request. 
+  These pull-requests mainly consist in updating the version part of the package 
+  name and the source hash.</para>
+<para>It can happen that non trivial updates include patches or more complex 
+  changes.</para>
+
+<para>Reviewing process:</para>
+
+<itemizedlist>
+  <listitem><para>Add labels to the pull-request. (Requires commit 
+      rights)</para>
+    <itemizedlist>
+      <listitem><para><literal>8.has: package (update)</literal> and any topic 
+          label that fit the updated package.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that the package versioning is fitting the 
+      guidelines.</para></listitem>
+  <listitem><para>Ensure that the commit text is fitting the 
+      guidelines.</para></listitem>
+  <listitem><para>Ensure that the package maintainers are notified.</para>
+    <itemizedlist>
+      <listitem><para>mention-bot usually notify GitHub users based on the 
+          submitted changes, but it can happen that it misses some of the 
+          package maintainers.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that the meta field contains correct 
+      information.</para>
+    <itemizedlist>
+      <listitem><para>License can change with version updates, so it should be 
+          checked to be fitting upstream license.</para></listitem>
+      <listitem><para>If the package has no maintainer, a maintainer must be 
+          set. This can be the update submitter or a community member that 
+          accepts to take maintainership of the package.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that the code contains no typos.</para></listitem>
+  <listitem><para>Building the package locally.</para>
+    <itemizedlist>
+      <listitem><para>Pull-requests are often targeted to the master or staging 
+          branch so building the pull-request locally as it is submitted can 
+          trigger a large amount of source builds.</para>
+        <para>It is possible to rebase the changes on nixos-unstable or 
+          nixpkgs-unstable for easier review by running the following commands 
+          from a nixpkgs clone.
+<screen>
+$ 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' />
+$ git rebase --onto nixos-unstable BASEBRANCH FETCH_HEAD <co 
+  xml:id='reviewing-rebase-4' />
+</screen>
+        <calloutlist>
+          <callout arearefs='reviewing-rebase-1'>
+            <para>This should be done only once to be able to fetch channel 
+              branches from the nixpkgs-channels repository.</para>
+          </callout>
+          <callout arearefs='reviewing-rebase-2'>
+            <para>Fetching the nixos-unstable branch.</para>
+          </callout>
+          <callout arearefs='reviewing-rebase-3'>
+            <para>Fetching the pull-request changes, <varname>PRNUMBER</varname> 
+              is the number at the end of the pull-request title and 
+              <varname>BASEBRANCH</varname> the base branch of the 
+              pull-request.</para>
+          </callout>
+          <callout arearefs='reviewing-rebase-3'>
+            <para>Rebasing the pull-request changes to the nixos-unstable 
+              branch.</para>
+          </callout>
+        </calloutlist>
+        </para>
+      </listitem>
+      <listitem>
+        <para>The <link xlink:href="https://github.com/madjar/nox">nox</link> 
+          tool can be used to review a pull-request content in a single command. 
+          It doesn't rebase on a channel branch so it might trigger multiple 
+          source builds. <varname>PRNUMBER</varname> should be replaced by the 
+          number at the end of the pull-request title.</para>
+<screen>
+$ nix-shell -p nox --run "nox-review -k pr PRNUMBER"
+</screen>
+      </listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Running every binary.</para></listitem>
+</itemizedlist>
+
+<example><title>Sample template for a package update review</title>
+<screen>
+##### Reviewed points
+
+- [ ] package name fits guidelines
+- [ ] package version fits guidelines
+- [ ] package build on ARCHITECTURE
+- [ ] executables tested on ARCHITECTURE
+- [ ] all depending packages build
+
+##### Possible improvements
+
+##### Comments
+
+</screen></example>
+</section>
+
+<section><title>New packages</title>
+
+<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>
+
+<itemizedlist>
+  <listitem><para>Add labels to the pull-request. (Requires commit 
+      rights)</para>
+    <itemizedlist>
+      <listitem><para><literal>8.has: package (new)</literal> and any topic 
+          label that fit the new package.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that the package versioning is fitting the 
+      guidelines.</para></listitem>
+  <listitem><para>Ensure that the commit name is fitting the 
+      guidelines.</para></listitem>
+  <listitem><para>Ensure that the meta field contains correct 
+      information.</para>
+    <itemizedlist>
+      <listitem><para>License must be checked to be fitting upstream 
+          license.</para></listitem>
+      <listitem><para>Platforms should be set or the package will not get binary 
+          substitutes.</para></listitem>
+      <listitem><para>A maintainer must be set, this can be the package 
+          submitter or a community member that accepts to take maintainership of 
+          the package.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that the code contains no typos.</para></listitem>
+  <listitem><para>Ensure the package source.</para>
+    <itemizedlist>
+      <listitem><para>Mirrors urls should be used when 
+          available.</para></listitem>
+      <listitem><para>The most appropriate function should be used (e.g. 
+          packages from GitHub should use 
+          <literal>fetchFromGitHub</literal>).</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Building the package locally.</para></listitem>
+  <listitem><para>Running every binary.</para></listitem>
+</itemizedlist>
+
+<example><title>Sample template for a new package review</title>
+<screen>
+##### Reviewed points
+
+- [ ] package path fits guidelines
+- [ ] package name fits guidelines
+- [ ] package version fits guidelines
+- [ ] package build on ARCHITECTURE
+- [ ] executables tested on ARCHITECTURE
+- [ ] `meta.description` is set and fits guidelines
+- [ ] `meta.license` fits upstream license
+- [ ] `meta.platforms` is set
+- [ ] `meta.maintainers` is set
+- [ ] build time only dependencies are declared in `nativeBuildInputs`
+- [ ] source is fetched using the appropriate function
+- [ ] phases are respected
+- [ ] patches that are remotely available are fetched with `fetchpatch`
+
+##### Possible improvements
+
+##### Comments
+
+</screen></example>
+</section>
+
+<section><title>Module updates</title>
+
+<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>
+
+<itemizedlist>
+  <listitem><para>Add labels to the pull-request. (Requires commit 
+      rights)</para>
+    <itemizedlist>
+      <listitem><para><literal>8.has: module (update)</literal> and any topic 
+          label that fit the module.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that the module maintainers are notified.</para>
+    <itemizedlist>
+      <listitem><para>Mention-bot notify GitHub users based on the submitted 
+          changes, but it can happen that it miss some of the package 
+          maintainers.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that the module tests, if any, are 
+      succeeding.</para></listitem>
+  <listitem><para>Ensure that the introduced options are correct.</para>
+    <itemizedlist>
+      <listitem><para>Type should be appropriate (string related types differs 
+          in their merging capabilities, <literal>optionSet</literal> and 
+          <literal>string</literal> types are deprecated).</para></listitem>
+      <listitem><para>Description, default and example should be 
+          provided.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that option changes are backward compatible.</para>
+    <itemizedlist>
+      <listitem><para><literal>mkRenamedOptionModule</literal> and 
+          <literal>mkAliasOptionModule</literal> functions provide way to make 
+          option changes backward compatible.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that removed options are declared with 
+      <literal>mkRemovedOptionModule</literal></para></listitem>
+  <listitem><para>Ensure that changes that are not backward compatible are 
+      mentioned in release notes.</para></listitem>
+  <listitem><para>Ensure that documentations affected by the change is 
+      updated.</para></listitem>
+</itemizedlist>
+
+<example><title>Sample template for a module update review</title>
+<screen>
+##### Reviewed points
+
+- [ ] changes are backward compatible
+- [ ] removed options are declared with `mkRemovedOptionModule`
+- [ ] changes that are not backward compatible are documented in release notes
+- [ ] module tests succeed on ARCHITECTURE
+- [ ] options types are appropriate
+- [ ] options description is set
+- [ ] options example is provided
+- [ ] documentation affected by the changes is updated
+
+##### Possible improvements
+
+##### Comments
+
+</screen></example>
+</section>
+
+<section><title>New modules</title>
+
+<para>New modules submissions introduce a new module to NixOS.</para>
+
+<itemizedlist>
+  <listitem><para>Add labels to the pull-request. (Requires commit 
+      rights)</para>
+    <itemizedlist>
+      <listitem><para><literal>8.has: module (new)</literal> and any topic label 
+          that fit the module.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that the module tests, if any, are 
+      succeeding.</para></listitem>
+  <listitem><para>Ensure that the introduced options are correct.</para>
+    <itemizedlist>
+      <listitem><para>Type should be appropriate (string related types differs 
+          in their merging capabilities, <literal>optionSet</literal> and 
+          <literal>string</literal> types are deprecated).</para></listitem>
+      <listitem><para>Description, default and example should be 
+          provided.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that module <literal>meta</literal> field is 
+      present</para>
+    <itemizedlist>
+      <listitem><para>Maintainers should be declared in 
+          <literal>meta.maintainers</literal>.</para></listitem>
+      <listitem><para>Module documentation should be declared with 
+          <literal>meta.doc</literal>.</para></listitem>
+    </itemizedlist>
+  </listitem>
+  <listitem><para>Ensure that the module respect other modules 
+      functionality.</para>
+    <itemizedlist>
+      <listitem><para>For example, enabling a module should not open firewall 
+          ports by default.</para></listitem>
+    </itemizedlist>
+  </listitem>
+</itemizedlist>
+
+<example><title>Sample template for a new module review</title>
+<screen>
+##### Reviewed points
+
+- [ ] module path fits the guidelines
+- [ ] module tests succeed on ARCHITECTURE
+- [ ] options have appropriate types
+- [ ] options have default
+- [ ] options have example
+- [ ] options have descriptions
+- [ ] No unneeded package is added to system.environmentPackages
+- [ ] meta.maintainers is set
+- [ ] module documentation is declared in meta.doc
+
+##### Possible improvements
+
+##### Comments
+
+</screen></example>
+</section>
+
+<section><title>Other submissions</title>
+
+<para>Other type of submissions requires different reviewing steps.</para>
+
+<para>If you consider having enough knowledge and experience in a topic and 
+  would like to be a long-term reviewer for related submissions, please contact 
+  the current reviewers for that topic. They will give you information about the 
+  reviewing process.
+The main reviewers for a topic can be hard to find as there is no list, but 
+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>
+
+<para>Container system, boot system and library changes are some examples of the 
+  pull requests fitting this category.</para>
+
+</section>
+
+<section><title>Merging pull-requests</title>
+
+<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>
+
+<!--
+The following paragraph about how to deal with unactive contributors is just a
+proposition and should be modified to what the community agrees to be the right
+policy.
+
+<para>Please note that contributors with commit rights unactive for more than 
+  three months will have their commit rights revoked.</para>
+-->
+
+<para>In a case a contributor leaves definitively the Nix community, he should 
+  create an issue or notify the mailing list with references of packages and 
+  modules he maintains so the maintainership can be taken over by other 
+  contributors.</para>
+
+</section>
+</chapter>