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.

Sign up for GitHub

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

Jump to bottom

brave: fix file dialog crash #50450

Merged
merged 1 commit into from Nov 22, 2018
Merged

brave: fix file dialog crash #50450

merged 1 commit into from Nov 22, 2018

Conversation

hyperfekt
Copy link
Contributor

@hyperfekt hyperfekt commented Nov 16, 2018 •
edited

Motivation for this change

Fixes crashing when using file chooser GUI (adapted from
93050a3).

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)
  • Fits CONTRIBUTING.md.

@hyperfekt hyperfekt mentioned this pull request Nov 16, 2018
Closed
@hyperfekt hyperfekt force-pushed the brave_file-dialog branch 4 times, most recently from 5635ceb to 88d0f1e Compare November 16, 2018 11:36
@Mic92
Copy link
Member

Mic92 commented Nov 16, 2018 •
edited

This symlink is broken for me:

$ ls -la /nix/store/ibhnf66bx7z3vqc223j9yg0b9mx2x7q3-brave/bin/.brave-wrapped
lrwxrwxrwx 1 root root 83 Jan  1  1970 /nix/store/ibhnf66bx7z3vqc223j9yg0b9mx2x7q3-brave/bin/.brave-wrapped -> /nix/store/ibhnf66bx7z3vqc223j9yg0b9mx2x7q3-brave/opt/brave.com/brave/brave-browser

There is a brave binary in there

$ ls -la /nix/store/ibhnf66bx7z3vqc223j9yg0b9mx2x7q3-brave/opt/brave.com/brave/
total 79954
dr-xr-xr-x 3 root root         4 Jan  1  1970 .
dr-xr-xr-x 3 root root         3 Jan  1  1970 ..
dr-xr-xr-x 2 root root         4 Jan  1  1970 MEIPreload
-r-xr-xr-x 1 root root 156069464 Jan  1  1970 brave

@hyperfekt
Copy link
Contributor Author

hyperfekt commented Nov 16, 2018 •
edited

I'm not sure what you're referring to? That directory has a whole bunch more files (total 193840), among them a brave-browser script. Did your build go wrong? I'm rather confused.

the whole output

[adrian@bismuth:/nix/store]$ ls -la /nix/store/ibhnf66bx7z3vqc223j9yg0b9mx2x7q3-brave/opt/brave.com/brave/
total 193840
dr-xr-xr-x 7 root root         0 Jan  1  1970 .
dr-xr-xr-x 3 root root         0 Jan  1  1970 ..
-r-xr-xr-x 3 root root 156069464 Jan  1  1970 brave
-r--r--r-- 8 root root      7870 Jan  1  1970 brave_100_percent.pak
-r--r--r-- 8 root root     19561 Jan  1  1970 brave_200_percent.pak
-r-xr-xr-x 3 root root      1928 Jan  1  1970 brave-browser
-r--r--r-- 8 root root  15477886 Jan  1  1970 brave_resources.pak
-r-xr-xr-x 8 root root    204416 Jan  1  1970 brave-sandbox
-r--r--r-- 8 root root    928381 Jan  1  1970 chrome_100_percent.pak
-r--r--r-- 8 root root   1269854 Jan  1  1970 chrome_200_percent.pak
dr-xr-xr-x 2 root root         0 Jan  1  1970 cron
-r--r--r-- 2 root root       690 Jan  1  1970 default-app-block
-r--r--r-- 8 root root  10218336 Jan  1  1970 icudtl.dat
dr-xr-xr-x 2 root root         0 Jan  1  1970 locales
dr-xr-xr-x 2 root root         0 Jan  1  1970 MEIPreload
-r--r--r-- 9 root root    113450 Jan  1  1970 natives_blob.bin
-r--r--r-- 8 root root      9427 Jan  1  1970 product_logo_128.png
-r--r--r-- 8 root root       668 Jan  1  1970 product_logo_16.png
-r--r--r-- 8 root root      1003 Jan  1  1970 product_logo_22.png
-r--r--r-- 8 root root      1137 Jan  1  1970 product_logo_24.png
-r--r--r-- 8 root root     25383 Jan  1  1970 product_logo_256.png
-r--r--r-- 8 root root      1617 Jan  1  1970 product_logo_32.png
-r--r--r-- 8 root root      4852 Jan  1  1970 product_logo_32.xpm
-r--r--r-- 8 root root      2739 Jan  1  1970 product_logo_48.png
-r--r--r-- 8 root root      3900 Jan  1  1970 product_logo_64.png
dr-xr-xr-x 4 root root         0 Jan  1  1970 resources
-r--r--r-- 8 root root  13039712 Jan  1  1970 resources.pak
dr-xr-xr-x 2 root root         0 Jan  1  1970 swiftshader
-r--r--r-- 8 root root   1036884 Jan  1  1970 v8_context_snapshot.bin
lrwxrwxrwx 3 root root        72 Jan  1  1970 xdg-mime -> /nix/store/zixfj1njqbb08zdgq955wyy85dp7kba9-xdg-utils-1.1.3/bin/xdg-mime
lrwxrwxrwx 3 root root        76 Jan  1  1970 xdg-settings -> /nix/store/zixfj1njqbb08zdgq955wyy85dp7kba9-xdg-utils-1.1.3/bin/xdg-settings

@worldofpeace
Copy link
Contributor

worldofpeace commented Nov 16, 2018 •
edited

The only broken symlink I'm seeing is to brave-browser

❯ ls -la                                                                                                                       
lrwxrwxrwx   83 root root 31 Dec  1969 .brave-wrapped -> /nix/store/ibhnf66bx7z3vqc223j9yg0b9mx2x7q3-brave/opt/brave.com/brave/brave-browser
.r-xr-xr-x 1.1k root root 31 Dec  1969 brave
lrwxrwxrwx   34 root root 31 Dec  1969 brave-browser -> /opt/brave.com/brave/brave-browser

Btw, just realizing that name doesn't have version in it 😄

So we might want to change that to
name = "brave-${version}"

@worldofpeace
Copy link
Contributor

Apparently the source archive has a bin that gets copied at the installPhase, so that's where that symlink comes from.

This should stop that:

diff --git a/pkgs/applications/networking/browsers/brave/default.nix b/pkgs/applications/networking/browsers/brave/default.nix
index 91670f9be09..55d52b33e4f 100644
--- a/pkgs/applications/networking/browsers/brave/default.nix
+++ b/pkgs/applications/networking/browsers/brave/default.nix
@@ -92,9 +92,9 @@ in stdenv.mkDerivation rec {
     unpackPhase = "dpkg-deb --fsys-tarfile $src | tar -x --no-same-permissions --no-same-owner";
 
     installPhase = ''
-        mkdir -p $out
+        mkdir -p $out $out/bin
 
-        cp -R usr/* $out
+        cp -R usr/share $out
         cp -R opt/ $out/opt
 
         export BINARYWRAPPER=$out/opt/brave.com/brave/brave-browser

brave: wrap with GAppsHook
4657092 
Fixes crashing when using file chooser GUI. Also added version to name and
removed extraneous copying from source archive.
@hyperfekt
Copy link
Contributor Author

Applied your suggestions, many thanks. :)

@worldofpeace
Copy link
Contributor

@hyperfekt I won't be able to reproduce this in my environment but you can confirm this fixes the issue?

@GrahamcOfBorg build brave

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: brave

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: brave

Partial log (click to expand)

hicolorPreFixupPhase
post-installation fixup
Wrapping program /nix/store/13yjjfvl5ncm92sm7avh74ndg5bbwcsm-brave-0.56.12/bin/brave
gzipping man pages under /nix/store/13yjjfvl5ncm92sm7avh74ndg5bbwcsm-brave-0.56.12/share/man/
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/13yjjfvl5ncm92sm7avh74ndg5bbwcsm-brave-0.56.12/bin
patching script interpreter paths in /nix/store/13yjjfvl5ncm92sm7avh74ndg5bbwcsm-brave-0.56.12
/nix/store/13yjjfvl5ncm92sm7avh74ndg5bbwcsm-brave-0.56.12/opt/brave.com/brave/cron/brave-browser: interpreter directive changed from "/bin/sh" to "/nix/store/r47p5pzx52m3n34vdgqpk5rvqgm0m24m-bash-4.4-p23/bin/sh"
checking for references to /build in /nix/store/13yjjfvl5ncm92sm7avh74ndg5bbwcsm-brave-0.56.12...
/nix/store/13yjjfvl5ncm92sm7avh74ndg5bbwcsm-brave-0.56.12

@hyperfekt
Copy link
Contributor Author

@worldofpeace Which one are you talking about? Both the rogue symlink and the crash on file selection are gone.

@worldofpeace
Copy link
Contributor

@worldofpeace Which one are you talking about? Both the rogue symlink and the crash on file selection are gone.

I mean't the crash on file selection.

This looks fine.

cc @Mic92

@worldofpeace worldofpeace merged commit ff55dd4 into NixOS:master Nov 22, 2018
@worldofpeace
Copy link
Contributor

@hyperfekt Thanks a lot ❇️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@worldofpeace worldofpeace worldofpeace left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hyperfekt @Mic92 @worldofpeace @GrahamcOfBorg