Firefox extensions discussion

[eyJhb]

Now that #91724 is merged, it would be nice to have a set of Firefox extensions, that one could easily pick from.

Some aspects have been discussed in the PR already, but making a separate issue seems better. The main reason for packaging is (https://github.com/NixOS/nixpkgs/pull/91724#issuecomment-737768399)

They're some of the most security critical and most used programs, I think packaging them would be a good idea.

We could make it a part of nixpkgs, e.g. nixpkgs.firefoxExtensions...., but there some things to keep in mind.

1. Automatically updating them some way ( e.g. like rycee does here https://gitlab.com/rycee/nur-expressions/-/tree/master/pkgs/firefox-addons )
2. Backporting to stable
3. Channels that hang (mostly unstable - security risk, but this is a general thing...)
4. Will create a lot of PRs? One each day or something maybe.

A solution could be, to have it as a separate repository.

[Mic92]

What if this repo was independent from nixpkgs. Than we would not require to backport updates. Would this not be easier? Than we also would not need to be worried about expression sizes since it does not make nixpkgs bigger.

[eyJhb]

The only "issue" is see with that, is that we move away from the Nixpkgs "menorepo" structure. But I do agree, that it seems like the best solution to do it like that.

However, how would users use it? Because if they need to pin it in their configs or something, then it will quickly become outdated. I think most users just want to run a nix-channels --update, and then do a rebuild. Having to recalculate a hash of a fetchFromGithub seems tedious for some.

[Mic92]

The only "issue" is see with that, is that we move away from the Nixpkgs "menorepo" structure. But I do agree, that it seems like the best solution to do it like that.

However, how would users use it? Because if they need to pin it in their configs or something, then it will quickly become outdated. I think most users just want to run a nix-channels --update, and then do a rebuild. Having to recalculate a hash of a fetchFromGithub seems tedious for some.

There multiple ways: nix-channel, niv or nix flakes. Also see https://github.com/NixOS/nixos-hardware/#setup

[Qubasa]

I am not sure if an auto update mechanism is needed. Brower addons in itself very rarely a security issue more often then not updating them automatically is the issue (because addon owner sold the right to marketing firms etc.). I would keep this simple and just let the user update it

[eyJhb]

The only "issue" is see with that, is that we move away from the Nixpkgs "menorepo" structure. But I do agree, that it seems like the best solution to do it like that. However, how would users use it? Because if they need to pin it in their configs or something, then it will quickly become outdated. I think most users just want to run a nix-channels --update, and then do a rebuild. Having to recalculate a hash of a fetchFromGithub seems tedious for some.

There multiple ways: nix-channel, niv or nix flakes. Also see https://github.com/NixOS/nixos-hardware/#setup

For a moment I forget how channels works, sorry!

I am not sure if an auto update mechanism is needed. Brower addons in itself very rarely a security issue more often then not updating them automatically is the issue (because addon owner sold the right to marketing firms etc.). I would keep this simple and just let the user update it

Do you then propose to keep them in nixpkgs? Or keep it away from nixpkgs in general, and let the user add them themselves?

EDIT: use the example that you gave in the PR

[Qubasa]

I would keep it away from nixpkgs in general, and let the user add them themselves. Or just having the most needed ones in nixpkgs even if they update slowly it's not really an issue.

[Atemu]

Split repos are a cool thing but, until Flakes are out of beta, they are a pain to deal with and having them in Nixpkgs would be more convenient in any case.

If the user was on stable and wanted fast updating addons, they could just use a fresher source like nixos-unstable or any separate, fresher repo.

I don't think it'd hurt to include addons in Nixpkgs.

Another consideration I'd like to add is where addons are fetched from. Should we just download the xpis from addons.mozilla.org or should we try to build from source (or both?).

[Qubasa]

I mean the building is just getting the source zipped and you are done. As often times the source code is only available through addons.mozilla.org I would just fetch it from there

[bqv]

nix-community/emacs-overlay has existed quite happily for a long time with almost no issues. I don't see how this would be different

[edolstra]

Nixpkgs is already too big. I would put this in a separate flake.

[eyJhb]

Also somewhat what I thought, it might got too big really fast. Can we then place it in a repo in nix-community? Not sure who has admin right on this/needs to accept this, @bqv ?

In the future we should be able to do it for Chrome as well! :)

[Atemu]

Nixpkgs is already too big. I would put this in a separate flake.

Splitting Nixpkgs subsets off into flakes is something I'd like to see too but flakes aren't even in stable Nix yet (much less something most Nix users have sufficient experience with) and I think such a decision should be made for all such package subsets, not just this one.

We can always spin this one out again once we have come to a consensus on how splitting Nixpkgs should be done but I don't think we should block on that.

Besides, even if we do end up deciding to put it into a flake instead of Nixpkgs, nearly all of the work done should be trivially portable.

[7c6f434c]

Splitting Nixpkgs subsets off into flakes is something I'd like to see too but flakes aren't even in stable Nix yet (much less something most Nix users have sufficient experience with) and I think such a decision should be made for all such package subsets, not just this one.

While I expect splitting Nixpkgs into flakes to cause problems, this is exactly the package set that will probably work fine even as a non-flake separate repo because the dependency is just Firefox, and the extension version compatibility behaviour is better than a typical language-package-set native-dependency compatibility behaviour.

[Mic92]

Also somewhat what I thought, it might got too big really fast. Can we then place it in a repo in nix-community? Not sure who has admin right on this/needs to accept this, @bqv ?

In the future we should be able to do it for Chrome as well! :)

Anyone in the nix-community org, can move repos there. I can invite anyone to become a member there and also do other administrative tasks if necessary.

[Mic92]

Nixpkgs is already too big. I would put this in a separate flake.

Splitting Nixpkgs subsets off into flakes is something I'd like to see too but flakes aren't even in stable Nix yet (much less something most Nix users have sufficient experience with) and I think such a decision should be made for all such package subsets, not just this one.

I find Niv quite easy to use in this case.

[beardhatcode]

I have some concerns regarding the preservation of settings. The fetchFirefoxAddon function generates a new extid depending on the hash of the extension url. I don't think Firefox will show the settings of abcdef@myext to qrstu@myext.

My suggestion would be to use the extid from the manifest and complain if there are duplicates in it. Alternatively allow specifying the extid.

[doronbehar]

I haven't read the whole thread, but I just updated my system and what happened??

I manage my firefox extensions imperatively and I'm fine with that! I don't want to bring Nix into every aspect of my life :sob:.

[bbigras]

@doronbehar maybe check https://github.com/NixOS/nixpkgs/pull/105796

[doronbehar]

Thanks. That mistake was rather disastrous in terms of my time wasted because with that commit merged to my channels I can install extensions but all of them got removed and now I have to give them all of the settings I had before :angry: .

[Qubasa]

I have some concerns regarding the preservation of settings. The fetchFirefoxAddon function generates a new extid depending on the hash of the extension url. I don't think firefox will show the settings of abcdef@myext to qrstu@myext.

My suggestion would be to use the extid from the manifest and complain if there are duplicates in it. Alternatively allow specifying the extid.

I opened up a pull request for this. Getting the extid from the addon is very complicated I would just stick to a fixed but unique name given by the user. https://github.com/NixOS/nixpkgs/pull/106000

[wiltaylor]

Yeah any chance we can have this as a separate Firefox package? So you have the main line one behaves like it currently does and we have a FirefoxWithAddons or something? I really don't want to waste a heap of my weekend reinstalling and re configuring my Firefox plugins again because it wasn't tested properly.

[archseer]

Thanks. That mistake was rather disastrous in terms of my time wasted because with that commit merged to my channels I can install extensions but all of them got removed and now I have to give them all of the settings I had before 😠 .

Even worse, the setting somehow propagated to my other machines via Firefox Sync so I got to reconfigure all the extensions I haven't touched in years.

[andir]

Until yesterdays PR that fixed the outbreak I wasn't even aware of the PR or else I'd have commented earlier.

I pretty much like the prospect of having the ability to declare my firefox extensions and have them sync'ed between all my devices without requiring firefox sync (or similar impurities).

My feedback here is not to discourage the contributor frrom further changes but just reflects my thoughts on the whole issue.

While overall positive with the idea the change looks very intrusive and is probably a hack (copying the binaries around…) and should probably have receive proper review on the method used. The PR that introduced the breakage didn't remotely comply with our contribution guidelines (just look at the commits…). Which should be the lower end of the bar. I welcome the added documentation to the newly added options but that doesn't guarantee any kind of release quality.

Modifying the Firefox code (and to that extend the wrapper) always deals with valuable state of the user. Regardless of the fact that this is on unstable or not we should take better precautions on these cases. The worst that can happen with a Firefox bump is most likely loosing the entire profile Thankfully we only lost plugins and their configuration with this change.

In the previous round of this change I commented with a request for tests (https://github.com/NixOS/nixpkgs/pull/74297#issuecomment-559240748) as the wrapper script is gaining in complexity with these kinds of hacks being applied. As the contributor stated that he didn't have the time back then and the PR went stale after a while I eventually forgot about the initiative. This time around I'd have appreciated a ping on the PR as the feedback would have probably been the same. We need to properly ensure we aren't screwing around with users data. The API surface of the wrapper script has laregly grown evolutionary and probably needs a make over. I am not asking this from the contributor of the latest change but having been in the know about this change would have ensured more active feedback from those that have been caring about Firefox for >3y now.

So lets please take a learning from this and be more careful with merging. Having to wait months for changes to go in because of formalities might sound stupid and is tiresome but IMHO proper reviews and pinging the right maintainers should be warranted.

The tip of the iceberg is discussing making things a flake while flakes aren't even beta quality software or standardized at all... If we want to be taken seriously we should probably not do such ad-hoc decisions that are incompatible with most of the users?

[Qubasa]

Until yesterdays PR that fixed the outbreak I wasn't even aware of the PR or else I'd have commented earlier.

I pretty much like the prospect of having the ability to declare my firefox extensions and have them sync'ed between all my devices without requiring firefox sync (or similar impurities).

My feedback here is not to discourage the contributor frrom further changes but just reflects my thoughts on the whole issue.

While overall positive with the idea the change looks very intrusive and is probably a hack (copying the binaries around…) and should probably have receive proper review on the method used. The PR that introduced the breakage didn't remotely comply with our contribution guidelines (just look at the commits…). Which should be the lower end of the bar. I welcome the added documentation to the newly added options but that doesn't guarantee any kind of release quality.

Modifying the Firefox code (and to that extend the wrapper) always deals with valuable state of the user. Regardless of the fact that this is on unstable or not we should take better precautions on these cases. The worst that can happen with a Firefox bump is most likely loosing the entire profile Thankfully we only lost plugins and their configuration with this change.

In the previous round of this change I commented with a request for tests (#74297 (comment)) as the wrapper script is gaining in complexity with these kinds of hacks being applied. As the contributor stated that he didn't have the time back then and the PR went stale after a while I eventually forgot about the initiative. This time around I'd have appreciated a ping on the PR as the feedback would have probably been the same. We need to properly ensure we aren't screwing around with users data. The API surface of the wrapper script has laregly grown evolutionary and probably needs a make over. I am not asking this from the contributor of the latest change but having been in the know about this change would have ensured more active feedback from those that have been caring about Firefox for >3y now.

So lets please take a learning from this and be more careful with merging. Having to wait months for changes to go in because of formalities might sound stupid and is tiresome but IMHO proper reviews and pinging the right maintainers should be warranted.

The tip of the iceberg is discussing making things a flake while flakes aren't even beta quality software or standardized at all... If we want to be taken seriously we should probably not do such ad-hoc decisions that are incompatible with most of the users?

Ah yes great idea asking for a testing harness. The only problem with it is that writing and maintaining it is hackier and more work then the Firefox wrapper implementation itself. Also I am intrigued of your definition of a hack, because copying executables and linking configuration into the right places is how NixOS works. Fact is, there is a reason nobody till now wrote a Firefox addon wrapper, because it's a ton of work. Was there a slip up on my part and on part of the reviewers? Yes and I'm sorry for the inconvenience it brought to people on unstable, I did my utmost to quickly address the issue, it certainly won't happen again. But also... it's always easy to point fingers from a safe distance and demanding a testing harness .

[bqv]

Honestly, I think its understandable for firefox, but for other general and less popular packages, I would not like to encourage dragging out PRs unnecesarily. Looking at the number of abandoned and stale PRs, the constant burnouts of PR authors, and hearing of other people who like me are entirely and absolutely disincentivised to contribute tangibly to nixpkgs, that's something that should be getting better, not worse.

I do happen to believe leaning on flakes will lead to that too, but I'm sure most of you are familiar with my opinions on both matters.

[7c6f434c]

The only problem with it is that writing and maintaining it is hackier and more work then the Firefox wrapper implementation itself.

(The real problem being that we do not have Nixpkgs-wide solution that is not as heavy-weight as NixOS tests but is capable enough to write a Firefox test… I am almost sure that my profile generation functions are close to being sufficient to double as tests, and I am also almost sure that they will be rejected due to duplicating functionality or something if I do try to convert them into hacky tests, has happenned last time I tried to submit medium-weight tests for GUI packages)

Also I am intrigued of your definition of a hack, because copying executables and linking configuration into the right places is how NixOS works.

NixOS is all about symlinks, actually copying an executable is rare. So there is a difference here.

But also... it's always easy to point fingers from a safe distance and demanding a testing harness .

The underlying question is: for default behaviour of Firefox, is stagnation (without stopping to track upstream releases, of course) better than occasional state-loss changes? «Yes, stagnation is better» and «No, stagnation is worse» both have significant support.

[xaverdh]

NixOS is all about symlinks, actually copying an executable is rare. So there is a difference here.

The firefox runtime is somewhat special in this regard (for "security" reasons). You can read up on that here and the why the alternative approach was shot down here.

edit: ah I think you already read it ;-)

[taku0]

I don't think automated tests could prevent the issue for this case. Ensuring updates don't break existing profiles is hard. It is not unusual that tests are bigger than actual code. Careful reviews and manual tests could save us, though I was too late to review it. An option is enforcing two or more approving reviews for non-trivial changes. For trivial changes, a committer can attach a label and make a bot approves it automatically. It would increase stale PRs, though.

[7c6f434c]

I don't think automated tests could prevent the issue for this case.

In a sense, what was missed is the very existence of the requirement…

An option is enforcing two or more approving reviews for non-trivial changes. For trivial changes, a committer can attach a label and make a bot approves it automatically. It would increase stale PRs, though.

Maybe restricting the policy to «feature» changes and «popular» packages could slow down the rate of risky changes without burdening the part where getting any reviewers interested is the hard part.

[xaverdh]

This is getting slightly off topic, but can't we automate pinging the maintainers?

[stale[bot]]

I marked this as stale due to inactivity. → More info

[ShalokShalom]

Was there ever any solution to this?

Do we have such a repo today?