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
plover: add dev-with-plugins #110658
base: master
Are you sure you want to change the base?
plover: add dev-with-plugins #110658
Conversation
This adds plover.dev-with-plugins, which is plover.dev with the added plover-plugins-manager dependency (which causes the plugin manager to appear in the Plover interface). Fixes NixOS#89341 in the first way; the second way is morally better but more difficult to do.
src = fetchFromGitHub { | ||
owner = "ross"; | ||
repo = "requests-futures"; | ||
rev = "d5cbf487c010dd84c4e030e279d48e7179e35335"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please either fetch a tag or change the version to unstable-XXXX-XX-XX.
pname = "plover-plugins-manager"; | ||
version = "0.5.16"; | ||
|
||
meta = with lib; { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meta belongs to the end.
meta = with lib; { | ||
description = "OpenSteno Plover stenography software plugin manager"; | ||
maintainers = with maintainers; [ tckmn ]; | ||
license = licenses.gpl2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gpl2Only or gpl2Plus?
}; | ||
|
||
# apply nix sys.path wrapper to plugin manager's python call | ||
postPatch = "patch -p1 <${./plugins-manager.patch}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
postPatch = "patch -p1 <${./plugins-manager.patch}"; | |
postPatch = '' | |
patch -p1 <${./plugins-manager.patch} | |
''; |
Why are you not using the patch in patches? It should support all options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are a couple of important changes to be made here, but I'm happy to see some progress made on this issue after all this time!
propagatedBuildInputs = [ pip pkginfo dev pygments readme_renderer requests requests-cache requests-futures setuptools wheel ]; | ||
}; | ||
|
||
dev-with-plugins = dev.overrideAttrs (old: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of thoughts:
Recommend adding a withPluginManager
passthrough to the dev
attribute and having this point to it. This allows one to override plover.dev
if they desire and still have access to withPluginManager
behavior without having to create an overlay (through (plover.dev.overrideAttrs (old: ...)).withPluginManager
)
Also I think it's probably confusing to have a withPackages
-sounding attribute in the packageset be an attribute that doesn't take an argument (namely, a list of the plugins). I notice that you used kebab-case to name it, which disambiguates it a little from functions like emacsWithPackages
but personally I'd like to see 1 of 3 things:
- Making it crystal clear exactly what this is -- ie. calling it something like
dev-with-plugin-manager
- Just adding
plugin-manager
todev
(making it disableable with an override, perhaps)- Honestly I think this is the behavior most people would be expecting by default anyway, since on every other platform I can think of, when you install plover dev, the plugin manager comes with.
- Implementing
emacsWithPackages
-like functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've distilled out my thoughts on this and I'm pretty sure the second option is the right way to do this. Plover coming with the plugin manager is expected behavior when installing Plover, so it's reasonable to have it be the default.
Ideally it would be disable-able by overriding the package for those who don't want it, and the third option (emacsWithPackages
-like functionality) could be implemented later by anyone who felt inclined to do so.
I marked this as stale due to inactivity. → More info |
Motivation for this change
This adds plover.dev-with-plugins, which is plover.dev with the added
plover-plugins-manager dependency (which causes the plugin manager to
appear in the Plover interface).
Fixes #89341 in the first way; the second way is morally better but more
difficult to do.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)