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

nixos/firefox: add more options #201033

Merged
merged 4 commits into from Dec 4, 2022
Merged

nixos/firefox: add more options #201033

merged 4 commits into from Dec 4, 2022

Conversation

linsui
Copy link
Contributor

@linsui linsui commented Nov 13, 2022

Description of changes
  1. add preferencesStatus for preferences
  2. add autoConfig since many config are not available in preferences
  3. add nativeMessagingHosts options
  4. fix error: The option `environment.etc."firefox/policies/policies.json".source' is used but not defined.

enableGnomeExtensions and enablePlasmaBrowserIntegration are not added since they are covered by other modules. wmc-mpris is not added since it's deprecated. chrome-token-signing is not added because it should be migrated to web-eid first.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

environment.systemPackages = [ cfg.package ];
environment.systemPackages = [
(cfg.package.override {
extraPrefs = cfg.autoConfig;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firefox may support read this from etc. https://bugzilla.mozilla.org/show_bug.cgi?id=1170092 We can also apply the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://github.com/pyllyukko/user.js#system-wide-installation-all-platforms /etc/firefox/syspref.js is supported but I didn't find docs and it seems the api is unstable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to read this from /etc if possible, as that avoids rebuilding the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AutoConfig in etc requires a patch so we should ask firefox maintains. @lovesegfault @mweinelt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no interest in this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then let's wait upstream. Using the wrapper for now should be good enough.

Copy link
Contributor

@danth danth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

The "used but not defined" error will hit unstable soon so we should try and get this merged.

nixos/modules/programs/firefox.nix Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/679

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message is a bit meaningless.

Comment on lines 134 to 137
policiesJSON =
policyFormat.generate
"firefox-policies.json"
{ inherit (cfg) policies; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
policiesJSON =
policyFormat.generate
"firefox-policies.json"
{ inherit (cfg) policies; };
policiesJSON = policyFormat.generate "firefox-policies.json" { inherit (cfg) policies; };

Comment on lines 75 to 78
- `"default"`: Read/Write: Settings appear as default even if factory default differs.
- `"locked"`: Read-Only: Settings appear as default even if factory default differs.
- `"user"`: Read/Write: Settings appear as changed if it differs from factory default.
- `"clear"`: Read/Write: Value has no effect. Resets to factory defaults on each startup.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What means Read/write here? Which settings? That is not clear from the explanation.

Comment on lines 71 to 81
description = mdDoc ''
The status of `firefox.preferences`.

`status` can be "default", "locked", "user" or "clear"
- `"default"`: Read/Write: Settings appear as default even if factory default differs.
- `"locked"`: Read-Only: Settings appear as default even if factory default differs.
- `"user"`: Read/Write: Settings appear as changed if it differs from factory default.
- `"clear"`: Read/Write: Value has no effect. Resets to factory defaults on each startup.
'';
type = types.enum [ "default" "locked" "user" "clear" ];
default = "locked";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we would order type, default and description but all other things are like this too, so whatever.

Comment on lines 86 to 89
AutoConfig files can be used to set and lock preferences that are not covered
by the policies.json for Mac and Linux. This method can be used to automatically
change user preferences or prevent the end user from modifiying specific
preferences by locking them.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would I find more information how to use this?

Comment on lines 96 to 104
browserpass = mkEnableOption (mdDoc "Browserpass support");
bukubrow = mkEnableOption (mdDoc "Bukubrow support");
ff2mpv = mkEnableOption (mdDoc "ff2mpv support");
fxCast = mkEnableOption (mdDoc "fx_cast support");
gsconnect = mkEnableOption (mdDoc "GSConnect support");
jabref = mkEnableOption (mdDoc "JabRef support");
passff = mkEnableOption (mdDoc "PassFF support");
tridactyl = mkEnableOption (mdDoc "Tridactyl support");
ugetIntegrator = mkEnableOption (mdDoc "Uget Integrator support");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we express this a bit smarter and shorter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure... What's your suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like mapAttrs (_: v: mkEnableOption (mdDoc v)) { browserpass = "Browserpass support"; … } maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it. Thanks!

description = mdDoc ''
The status of `firefox.preferences`.

`status` can be "default", "locked", "user" or "clear"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`status` can be "default", "locked", "user" or "clear"
`status` can assume the following values:

@linsui linsui force-pushed the firefox branch 2 times, most recently from 1d6a730 to f3555d4 Compare November 18, 2022 03:14
@linsui
Copy link
Contributor Author

linsui commented Nov 18, 2022

I applied suggestions and split the commits.

@linsui
Copy link
Contributor Author

linsui commented Dec 4, 2022

Is this good enough to merge? What else should I do?

@AndersonTorres
Copy link
Member

Nothing.

@AndersonTorres AndersonTorres merged commit 3ec5fa6 into NixOS:master Dec 4, 2022
@linsui linsui deleted the firefox branch December 4, 2022 15:18
@linsui
Copy link
Contributor Author

linsui commented Dec 4, 2022

Thanks!

@RossComputerGuy
Copy link
Contributor

Can we get this backported to 22.11 please? I ran into an error that this PR will fix.

@github-actions
Copy link
Contributor

Successfully created backport PR #210557 for release-22.11.

@flokli
Copy link
Contributor

flokli commented Apr 20, 2023

Hmmh, the way this is currently wired together doesn't allow passing nixpkgs.config.firefox additional, not-yet-wired-into-the-module-system native messaging hosts: #227303. Would welcome any input :-)

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

Successfully merging this pull request may close these issues.

None yet

9 participants