doc: explain pull request template #28589

disassembler · 2017-08-26T15:25:16Z

Motivation for this change

Give detailed sections in nixpkgs contributing section of what the different sections of the PR template are and how to fulfill the requirements for each checkbox. This addresses #28579.

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

Mic92

Thanks for taking the time to write this down. I largely agree on the content.

I am not a good proof reader though, so maybe somebody else should read it too.

Mic92 · 2017-08-26T18:01:25Z

doc/submitting-changes.xml

+      <literal>fetch*</literal> functions and files outside the Nix store.
+      Depending on the operating system access to other resources are blocked
+      as well (ex. inter process communication is isolated on Linux); see <link
+      xlink:href="https://nixos.org/nix/manual/#description-45">build-use-sandbox</link>


@other reviewer
Since both documents are in the same documentation, what is the right way to link them in docbook?

I don't think you can because these are in separate documents. This is linking the nix manual, but this is in the nixpkgs manual. Someone that knows docbook better than me can correct me if I'm wrong here.

Mic92 · 2017-08-26T18:04:36Z

doc/submitting-changes.xml

+      maintainer to verify the functionality of the package. If there are
+      existing tests for the package, they should be ran to verify your changes
+      do not break the tests. Tests only apply to packages with NixOS modules
+      defined and can only be ran on NixOS. For more details on writing and


- can be only ran on NixOS + can be only ran on Linux

our tests are using qemu so they are not limited to NixOS.

Mic92 · 2017-08-26T18:14:15Z

doc/submitting-changes.xml

+    </para>
+    <para>
+      Depending if you use NixOS or other platforms you can use one of the
+      following methods to enable sandboxing:


what I forgot to add here is the following:

- following methods to enable sandboxing. + following methods to enable sandboxing <emphasis role="bold">before</emphasis> building the package.

Mic92 · 2017-08-26T18:16:35Z

doc/submitting-changes.xml

+      <itemizedlist>
+        <listitem>
+          <para><emphasis role="bold">Enable sandboxing for single build</emphasis>:
+            <screen>nix-shell -I nixpkgs=/path/to/nixpkgs --option build-use-sandbox true -p hello</screen>


append:

(replace <literal>hello</literal> with the package you want to build)

joachifm · 2017-08-26T19:47:23Z

doc/submitting-changes.xml

+      <itemizedlist>
+        <listitem>
+          <para><emphasis role="bold">Enable sandboxing for single build</emphasis>:
+            <screen>nix-shell -I nixpkgs=/path/to/nixpkgs --option build-use-sandbox true -p hello</screen>


The way this is worded gives the impression that an ordinary user can control sandbox usage even if they are using the Nix daemon.

Are options ignored, when nix-daemon is used? Then we should probably not mention this at all. Otherwise people, will fall into the trap.

I agree it'd be better to leave this out, or at least add some caveats.

globin

Apart from the minor nitpick, very nice!

globin · 2017-08-27T19:32:43Z

doc/submitting-changes.xml

+      Packages with automated tests are much more likely to be merged in a
+      timely fashion because it doesn't require as much manual testing by the
+      maintainer to verify the functionality of the package. If there are
+      existing tests for the package, they should be ran to verify your changes


globin · 2017-08-27T19:32:55Z

doc/submitting-changes.xml

+      maintainer to verify the functionality of the package. If there are
+      existing tests for the package, they should be ran to verify your changes
+      do not break the tests. Tests only apply to packages with NixOS modules
+      defined and can only be ran on NixOS. For more details on writing and


grahamc

Really really great. A few nits to be picked, and I think the part about why sandboxing is disabled by default should be corrected prior to merging. Thank you a lot for writing this :D

grahamc · 2017-08-29T13:45:02Z

doc/submitting-changes.xml

+      Depending on the operating system access to other resources are blocked
+      as well (ex. inter process communication is isolated on Linux); see <link
+      xlink:href="https://nixos.org/nix/manual/#description-45">build-use-sandbox</link>
+      in nix manual for details.


grahamc · 2017-08-29T13:45:10Z

doc/submitting-changes.xml

+      for each build process. It is used to remove further hidden dependencies
+      set by the build environment to improve reproducibility. This includes
+      access to the network during the build outside of
+      <literal>fetch*</literal> functions and files outside the Nix store.


<function> over <literal> I think?

grahamc · 2017-08-29T13:46:19Z

doc/submitting-changes.xml

+    </para>
+    <para>
+      Sandboxes are not enabled by default in Nix as there are cases where it
+      makes building packages harder (for example <command>npm


Sandboxing is disabled by default not because it is harder, but because it is slower: NixOS/nix#179

grahamc · 2017-08-29T13:50:38Z

doc/submitting-changes.xml

+  <section>
+    <title>Built on platform(s)</title>
+    <para>
+      Many <literal>Nix</literal> packages are designed to run on multiple


Nix isn't a literal here

grahamc · 2017-08-29T13:52:52Z

doc/submitting-changes.xml

+    </para>
+    <para>
+      review changes from pull request number 12345:
+      <screen>nix-shell -p nox --run nox-review 12345</screen>


Missing the command pr, should be: nox-review pr 12345

grahamc · 2017-08-29T13:53:39Z

doc/submitting-changes.xml

+      It's important to test any executables generated by a build when you
+      change or create a package in nixpkgs. This can be done by looking in
+      <filename>./result/bin</filename> and running any files in there, or at a
+      minimum, the main file for the package. For example, if you make a change


file is ambiguous, I think you mean the main executable for the package

Mic92 approved these changes Aug 26, 2017

View reviewed changes

joachifm reviewed Aug 26, 2017

View reviewed changes

globin approved these changes Aug 27, 2017

View reviewed changes

disassembler force-pushed the doc-pull-request-template branch from 8d67106 to 3b5e1e9 Compare August 29, 2017 13:39

grahamc requested changes Aug 29, 2017

View reviewed changes

disassembler force-pushed the doc-pull-request-template branch from 3b5e1e9 to c2d0917 Compare August 29, 2017 14:03

doc: explain pull request template

aae7745

disassembler force-pushed the doc-pull-request-template branch from c2d0917 to aae7745 Compare August 29, 2017 14:15

grahamc approved these changes Aug 29, 2017

View reviewed changes

Mic92 merged commit e682cfc into NixOS:master Aug 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: explain pull request template #28589

doc: explain pull request template #28589

disassembler commented Aug 26, 2017

Mic92 left a comment

Mic92 Aug 26, 2017

disassembler Aug 29, 2017

Mic92 Aug 26, 2017

Mic92 Aug 26, 2017 •

edited

Mic92 Aug 26, 2017 •

edited

joachifm Aug 26, 2017

Mic92 Aug 27, 2017 •

edited

joachifm Aug 27, 2017

globin left a comment

globin Aug 27, 2017

globin Aug 27, 2017

grahamc left a comment

grahamc Aug 29, 2017

grahamc Aug 29, 2017

grahamc Aug 29, 2017

grahamc Aug 29, 2017

grahamc Aug 29, 2017

grahamc Aug 29, 2017

doc: explain pull request template #28589

doc: explain pull request template #28589

Conversation

disassembler commented Aug 26, 2017

Motivation for this change

Things done

Mic92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Aug 26, 2017 • edited

Choose a reason for hiding this comment

Mic92 Aug 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Aug 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

globin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Aug 26, 2017 •

edited

Mic92 Aug 26, 2017 •

edited

Mic92 Aug 27, 2017 •

edited