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

firefox: add support for firefox derivatives #5128

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

brckd
Copy link
Contributor

@brckd brckd commented Mar 13, 2024

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. Package not yet compatible. #4926 would be resolved if fixed.
  • Change is backwards compatible.

  • Code formatted with ./format.

  • 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.

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

  • 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

    • 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

@tomodachi94

This comment has been minimized.

@brckd brckd force-pushed the firefox-forks branch 3 times, most recently from 475b7c9 to 3d0fda0 Compare March 13, 2024 19:18
@brckd
Copy link
Contributor Author

brckd commented Mar 13, 2024

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
Copy link
Contributor Author

brckd commented Mar 13, 2024

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

@brckd
Copy link
Contributor Author

brckd commented Mar 15, 2024

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 brckd mentioned this pull request Mar 15, 2024
6 tasks
@brckd brckd force-pushed the firefox-forks branch 5 times, most recently from a643528 to e850cc5 Compare March 16, 2024 15:41
@brckd
Copy link
Contributor Author

brckd commented Mar 16, 2024

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

@brckd brckd marked this pull request as ready for review March 16, 2024 18:31
@brckd
Copy link
Contributor Author

brckd commented Mar 16, 2024

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?

@tomodachi94
Copy link
Contributor

Any hard blockers that are preventing this from being merged?

@brckd
Copy link
Contributor Author

brckd commented Apr 9, 2024

A maintainer needs to review it.

@rycee
Copy link
Member

rycee commented Apr 28, 2024

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).

@brckd
Copy link
Contributor Author

brckd commented Apr 30, 2024

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
Copy link
Contributor Author

brckd commented Jun 15, 2024

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.

Adds support for Firefox forks by introducing methods that create
generic configs and options. Additional configs and options can be
added in separate modules.
@brckd
Copy link
Contributor Author

brckd commented Jun 26, 2024

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 #5128 (comment) there is still some refactoring that could be done, like minimizing the diff.

@brckd
Copy link
Contributor Author

brckd commented Jun 30, 2024

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 brckd requested a review from rycee June 30, 2024 19:37
@kkoniuszy
Copy link
Contributor

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.

kkoniuszy@7752389

@brckd
Copy link
Contributor Author

brckd commented Jul 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants