Skip to content

brave: fix overriding and update 1.64.116 -> 1.64.122#303570

Merged
wegank merged 3 commits intoNixOS:masterfrom
spitulax:brave-fix
Apr 12, 2024
Merged

brave: fix overriding and update 1.64.116 -> 1.64.122#303570
wegank merged 3 commits intoNixOS:masterfrom
spitulax:brave-fix

Conversation

@spitulax
Copy link
Copy Markdown
Contributor

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg Bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Apr 12, 2024
@wegank
Copy link
Copy Markdown
Member

wegank commented Apr 12, 2024

I'm not a fan of duplicating arguments like commandLineArgs ? "". Instead, how about this, which is a three-line change:

# Expression generated by update.sh; do not edit it by hand!
{ stdenv, callPackage, ... }@args:

if stdenv.isAarch64 then
  callPackage ./make-brave.nix (removeAttrs args [ "callPackage" ]) {
    pname = "brave";
    version = "1.64.116";
    url = "https://github.com/brave/brave-browser/releases/download/v1.64.116/brave-browser_1.64.116_arm64.deb";
    hash = "sha256-XC3GnutiTYdCOJPegj8MRYC5dRrBoKBg4k50ZFrlj4E=";
    platform = "aarch64-linux";
  }
else if stdenv.isx86_64 then
  callPackage ./make-brave.nix (removeAttrs args [ "callPackage" ]) {
    pname = "brave";
    version = "1.64.116";
    url = "https://github.com/brave/brave-browser/releases/download/v1.64.116/brave-browser_1.64.116_amd64.deb";
    hash = "sha256-mnvFPfZu44TZGdUb+AxaJbecQrXkIzJkYvB4GO55uv0=";
    platform = "x86_64-linux";
  }
else
  throw "Unsupported platform."

@spitulax
Copy link
Copy Markdown
Contributor Author

I'm not a fan of duplicating arguments like commandLineArgs ? "". Instead, how about this, which is a three-line change:

# Expression generated by update.sh; do not edit it by hand!
{ stdenv, callPackage, ... }@args:

if stdenv.isAarch64 then
  callPackage ./make-brave.nix (removeAttrs args [ "callPackage" ]) {
    pname = "brave";
    version = "1.64.116";
    url = "https://github.com/brave/brave-browser/releases/download/v1.64.116/brave-browser_1.64.116_arm64.deb";
    hash = "sha256-XC3GnutiTYdCOJPegj8MRYC5dRrBoKBg4k50ZFrlj4E=";
    platform = "aarch64-linux";
  }
else if stdenv.isx86_64 then
  callPackage ./make-brave.nix (removeAttrs args [ "callPackage" ]) {
    pname = "brave";
    version = "1.64.116";
    url = "https://github.com/brave/brave-browser/releases/download/v1.64.116/brave-browser_1.64.116_amd64.deb";
    hash = "sha256-mnvFPfZu44TZGdUb+AxaJbecQrXkIzJkYvB4GO55uv0=";
    platform = "x86_64-linux";
  }
else
  throw "Unsupported platform."

That was my first solution too. I've taken a look at chromium package source and it puts each arguments in default.nix, so I went with that. I suppose this is a better solution and I'm going to change it.

@wegank wegank merged commit ed5cb90 into NixOS:master Apr 12, 2024
@ribru17
Copy link
Copy Markdown
Contributor

ribru17 commented Apr 14, 2024

Hello, sorry to butt in, but how should something like commandLineArgs be applied after this change? Before I had:

  brave = (pkgs.brave.override {
    commandLineArgs = [ "--force-device-scale-factor=1.5" ];
  });

but now this fails to build. Sorry for the noobie question!

EDIT: Something like the following will build, but not apply the actual options:

  brave = (pkgs.brave.overrideAttrs (oldAttrs: {
    commandLineArgs = [ "--force-device-scale-factor=1.5" ];
  }));

@spitulax
Copy link
Copy Markdown
Contributor Author

Hello, sorry to butt in, but how should something like commandLineArgs be applied after this change? Before I had:

  brave = (pkgs.brave.override {
    commandLineArgs = [ "--force-device-scale-factor=1.5" ];
  });

but now this fails to build. Sorry for the noobie question!

EDIT: Something like the following will build, but not apply the actual options:

  brave = (pkgs.brave.overrideAttrs (oldAttrs: {
    commandLineArgs = [ "--force-device-scale-factor=1.5" ];
  }));

That is the problem this PR should fix, but this PR has not been merged to nixpkgs-unstable and nixos-unstable as seen in https://nixpk.gs/pr-tracker.html?pr=303570. If you're using those branches you might want to override brave with the package from the master branch until this gets merged to the other branches.

@ribru17
Copy link
Copy Markdown
Contributor

ribru17 commented Apr 14, 2024

Ah, ok that makes sense. Thank you so much!!

@spitulax spitulax deleted the brave-fix branch April 16, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants