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

Fix firefox wrapper #105796

Merged
merged 1 commit into from Dec 4, 2020
Merged

Fix firefox wrapper #105796

merged 1 commit into from Dec 4, 2020

Conversation

Qubasa
Copy link
Contributor

@Qubasa Qubasa commented Dec 3, 2020

Motivation for this change

Fixing #91724 (comment)
Sorry @Mic92 I missed that detail.

Things done

Only enforce nix extensions if at least one is defined

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@taku0
Copy link
Contributor

taku0 commented Dec 3, 2020

Empty extraExtensions should remove extensions installed by extraExtensions.

@taku0
Copy link
Contributor

taku0 commented Dec 3, 2020

We also set xpinstall.signatures.required to true if the policy is disabled.

@Qubasa
Copy link
Contributor Author

Qubasa commented Dec 3, 2020

Fixed both. I introduced a new flag called enforceExtraExtensions to have old addons deleted when the extraExtension is changed from having addons to having an empty list. I changed the documentation accordingly

@lukegb
Copy link
Contributor

lukegb commented Dec 3, 2020

Would you mind rebasing and squashing the commits to help clean up the history? You seem to have a ton of merge master-into-master commits, as well as commits with duplicate summaries.

@Qubasa
Copy link
Contributor Author

Qubasa commented Dec 3, 2020

After considering I changed the flag enforceExtraExtensions to enableExtraExtensions

@taku0
Copy link
Contributor

taku0 commented Dec 3, 2020

LGTM

@andir
Copy link
Member

andir commented Dec 4, 2020

Please add a proper commit message with reasoning and a why. Currently it doesn't even conform with the contribution guidelines.

@teto
Copy link
Member

teto commented Dec 4, 2020

question: I have tried to use your MR yesterday without this fix and my manual extensions were gone. I've tried to rebuild with this fix and they are still gone. Are they gone forever (i.e., do I need to reinstall them) or they are just hidden and I can recover them ?
(or 3rd possiblity, I used the wrapped firefox within the home-manager module that does its own configuration too, maybe they clashed ?).
ls -lt ~/.mozilla/firefox/q1pprbmm.default/extensions shows me only the extension configured via nix so I suppose they are gone (not an issue, I have just a handful of them)

@Qubasa
Copy link
Contributor Author

Qubasa commented Dec 4, 2020

question: I have tried to use your MR yesterday without this fix and my manual extensions were gone. I've tried to rebuild with this fix and they are still gone. Are they gone forever (i.e., do I need to reinstall them) or they are just hidden and I can recover them ?
(or 3rd possiblity, I used the wrapped firefox within the home-manager module that does its own configuration too, maybe they clashed ?).
ls -lt ~/.mozilla/firefox/q1pprbmm.default/extensions shows me only the extension configured via nix so I suppose they are gone (not an issue, I have just a handful of them)

Yes they are gone, but the addon config data is still there so after reinstalling them manually it should just work

@Qubasa
Copy link
Contributor Author

Qubasa commented Dec 4, 2020

LGTM

I changed the flag enforceExtraExtensions to enableExtraExtensions because documenting and explaining which edge case it covers is too complicated. Hiding this complexity away behind an enableExtraExtensions which only allows nixExtensions when enabled and not when the extraExtension list is non empty was the reason behind my change.

@taku0
Copy link
Contributor

taku0 commented Dec 4, 2020

Please refer CONTRIBUTING.md for the convention for commit messages in nixpkgs.

For example, firefox: fix wrapper removing exiting addons by default or something would be better.

@Qubasa Qubasa force-pushed the fix_firefox_wrapper branch 2 times, most recently from f39bb83 to 81f2019 Compare December 4, 2020 14:44
@Qubasa Qubasa force-pushed the fix_firefox_wrapper branch 3 times, most recently from a6a9e4e to 2d667f4 Compare December 4, 2020 16:27
@Qubasa Qubasa requested review from andir and Mic92 December 4, 2020 16:44
@Mic92 Mic92 merged commit 29566ca into NixOS:master Dec 4, 2020
DisableAppUpdate = true;
} //
{
lib.optionalAttrs usesNixExtensions {
ExtensionSettings = {
"*" = {
blocked_install_message = "You can't have manual extension mixed with nix extensions";
Copy link
Contributor

Choose a reason for hiding this comment

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

This message should explain how to disable this enforcement.

Copy link
Member

Choose a reason for hiding this comment

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

@kevincox can you make a PR?

@wiltaylor
Copy link
Contributor

Any chance we can roll these changes back till its tested properly. This has had a massive impact on most of the user base.

For example it uninstalled of my extensions then when firefox sync ran it uninstalled all of them on all of my machines. Not only that, because i use container tabs it wiped all of my cookies.

This feature should only be opt in.

@Dietr1ch
Copy link
Contributor

Dietr1ch commented Dec 6, 2020

TBH at this point after a sync that removed my extensions on all my machines a rollback won't help much.
I'd just take this as a lesson to back up my browser profiles and would be better off if we were all shown
the nix-way of managing extensions with Firefox.

Now, what should this config look like if we were to use nix to manage common extensions like ublock-origin and maybe an obscure one that's not packaged yet and we'll need to provide the urls and hashes?

programs = {
  firefox = {
    enable = true;
  };
};

@xaverdh
Copy link
Contributor

xaverdh commented Dec 13, 2020

Now, what should this config look like if we were to use nix to manage common extensions like ublock-origin and maybe an obscure one that's not packaged yet and we'll need to provide the urls and hashes?

programs = {
  firefox = {
    enable = true;
  };
};

You can use something like

{
environment.systemPackages = 
  let noscript = pkgs.fetchFirefoxAddon {
      name = "noscript";
      url = "https://addons.mozilla.org/firefox/downloads/latest/noscript/latest.xpi";
      sha256 = "sha256-Pj4AAm22plzV7rvkosIU/X70N8j00X+lkchfYeq50eU=";
    };
  in [ (firefox.override { nixExtensions = [ noscript ]; }) ];
}

The hash can be obtained by not e.g. specifying one, and trying to build.
The way the url is chosen here, you will get a hash mismatch once the version changes. You can use a version specific url instead (more principled approach, but then you need to actively track updates I guess, at least until there is some tooling / effort in nixpkgs proper).

@peti
Copy link
Member

peti commented Dec 14, 2020

That's a pretty bad solution, though. This means that everyone has to maintain a package set of browser extensions (versions, hashes, etc.) by themselves, duplicating each other efforts. If those extensions were available to choose from in Nixpkgs, this might make sense, but as-is this just adds maintenance effort for the users to duplicate a perfectly good package manager that's already included in Firefox anyway.

@xaverdh
Copy link
Contributor

xaverdh commented Dec 14, 2020

That's a pretty bad solution, though. This means that everyone has to maintain a package set of browser extensions (versions, hashes, etc.) by themselves, duplicating each other efforts. If those extensions were available to choose from in Nixpkgs, this might make sense, but as-is this just adds maintenance effort for the users to duplicate a perfectly good package manager that's already included in Firefox anyway.

Yes this should ideally be managed by a concerted effort of the community to provide a package set for addons. Ideally it would also be seeded automatically from AMO. There are still valid use cases for manually packaging addons, e.g. if you develop your own addon and don't want to have it on AMO (yet).
The downside to the current "package manager" in firefox is that its based on a gui, not declarative at all, etc..
Say if you don't want to use firefox sync, (e.g. for privacy reasons, but there are others), then you would have to manually (re)install all extensions when setting up a new system.

@Mic92
Copy link
Member

Mic92 commented Dec 14, 2020

That's a pretty bad solution, though. This means that everyone has to maintain a package set of browser extensions (versions, hashes, etc.) by themselves, duplicating each other efforts. If those extensions were available to choose from in Nixpkgs, this might make sense, but as-is this just adds maintenance effort for the users to duplicate a perfectly good package manager that's already included in Firefox anyway.

We discuss having them in a nix-community maintained repo here: #105783

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