Weechat plugin split #25274

lheckemann · 2017-04-27T17:03:43Z

Motivation for this change

This change moves each language plugin into a separate output, significantly reducing weechat's closure size. It also adds a withPlugins function to weechat, which generates a wrapper for adding the plugins.

This relies on weechat/weechat#971 and so probably shouldn't be merged until the next stable release which will (hopefully!) include that change.

This is probably my biggest nixpkgs change yet, so I'm PRing it now in search of feedback — any input would be highly appreciated! Particularly, the way in which I add the withPlugins function to weechat. This should allow using it like weechat.withPlugins [weechat.ruby weechat.guile], but is there a more elegant way of adding it than the set union I'm doing here? I'd also like to know what people think about the mechanism as a whole, i.e. how this runtime composition is implemented.

Checklist before this can be merged:

~~Search WEECHAT_EXTRA_LIBDIR for plugins weechat/weechat#971 merged and incorporated into a release~~ patch added to expression instead
Remove references from main weechat derivation to python and perl packages — these should be in the wrapper as well
Document it — but where? This isn't specific to NixOS (so probably not the NixOS manual), nor is it only applicable to people intending to contribute to nixpkgs (so probably not the nixpkgs manual). Do we have an "end-user's guide" to nixpkgs?

Ideas:

Potentially handle the plugins in a more unified way; I'll be experimenting with this a bit.
Allow managing user config as well as plugins

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 · 2017-04-27T17:03:46Z

@lheckemann, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @lovek323 and @aneeshusa to be potential reviewers.

lheckemann · 2017-04-27T22:23:41Z

I would also appreciate if anyone could comment on whether they think lheckemann@e8bf1f9 is an improvement on this. It has yet to address the issue of plugin-specific environment variables properly, so is also WIP on top of this.

garbas · 2017-04-27T23:06:03Z

pkgs/applications/networking/irc/weechat/default.nix

+        platforms = stdenv.lib.platforms.unix;
+      };
+    };
+in weechat // {


I think more standard way to extend expressions is to rely on .override function. That means that have to specify withPlugins as argument of weechat expression.

When somebody wants to use a customized weechat they will do something like:

custom_weechat = pkgs.weechat.override { guileSupport = false; withPlugins = [ plugin1, plugin2 ]; };

You can check neovim expression how I did it to conditionally (when configuration is present) return wrapped and configured neovim.

I'm aware of this; the idea behind the approach I've used here is to take advantage of weechat's modularity to significantly reduce weechat's closure size without the need for rebuilding weechat itself (as is necessary if any of the xSupport flags are passed in). Customising weechat's plugins has already been possible simply by disabling them in the build, but that means that it needs to be rebuilt on the user's end which somewhat dampens the resulting improvement in closure size as all the build deps need to be available (at least initially) and the build takes longer.

using withPlugins in does not go against reducing weechat's closure size. If you looked at neovim's expression you will see that by default default weechat is returned. if plugins are set you wrap the returning weechat executable. The only things that changes is how you go about customizing this. The preferred way to do this is via .override function.

Right, that makes sense... so something like d421f4aefcaddf0ec07582ffb62fa6f625dd8f96 ?

(usage: weechat.override {withPlugins = ["perl" "python" "ruby"];})

garbas · 2017-04-27T23:39:16Z

@lheckemann to not wait for upstream you could also point to that PR and add it to the patches list.

lheckemann · 2017-04-28T06:22:49Z

Of course, I forgot about that!

garbas · 2017-04-28T12:09:04Z

pkgs/applications/networking/irc/weechat/default.nix

+      src = fetchFromGitHub {
+        owner = "lheckemann";
+        repo = "weechat";
+        rev = "546e260fd58e41db20e7bc4ec5585e93fb6994c1";


i this v1.7.1 tag?

No, it's master as of yesterday + my patch. I've changed it back to use v1.7.1 plus my patch
368edaf

sorry commenting on old code :)

garbas

@lheckemann 💯 on the change. I would just squash the commits before we merge this to nixpkgs.

lheckemann · 2017-04-28T12:13:57Z

Yes, that was my plan. I also still need to fix the perl/python path stuff in the wrapper — currently just weechat will still depend on perl and python, without even bringing in the plugins.

lheckemann · 2017-04-28T12:15:03Z

It also still needs docs, which I don't know where to put.

lheckemann · 2017-04-28T12:38:47Z

This definitely isn't ready for merging yet — for instance, fish.py from the official scripts repo doesn't work with it

garbas · 2017-04-28T13:17:24Z

@lheckemann I think you can open a new section about weechat at Chapter 10. Package Notes

I'll do a bit of dreaming in this comment and you stop me when I go too far :P Also I'm interested to work on this myself, just for some things I'm too short on my skills.

(1) nixos module: maybe it would be worthwhile to also create a nixos module for weechat. eg

programs.weechat.enable = true;
programs.weechat.plugins = [ ... ];
programs.weechat.<other-options>...

(2) like with plugins it would be nice to have a variable to configure weechat to write logs in location other then $WEECHAT_HOME/logs, we could add this as an argument like withPlugins

(3) configuring weechat: it would be also nice to be able to disallow weechat to write configuration. and create a nice way to configure weechat (eg. like i already do with neovim for example), we could add this as an argument like withPlugins

(4) work on weechat2nix script that will package weechat plugins

lheckemann · 2017-04-29T08:53:16Z

Not sure how useful this would be, given that weechat doesn't really require any system-wide configuration — most modules, as far as I can tell, exist because they require system-wide settings e.g. programs.zsh for registering it as the users' default shell, programs.cdemu for setting appropriate permissions, etc. A module for weechat, as far as I can tell, wouldn't provide anything that installing an expression wouldn't already.
Yes.
Yes. Generally, separating the state and the config would be nice.
I'm not aware of any plugins that don't ship with weechat. Most extra functionality seems to be implemented through scripts built on the existing language plugins.

garbas · 2017-05-02T11:57:34Z

you're probably right. .override to configure weechat is enough. maybe having a service that would spawn weechat+tmux like i do here
& 3. I would love to poke with weechat's code but this goes beyond my skills.
Sorry for the confusion. I was talking about scripts. instead of using weechat to pull in unknown code we can use nix. That would of-course require us to disable /scripts command.

lheckemann · 2017-05-02T13:01:23Z

Yeah, I'll have a look at making those changes to weechat in the next few days.

As for script installation, yes I guess that would be the logical next step from there. I'll see what I can do!

lheckemann · 2017-05-09T13:58:36Z

The python plugin needs some python libraries on its path. I think the right solution would be to allow passing in a python environment as created by python3.withPackages, but I'm not sure how best to expose this. @garbas do you have any ideas for what a nice way to do this would look like? Some ideas so far…

weechat.override { configure = {
  plugins = ["python"];
  environment = {
      PYTHON_HOME = python.withPackages (ps: with ps; [pycrypto dbus-python]);
  };
};}

But I'm not really sure if this is a nice way to do it, particularly because it relies on "magical" values so much. It would be nicer if it could somehow be included in the plugins attribute, something like

weechat.override { configure = {
  plugins = available: [
    (available.python.withPackages (ps: with ps; [pycrypto python-dbus]))
  ];
};}

or even

weechat.override {
  configure = {availablePlugins, ...}: {
    plugins = [availablePlugins.python.withPackages (ps: with ps; [pycrypto python-dbus])];
  };
}

But all of these look rather complicated to use. Any thoughts?

FRidh · 2017-05-30T18:06:13Z

@lheckemann does the basic package, without plugins, need Python, or is it only needed for a plugin? If it does not build against a Python env, then you can just add the Python env to PATH.

FRidh · 2017-05-30T18:04:46Z

pkgs/applications/networking/irc/weechat/default.nix

+            pluginFile = "${weechat.python}/lib/weechat/plugins/python.so";
+            withPackages = pkgsFun: (python // {
+              extraEnv = ''
+                export PYTHONHOME="${pythonPackages.python.withPackages pkgsFun}"


This is not going to work. python.withPackages uses itself --set PYTHONHOME.

It worked for me.

What python.withPackages does is build an environment containing symlinks to all the packages and a python wrapper which sets PYTHONHOME to the location of the resulting environment. The wrapper won't actually be used by weechat's python plugin, but the python plugin does load the python interpreter as a shared library and at that point the interpreter looks at PYTHONHOME to see where to import stuff from.

FRidh · 2017-05-30T18:10:46Z

pkgs/applications/networking/irc/weechat/default.nix

+        ln -s $plugin $out/plugins
+      done
+    '';
+  in writeScriptBin "weechat" ''


Have you considered using buildEnv for composition? This does look neat btw.

Not sure how that would work in this case, since the paths are the actual shared libraries and not the derivations.

lheckemann · 2017-05-30T21:14:50Z

does the basic package, without plugins, need Python, or is it only needed for a plugin? If it does not build against a Python env, then you can just add the Python env to PATH.

The python plugin for weechat requires python obviously. It does not execute python as a process though, it uses the library interface to the interpreter, so I think the approach I'm taking is the right one?

lheckemann · 2017-06-07T12:20:06Z

That's it documented. If nobody has any further objections, I'll rebase and squash for the actual merge.

0xABAB · 2017-07-08T20:56:26Z

doc/package-notes.xml

+<section xml:id="sec-weechat">
+<title>Weechat</title>
+<para>
+Weechat can currently be configured to include your choice of plugins.


"You can configure plugins as follows" is shorter and says the same.

Explicitly mentioning that this is an expression to install is more helpful, as some other packages are configured via nixpkgs config settings instead.

lheckemann · 2017-09-05T09:43:56Z

@FRidh @garbas ping?

FRidh · 2017-09-05T09:47:00Z

@lheckemann I'd like to give the final word to whoever maintains this

lheckemann · 2017-09-05T09:48:28Z

Oh yeah, maybe @the-kenny and @lovek323 would like to weigh in as well :)

disassembler · 2017-11-01T01:52:13Z

@lheckemann merge conflicts can you rebase? Also @garbas @the-kenny and @lovek323 we're looking for feedback here. If we don't get a response, I say we go ahead and merge it.

Also add a wrapper generator that allows adding the plugins back conveniently and corresponding documentation in the package notes section of the nixpkgs manual.

jluttine · 2017-11-03T10:29:30Z

Would this PR make it possible to use plugins like https://github.com/wee-slack/wee-slack which, for instance, depend on other Python packages?

lheckemann · 2017-11-03T11:09:11Z

@jluttine yes, see the addition to the manual for details on how. The packages in question do need to be included in nixpkgs though.

Ma27 · 2017-11-04T08:35:56Z

@lheckemann are there any remaining blockers? I've packaged some Weechat plugins for Nixpkgs and I'd try to make them compatible with this in the next time :-)

lheckemann · 2017-11-04T08:37:07Z

Just approval from maintainers.

Ma27 · 2017-11-04T09:03:45Z

ah nice :-D
will try to make my weechat derivations compatible with this :-)

Ma27 · 2017-11-16T16:35:31Z

@lheckemann just to ensure that I got this right: your change adds the ability to builda weechat with plugins.
My PR (see #28897) adds an FHS wrapper to make it possible that the scripts inside are buildable with nix. It surely needs some refactoring (I hacked this in ~1h to have an example that can be used for further discussion about this), but in the end it should live happily together with this, right?

lheckemann · 2017-11-17T09:32:24Z

Yes, in principle: they're concerned with different parts of weechat, mine is for plugins and yours for scripts. I think the "patch weechat to search an alternative path" approach would be nicer than building a FHS env for scripts too, but I don't see any reason the latter approach would be incompatible with this PR either.

Ma27 · 2017-11-17T11:05:00Z

just found your weechat patch. If such an approach is applicable to weechat scripts as well, I'll close this and open another PR

…

On 17. Nov 2017, at 10:32 AM, Linus Heckemann ***@***.***> wrote: Yes, in principle: they're concerned with different parts of weechat, mine is for plugins and yours for scripts. I think the "patch weechat to search an alternative path" approach would be nicer than building a FHS env for scripts too, but I don't see any reason the latter approach would be incompatible with this PR either. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

jluttine · 2018-03-15T11:39:59Z

I started using this feature and it seems really great. I figured out how to add Python dependencies that some scripts need, but I couldn't figure out how to do the same for Lua dependencies.. There's no lua.withPackages apparently as there is for Python. Any ideas? I'm trying to add cjson Lua package required by this script: https://github.com/torhve/weechat-matrix-protocol-script

lheckemann · 2018-03-15T12:04:32Z

Yes, it doesn't currently have support for this, largely because lua library support in nixpkgs isn't that great in general as far as I remember. I seem to remember @teto doing some work on that, maybe you can advise?

teto · 2018-03-15T12:32:10Z

the work is visible here #33903. It works well except for some torch packages. I have yet to understand what's the problem there.

jluttine · 2018-03-15T12:34:37Z

Ok, thanks! So at the moment, weechat cannot be used with lua dependencies. (?)

teto · 2018-03-15T12:36:46Z

Other packages hack their way around via adjusting the LUA_PATH in a wrapper but if you are not hurry, there should be a better way soon :)

lheckemann force-pushed the weechat-plugin-split branch from 56cf3ef to 049036f Compare April 27, 2017 17:04

lheckemann mentioned this pull request Apr 27, 2017

Can we undeprecate/unlock the wiki? #22599

Closed

garbas reviewed Apr 27, 2017

View reviewed changes

garbas reviewed Apr 28, 2017

View reviewed changes

garbas approved these changes Apr 28, 2017

View reviewed changes

lheckemann mentioned this pull request May 3, 2017

Snap package for Ubuntu Core weechat/weechat#979

Closed

pSub added the 2.status: work-in-progress label May 5, 2017

lheckemann mentioned this pull request May 29, 2017

Nixpkgs manual scope #26211

Open

FRidh reviewed May 30, 2017

View reviewed changes

lheckemann force-pushed the weechat-plugin-split branch from b041f15 to 9fb0d2a Compare June 7, 2017 11:38

lheckemann force-pushed the weechat-plugin-split branch 2 times, most recently from 8415302 to f0291e0 Compare July 8, 2017 20:05

lheckemann changed the title ~~(WIP) Weechat plugin split~~ Weechat plugin split Jul 8, 2017

0xABAB reviewed Jul 8, 2017

View reviewed changes

lheckemann force-pushed the weechat-plugin-split branch 2 times, most recently from ceefe38 to cb8360a Compare August 12, 2017 14:03

lheckemann force-pushed the weechat-plugin-split branch from cb8360a to e79a846 Compare September 5, 2017 09:47

weechat: split plugins into separate outputs

05f96bf

Also add a wrapper generator that allows adding the plugins back conveniently and corresponding documentation in the package notes section of the nixpkgs manual.

lheckemann force-pushed the weechat-plugin-split branch from e79a846 to 05f96bf Compare November 3, 2017 10:25

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Nov 3, 2017

This was referenced Nov 4, 2017

weechat-matrix-bridge: init at 2017-03-28 #28738

Merged

weechat: add FHS wrapper #28897

Closed

FRidh merged commit 4bd9b3b into NixOS:master Nov 4, 2017

lheckemann deleted the weechat-plugin-split branch November 4, 2017 08:56

lheckemann mentioned this pull request Mar 13, 2018

weechat no longer includes guile plugin on master #25270

Closed

lheckemann mentioned this pull request Dec 5, 2018

weechat: seperate weechat-unwrapped from wrapper #44102

Merged

9 tasks

Weechat plugin split #25274

Weechat plugin split #25274

Conversation

lheckemann commented Apr 27, 2017 • edited Loading

Motivation for this change

Things done

mention-bot commented Apr 27, 2017

lheckemann commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garbas commented Apr 27, 2017

lheckemann commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garbas left a comment

Choose a reason for hiding this comment

lheckemann commented Apr 28, 2017

lheckemann commented Apr 28, 2017

lheckemann commented Apr 28, 2017

garbas commented Apr 28, 2017 • edited Loading

lheckemann commented Apr 29, 2017

garbas commented May 2, 2017

lheckemann commented May 2, 2017

lheckemann commented May 9, 2017 • edited Loading

FRidh commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lheckemann commented May 30, 2017

lheckemann commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lheckemann commented Sep 5, 2017

FRidh commented Sep 5, 2017

lheckemann commented Sep 5, 2017

disassembler commented Nov 1, 2017 • edited Loading

jluttine commented Nov 3, 2017

lheckemann commented Nov 3, 2017

Ma27 commented Nov 4, 2017

lheckemann commented Nov 4, 2017

Ma27 commented Nov 4, 2017

Ma27 commented Nov 16, 2017

lheckemann commented Nov 17, 2017

Ma27 commented Nov 17, 2017 via email

jluttine commented Mar 15, 2018

lheckemann commented Mar 15, 2018

teto commented Mar 15, 2018

jluttine commented Mar 15, 2018

teto commented Mar 15, 2018

lheckemann commented Apr 27, 2017 •

edited

Loading

garbas commented Apr 28, 2017 •

edited

Loading

lheckemann commented May 9, 2017 •

edited

Loading

disassembler commented Nov 1, 2017 •

edited

Loading