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

plover.dev-with-plugins: init #119702

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

plover.dev-with-plugins: init #119702

wants to merge 8 commits into from

Conversation

evils
Copy link
Member

@evils evils commented Apr 17, 2021

based on / closes #110658
closes #89341

Motivation for this change

help some community members

Things done
  • 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.

remaining issues (help wanted)

plugins don't work in a nix-shell
upstream recommendations (don't use the patch, use dist_main entry point) break things

sha256 = "1q4y3dl0p1yz9q2jb6hkhdskhbci7bfwgs5fvm8i09r011j0bghr";
};

patches = [ ./plugins_manager.patch ];
Copy link
Member Author

Choose a reason for hiding this comment

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

without the patch plugins can't be managed

@@ -46,6 +46,11 @@ rec {
sha256 = "0kf27ykw6cjlpyr47nqad5hvpc1sza8x4lpnprwksd70y9hjs40g";
};

# upstream recommendation
postPatch = ''
sed -i 's/^\(\s*plover = plover.\)main:main$/\1dist_main:main/' setup.cfg
Copy link
Member Author

Choose a reason for hiding this comment

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

with this change plover still works in a nix-shell
but fails to start in my environment

switch to fetchFromGitHub

and correct license
This reverts commit 3a6ac8e6d351e966baaf40a5ce573b0d7fd79238.

this change prevents starting in my user environment
  (still works in a pure shell)
without it:
Error while finding module specification for 'plover_plugins_manager.pip_wrapper'
  (ModuleNotFoundError: No module named 'plover_plugins_manager')
src = fetchFromGitHub {
owner = "openstenoproject";
repo = "plover";
rev = "54dbcf4ea73cc1ecc1d7c70dbe7bdb13f055d101";
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
rev = "54dbcf4ea73cc1ecc1d7c70dbe7bdb13f055d101";
rev = "v${version}";

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, they're doing a moving tag of "continuous" but still have a versioning scheme that's no longer integrated with git


buildPythonPackage rec {
pname = "requests-futures";
version = "0.9.9-27-gf126048";
Copy link
Member

Choose a reason for hiding this comment

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

From where did you get the version number?

Copy link
Member Author

@evils evils Jun 1, 2021

Choose a reason for hiding this comment

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

git describe --tags minus the leading v

};
};

dev-with-plugins = dev.overrideAttrs (old: {
Copy link
Contributor

@lambdadog lambdadog Aug 11, 2021

Choose a reason for hiding this comment

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

I can only echo my thoughts on #110658 that including the plugin manager is expected functionality with the dev branch of plover. It's what you'll get if you're installing plover dev with any other package manager, or the appimage, or whatever else.

If anything, now that my experience with Nix has grown, I'd like to see an argument that can be overridden (to false) on plover dev that enables the package manager by default, rather than creating two separate derivations that are built, something like

plover.dev.override {
  withPluginManager = false;
}

Copy link
Contributor

@lambdadog lambdadog Aug 11, 2021

Choose a reason for hiding this comment

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

Actually, thinking further, I'm not exactly happy that the plugin-manager derivation is exposed as it is either.

While there is the potential usecase that one would want to override the plugin manager, the way it's currently exposed gives no particularly elegant ability to do so (other than simply repeating your plover-with-plugins derivation wholesale). It shouldn't be exposed in the plover tree's top level as it currently is. My best image for how it should be exposed while giving a way to override it probably looks like putting it under a subtree like plover.devPlugins.plugin-manager, then plover.dev could expose, perhaps, both a withPluginManager and plugins override argument. If you wanted to swap out the plugin manager with an overridden version, you would set withPluginManager to false, then add your overridden version to plugins.

I'm more than happy for more brainstorming on the subject, though, I admit this probably isn't the most elegant solution but I think we need to deal with a couple of concerns, including:

  1. the plugin-manager being included being an expected default
  2. allowing users to disable it if they wish
  3. making the plover subtree not bloated, and
  4. considering the possibility of extending plover with a withPlugins-like functionality in the future, which this is already great groundwork for

Copy link
Member Author

Choose a reason for hiding this comment

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

the right solution™ is probably to accept a list of plugins
then the (default) setting of an empty list would not provide the plugin manager

as far as i know this would require packaging all the plugins in nixpkgs
which i'm not willing to do because plover doesn't work on wayland...

for now, setting plover.dev to include the plugin manager does seem like the right way forward

unfortunately i seem to recall the issue with this PR being,
plover not reliably finding the site packages (plugins) installed by the plugin manager
(between a nix-env install, a nix-shell, and a NixOS systemPackages install)
which is something i don't know how to fix, but would like to see fixed before seeing this PR merged
alternatively, we could merge something that works in 1 use-case and let users get annoyed enough to fix it themselves

Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably see if it's possible to pass the site packages in an environmental variable?

It would need to be the same place recognized by the plugin manager and writeable though, but the only two cases I see are:

  1. executed by a user with a $HOME directory, and
  2. executed by a user without a $HOME directory (which, frankly, given the use-case of plover may not be a problem in the first place)

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be done with makeWrapper using the --run flag and a '-quoted argument to it using $HOME, you can see how my emacs config using Nix does similar here

I imagine something like (assuming the variable needed is SITE_PACKAGES and both the plugin manager and plover would read from it correctly)

makeWrapper "PATH/TO/PLOVER/EXECUTABLE" "$out/bin/plover"
  --run 'mkdir -p "$HOME/PATH/TO/YOUR/SITEPACKAGES"'
  --run 'export SITE_PACKAGES="$HOME/PATH/TO/YOUR/SITEPACKAGES"'

would work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, you would also need the site packages that nixpkgs' buildPythonPackage creates, but I'm sure it's possible to do with a bit of work.

At worst you could probably do some patching of plover and the plugin manager here or there to enable it.

Copy link
Contributor

@lambdadog lambdadog Aug 17, 2021

Choose a reason for hiding this comment

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

Upon research you may be interested in the PYTHONUSERBASE environmental variable, which I don't believe that buildPythonPackage co-opts for its own usage.

@stale
Copy link

stale bot commented Apr 18, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 18, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
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.

Add Plover plugins
4 participants