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

browserpass: 2017-04-11 -> 1.0.5 #26541

Merged
merged 1 commit into from
Jun 30, 2017
Merged

browserpass: 2017-04-11 -> 1.0.5 #26541

merged 1 commit into from
Jun 30, 2017

Conversation

dermetfan
Copy link
Member

Motivation for this change

See nix-community/home-manager#16 (comment).

This also updates the NixOS module.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@dermetfan, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pajowu to be a potential reviewer.

@rycee
Copy link
Member

rycee commented Jun 13, 2017

Looks good to me but I've never used this so cannot say for certain ☺

CC @pajowu

'';
};
};
programs.browserpass.enable = mkEnableOption "the NativeMessaging configuration for installed browsers.";
Copy link
Member

Choose a reason for hiding this comment

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

This would only work for chrome, right? Maybe mention this in the description.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I could not get it to work with firefox. b4caf7a looks like it tried to add firefox support but from what I can see it does not affect the NixOS module.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dermetfan unfortunately it doesn't work because Firefox expects the path to the native messaging hosts to be /usr/{lib,lib64,share}/mozilla/native-messaging-hosts/<name>.json, see https://wiki.mozilla.org/WebExtensions/Native_Messaging

As far as I can tell the only solution is to patch Firefox to use an environment variable (like MOZ_PLUGIN_PATH) or to wrap it in buildFHSUserEnv.

Copy link
Member Author

@dermetfan dermetfan Jun 15, 2017

Choose a reason for hiding this comment

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

If I understand correctly that would be implemented in the firefox wrapper. Let's merge this first and open another issue if desired since we didn't have firefox support before this PR.

@rycee
Copy link
Member

rycee commented Jun 23, 2017

Any opposition to a merge of this PR as it is now?

@dermetfan
Copy link
Member Author

ping @fpletz

@joachifm joachifm merged commit 74b3a8f into NixOS:master Jun 30, 2017
@dermetfan dermetfan deleted the browserpass branch June 30, 2017 20:28
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.

6 participants