home-manager icon indicating copy to clipboard operation
home-manager copied to clipboard

firefox: add support for firefox derivatives

Open brckd opened this issue 1 year ago • 13 comments

Adds support for Firefox derivatives.

Description

Adds support for Firefox forks by introducing methods that create generic configs and options. Additional configs and options can be added in separate modules.

Most changes are just formatting. Compare them here or with git diff -w.

Browsers that can benefit from this (Reference implementations)

Browser Changes Resolves Definition Consumption
Firefox Functionality remains unchanged. View View
Librewolf Extends the existing module. Resolves #4179, resolves #2803, closes #5004 and closes #3339. View View
Floorp New Module. Resolves #4912. View View
Mullvad Browser New Module WIP. Could resolve #4926.
  • [x] Change is backwards compatible.

  • [x] Code formatted with ./format.

  • [x] Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes. firefox-profile-settings already fails for me on the master branch, but other tests pass.

  • [x] Test cases updated/added. See example. Functionality remains the same. Generic tests added.

  • [x] Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • [x] Added myself as module maintainer. See example. Wondering if I should add myself to mkFirefoxModule despite it not being a pure module.

Maintainer CC

@rycee @kira-bruneau

brckd avatar Mar 13 '24 00:03 brckd

In theory this could work for Zotero as well, being based on some sort of Mozilla technology and having the same profiles system as Firefox and derivatives. For example, extensions are stored there, which could make declarative setup easier.

tomodachi94 avatar Mar 13 '24 01:03 tomodachi94

Edited the PR to contain fewer changes. Additional configs and options for other browsers are probably not scope of this PR and can be added once this is merged.

brckd avatar Mar 13 '24 19:03 brckd

Derived branches Librewolf and Floorp seem to work. Will add tests for derivatives soon.

brckd avatar Mar 13 '24 21:03 brckd

It would probably more elegant to keep the old module, move it to modules/programs/firefox/mkFirefoxModule.nix (or perhaps modules/lib/firefox?) and wrap it into a method that accepts config parameters. Then just import it in modules/programs/firefox/default.nix as

let mkFirefoxModule = import ./mkFirefoxModule.nix; in {
  imports = [
    mkFirefoxModule { /** configs **/ }
    # ...
  ]
}

I'll work on the test first to finish the draft though.

brckd avatar Mar 15 '24 17:03 brckd

Created the mkFirefoxModule method and added generic tests. This means the PR ticks all boxes and is ready for review! :tada:

brckd avatar Mar 16 '24 18:03 brckd

Here are some things that could still be worked on:

  • instead of passing configs to mkFirefoxModule, make them internal options
  • move generic module factories to modules/lib

What do y'all think?

brckd avatar Mar 16 '24 18:03 brckd

Any hard blockers that are preventing this from being merged?

tomodachi94 avatar Apr 09 '24 04:04 tomodachi94

A maintainer needs to review it.

brckd avatar Apr 09 '24 15:04 brckd

Thanks a lot! I'll had a quick look and overall everything looks great. I just added a few minor comments, and you'll also need to add yourself to maintainers.nix (and change maintainers.bricked to hm.maintainers.bricked).

rycee avatar Apr 28 '24 22:04 rycee

Thanks for the review! My NixOS installation is currently broken, so it'a difficult for me to test the suggested changes, but I will make sure to try them soon.

brckd avatar Apr 30 '24 10:04 brckd

Uh yea so I had like a personality crisis but I might reinstall NixOS next week and see how far I get. Sorry for letting you wait.

brckd avatar Jun 15 '24 07:06 brckd

I implemented the suggested changes and ~~waiting for the tests to finish~~ the failed test seems to be unrelated. I will be updating the linked test branches for the derivatives to confirm that the changes were sufficient. As mentioned in https://github.com/nix-community/home-manager/pull/5128#issuecomment-2002081452 there is still some refactoring that could be done, like minimizing the diff.

brckd avatar Jun 26 '24 18:06 brckd

I updated the reference implementations of derivatives and how they can be used in working configs. Librewolf and Floorp work flawlessly, but Mullvad Browser's package doesn't seem to be compatible.

brckd avatar Jun 30 '24 19:06 brckd

There's a small issue with your LibreWolf module, but that's not exactly the scope of this PR. The appName used in the disclaimer salt for the default search engine name is supposed to be LibreWolf, but it's set to Librewolf. This produces different hashes than the browser does and results in the default search engine setting not working for any custom engines. It works for the stock ones because Firefox skips checking the hash for those.

https://github.com/kkoniuszy/home-manager/commit/7752389b13efe4cb789c6481d157da8a53f58c56

kkoniuszy avatar Jul 10 '24 23:07 kkoniuszy

Thanks for pointing it out! I will make sure your suggestion will be part of a future PR after this one is merged. UPDATE: Your commit has also been rebased onto the reference implementation.

brckd avatar Jul 11 '24 20:07 brckd

The search engine stuff broke again due to some changes in librewolf 128. Looks like the name that it uses in the disclaimer is now Firefox. It can be seen in about:support under 'Application Basics -> Name'.

image

With Floorp 11.15 it matches.

image

This is obviously some bug in librewolf, but to work around I added a separate internalName option that is used specifically for the search engines: https://github.com/kkoniuszy/home-manager/commit/d98a83067c28bfbffd219a5509b2573982c2bdc2. I don't think we should bother with such a workaround though, librewolf will likely fix their branding stuff anyways and then it would have to be reverted.

kkoniuszy avatar Jul 27 '24 08:07 kkoniuszy

Hm, it looks like the app name is available in an .ini file. Maybe we could read that in an activation script and substitute it before hashing?

> cat /nix/store/j3p1hplzqr6kjh7vi0wnq81ild6lmd7c-librewolf-128.0-2/lib/librewolf/application.ini
; This file is not used. If you modify it and want the application to use
; your modifications, move it under the browser/ subdirectory and start with
; the "-app /path/to/browser/application.ini" argument.
[App]
Vendor=LibreWolf
Name=Firefox
RemotingName=librewolf
CodeName=LibreWolf

Works with Floorp and vanilla FF too

❯ cat s050ma99jb345cxzm2y555k4kiybx51h-floorp-11.15.0/lib/floorp/application.ini 
; This file is not used. If you modify it and want the application to use
; your modifications, move it under the browser/ subdirectory and start with
; the "-app /path/to/browser/application.ini" argument.
[App]
Vendor=Mozilla
Name=Floorp
RemotingName=floorp

❯ cat 9w84m956zrxh1lfiwxnkzkralmld1yfx-firefox-128.0/lib/firefox/application.ini 
; This file is not used. If you modify it and want the application to use
; your modifications, move it under the browser/ subdirectory and start with
; the "-app /path/to/browser/application.ini" argument.
[App]
Vendor=Mozilla
Name=Firefox
RemotingName=firefox

kkoniuszy avatar Jul 27 '24 08:07 kkoniuszy

I modified the script that builds search.json.mozlz4 to read the proper appName from lib/*/application.ini in the firefox package. It should finally solve this problem. https://github.com/kkoniuszy/home-manager/commit/55d53dd847ed8d1d490e9dda65625cb97e49fba9

kkoniuszy avatar Jul 27 '24 09:07 kkoniuszy

Just a question here, but why is the vendorPath and configPath separated?

Mikilio avatar Jul 27 '24 19:07 Mikilio

@Mikilio This distinction was already made before this PR, so I can only guess why the original author decided to do so. configPath like .mozilla/firefox stores browser configs. vendorPath like .mozilla on the other side isn't used by most derivatives. If it exists, it's currently currently only used for native messaging. So in terms of functionality it's not related to configPath. configPath is just conventionally a subpath of vendorPath.

brckd avatar Jul 27 '24 20:07 brckd

@Mikilio This distinction was already made before this PR, so I can only guess why the original author decided to do so. configPath like .mozilla/firefox stores browser configs. vendorPath like .mozilla on the other side isn't used by most derivatives. If it exists, it's currently currently only used for native messaging. So in terms of functionality it's not related to configPath. configPath is just conventionally a subpath of vendorPath.

Got it, in that case the Floorp module definitely needs to set this (it's currently not). It's not included in this branch, though, do you intend to merge your other branches separately?

Mikilio avatar Jul 27 '24 22:07 Mikilio

I've now rebased on latest master (taking the language packs commit into account), squashed, and merged to master. I think it is good to get this in as soon as possible to avoid further merge conflicts. So please give latest master a try and see if it all works OK.

rycee avatar Jul 28 '24 23:07 rycee

@Mikilio This distinction was already made before this PR, so I can only guess why the original author decided to do so. configPath like .mozilla/firefox stores browser configs. vendorPath like .mozilla on the other side isn't used by most derivatives. If it exists, it's currently currently only used for native messaging. So in terms of functionality it's not related to configPath. configPath is just conventionally a subpath of vendorPath.

Got it, in that case the Floorp module definitely needs to set this (it's currently not). It's not included in this branch, though, do you intend to merge your other branches separately?

I am currently on vacation, so do feel free to create your own PR! Once I am back I will create PRs for the remaining browsers.

brckd avatar Jul 30 '24 10:07 brckd