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

zen-browser: init at 1.0.1-a.22 #347222

Merged
merged 2 commits into from
Nov 30, 2024
Merged

zen-browser: init at 1.0.1-a.22 #347222

merged 2 commits into from
Nov 30, 2024

Conversation

matthewpi
Copy link
Member

@matthewpi matthewpi commented Oct 8, 2024

https://zen-browser.app/

Closes #327982

NOTE: This package takes quite a lot of resources to build. On my desktop (7900X) and on my build server (5950X) the build takes upwards of about 30-35 minutes. This is not a package you want to compile yourself if you can avoid it.

If anyone wants to be added as a maintainer to this package, please leave a comment and I will add you.

@Zh40Le1ZOOB thanks for doing the legwork to get this packaged. I based this package on the one you linked in the associated package request, but merged in a bunch of the options from the firefox package already in nixpkgs.

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.11 Release Notes (or backporting 23.11 and 24.05 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.

@matthewpi matthewpi force-pushed the zen-browser branch 4 times, most recently from 4484966 to ddda268 Compare October 8, 2024 16:31
@Zh40Le1ZOOB
Copy link

If anyone wants to be added as a maintainer to this package, please leave a comment and I will add you.

Could you please add me as a maintainer? I'm really happy I could be a maintainer in Nixpkgs :D

@Titaniumtown
Copy link
Contributor

I too would like to be added as a maintainer for this package if possible :)

@matthewpi
Copy link
Member Author

If anyone wants to be added as a maintainer to this package, please leave a comment and I will add you.

Could you please add me as a maintainer? I'm really happy I could be a maintainer in Nixpkgs :D

In order to be added as a maintainer you need to have an entry in maintainers/maintainer-list.nix.

I'm not sure if I'm supposed to add you as nixpkgs would likely prefer users add themselves rather than getting someone else to do it for them.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Oct 8, 2024
@Titaniumtown
Copy link
Contributor

After ~2 hours on my laptop, it built. Typing from the browser right now! Seems functionally it works fine :)

@ofborg ofborg bot requested a review from Titaniumtown October 8, 2024 22:24
@Vinetos
Copy link
Member

Vinetos commented Oct 9, 2024

Hey !
I'm currently building the package on ~5 y/o laptop
I'll use the browser during this day looking for any bugs

Could I be added as a maintainer too ?
I would like to contribute to package and maintain this browser :D

@Zh40Le1ZOOB
Copy link

If anyone wants to be added as a maintainer to this package, please leave a comment and I will add you.

Could you please add me as a maintainer? I'm really happy I could be a maintainer in Nixpkgs :D

In order to be added as a maintainer you need to have an entry in maintainers/maintainer-list.nix.

I'm not sure if I'm supposed to add you as nixpkgs would likely prefer users add themselves rather than getting someone else to do it for them.

Is it allowed to add others to maintainers list in a pull request? Or may I add my self at next update? 🤔

@Vinetos
Copy link
Member

Vinetos commented Oct 9, 2024

If anyone wants to be added as a maintainer to this package, please leave a comment and I will add you.

Could you please add me as a maintainer? I'm really happy I could be a maintainer in Nixpkgs :D

In order to be added as a maintainer you need to have an entry in maintainers/maintainer-list.nix.
I'm not sure if I'm supposed to add you as nixpkgs would likely prefer users add themselves rather than getting someone else to do it for them.

Is it allowed to add others to maintainers list in a pull request? Or may I add my self at next update? 🤔

It is preferable for users to add themselves. At least, the commit must be authored and signed by the added user.

If @matthewpi gives you a write access to its fork's branch, you would be able to add yourself and the change will be in this MR.

@Vinetos
Copy link
Member

Vinetos commented Oct 9, 2024

Hey
I seems to have a build failure :

➜ nix run nixpkgs#nixpkgs-review -- pr --print-result 347222 --no-shell
[...]

[1/0/2 built] building zen-browser-unwrapped-1.0.1-a.7 (buildPhase): 00:22:21 Javascript error: chome://browser/content/parent/ext-browser.js, line: TypeError: currentTab.linkedBrowser is null 

Does someone got the same ? The thing is the build seems to be stuck forever in this state

@matthewpi
Copy link
Member Author

Hey I seems to have a build failure :

➜ nix run nixpkgs#nixpkgs-review -- pr --print-result 347222 --no-shell
[...]

[1/0/2 built] building zen-browser-unwrapped-1.0.1-a.7 (buildPhase): 00:22:21 Javascript error: chome://browser/content/parent/ext-browser.js, line: TypeError: currentTab.linkedBrowser is null 

Does someone got the same ? The thing is the build seems to be stuck forever in this state

How powerful is the system you are building this on? It's not uncommon for the output to become "stuck" and not print anything for awhile, especially on lower-end systems.

@Vinetos
Copy link
Member

Vinetos commented Oct 9, 2024

Hey I seems to have a build failure :

➜ nix run nixpkgs#nixpkgs-review -- pr --print-result 347222 --no-shell
[...]

[1/0/2 built] building zen-browser-unwrapped-1.0.1-a.7 (buildPhase): 00:22:21 Javascript error: chome://browser/content/parent/ext-browser.js, line: TypeError: currentTab.linkedBrowser is null 

Does someone got the same ? The thing is the build seems to be stuck forever in this state

How powerful is the system you are building this on? It's not uncommon for the output to become "stuck" and not print anything for awhile, especially on lower-end systems.

Well, I am not really on lower-end gear (Ryzen 9 5900X, 32 Go of RAM) but good news, the build is continuing.
I have waited something like 10 mins (wtf was the build doing) and now its continuing

1 package built:
zen-browser-unwrapped

Result of nixpkgs-review pr 347222 run on x86_64-linux 1

1 package built:
  • zen-browser-unwrapped

@Titaniumtown
Copy link
Contributor

Hey I seems to have a build failure :

➜ nix run nixpkgs#nixpkgs-review -- pr --print-result 347222 --no-shell
[...]

[1/0/2 built] building zen-browser-unwrapped-1.0.1-a.7 (buildPhase): 00:22:21 Javascript error: chome://browser/content/parent/ext-browser.js, line: TypeError: currentTab.linkedBrowser is null 

Does someone got the same ? The thing is the build seems to be stuck forever in this state

That is not a build failure. The build continues and succeeds. I saw the same log message, but it had no effect on the success of the build.

@Titaniumtown
Copy link
Contributor

New release pushed with critical security fixes (from upstream firefox): https://github.com/zen-browser/desktop/releases/tag/1.0.1-a.8

@Eveeifyeve
Copy link
Contributor

Eveeifyeve commented Nov 30, 2024

That still leave the issue of why or fixed the fact that the closure size of llvm & approvals from earlier.

@winterqt
Copy link
Member

After further looking into surfer.json file, There is no issues here what so ever in latest zen browser as firefox was updated 133.0 from 132.0.2 which is the latest version, so I see no security problems. If you could clarify on the term Zen Browser v1.0.1-a.22 actually rolls back to v1.0.1-a.19 @winterqt that would be great.

It was updated in HEAD, but not in the currently released version. Please see here and here.

@Titaniumtown
Copy link
Contributor

No reason for this not to be merged

There's actually quite a few reasons this definitely was not ready to be merged:

1. The two approvals from committers have not been approvals, and have instead been [deferrals to other committers](https://github.com/NixOS/nixpkgs/pull/347222#pullrequestreview-2469113111), or a ["let's wait for upstream to stabilize."](https://github.com/NixOS/nixpkgs/pull/347222#pullrequestreview-2470357995)

2. Zen Browser v1.0.1-a.22 actually rolls back to v1.0.1-a.19, which is [FF 132.0.1](https://github.com/zen-browser/desktop/tree/1.0.1-a.22?tab=readme-ov-file&rgh-link-date=2024-11-30T03%3A44%3A52Z#%EF%B8%8F-compatibility), a version which is [not supported upstream](https://endoflife.date/firefox), and [vulnerable to multiple CVEs](https://www.mozilla.org/en-US/security/advisories/mfsa2024-63/). **We are not shipping a browser that has known vulnerabilities.** If a browser fork is this irresponsible with their release process (132.0.1 was known vulnerable when they rolled back to it, and it's not even the latest point release of 132.0), I do not feel confident in their security handling policy enough for us to package this to our users.

3. Nobody investigated why or fixed the fact that the [closure size has 5 GiB of LLVM](https://github.com/NixOS/nixpkgs/pull/347222#issuecomment-2496976507) in it.

I understand that a lot of people really want to see this in Nixpkgs, but it's downright irresponsible for us to ship this to our users. I think it would be best for this to be packaged out-of-tree for now.

ok, fair criticism. we'll see how this plays out.

@AyushmanOfficial
Copy link

I'm new here, so please excuse me. If a package is accepted, will it be added to the stable channel? Or is that something else entirely?

@HeitorAugustoLN
Copy link
Member

@AyushmanOfficial it can be backported to the stable channel, after merged.

@Eveeifyeve

This comment was marked as duplicate.

@dustsucker
Copy link

Why is this merged but not in unstable? Or Rather, why was it reverted in #360291 ?

@Ari-43
Copy link

Ari-43 commented Dec 1, 2024

Why is this merged but not in unstable? Or Rather, why was it reverted in #360291 ?

See this comment

@Titaniumtown
Copy link
Contributor

@matthewpi have you made another PR to follow this one up?

@brianmay
Copy link
Contributor

brianmay commented Dec 2, 2024

It sounds like we have to wait for a new release of Zen that addresses the 2nd point.

To be fair to the upstream they do label it as "alpha version". I think it is understandable that their releases might have known security issues. But maybe somebody should provide some feedback all the same - if that is possible. Maybe reverting back to an old Firefox version in order to reduce breakage at the expense of re-introducing old security issues wasn't the best decision...

@Eveeifyeve
Copy link
Contributor

Eveeifyeve commented Dec 8, 2024

Hey they are releasing a beta version tomorrow lets wait for that, then we can make a new pr.
I will also help out trying to solve the llvm size of 2gib, which could become an issue for hydra.
As well support help out to support darwin when my laptop is fixed.

@matthewpi

@Eveeifyeve
Copy link
Contributor

Eveeifyeve commented Dec 10, 2024

Zen released a new version (aka beta) and I am working on a new pr, and Darwin support will be becoming soon.

@nicball
Copy link

nicball commented Dec 10, 2024

On the topic of reducing closure size, the file chrome/toolkit/content/global/buildconfig.html inside onmi.ja contains paths to clang and rustc. Seems that's the only appearance. Can we get rid of it somehow?

@BlueFox1616
Copy link

The zen beta has been released btw

@Eveeifyeve
Copy link
Contributor

On the topic of reducing closure size, the file chrome/toolkit/content/global/buildconfig.html inside onmi.ja contains paths to clang and rustc. Seems that's the only appearance. Can we get rid of it somehow?

Maybe, please mention a way I could do that, because I can only think is by doing a patch.

@winterqt
Copy link
Member

While I appreciate the effort to fix the issues mentioned here, it is unlikely that this will be merged in the near future, so I would recommend maybe holding off on this for now (or contributing your efforts to an out-of-tree package).

@Eveeifyeve Eveeifyeve mentioned this pull request Dec 10, 2024
13 tasks
@Noiidor
Copy link

Noiidor commented Dec 10, 2024

While I appreciate the effort to fix the issues mentioned here, it is unlikely that this will be merged in the near future, so I would recommend maybe holding off on this for now (or contributing your efforts to an out-of-tree package).

May i ask why? From what i understand, the main reason is security issues. Isn't almost every piece of software have some issues in terms of security? I'm pretty sure that many people will rather choose to have a package faster with some vulnerabilities, then not have a package and wait an uncertain amount of time for upstream fix. I may be completely wrong, please explain it to me.

@JulesdeCube
Copy link
Contributor

JulesdeCube commented Dec 10, 2024

While I appreciate the effort to fix the issues mentioned here, it is unlikely that this will be merged in the near future, so I would recommend maybe holding off on this for now (or contributing your efforts to an out-of-tree package).

May i ask why? From what i understand, the main reason is security issues. Isn't almost every piece of software have some issues in terms of security? I'm pretty sure that many people will rather choose to have a package faster with some vulnerabilities, then not have a package and wait an uncertain amount of time for upstream fix. I may be completely wrong, please explain it to me.

Every software have security issue. It's true that they exists but they are not discover. here we have well known security issues (see Mozilla Foundation Security Advisory 2024-63) and the editor recommendation is to update the software.

Furthermore when you download/install software you except it not to be a security issue you and don't have to make some research to check that the program is vulnerable. Also the program can be used as dependencies of an other one. If you need to check all the package and derivation of your system don't use NixOS, use Gentoo and build all your package from source.

Here we are speaking of a program that is always open and has a huge attack surface so no It's not a good idea to publish that. In case you want the derivation build it your self (nix run github:Eveeifyeve/nixpkgs/zen-browser#zen-browser).

To finish Nix and NixOS are used in production environment where you definitively don't want to have vulnerable/broken packages.

@Aleksanaa
Copy link
Member

Maybe, please mention a way I could do that, because I can only think is by doing a patch.

https://nixos.org/manual/nixpkgs/unstable/#fun-remove-references-to

@Eveeifyeve
Copy link
Contributor

May i ask why? From what i understand, the main reason is security issues. Isn't almost every piece of software have some issues in terms of security? I'm pretty sure that many people will rather choose to have a package faster with some vulnerabilities, then not have a package and wait an uncertain amount of time for upstream fix. I may be completely wrong, please explain it to me.

It's not just security issues it also includes major breaking changes, until it's stable the solution is build from my pr.

Every software have security issue. It's true that they exists but they are not discover. here we have well known security issues (see Mozilla Foundation Security Advisory 2024-63) and the editor recommendation is to update the software.

Yes this is true, but for most browsers that people are using, require it to be up to date and with no vulnerabilities.

Furthermore when you download/install software you except it not to be a security issue you and don't have to make some research to check that the program is vulnerable. Also the program can be used as dependencies of an other one. If you need to check all the package and derivation of your system don't use NixOS, use Gentoo and build all your package from source.

At the moment it is because it's in beta not fully stable and it may miss a FF version that patches FF vulnerabilities or changes the way of the package.

Here we are speaking of a program that is always open and has a huge attack surface so no It's not a good idea to publish that. In case you want the derivation build it your self (nix run github:Eveeifyeve/nixpkgs/zen-browser#zen-browser).

Or add it to your channels or add it to your flake.

@Noiidor
Copy link

Noiidor commented Dec 11, 2024

While I appreciate the effort to fix the issues mentioned here, it is unlikely that this will be merged in the near future, so I would recommend maybe holding off on this for now (or contributing your efforts to an out-of-tree package).

May i ask why? From what i understand, the main reason is security issues. Isn't almost every piece of software have some issues in terms of security? I'm pretty sure that many people will rather choose to have a package faster with some vulnerabilities, then not have a package and wait an uncertain amount of time for upstream fix. I may be completely wrong, please explain it to me.

Every software have security issue. It's true that they exists but they are not discover. here we have well known security issues (see Mozilla Foundation Security Advisory 2024-63) and the editor recommendation is to update the software.

Furthermore when you download/install software you except it not to be a security issue you and don't have to make some research to check that the program is vulnerable. Also the program can be used as dependencies of an other one. If you need to check all the package and derivation of your system don't use NixOS, use Gentoo and build all your package from source.

Here we are speaking of a program that is always open and has a huge attack surface so no It's not a good idea to publish that. In case you want the derivation build it your self (nix run github:Eveeifyeve/nixpkgs/zen-browser#zen-browser).

To finish Nix and NixOS are used in production environment where you definitively don't want to have vulnerable/broken packages.

That's completely valid reason, thanks everyone above for clarification! Also thanks for the tip about building derivations as separate input. Does this mean that no cache(like Cachix for Hyprland) for this package is available and it should be built on my machine?

@youwen5
Copy link
Member

youwen5 commented Dec 11, 2024

Well, there would be no binary cache (Cachix or otherwise) unless someone makes a cache and pushes artifacts to it.

Shameless plug but you can use my flake youwen5/zen-browser-flake to run Zen Browser on x86_64-linux and aarch64-linux. It fetches, patches, and wraps the upstream binaries released by Zen Browser to run on NixOS, so it won't use up a ton of resources compiling from source. Also, I configured GitHub Actions to automatically keep it up to date with the latest upstream Zen releases.

@Luk45135
Copy link

Im a bit confused why is the security thing still a topic? In the beta its based on FF 133. And even if, we could just mark it as insecure. And why is stability a topic either i mean we have cosmic-comp in the main repo and that still in alpha 2.

@Mikilio
Copy link
Contributor

Mikilio commented Dec 11, 2024

It seems that firefox forks cost a lot of resources to build and pop in and out of existence too quickly, so the members recently decided to be stricter with the requirements to accepting such a package. This package in particular uses even more resources than expected. It's a bummer, but infrastructure is not free.

I'm not a member though, so don't cite me on this.

@Luk45135
Copy link

Oh, ok interesting, that makes sense. And i guess people are not that big of a fan of making -bin packages.

@arunoruto
Copy link
Contributor

Oh, ok interesting, that makes sense. And i guess people are not that big of a fan of making -bin packages.

Sometimes you will see both the build package and a -bin version of the package (like with jaxlib).

Maybe there is a potential solution so everyone would be happy with: offload the package building & nix packaging onto the zen-browser team. Create a flake on their repo, which packages the browser, so folks interested in it, can get it from there. I am using this to get nightly builds from helix and wezterm, and those projects go even further and offer cached builds on cachix! If the zen team isn't keen on offering their resources to build this for nix again, one could also package their binary release for now :)

The only thing needed to do, is to convince the zen team, to incorporate the flake into their project (with all of the packaging stuff around it), and have some ppl be willing to maintain it if it breaks, but it seems that there are a few folks eagirly working on it!

And yeah, I am aware of zen-browser/desktop#78, but maybe they will consider it again. Or plan B, someone offers a repo, like the one from @Eveeifyeve :)

@Mikilio
Copy link
Contributor

Mikilio commented Dec 11, 2024

Someone did literally that #347222 (comment)

@Luk45135
Copy link

Yeah i know im using it right now. I just thought that building from source would be more nix style, especially in the main repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package request: Zen Browser