tridactyl-native: init at 1.14.9 #61703
Conversation
|
@infinisil friendly ping in case this got lost :) If you saw it and don't have time for a review right now thats also perfectly fine, just say so. |
timokau
force-pushed
the
tridactyl-native-init
branch
from
cd13e28
to
70b64ea
Compare
timokau
force-pushed
the
tridactyl-native-init
branch
from
70b64ea
to
acd171c
Compare
|
Hm, I can't get it to work myself. I rebuilt my firefox with this patch, and I know it has a reference to the json file, but after installing the extension, the Do you know what the problem could be? (I did restart firefox) |
|
Did you have tridactyl with the native messenger installed before? I initially ran into a problem because the user manifest in |
|
Nope, never used tridactyl before and ~/.mozilla/native-messaging-hosts only contains an old browserpass one. |
|
I'm not using the latest FF though, but rather 66.0.3, I only applied this PR's patch on my systems nixpkgs. I'll try with a firefox from this PR's commit directly |
That seemed to do it :D. Yay for reproducibility. Not sure why it didn't work with the older FF though, oh well. |
| @@ -67,6 +68,7 @@ let | |||
| ([ ] | |||
| ++ lib.optional (cfg.enableBrowserpass or false) (lib.getBin browserpass) | |||
| ++ lib.optional (cfg.enableBukubrow or false) bukubrow | |||
| ++ lib.optional (cfg.enableTridactylNative or false) tridactyl-native | |||
There was a problem hiding this comment.
One more thing though: I think the Native part can be dropped, it's also not used for all other packages. Same for the package attribute tridactyl-native -> tridactyl, package path and name too.
There was a problem hiding this comment.
I disagree: if we ever get firefox plugin support in nixpkgs, it would be confusing to already have a package called tridactyl. It should be clear for the user what is being installed and what is not (they will have to install the actual plugin through other means).
Do you insist on the name change?
There was a problem hiding this comment.
Hmm, but then it's still the tridactyl package from the same repo, and we'd probably just make it have multiple outputs, one for the native part, one for the extension part.
There was a problem hiding this comment.
But shouldn't we still separate it then? There is good reason firefox doesn't install native messengers itself, it basically sidesteps the whole permission mechanism.
There was a problem hiding this comment.
Usage with separate derivations for native and extension:
{ pkgs }: {
nativePart = pkgs.tridactyl-native;
extensionPart = pkgs.tridactyl-extension;
}Usage with a single derivation using multiple outputs:
{ pkgs }: {
nativePart = pkgs.tridactyl.native; # Or pkgs.tridactyl because it's the default output
extensionPart = pkgs.tridactyl.extension;
}I don't see how the latter could be a problem, they really only differ by s/-/..
There was a problem hiding this comment.
Yes, but shouldn't extensionPart be the default? When I install tridactyl, I expect to be installing the plugin. I'd be surprised if it (only) installed the native messenger by default.
There was a problem hiding this comment.
If we ever do get support for packaging extensions, I'd guess we'd put it in browserExtensions.tridactyl or so anyways, and you could install the native one via pkgs.tridactyl. Anyways, if you really think we should leave the -native part in the name then so be it.
There was a problem hiding this comment.
I still do think that the -native postfix reduces confusion and I don't see any drawbacks to using it, so I'd rather keep it.
Thanks a lot for the review :)
Yeah that's weird, maybe there were some changes in how native plugins work. |
Motivation for this change
Tridactyl requires a native messenger plugin for many of its features (like reading an rc file or shelling out). I'd like to be able to install that declaratively.
@infinisil can you review this? I know near to nothing about firefox in nixpkgs and I basically copied what bukubrow does.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)