Making the Pull Request Template more beginner friendly (nixpkgs)

Help
1

Background

About one year ago, I started using NixOS and submitted my first Pull Request (PR) to nixpkgs.
Today I submitted my 5th PR after not having submitted one in 3 months.

Even though I already got 4 PRs merged, I admittedly still haven’t understood all Things done items from the Pull Request Template.

That said, I think it could be made more beginner friendly, which I’m intending to do by asking a couple of beginner’s questions (that I had or still have) and hopefully the Nix community may be able to answer:

Questions

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)

Okay, so I have to test “using sandboxing”. But:

  1. Where I do put the nix.useSandbox option?
    (Do I have to include it in my /etc/nixos/configuration.nix or can I pass it directly to whatever tool I have to use for the test?)
  2. How do I actually test?
    (Is there a command to test? Or is this just a prerequisite for the following “Tested …” items?)

Built on platform(s)

  1. Am I expected to test multiple platforms or is this just an info?
    (If it is just an info, it maybe shouldn’t be under “Things done”.)

macOS

  1. Is this the same as “darwin”?

other Linux distributions

  1. This means using the Nix package manager in another Linux distro, right?

Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)

Okay, so I should look inside nixos/tests. But:

  1. How do I find out if there’s a test applicable for my change?
  2. And if there is, how do I actually run that test?
  3. And if there is not, how can I add a test myself?

Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"

Okay, so I should probably run this inside my branch, but wip seems to only work when the changes are not yet committed.

  1. How can I test the compilation for my committed changes?

Tested execution of all binary files (usually in ./result/bin/)

Okay, so I should test the binary files that are usually in ./result/bin. But:

  1. What command do I have to run in order to create those binary files in the first place?
  2. And where do I find the result then?
    (It’s a relative path, but relative to which path? The path I ran the build command in?)

Determined the impact on package closure size (by running nix path-info -S before and after)

Okay, so when I run this in my branch I get the following:

$ nix path-info -S
error: Please be informed that this pseudo-package is not the only part of
Nixpkgs that fails to evaluate. You should not evaluate entire Nixpkgs
without some special measures to handle failing packages, like those taken
by Hydra.
  1. How do determine the impact correctly?
  2. And what do I do with that output, just paste it here [in the PR]?

Other than that, I was a bit confused by @GrahamcOfBorg:

  1. What do the labels mean, e.g.:
    image
    image774×56 12.2 KB

    image
    image794×44 13.2 KB
  2. I saw that others interacted with @GrahamcOfBorg: Should I as well? E.g.:
    image
    image
6 Likes
2

Thanks, that’s a lot of good questions. The issue template has accumulated quite a bit of cruft and should definitely be improved.

  1. Nix sandoxing is enabled by setting sandbox = true in the nix.conf or by passing --option sandbox true as an argument to nix-build.

    On NixOS, adding nix.useSandbox = true; to /etc/nixos/configuration.nix will ultimately set sandbox = true in the nix.conf file.

    The command-line argument can only be passed in a single-user install of nix, or if the user is part of the trusted-users list in the nix.conf file.

    Also if you already built the package without sandboxing, some explanation is needed on how to clean the /nix/store of the previous build.

  2. The “test” is successful if the build is succeeding with sandboxing enabled. Some build scripts try to access the network and we want to avoid that to make nixpkgs more reproducible.

  3. Only check the platforms that you have tested on. This is mostly useful for the reviewer as they might decide to test on more platforms.

  4. Technically speaking macOS is the distribution and Darwin is the kernel. Like Ubuntu is the distribution and Linux is the kernel. I think most of the time they are used interchangeably since kernel and distribution versions are tightly coupled.

  5. Yes, for example if nix was installed on Ubuntu.

  6. Derivation tests are automatically executed at build time. NixOS modules might have some tests and I don’t know how to run them either.

  7. TODO

  8. TODO

  9. It should be fixed to nix-shell -p nox --run "nox-review wip --against origin/master" where origin/master points to the branch you want to merge into. nox-review’s goal is to find all the packages that are affected by your change and build them as well to make sure that nothing got broken.

  10. I think they meant to actually execute the output. For example if you upgraded the pkgs.hello package, run result/bin/hello and check that it works. Even if a package compiled it doesn’t necessarily means that it works at runtime.

  11. The result is the symlink that is created on nix-build. Eg: in the top of the nixpkgs checkout if you run nix-build -A hello then readlink ./result will give you something like “/nix/store/kmwd1hq55akdb9sc7l3finr175dajlby-hello-2.10”

  12. I’m not sure what this is for either. TODO

  13. TODO

  14. This bot does a couple of checks for us automatically. The rebuild labels assign a range of packages that will be rebuilt because of the changes. It’s mainly used to avoid surprise mass rebuilds. For example if you change the openssl version it would rebuild a lot of things. In that case the reviewers would ask you to submit the PR against the “staging” branch.

  15. Eval should happen automatically. The bot is building against multiple platforms so it’s a good way to test that the package works on Darwin for example if you only have a Linux box.

Hope this helps

2 Likes
3

  1. As the link hints, it is a NixOS option, so it needs to go to your
    system configuration. If you click the “declared by” link, you will
    see that what the option does is set the option in /etc/nix/nix.conf
    file. Passing it direcly might be possible using --option sandbox true but I am not sure if that works when using nix daemon (default on
    NixOS).

  2. Just making sure that it builds with sandboxing enabled.

  3. If you can it, it is good idea to test it on as many platforms as
    possible. But since many people do not have access to MacOS so it
    cannot be a hard requirement.

  4. Yep.

  5. Yep.

  6. You can look for a file named ${package-name}.nix, or grep the
    package attribute name in the tests directory, or look into
    https://github.com/NixOS/nixpkgs/blob/83465b9ba18bb0b643f1e7cddf530452ef6ec499/nixos/release.nix#L246-L415.
    In the future, borg should take care of that
    Feature request: list of tests to rebuild · Issue #149 · NixOS/ofborg · GitHub

  7. nix-build nixos/release.nix -A tests.${test-name}.x86_64-linux

  8. Not sure if this is documented anywhere, other than for simple
    installed tests provided by some packages
    (Add installed tests · Issue #34987 · NixOS/nixpkgs · GitHub). Look into other tests
    and see how they work.

  9. Not sure, nox-review just kills my computer so I do not run it. I
    just try to build and test few random dependent packages.
    GitHub - Mic92/nixpkgs-review: Review pull-requests on https://github.com/NixOS/nixpkgs might be interesting.

  10. nix-build command produces result directories in the current
    working directory by default.

  11. Unless you add --no-out-link flag, that is.

  12. You need to specify the attribute name, like with any other nix
    command (e.g. nix path-info -S -f . udisks), see nix path-info --help. It is mostly meant to catch a massive increase, for example
    udisks went from ~100 MB to ~350 MB.

  13. If it is massive increase, try to find out why and list the reason.

  14. It means how many packages will need to be rebuild. When this is a
    large number, the pull request should be merged to staging instead of
    master.

  15. You need to request an access on the ofborg repo (e.g.
    Add knedlsepp to config.extra-known-users by knedlsepp · Pull Request #173 · NixOS/ofborg · GitHub).

2 Likes
4

The only thing that is missing is in nix-review is to test uncommitted changes. For pull requests that are evaluated by ofBorg it is pretty fast, since it uses the reuses the list of changed packages from it.

5

I’m working on, which should solve a few miles of those questions…

https://github.com/NixOS/nixpkgs/pull/92612

6

I just came across this issue, and the pull request marked as solution by @blaggacao was closed, so this is not solved. I think the template is very confusing for the reasons discussed here.

3 Likes
7

It would need somebody capable of overcoming the obstacles I encountered.

1 Like