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

brave: 0.25.2 -> 0.56.12 #49685

Merged
merged 1 commit into from Nov 11, 2018
Merged

brave: 0.25.2 -> 0.56.12 #49685

merged 1 commit into from Nov 11, 2018

Conversation

rht
Copy link
Member

@rht rht commented Nov 3, 2018

Motivation for this change

This switches to a chromium-based browser from a muon-based one.

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.

@emmanuelrosa
Copy link
Contributor

The /nix/store/*-brave/share/applications/brave-browser.desktop file has the executable path (Exec) set to /usr/bin/brave-browser.

@rht
Copy link
Member Author

rht commented Nov 8, 2018

@emmanuelrosa thanks for testing for the blind spot (there are probably other issues with the dpkg /opt paths though); I had just fixed it.

@emmanuelrosa
Copy link
Contributor

Yeah, that happens with packages from other distros. Here's a list of what may need patching:

share/gnome-control-center/default-apps/brave-browser.xml
opt/brave.com/brave/xdg-settings
opt/brave.com/brave/xdg-mime
share/menu/brave-browser.menu
opt/brave.com/brave/default-app-block

@rht
Copy link
Member Author

rht commented Nov 10, 2018

I have patched brave-browser.xml, brave-browser.menu, default-app-block, but xdg-settings and xdg-mime have lots of assumptions on the path of gnom, /usr/share, /usr/bin/file, ... that it is impractical to patch every single path by hand.

@worldofpeace
Copy link
Contributor

but xdg-settings and xdg-mime have lots of assumptions on the path of gnom, /usr/share, /usr/bin/file, ... that it is impractical to patch every single path by hand.

Those scripts are currently within xdg_utils so you can remove them from $out and add a symlink to them instead.

@rht
Copy link
Member Author

rht commented Nov 11, 2018

Those scripts are currently within xdg_utils so you can remove them from $out and add a symlink to them instead.

Ok, fixed the scripts.

@worldofpeace
Copy link
Contributor

@rht I've just checked and the attribute name is brave so can you reformat the commit msg to be?

brave: 0.25.2 -> 0.55.22

Also I went ahead and switched your path corrections to use substituteInPlace.
substituteAll is for something else see

substituteAll() {

quoted everything, and made the derivation not explicitly declare phases as per #28910.
(probably add this to the commit msg)

diff --git a/pkgs/applications/networking/browsers/brave/default.nix b/pkgs/applications/networking/browsers/brave/default.nix
index 24f606e5c3a..6a9726b3ec3 100644
--- a/pkgs/applications/networking/browsers/brave/default.nix
+++ b/pkgs/applications/networking/browsers/brave/default.nix
@@ -79,43 +79,44 @@ in stdenv.mkDerivation rec {
         sha256 = "0111wbzyp2n8s0yskzq7xp7ipv5l6nyqzjczhh23j8yagm084kki";
     };
 
-    phases = [ "unpackPhase" "installPhase" ];
+    dontConfigure = true;
+    dontBuild = true;
 
     nativeBuildInputs = [ dpkg ];
 
     unpackPhase = "dpkg-deb --fsys-tarfile $src | tar -x --no-same-permissions --no-same-owner";
 
     installPhase = ''
-        mkdir -p $out
+        mkdir -p "$out"
 
-        cp -R usr/* $out
-        cp -R opt/ $out/opt
+        cp -R "usr/"* "$out"
+        cp -R "opt" "$out/opt"
 
-        export BINARYWRAPPER=$out/opt/brave.com/brave/brave-browser
+        export BINARYWRAPPER="$out/opt/brave.com/brave/brave-browser"
 
         # Fix path to bash in $BINARYWRAPPER
-        substituteInPlace $BINARYWRAPPER \
-            --replace /bin/bash ${stdenv.shell}
+        substituteInPlace "$BINARYWRAPPER" \
+            --replace "/bin/bash" "${stdenv.shell}"
 
-        ln -sf $BINARYWRAPPER $out/bin/brave-browser
+        ln -sf "$BINARYWRAPPER" "$out/bin/brave-browser"
 
         patchelf \
             --set-interpreter "$(cat $NIX_CC/nix-support/dynamic-linker)" \
-            --set-rpath "${rpath}" $out/opt/brave.com/brave/brave
+            --set-rpath "${rpath}" "$out/opt/brave.com/brave/brave"
 
         # Fix paths
-        substituteAllInPlace $out/share/applications/brave-browser.desktop \
-            --replace /usr/bin/brave-browser $out/bin/brave-browser
-        substituteAllInPlace $out/share/gnome-control-center/default-apps/brave-browser.xml \
-            --replace /opt/brave.com $out/opt/brave.com
-        substituteAllInPlace $out/share/menu/brave-browser.menu \
-            --replace /opt/brave.com $out/opt/brave.com
-        substituteAllInPlace $out/opt/brave.com/brave/default-app-block \
-            --replace /opt/brave.com $out/opt/brave.com
+        substituteInPlace "$out/share/applications/brave-browser.desktop" \
+            --replace "/usr/bin/brave-browser" "$out/bin/brave-browser"
+        substituteInPlace "$out/share/gnome-control-center/default-apps/brave-browser.xml" \
+            --replace "/opt/brave.com" "$out/opt/brave.com"
+        substituteInPlace "$out/share/menu/brave-browser.menu" \
+            --replace "/opt/brave.com" "$out/opt/brave.com"
+        substituteInPlace "$out/opt/brave.com/brave/default-app-block" \
+            --replace "/opt/brave.com" "$out/opt/brave.com"
 
         # Replace xdg-settings and xdg-mime
-        ln -sf ${xdg_utils}/bin/xdg-settings $out/opt/brave.com/brave/xdg-settings
-        ln -sf ${xdg_utils}/bin/xdg-mime $out/opt/brave.com/brave/xdg-mime
+        ln -sf "${xdg_utils}/bin/xdg-settings" "$out/opt/brave.com/brave/xdg-settings"
+        ln -sf "${xdg_utils}/bin/xdg-mime" "$out/opt/brave.com/brave/xdg-mime"
     '';
 
     meta = with stdenv.lib; {

@rht
Copy link
Member Author

rht commented Nov 11, 2018

Also I went ahead and switched your path corrections to use substituteInPlace.

Ok I thought substituteInPlace does the replacement only once instead on all occurences. The naming substituteAll is misleading where it should have been substituteEnv.

I will incorporate your changes except for the quoting.
As with the quoting, I don't think they are necessary (see e.g. https://github.com/NixOS/nixpkgs/blob/cf7e7127c56ab04efaf1459d09810735bae52ccd/pkgs/applications/networking/browsers/tor-browser-bundle/default.nix).

@rht rht changed the title brave-browser: 0.25.2 -> 0.55.22 brave: 0.25.2 -> 0.55.22 Nov 11, 2018
@worldofpeace
Copy link
Contributor

Still think that quoting anything that had a variable in would be a good idea.

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

shrinking /nix/store/91n2ryszjzlclfwmrb0nidk3szzilbl8-brave/opt/brave.com/brave/brave
shrinking /nix/store/91n2ryszjzlclfwmrb0nidk3szzilbl8-brave/opt/brave.com/brave/swiftshader/libGLESv2.so
shrinking /nix/store/91n2ryszjzlclfwmrb0nidk3szzilbl8-brave/opt/brave.com/brave/swiftshader/libEGL.so
gzipping man pages under /nix/store/91n2ryszjzlclfwmrb0nidk3szzilbl8-brave/share/man/
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/91n2ryszjzlclfwmrb0nidk3szzilbl8-brave/bin
patching script interpreter paths in /nix/store/91n2ryszjzlclfwmrb0nidk3szzilbl8-brave
/nix/store/91n2ryszjzlclfwmrb0nidk3szzilbl8-brave/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/91n2ryszjzlclfwmrb0nidk3szzilbl8-brave...
/nix/store/91n2ryszjzlclfwmrb0nidk3szzilbl8-brave

@rht
Copy link
Member Author

rht commented Nov 11, 2018

Still think that quoting anything that had a variable in would be a good idea.

All the $out in https://nixos.org/nixpkgs/manual are unquoted btw. The corner case that might lead to quoting having to be enforced was raised in #26951, but there was no resolution to it.

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Everything looks good here 👍

@emmanuelrosa
Copy link
Contributor

Unfortunately the browser no longer executes:

[emmanuel@~/]$ brave-browser -v
Fontconfig warning: "/etc/fonts/fonts.conf", line 86: unknown element "blank"
Trace/breakpoint trap
[emmanuel@~/]$ [12596:12596:1111/110757.228434:ERROR:sandbox_linux.cc(379)] InitializeSandbox() called with multiple threads in process gpu-process.
[12596:12596:1111/110757.228829:ERROR:broker_posix.cc(43)] Invalid node channel message

At that point it exits. An older version of the package works:

[emmanuel@~/]$ brave-browser -v
Fontconfig warning: "/etc/fonts/fonts.conf", line 86: unknown element "blank"
[13117:13117:1111/111010.229082:ERROR:sandbox_linux.cc(379)] InitializeSandbox() called with multiple threads in process gpu-process.                                                            
[13080:13191:1111/111010.468441:ERROR:object_proxy.cc(615)] Failed to call method: org.freedesktop.Notifications.GetCapabilities: object_path= /org/freedesktop/Notifications: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.Notifications was not provided by any .service files                                                                                            
[13080:13101:1111/111011.972065:ERROR:rewards_service_impl.cc(133)] Failed to read file: /home/emmanuel/.config/BraveSoftware/Brave-Browser/Default/ledger_state

Here's a diff from the package that works to the current one:

33c33,34
<   zlib
---
>   zlib,
>   xdg_utils
67a69
>     xdg_utils
80c82,83
<     phases = [ "unpackPhase" "installPhase" ];
---
>     dontConfigure = true;
>     dontBuild = true;
90c93
<         cp -R opt/ $out
---
>         cp -R opt/ $out/opt
92,93c95,98
<         # Fix path to bash in $out/opt/brave.com/brave/brave-browser
<         substituteInPlace $out/opt/brave.com/brave/brave-browser \
---
>         export BINARYWRAPPER=$out/opt/brave.com/brave/brave-browser
> 
>         # Fix path to bash in $BINARYWRAPPER
>         substituteInPlace $BINARYWRAPPER \
96c101
<         ln -sf $out/opt/brave.com/brave/brave-browser $out/bin/brave-browser
---
>         ln -sf $BINARYWRAPPER $out/bin/brave-browser
101a107,119
>         # Fix paths
>         substituteInPlace $out/share/applications/brave-browser.desktop \
>             --replace /usr/bin/brave-browser $out/bin/brave-browser
>         substituteInPlace $out/share/gnome-control-center/default-apps/brave-browser.xml \
>             --replace /opt/brave.com $out/opt/brave.com
>         substituteInPlace $out/share/menu/brave-browser.menu \
>             --replace /opt/brave.com $out/opt/brave.com
>         substituteInPlace $out/opt/brave.com/brave/default-app-block \
>             --replace /opt/brave.com $out/opt/brave.com
> 
>         # Replace xdg-settings and xdg-mime
>         ln -sf ${xdg_utils}/bin/xdg-settings $out/opt/brave.com/brave/xdg-settings
>         ln -sf ${xdg_utils}/bin/xdg-mime $out/opt/brave.com/brave/xdg-mime

@worldofpeace
Copy link
Contributor

Ahh it needs dontPatchELF = true; and it's fine.

@emmanuelrosa
Copy link
Contributor

Yep, adding dontPatchELF = true; to the package fixed it. Thanks, @worldofpeace

@rht
Copy link
Member Author

rht commented Nov 11, 2018

Added dontPatchELF = true;.

@rht
Copy link
Member Author

rht commented Nov 11, 2018

Additionally, I renamed the binary from brave-browser to brave.

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg build brave

Just to check one more time FTR, should be good otherwise.

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

installing
warning: working around a Linux kernel bug by creating a hole of 2007040 bytes in ‘/nix/store/cigarm8w6nmpkl5skjrjkjlsfmjqxy3a-brave/opt/brave.com/brave/brave’
post-installation fixup
gzipping man pages under /nix/store/cigarm8w6nmpkl5skjrjkjlsfmjqxy3a-brave/share/man/
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/cigarm8w6nmpkl5skjrjkjlsfmjqxy3a-brave/bin
patching script interpreter paths in /nix/store/cigarm8w6nmpkl5skjrjkjlsfmjqxy3a-brave
/nix/store/cigarm8w6nmpkl5skjrjkjlsfmjqxy3a-brave/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/cigarm8w6nmpkl5skjrjkjlsfmjqxy3a-brave...
/nix/store/cigarm8w6nmpkl5skjrjkjlsfmjqxy3a-brave

@worldofpeace
Copy link
Contributor

Additionally, I renamed the binary from brave-browser to brave.

Then you need to change

substituteInPlace $out/share/applications/brave-browser.desktop \
            --replace /usr/bin/brave-browser $out/bin/brave-browser

@rht
Copy link
Member Author

rht commented Nov 11, 2018

Then you need to change

Ah, right, fixed.

@rht
Copy link
Member Author

rht commented Nov 11, 2018

Speaking of which, 0.56.12 was just released 2 days ago. So I am updating the version further.

@rht rht changed the title brave: 0.25.2 -> 0.55.22 brave: 0.25.2 -> 0.56.12 Nov 11, 2018
@rht
Copy link
Member Author

rht commented Nov 11, 2018

@GrahamcOfBorg build brave

@worldofpeace
Copy link
Contributor

@rht Currently you need to be recognized as a trusted user or known user to use the ofborg tooling that way.

Also I just noticed that they're shipping icons but they're not installed to the correct locations.
So this should fix that

diff --git a/pkgs/applications/networking/browsers/brave/default.nix b/pkgs/applications/networking/browsers/brave/default.nix
index fff87025197..3c0993d8733 100644
--- a/pkgs/applications/networking/browsers/brave/default.nix
+++ b/pkgs/applications/networking/browsers/brave/default.nix
@@ -115,6 +115,15 @@ in stdenv.mkDerivation rec {
         substituteInPlace $out/opt/brave.com/brave/default-app-block \
             --replace /opt/brave.com $out/opt/brave.com
 
+        # Correct icons location
+        icon_sizes=("16" "22" "24" "32" "48" "64" "128" "256")
+
+        for icon in ''${icon_sizes[*]}
+        do
+            mkdir -p $out/share/icons/hicolor/$icon\x$icon/apps
+            ln -s $out/opt/brave.com/brave/product_logo_$icon.png $out/share/icons/hicolor/$icon\x$icon/apps/brave-browser.png
+        done
+
         # Replace xdg-settings and xdg-mime
         ln -sf ${xdg_utils}/bin/xdg-settings $out/opt/brave.com/brave/xdg-settings
         ln -sf ${xdg_utils}/bin/xdg-mime $out/opt/brave.com/brave/xdg-mime

I didn't move them because I'm not sure they expect them to be there.

@rht
Copy link
Member Author

rht commented Nov 11, 2018

@worldofpeace I added your latest patch, which is a lot more succinct than how it is being done in chromium/google-chrome,

    for icon_file in chrome/app/theme/chromium/product_logo_*[0-9].png; do
      num_and_suffix="''${icon_file##*logo_}"
      icon_size="''${num_and_suffix%.*}"
      expr "$icon_size" : "^[0-9][0-9]*$" || continue
      logo_output_prefix="$out/share/icons/hicolor"
      logo_output_path="$logo_output_prefix/''${icon_size}x''${icon_size}/apps"
      mkdir -vp "$logo_output_path"
      cp -v "$icon_file" "$logo_output_path/$packageName.png"
    done

@worldofpeace
Copy link
Contributor

@rht Yeah that's a little complex. I'm assuming here that icon sizes will remain fixed though.

@worldofpeace
Copy link
Contributor

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

installing
warning: working around a Linux kernel bug by creating a hole of 2007040 bytes in ‘/nix/store/mhw7141k0q659qx1a5f79fgk292w9gjn-brave/opt/brave.com/brave/brave’
post-installation fixup
gzipping man pages under /nix/store/mhw7141k0q659qx1a5f79fgk292w9gjn-brave/share/man/
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/mhw7141k0q659qx1a5f79fgk292w9gjn-brave/bin
patching script interpreter paths in /nix/store/mhw7141k0q659qx1a5f79fgk292w9gjn-brave
/nix/store/mhw7141k0q659qx1a5f79fgk292w9gjn-brave/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/mhw7141k0q659qx1a5f79fgk292w9gjn-brave...
/nix/store/mhw7141k0q659qx1a5f79fgk292w9gjn-brave

@worldofpeace
Copy link
Contributor

I've tested and:

  • the binary executes correctly
  • desktop file appears and has correct icons
  • clicking the desktop item launches brave

Everything works fine within the browser.

LGTM 👍

cc @Mic92 @infinisil (mergers)

@infinisil infinisil merged commit 03187b7 into NixOS:master Nov 11, 2018
@rht rht deleted the brave-0.55.22 branch November 11, 2018 22:10
@worldofpeace
Copy link
Contributor

@rht Thank you for your cooperation and contribution ❇️

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

5 participants