Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix electron crash when using file chooser dialog #55057

Merged
merged 3 commits into from Jun 25, 2019

Conversation

svalaskevicius
Copy link
Contributor

@svalaskevicius svalaskevicius commented Feb 1, 2019

Motivation for this change

#50985

Thanks @jtojnar for the solution!

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 1, 2019

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

@dtzWill
Copy link
Member

dtzWill commented Mar 27, 2019

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

@pacien
Copy link
Contributor

pacien commented Jun 7, 2019

@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.

@svalaskevicius
Copy link
Contributor Author

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

Unless anyone could help ticking them on? :)

@svalaskevicius
Copy link
Contributor Author

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

@jtojnar
Copy link
Contributor

jtojnar commented Jun 19, 2019

-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.

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution looks good from GTK point of view but I did not test it.

@svalaskevicius
Copy link
Contributor Author

svalaskevicius commented Jun 19, 2019

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

@svalaskevicius
Copy link
Contributor Author

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'...

@svalaskevicius svalaskevicius force-pushed the fix-electron-file-chooser-dialog branch from 4fc1758 to bf36d56 Compare June 19, 2019 19:33
@svalaskevicius
Copy link
Contributor Author

reworded commit message as per contributing doc

@svalaskevicius
Copy link
Contributor Author

thanks for approving @jtojnar

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

This is what upstream uses as well
This is now provided by electron itself.
@Mic92 Mic92 merged commit b552d72 into NixOS:master Jun 25, 2019
@Mic92
Copy link
Member

Mic92 commented Jun 25, 2019

Just tested this with rambox

@svalaskevicius svalaskevicius deleted the fix-electron-file-chooser-dialog branch July 18, 2019 18:53
@oneingan oneingan mentioned this pull request Nov 8, 2019
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants