floorp: add module and fix tests for Firefox derivarives

[Mikilio]

Description

As a follow up to #5128 I decided to create a PR for the floorp module. I am adding @brckd as maintainer since he is the original creator of this module and also the maintainer of the firefox module function. I will add myself as a maintainer as soon as my standing PR #5252 gets merged as it would add me to the mainainers list. I intend to implement declarative PWA for this module in either this PR or another one depending on how long it takes to merge.

Checklist

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

• Test cases updated/added. See example.

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

Maintainer CC

[brckd]

Thanks for your PR! But does declarative PWA only work with Floorp, or can it be applied to other Firefox derivatives too? In the latter case, I would suggest you to create a separate PR modifying the Firefox module factory instead :D

[brckd]

You could add tests like in https://github.com/brckd/home-manager/blob/792757f643cedc13f02098d8ed506d82e19ec1da/tests/modules/programs/firefox/firefox.nix if you want

[Mikilio]

Thanks for your PR! But does declarative PWA only work with Floorp, or can it be applied to other Firefox derivatives too? In the latter case, I would suggest you to create a separate PR modifying the Firefox module factory instead :D

So it seems like it's generally possible, but it's not like Mozilla actually supports it. I personally found a nix way to do it, which is to just place the manifest files at their locations and create corresponding desktop entries, but it still seems very nonstandard to me and maybe not suitable for a home-manager option. I personally came to the conclusion that PWAs are not reproducible, and I am thinking of scrapping this idea entirely. If you are interested in the hacky solution, I am willing to share. Except that, since the options to start the browsers in kiosk/ssb mode are not the same, so it couldn't be done in the module factory.

[Mikilio]

After some useless pondering, I am considering to call it ready for merge after adding the tests.

[brckd]

Jup, looks good then!

[brckd]

The PWA feature could be implemented in a separate PR.

[brckd]

Thanks for adding the tests!

[Mikilio]

I think the errors has something to do with this dependency not properly resolving the outputs of the current config before testing.

[Mikilio]

I've been running this for more than a month now, and I use every option myself.

[brckd]

@Mikilio I think you forgot to import the module in modules/modules.nix!

[Mikilio]

Thanks for the hint, I did not know about that. After adding it, the tests failed for other reasons.

There were two causes for the tests failing:

1. The expected data often had the name firefox instead of floorp. I solved this with substitution.
2. The hashes for the search engines were different. As acquiring the correct hash is not reproducible, I have made a jq query that replaces the hashes with null.

Requesting review after tests succeed.

[brckd]

Oh, thanks for pointing that out! I wonder how that went unnoticed in my tests.
Since this fix may he required in other changes, please consider making this a separate PR or just mention it in the title of this one.
Something like floorp: add module and fix tests for Firefox derivarives

[brckd]

Perfect! The tests for both firefox and floorp work, and the failing test is not related, like you said.

[Mikilio]

Who can merge this?

[brckd]

Afaik, only rycee can.

[Mikilio]

I can't select him as a reviewer. Never had this before. Do I ping him?

EDIT: I'll just do this @rycee

[Seikm]

@Mikilio I was testing your floorp implementation, but the "policies" option is not working. I assume the problem is that the "policies.json" file is being placed inside lib/firefox/distribution/, and not in lib/floorp/distribution/. Same thing with the extensions folder.

I spent hours trying to fix this with override or overlays but I'm too new to nix to be able to do it, so I hope you can fix it

[Mikilio]

Good catch, I have not personally tested policies. I will fix that over the weekend and add a test.

[brckd]

My bad once again.

home-manager/modules/programs/firefox/mkFirefoxModule.nix

Line 815 in 127ccc3

programs.firefox.policies = {

should probably be moved into

home-manager/modules/programs/firefox/mkFirefoxModule.nix

Line 1015 in 127ccc3

} // setAttrByPath modulePath { finalPackage = wrapPackage cfg.package; });

[Mikilio]

Whatever the fix is. The test should have caught that. So I'd say the test needs a fix too.

[Mikilio]

So the test for policies wasn't running the test but an empty test instead. My following commit fixes that, which means the test now fails as expected. The culprit was some internal behavior of mkIf. To illustrate, I have simplified the case in nix repl:

nix-repl> pkgs = import <nixpkgs> {}

nix-repl> mkIf = pkgs.lib.mkIf

nix-repl> config = mkIf true ({a=1;}) // {b=2;}

nix-repl> :p config
{ _type = "if"; b = 2; condition = true; content = { a = 1; }; }

[Mikilio]

And as for the reason why the policies are not set, it seems the issue is with Nixpkgs. I will investigate and link my PR to nixpkgs here.

[rycee]

Sorry for the delay. I just merged #5908, which should make the flake include a Nixpkgs that contains that PR commit.

[Mikilio]

Ok, I will then sync and clean my fork.

[rycee]

@Mikilio Thanks. Unfortunately the CI is a bit unhappy, perhaps you forgot to git add a file?

error: path '/home/runner/work/home-manager/home-manager/tests/modules/programs/firefox/container-id-out-of-range.nix' does not exist

[Mikilio]

Nope, some actual things in the testing changed since then, so I need to refactor. Should not take long.

[Mikilio]

Looks like my LSP is not using the right formatter.

[rycee]

Thanks! Looks good overall, the main issue I find is that the search and bookmark tests don't work. Specifically, these commands fail

nix build -L .#test-floorp-profiles-bookmarks
nix build -L .#test-floorp-profiles-search
nix build -L .#test-firefox-profiles-bookmarks
nix build -L .#test-firefox-profiles-search

With the following patch they passed for both firefox and floorp:

diff --git a/tests/modules/programs/firefox/profiles/bookmarks/default.nix b/tests/modules/programs/firefox/profiles/bookmarks/default.nix
index 7bf508ab0..3e0e828e1 100644
--- a/tests/modules/programs/firefox/profiles/bookmarks/default.nix
+++ b/tests/modules/programs/firefox/profiles/bookmarks/default.nix
@@ -9,6 +9,12 @@ let
 
   firefoxMockOverlay = import ../../setup-firefox-mock-overlay.nix modulePath;
 
+  withName = path:
+    pkgs.substituteAll {
+      src = path;
+      name = cfg.wrappedPackageName;
+    };
+
 in {
   imports = [ firefoxMockOverlay ];
 
@@ -70,7 +76,7 @@ in {
 
       assertFileContent \
         $bookmarksUserJs \
-        ${./expected-bookmarks-user.js}
+        ${withName ./expected-bookmarks-user.js}
 
       bookmarksFile="$(sed -n \
         '/browser.bookmarks.file/ {s|^.*\(/nix/store[^"]*\).*|\1|;p}' \
@@ -78,7 +84,7 @@ in {
 
       assertFileContent \
         $bookmarksFile \
-        ${./expected-bookmarks.html}
+        ${withName ./expected-bookmarks.html}
     '';
   });
 }
diff --git a/tests/modules/programs/firefox/profiles/search/default.nix b/tests/modules/programs/firefox/profiles/search/default.nix
index 8a4c98e9b..bc6fbc651 100644
--- a/tests/modules/programs/firefox/profiles/search/default.nix
+++ b/tests/modules/programs/firefox/profiles/search/default.nix
@@ -9,6 +9,12 @@ let
 
   firefoxMockOverlay = import ../../setup-firefox-mock-overlay.nix modulePath;
 
+  withName = path:
+    pkgs.substituteAll {
+      src = path;
+      name = cfg.wrappedPackageName;
+    };
+
 in {
   imports = [ firefoxMockOverlay ];
 
@@ -88,12 +94,33 @@ in {
       };
     };
   } // {
-    nmt.script = ''
+    nmt.script = let
+
+      noHashQuery = ''
+        'def walk(f):
+             . as $in
+             | if type == "object" then
+                 reduce keys[] as $key
+                 ( {}; . + { ($key): ($in[$key] | walk(f)) } | f )
+               elif type == "array" then
+                 map( walk(f) )
+               else
+                 f
+               end;
+             walk(if type == "object" then
+                     if has("hash") then .hash = null else . end |
+                     if has("privateHash") then .privateHash = null else . end
+                  else
+                     .
+                  end)' '';
+
+    in
+    ''
       function assertFirefoxSearchContent() {
         compressedSearch=$(normalizeStorePaths "$1")
 
         decompressedSearch=$(dirname $compressedSearch)/search.json
-        ${pkgs.mozlz4a}/bin/mozlz4a -d "$compressedSearch" >(${pkgs.jq}/bin/jq . > "$decompressedSearch")
+        ${pkgs.mozlz4a}/bin/mozlz4a -d "$compressedSearch" >(${pkgs.jq}/bin/jq ${noHashQuery} > "$decompressedSearch")
 
         assertFileContent \
           $decompressedSearch \
@@ -102,11 +129,11 @@ in {
 
       assertFirefoxSearchContent \
         home-files/${cfg.configPath}/search/search.json.mozlz4 \
-        ${./expected-search.json}
+        ${withName ./expected-search.json}
 
       assertFirefoxSearchContent \
         home-files/${cfg.configPath}/searchWithoutDefault/search.json.mozlz4 \
-        ${./expected-search-without-default.json}
+        ${withName ./expected-search-without-default.json}
     '';
   });
 }

[Mikilio]

sigh It's like most my changes to the tests got rebased away. I'll do this properly later. Why did the CI pass, actually?

[rycee]

Thanks! Merged to master now 🙂

Btw, the tests guarded by enableBig are not run in CI due to large downloads. Unfortunately they have to be run manually.

[brckd]

Did someone check the tests manually?

[rycee]

I ran the tests manually before merging.

[brckd]

This module sets the mkFirefoxModule option visible = true; while the LibreWolf module does not. This option controls whether the generic options for Firefox derivatives show up under that module's name. I originally intended it to be only enabled for the Firefox module itself, because that's how modules for Chromium derivatives handle it as well. Should we show generic options only for Firefox or for all derivatives?

[Mikilio]

I have no personal, opinion on this, but if I was new to nix I wouldn't know they exist if they were hidden from the docs. I wouldn't know how to make the connection that all Firefox options also exist here.

[brckd]

IIRC if the visibility option is turned off, a note is appended to the enable option, saying where generic options can he found. But you are right, turning visibility off is bad for discoverablity and doesn't have major benefits I can think of. We could consider removing that option and making all options visible. IIRC it was only an internal option anyways.