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

rxvt-unicode: rewrite plugin system #77347

Merged
merged 6 commits into from Feb 10, 2020
Merged

rxvt-unicode: rewrite plugin system #77347

merged 6 commits into from Feb 10, 2020

Conversation

rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Jan 8, 2020

Motivation for this change

The current way to add plugins to urxvt is not extensible and very much not user friendly: let's change that!

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Added documentation
  • Tested terminfo
  • Tested perl packages
  • Tested bidi
  • Determined the impact on package closure size (negligible)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @doronbehar

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/what-are-your-goals-for-20-03/4773/11

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Besides what I commented, this is definitely a state of the art packaging. Thanks for your contribution!

includes all available plugins. To make use of this functionality, use an
overlay or directly install an expression that overrides its configuration
such as
<programlisting>rxvt-unicode.override { configure = { availablePlugins, ... }: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I'm totally fond of this type of override. Why not go with the usual with<Plugin> scheme? I.e withVtwheel or withResizefont?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pretty much stole this design from the weechat package. You can see it in the nixpkgs manual It's a little verbose but it's completely configurable.

withVtwheel would require some kind of code generation or hard-coding a few combinations of plugins.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the same note of my confusion below, I tend to think it will be better... EDIT: if it'll end up as a matter of postInstall and + lib.optional it will be easy to do what ever a user may wish with an override such as:

postInstall = oldAttrs.postInstall + ''
  # shell code here
''

doc/builders/packages/urxvt.xml Show resolved Hide resolved
<literal>extraDeps</literal> and <literal>perlDeps</literal> can be used
to install extra packages. This is a handy way to provide dependencies to
your custom perl plugins or install some tools needed by a script.
<programlisting>rxvt-unicode.override { configure = { availablePlugins, ... }: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever the override scheme will go like eventually, I would like this part to list the required Perl / other dependencies for each plugin. The plugin I packaged once - bidi, requires fribidi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a plugin is perl module simply adding it to the plugins should just work because its closure already contains all the dependencies.
perlDeps is meant to add some perl modules you may need if you are testing/working on a custom plugin (not installed but somewhere in ~)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right I'm confused, why are there also plugins, perlDeps, and extraDeps if any given derivation which has a file in it's $out/lib/urxvt/perl/ will pull all of it's needed dependencies if added to plugins? It seems overly complicated now.

BTW 1 more thing I've noticed is that urxvt_bidi, the plugin I contributed in the past, is generally a normal perl module, the only thing it has relevant for urxvt is just another file that is installed with postInstall, see:

https://github.com/rnhmjoj/nixpkgs/blob/16c22b01b5e706cc0e3b5371ae5409977e618ed8/pkgs/applications/misc/rxvt-unicode-plugins/urxvt-bidi/default.nix#L15-L17

Yet in the old plugins configuration system, it needed to be added also to perlDeps in addition to plugins... I'm just trying to understand what makes it an exception and perhaps we could overcome this. Perhaps putting the external dependency fribidi (which was the original trouble maker) to propagatedBuildInputs might help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right I'm confused, why are there also plugins, perlDeps, and extraDeps if any given derivation which has a file in it's $out/lib/urxvt/perl/ will pull all of it's needed dependencies if added to plugins? It seems overly complicated now.

I've rephrased the explanation I give in the manual and added two examples. I hope it's clear now. The idea is basically:

  • plugins: to add urxvt plugins
  • extraDeps: to add any kind of package that it's not specifically a plugin to the wrapper
  • perlDeps: to add Perl packages/modules so that you can use them in your plugins, which are not packaged in nixpkgs but are in ~/.urxvt/ext

BTW 1 more thing I've noticed is that urxvt_bidi

What's special about urxvt_bidi is that it's a Perl package (you built it with buildPerlPackage) and that it needs to be added to the PERL5LIB path. Other plugins are simple Perl scripts that urxvt looks for in the URXVT_PERL_LIB path.

</programlisting>
This will make the urxvt wrapper pick up the dependency and set up the perl
path accordingly.
</para>
Copy link
Contributor

Choose a reason for hiding this comment

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

On the same note as in the comment just above this, It'd be nice if another paragraph will kindly ask packagers to document the required dependencies for their plugin, if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rnhmjoj Could you please take care of this suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you mean documenting dependecies of the user, I don't think there will be any need: with this method users don't have to worry about them. All a package author has to do is to specify the the dependencies like in any other derivation (buildInputs, patching shebangs, whatever..). The only special case I can think of is urxvt-bidi, which requires setting a special value in the .passthru to signal it depends on the perl module it self provides.

Copy link
Contributor

Choose a reason for hiding this comment

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

O.K, nice :) Feel free to mark this conversation and the others as resolved.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/110

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Feb 10, 2020

So, @worldofpeace can this make it into 20.03 or it already too late?

@doronbehar
Copy link
Contributor

@rnhmjoj I'd like to approve this PR. It's kind of hard for me to read the new documentation while it's in the PR's changed files :) Could please just show me an example of an override for the urxvt perhaps with bidi as a plugin so I could easily verify that it works and all?

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Feb 10, 2020

I'd like to approve this PR. It's kind of hard for me to read the new documentation while it's in the PR's changed files :)

Yeah, docbook itsn't as nice as markdown to look at. Here's the compiled manual for nixpkgs: manual.zip. Look for the urxvt section.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Feb 10, 2020

Could please just show me an example of an override for the urxvt perhaps with bidi as a plugin so I could easily verify that it works and all?

Installing bidi should simply be:

rxvt-unicode.override { configure = { availablePlugins, ... }: {
    plugins = [ availablePlugins.bidi ];
  }
}

@worldofpeace
Copy link
Contributor

@rnhmjoj I couldn't review for lack of time, but being you are a committer now you don't have to await for my approval. I have no rejections if you can try to merge this before https://discourse.nixos.org/t/nixos-20-03-feature-freeze/5655/31

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Feb 10, 2020

I have no rejections if you can try to merge this before

I was asking you as a release manager. If it's all right I'll go on and merge this: I'm fairly confident in it as I tested the changes several times by now.

@rnhmjoj rnhmjoj merged commit 565724c into NixOS:master Feb 10, 2020
@worldofpeace
Copy link
Contributor

I have no rejections if you can try to merge this before

I was asking you as a release manager. If it's all right I'll go on and merge this: I'm fairly confident in it as I tested the changes several times by now.

You opened it in Jan 10 so it does belong in 20.03 in my opinion. Thanks for doing this 👍

@doronbehar
Copy link
Contributor

👍 Works.

arcnmx added a commit to arcnmx/nixexprs that referenced this pull request Feb 16, 2020
Adapt to plugin system introduced in
NixOS/nixpkgs#77347
arcnmx added a commit to arcnmx/nixexprs that referenced this pull request Feb 16, 2020
Adapt to plugin system introduced in
NixOS/nixpkgs#77347
@rnhmjoj rnhmjoj deleted the urxvt branch April 7, 2020 12:43
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.

None yet

4 participants