How does this relate to electron apps? Is lib/electron used by them as an interpreter (e.g. in shebang)?

#58273 is fixed by this as well, I think?

@jtojnar wrote:

How does this relate to electron apps? Is lib/electron used by them as an interpreter (e.g. in shebang)?

Some packages like riot-desktop, which is affected by the issue solved by this PR, launch the electron binary to wrap web apps.

Right.. Lots of checkboxes to tick to merge this... I'll see when I have a chance..

Unless anyone could help ticking them on? :)

Are all of them mandatory? I only have nixos on my machines running - no osx around..

-Are all of them mandatory? I only have nixos on my machines running - no osx around..

They serve mainly to let reviewers know what you already did and that you are familiar with the contributing guidelines. Like many things around Nix, the individual checkboxes are left for PR authors to decide if they need them.

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

This should be done almost every time always every time, since without it, author cannot know if the change works. Sandboxing is also important to rule out accidental impurities that would break build for other people (most importantly, Hydra builds with sandboxing enabled).

Built on platform(s)

This ties in to the first check mark. It is mostly to let the reviewers know where, you are certain, the change works. The more platforms are verified the better but we do not require user to check platforms they do not have access to.

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

Here the applicability is described directly.

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

This would be very nice to have but the command is often very resource intensive, therefore some people are unable to run it. For that reason, we do not mandate it. Also not that nox is somewhat abandoned and many people use nix-review instead.

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

This is also a good idea to make sure everything works as expected. If you are very certain your change does not affect the binaries, then you can skip it. For libraries, I usually just verify that the apps that use them still link and at least one of them can be run. Generally, if you update an application nothing depends on, checking if it starts is probably enough; for widely used libraries (e.g. GTK) you should be more diligent.

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

This one would be nice to have for any update but I only ever bother when I change the set of dependencies. There might be a size creep in a release without dependency changes but that is probably extremely rare.

Assured whether relevant documentation is up to date

This is very important but discovering all relevant docs can be hard. You should try greping Nixpkgs for relevant keywords. Hopefully, the maintainers will let you know if you miss something.

Fits CONTRIBUTING.md.

This is probably the only hard requirement. You can decide to ignore some points from there if you have a good reason, but that is better left for experienced maintainers.

wow. thanks @jtojnar for the explanation! - I think we could even extend the github PR template with your text :)

re sandbox, I read this is the default, but to be sure I also checked with nix-env --option sandbox true -i electron

woah excellent tool!

$ nix-review pr https://github.com/NixOS/nixpkgs/pull/55057                                                                               
$ git fetch --force https://github.com/NixOS/nixpkgs master:refs/nix-review/0 pull/55057/head:refs/nix-review/1                                                              
remote: Enumerating objects: 942, done.
remote: Counting objects: 100% (942/942), done.
remote: Total 1468 (delta 942), reused 942 (delta 942), pack-reused 526
Receiving objects: 100% (1468/1468), 656.42 KiB | 2.12 MiB/s, done.
Resolving deltas: 100% (1081/1081), completed with 336 local objects.
From https://github.com/NixOS/nixpkgs
 * [new branch]              master               -> refs/nix-review/0
 * [new ref]                 refs/pull/55057/head -> refs/nix-review/1
$ git worktree add /home/sarunas/.cache/nix-review/pr-55057/nixpkgs 7cb82ca2d369018bf488c06f15e8336b8d07c39f                                                                 
Preparing worktree (detached HEAD 7cb82ca2d36)
Checking out files: 100% (19123/19123), done.
HEAD is now at 7cb82ca2d36 Merge pull request #63490 from jonringer/bump_uproot
$ nix-env -f /home/sarunas/.cache/nix-review/pr-55057/nixpkgs -qaP --xml --out-path --show-trace                                                                             
$ git merge --no-commit 4fc1758c090331fab50311e2be6e9e83ab687381
Auto-merging pkgs/development/tools/electron/default.nix
Automatic merge went well; stopped before committing as requested
$ nix-env -f /home/sarunas/.cache/nix-review/pr-55057/nixpkgs -qaP --xml --out-path --show-trace --meta                                                                      
$ nix build --no-link --keep-going --max-jobs 8 --option build-use-sandbox true -f /home/sarunas/.cache/nix-review/pr-55057/build.nix                                        
[7 built, 73 copied (277.7 MiB), 117.2 MiB DL]
https://github.com/NixOS/nixpkgs/pull/55057
4 package were build:
electron nix-tour rambox-pro riot-desktop

[0.0 MiB DL]
$ nix-shell /home/sarunas/.cache/nix-review/pr-55057/shell.nix
these paths will be fetched (1.50 MiB download, 8.26 MiB unpacked):
  /nix/store/0dafhdnyj083nf0x9dvwk18m3lq6xl48-bash-interactive-4.4-p23-doc
  /nix/store/facixij6j1kq7g89z3kq4rcgnpr3kc68-bash-interactive-4.4-p23-man
  /nix/store/mhxrf6nbn2ylhj3i5h36kh0h27gsjdqx-bash-interactive-4.4-p23
  /nix/store/sw6pzval0dq3scy568acvzxkssljrz9b-readline-7.0p5
  /nix/store/sy0n5vqn2l16hb3w0yjxkixbfdmsvabk-bash-interactive-4.4-p23-dev
  /nix/store/w4qgvfgmkrszs3184inqjmvq6nwvr3wp-bash-interactive-4.4-p23-info
copying path '/nix/store/0dafhdnyj083nf0x9dvwk18m3lq6xl48-bash-interactive-4.4-p23-doc' from 'https://cache.nixos.org'...
copying path '/nix/store/w4qgvfgmkrszs3184inqjmvq6nwvr3wp-bash-interactive-4.4-p23-info' from 'https://cache.nixos.org'...
copying path '/nix/store/facixij6j1kq7g89z3kq4rcgnpr3kc68-bash-interactive-4.4-p23-man' from 'https://cache.nixos.org'...
copying path '/nix/store/sw6pzval0dq3scy568acvzxkssljrz9b-readline-7.0p5' from 'https://cache.nixos.org'...
copying path '/nix/store/mhxrf6nbn2ylhj3i5h36kh0h27gsjdqx-bash-interactive-4.4-p23' from 'https://cache.nixos.org'...
copying path '/nix/store/sy0n5vqn2l16hb3w0yjxkixbfdmsvabk-bash-interactive-4.4-p23-dev' from 'https://cache.nixos.org'...

reworded commit message as per contributing doc

thanks for approving @jtojnar

just to check what are the next steps for this pr? or just wait? :)

Just tested this with rambox