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: add update script #108029

Merged
merged 2 commits into from Jan 1, 2021
Merged

brave: add update script #108029

merged 2 commits into from Jan 1, 2021

Conversation

danieldk
Copy link
Contributor

Motivation for this change

The Brave package often seems to get very outdated. This is bad for a
browser, where vulnerabilities are high impact.

This change adds an update script, so that r-ryantm will suggest
updates. We find the latest version using their Debian package
database (since we are using the Debian package anyway).

While at it, use the update script to update Brave ;):

brave: 1.18.75 -> 1.18.77

Changelog:
https://community.brave.com/t/release-channel-1-18-77/191009

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@danieldk
brave: add update script
4d9d826 
The Brave package often seems to get very outdated. This is bad for a
browser, where vulnerabilities are high impact.

This change adds an update script, so that r-ryantm will suggest
updates. We find the latest version using their Debian package
database (since we are using the Debian package anyway).
@danieldk
brave: 1.18.75 -> 1.18.77
e2a9d90 
Changelog:
https://community.brave.com/t/release-channel-1-18-77/191009
@ofborg ofborg bot requested review from nasirhm, rht and uskudnik December 31, 2020 10:22
@bbjubjub2494
Copy link
Member

bbjubjub2494 commented Dec 31, 2020 •
edited

Great idea, I knew we needed such a script but I didn't know how to do it. I ran the script, my results are consistent with yours

A quick suggestion from my experience looking at a few brave PRs: the list of shared objects has to be maintained by hand. An easy way to know if it's good is to just run brave --version: if there's something missing it will crash, otherwise it just exits. I believe ofborg runs installCheck; we could put it in there so that we know immediately if it's good or not.

Capture d’écran de 2020-12-31 12-39-21

Edit: whoops, this isn't well cropped.

@bbjubjub2494
Copy link
Member

fleshed out my suggestion:

  doInstallCheck = true;
  installCheckPhase = ''
    # call directly because upstream wrapper suppresses errors
    $out/opt/brave.com/brave/brave --version
  '';

If you intentionally mess up the rpath:

/nix/store/z378c0l156zyvj91fq3d79rr30xsb91n-brave-1.18.77/opt/brave.com/brave/brave: error while loading shared libraries: libgtk-3.so.0: cannot open shared object file: No such file or directory

Otherwise, it passes

@danieldk
Copy link
Contributor Author

fleshed out my suggestion:

  doInstallCheck = true;
  installCheckPhase = ''
    # call directly because upstream wrapper suppresses errors
    $out/opt/brave.com/brave/brave --version
  '';

If you intentionally mess up the rpath:

/nix/store/z378c0l156zyvj91fq3d79rr30xsb91n-brave-1.18.77/opt/brave.com/brave/brave: error while loading shared libraries: libgtk-3.so.0: cannot open shared object file: No such file or directory

Otherwise, it passes

Sounds good! Could you open a PR for this?

nasirhm
nasirhm approved these changes Dec 31, 2020
View reviewed changes
Copy link
Contributor

@nasirhm nasirhm left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you for working on it @danieldk .

@bbjubjub2494 bbjubjub2494 mentioned this pull request Dec 31, 2020
Merged
10 tasks
@danieldk danieldk merged commit 4df737c into NixOS:master Jan 1, 2021
@danieldk danieldk deleted the brave-update-script branch January 1, 2021 09:06
@danieldk
Copy link
Contributor Author

danieldk commented Jan 1, 2021

Thanks for the reviews! 👍

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

@otavio otavio otavio approved these changes

@bbjubjub2494 bbjubjub2494 bbjubjub2494 approved these changes

@nasirhm nasirhm nasirhm approved these changes

@rht rht Awaiting requested review from rht

@uskudnik uskudnik Awaiting requested review from uskudnik

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@danieldk @bbjubjub2494 @otavio @nasirhm