nixos/weechat: add module #45728
nixos/weechat: add module #45728
Conversation
|
@timokau after yesterday's discussion I was motivated to have a look at it. Seems as it's sufficient to load scripts by specifying their absolute paths (means, store paths :D). Would be cool if you could have a look at this. @yegortimoshenko as I wanted to hack on a weechat module for NixOS I used your PR from January as base. It would be awesome if you could have a look at this! :) |
|
Success on aarch64-linux (full log) Attempted: weechat Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: weechat Partial log (click to expand)
|
| options = { | ||
| name = mkOption { | ||
| type = str; | ||
| description = "Path to the script in the derivation."; |
There was a problem hiding this comment.
I was thinking about "name of the script", but well, there are only two hard things in computer science, right? ;)
But seriously, you're right, path is much better, however this can be dropped as you stated later on.
| default = []; | ||
| example = literalExample '' | ||
| [ | ||
| { name = "share/jabber.py"; script = pkgs.weechat-xmpp } |
There was a problem hiding this comment.
Why not just "${pkgs.weechat-xmpp}/share/jabber.py"? Or maybe even force the script packages to be in a standard format and only require the package.
There was a problem hiding this comment.
you're absolutely right! I originally wanted to define a pre-defined output structure for derivations which feature weechat scripts, but dropped the idea later and kept the (admittedly too complex submodule type).
| ''; | ||
| }; | ||
| sessionName = mkOption { | ||
| description = "Name of the `screen' session for weechat."; |
There was a problem hiding this comment.
I think it is better to use normal quotes here. I don't think the tex-like backtick/normal tick combination is very common in plain text.
There was a problem hiding this comment.
not sure though. I've seen this fairly often in the Nix(OS) context including descriptions for options and wanted to be convenient. But feel free to correct me, I'm not sure about this :)
There was a problem hiding this comment.
You're right, I can find a couple of examples of both simple quotes and ' quotes in man configuration.nix`. I personally prefer simple quotes but since there is apparently no consensus I'm fine with whatever you prefer.
| wantedBy = [ "multi-user.target" ]; | ||
| }; | ||
|
|
||
| services.weechat.init = mkIf (builtins.length cfg.scripts != 0) (concatStringsSep "\n" |
There was a problem hiding this comment.
Is this mkif really necessary? Wouldn't it just be an empty string in the case of length 0?
There was a problem hiding this comment.
You're absolutely right, this makes the expression way easier to read, thanks!
| }; | ||
|
|
||
| services.weechat.init = mkIf (builtins.length cfg.scripts != 0) (concatStringsSep "\n" | ||
| (map (script: "/script load ${script.script}/${script.name}\n") cfg.scripts)); |
There was a problem hiding this comment.
I don't have very much experience with the module system and merging and everything -- will these scripts be loaded before the rest of the init code is executed? I think they should be.
There was a problem hiding this comment.
I'm not sure either, but you're right, it to avoid breaking scripts it might be helpful to use mkBefore to be absolutely sure :)
| systemd.services.weechat-apply-init = let | ||
| initFile = pkgs.writeText "weechat-init" cfg.init; | ||
| in { | ||
| script = "sed 's/^/*/' ${initFile} > ${cfg.root}/weechat_fifo"; |
There was a problem hiding this comment.
according to the docs (https://weechat.org/files/doc/stable/weechat_user.en.html#fifo_plugin), the * at the beginning of a string is needed when passing commands to weechat through weechat_fifo. This was actually introduced in the previous commit I didn't author, but it's more convenient to type /cmd args... without the star at the beginning.
There was a problem hiding this comment.
Oh so the sed script is replacing the nothing at the beginning of each line with a literal *. I thought that was some kind of special syntax or something. We could do the same with the /, I'm unsure about that though. Should be fine either way as long as the expected format is documented.
There was a problem hiding this comment.
I just used the code from the previous commit. However I think that this is the right way to go as the star is only used with fifo.
However the / at the beginning is also used when running weechat from an active session, so it's IMHO more convenient to keep this.
|
Thanks! Any reason you went with a nixos module? I feel like adding the configuration through a package wrapper may be nicer. |
I originally wanted to make the weechat module of @yegortimoshenko ready to merge and wanted to see if it's possible to easily add support for scripts (which is easily doable by adding I'm not sure if we want to duplicate the |
Ma27
force-pushed
the
nixos/weechat-module
branch
from
11bcb85
to
299926a
Compare
|
Success on aarch64-linux (full log) Attempted: weechat Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: weechat Partial log (click to expand)
|
|
How does that There is the
In that case we wouldn't need the |
Ma27
force-pushed
the
nixos/weechat-module
branch
from
299926a
to
2cea1d7
Compare
|
Success on aarch64-linux (full log) Attempted: weechat Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: weechat Partial log (click to expand)
|
|
I learned about However |
|
Of course, looking forward to it :) |
Ma27
force-pushed
the
nixos/weechat-module
branch
from
2cea1d7
to
abf188c
Compare
|
@timokau your idea seems fairly reasonable, just implementeda draft for this :) |
|
btw, do you think we can sneak this into 18.09? :) |
doc/package-notes.xml
Outdated
| init = '' | ||
| /set foo bar | ||
| /server add freenode chat.freenode.org | ||
| /even |
There was a problem hiding this comment.
The first two examples are probably enough 😄
doc/package-notes.xml
Outdated
| The <literal>configure</literal> method can be used to pass commands to the program: | ||
| <programlisting>weechat.override { | ||
| configure = { availablePlugins, ... }: { | ||
| plugins = builtins.attrValues availablePlugins; |
There was a problem hiding this comment.
Maybe an example that doesn't install all plugins would be better / more realistic.
There was a problem hiding this comment.
Or is this actually the list of available plugins? If so, why have both that and the availablePlugins parameter?
There was a problem hiding this comment.
this is the original purpose of the configure parameter. You override weechat and tell it which plugins to use (or override one, e.g. use Python with several modules loaded).
This is still needed if you specify configure (which is why I added this line to the docs to provide a working example), but this was implemented before.
There was a problem hiding this comment.
Oh right, I forgot that plugins and scripts are two entirely different things. Maybe that should be made explicit in the docs. So this actually just adds all available plugins?
There was a problem hiding this comment.
You're right, the terms script and plugin seem to be the same thing, I made the mistake several times as well.
However I don't think that it's needed to add another sentence here, when you have a look at the WeeChat docs in the nixpkgs manual, you'll see that how to configure and use plugins with the Nix package is explained in the previous paragraph.
There was a problem hiding this comment.
Right, its easy to get lost in diffs and miss the context. However I think a short aside like "note that plugins are not the same as scripts. See the scripts section for more information" or something when plugins are first introduced may be useful.
There was a problem hiding this comment.
Also does that mean that no plugins are installed if I pass in a config without specifying plugins? The default (all plugins) should probably be kept if plugins aren't specified.
There was a problem hiding this comment.
The code before which configures a weechat environment with the specified plugins explicitly expects a plugin attr which returns a list of plugins to be activated. Unless they're given, an error will be thrown.
The init and scripts attrs are completely optional.
There was a problem hiding this comment.
Why not change inherit (config) plugins to plugins = config.plugins or availablePlugins? Requiring that argument made sense when it was the only argument but makes less sense now. I suspect most people don't care much about a couple of MB in the closure size of an end user application.
doc/package-notes.xml
Outdated
| </para> | ||
| <para> | ||
| The addition of the <literal>init</literal> parameter to the <literal>override</literal> API | ||
| of WeeChat allows <literal>Nix</literal> to generate packages that autoconfigure themselves. |
There was a problem hiding this comment.
You can set values for your scripts before weechat actually starts. But you're right, re-reading it makes the sentence quite odd 😅
There was a problem hiding this comment.
So basically just "you can configure WeeChat"? I think the formulation should be improved, at least I'm having a hard time with it.
doc/package-notes.xml
Outdated
| plugins = builtins.attrValues availablePlugins; | ||
|
|
||
| scripts = [ | ||
| "${pkgs.weechat-xmpp}/share/jabber.py" |
There was a problem hiding this comment.
Standardization would be really nice, but not a must in this PR. Without it discoveribility suffers though.
There was a problem hiding this comment.
ack! I may have made the mistake that I started packaging scripts before a proper weechat support was implemented. I'll keep that in mind, we may want to use a metapackage like weechatScripts anyway:)
There was a problem hiding this comment.
Yeah something like
scripts = with pkgs.weechatScripts [
xmpp
matrix-protocol
]
Would be pretty neat. It would be possible to remain backwards compatibility, although I'm not sure if its worth it.
There was a problem hiding this comment.
I'm not sure, but I'd even say that this is out of scope. This PR already includes script/config support for the package and a module and I'd say that splitting single scripts into their own metapackage should be done in a second PR.
Regarding the change itself: I'm definetely in favor of it, users don't need to know about the structure inside a given store path, they simply need to know their packages, furthermore this is way more readable IMHO.
Regarding backwards compatibility: I've seen several people using these packages, so I think that just moving them is a bad idea. Let's place them into pkgs/top-level/aliases.nix and print a deprecation warning if this attribute path is used.
There was a problem hiding this comment.
Yeah as I said, its not a must-have for this PR. But do consider that changing it afterwards would be harder because of backwards compatibility considerations. Once users start using scripts = <list of store paths>, it would be harder to change that. Also I would assume that there currently are not many users of the already packaged scripts (considering the lack of support in actually loading them). So changes are still easy but will get less so over time.
Regarding the scope of this PR: I'm honestly not entirely convinced by the usefulness of the service module, but I'm fine with it if you think its useful.
There was a problem hiding this comment.
Oh I should've read your entire comment first. If you've seen people using them already you could keep the current structure but also add a symlink to a standardized location (and the alias).
There was a problem hiding this comment.
good point. However I'd much rather keep support for strings as input for scripts, especially on non-NixOS systems with Nix as package manager this may be helpful I guess, so we wouldn't break user's setups.
There was a problem hiding this comment.
Scripts outside of the nix store would be an impurity that would make the config less portable. I see how it could be useful however, so if you want to support that its fine by me.
There was a problem hiding this comment.
I guess you're right, packaging weechat scripts shouldn't be too hard.I'll see how much time it costs to implement this, then I may add this to this PR.
|
|
||
| init = let | ||
| init = config.init or ""; | ||
| scripts = builtins.concatStringsSep "\n" (map (s: "/script load ${s}") (config.scripts or [])); |
There was a problem hiding this comment.
Why first concat with newlines just to replace them by ;?
| in builtins.replaceStrings [ "\n" ] [ ";" ] (scripts + init); | ||
|
|
||
| mkWeechat = bin: (writeScriptBin bin '' | ||
| #!${stdenv.shell} |
There was a problem hiding this comment.
Shouldn't makeWrapper be capable of all this?
There was a problem hiding this comment.
Maybe its easier without makeWrapper though, not sure.
| #!${stdenv.shell} | ||
| export WEECHAT_EXTRA_LIBDIR=${pluginsDir} | ||
| ${lib.concatMapStringsSep "\n" (p: lib.optionalString (p ? extraEnv) p.extraEnv) plugins} | ||
| exec ${weechat}/bin/${bin} --run-command "${init}" "$@" |
There was a problem hiding this comment.
If subsequent --run-commands overwrite the first one, maybe we could check for --run-command or -r in $@ and concat the two. It would add a bit of complexity but just silently overwriting may confuse users.
Not sure what the policy for the "feature freeze" is. I'd be in favor (since this doesn't update weechat and is unlikely to break anything) but we should ask one of the release managers once this is merged. |
Ma27
force-pushed
the
nixos/weechat-module
branch
from
abf188c
to
306791e
Compare
|
@timokau thanks a lot for your feedback! Would you mind having a look at this a second time? :) |
Ma27
force-pushed
the
nixos/weechat-module
branch
from
306791e
to
2e2b2e8
Compare
|
@timokau I added myself as maintainer and introduced the |
Ma27
force-pushed
the
nixos/weechat-module
branch
from
712eb03
to
f5becfb
Compare
GrahamcOfBorg
added
8.has: clean-up
10.rebuild-linux: 1-10
and removed
10.rebuild-linux: 0
labels
|
Success on aarch64-linux (full log) Attempted: weechat Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: weechat Partial log (click to expand)
|
|
Great, thank you for your patience :) Looks good now. Just FYI, GitHub doesn't send email notifications when you force-push. I only noticed because of ofBorgs rebuilds. |
|
@samueldr can we include this in 18.09? It
Since weechat doesn't have reverse dependencies and breakage is very unlikely I think it should be included. What do you think? |
|
#46324 should also be included in 18.09 if we include this. |
|
Oof, the backport policies are really about updating software, and updating software revisions for non-major updates. I don't know if a new module should be backported. it's kind of a lot of things, and the package has deviated considerably compared to how it worked prior to this PR. SInce nix, nixos and modules can be composed pretty easily, I'm not sure this should be backported. Though the version update (only) could and probably should be backported. (Though it does not seem to have security fixes, it does have stability fixes.) I'm not strictly against backporting this, but I'm not sure it should be. I'm don'T know about previous precedents of backports of non-trivial module updates. And then this would create such a precedent. Thoughts, @vcunat? |
|
On 18-09-07 19:47, Samuel Dionne-Riel wrote:
Oof, the backport policies are really about updating software, and updating software revisions for non-major updates. I don't know if a new module should be backported. it's kind of a lot of things, and the package has deviated considerably compared to how it worked prior to this PR.
Yes but the most "risky" changes are all additions, so no regressions. And since weechat is an end-user application and not a library, potential for breakage is limited and can be tested pretty quickly.
SInce nix, nixos and modules can be composed pretty easily, I'm not sure this should be backported.
Can they? I haven't found a good way to compose modules from different NixOS versions yet. For me the most important part of this is the scripts support, which can be used by simply fetching the weechat package from unstable. So I don't care very strongly about backporting it, but I still think it would be better. I understand the arguments against it however -- feature freeze is feature freeze.
… --
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub:
#45728 (comment)
|
|
Note that while #46324 was a breakage, it was not a regression. Just a small bug in the newly added features. |
|
@samueldr: it's just a new module, right? Those who don't enable it shouldn't be affected at all by that. |
|
A new module and configurability added to the weechat package. Still shouldn't affect anybody not using it. |
|
Sorry, I meant to reply earlier. I must have misread that it was changing an existing module. Probably by relying on the description "removing [...] adding support for [...]" instead of looking at the actual code. In that case it is a fine candidate for a backport I guess. |
No worries, we've learned at last NixCon that release management is quite a hard and time-consuming task :)
great! I'm currently using the changes with a recent 18.09 checkout personally, I'll recheck and file a PR later to do the backport, ok? |
|
Yes, you can tag me in the backport PR. Thanks for making the PR! |
|
Hey--how is this intended to be used? Anyone have an example? The documentation suggests this is geared so that you can attach to the screen session (command to do so is mentioned, and modification to screenrc suggested, etc.), but that is a non-starter AFAICT since screen needs to be suid for that to work and our screen is not. Which makes me wonder--given this was merged so recently... how are folks using it? Or is there a way to attach I'm missing (sorry!)? Or to run as a user service, perhaps? Thanks! |
|
Hey! Thanks a lot for you interest in this module! |
|
Sure thing! No hurry at all :). See you in a few! |
|
first of all sorry for the delay, I meant write earlier. After debugging I realised that I used a |
…er capabilities Previously you either had to set the setuid bit yourself or workaround `isSystemUser = true` (for a loginable shell) to access the weechat screen. `programs.screen` shouldn't do this by default to avoid taking too much assumptions about the setup, however `services.weechat` explicitly requires tihs. See NixOS#45728
…er capabilities Previously you either had to set the setuid bit yourself or workaround `isSystemUser = true` (for a loginable shell) to access the weechat screen. `programs.screen` shouldn't do this by default to avoid taking too much assumptions about the setup, however `services.weechat` explicitly requires tihs. See NixOS#45728 (cherry picked from commit 018573b)
Motivation for this change
Follow-up for #33523. Removed static uid/gid as proposed here (#33523 (comment)) and added support for loading custom scripts.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)