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

@rnhmjoj rnhmjoj changed the title Urxvt rxvt-unicode: rewrite plugin system Jan 8, 2020
@rnhmjoj rnhmjoj force-pushed the rnhmjoj:urxvt branch from d41225a to 16c22b0 Jan 9, 2020
@rnhmjoj rnhmjoj marked this pull request as ready for review Jan 9, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Jan 9, 2020

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

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, ... }: {

This comment has been minimized.

@doronbehar

doronbehar Jan 10, 2020
Contributor

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?

This comment has been minimized.

@rnhmjoj

rnhmjoj Jan 10, 2020
Author Contributor

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.

This comment has been minimized.

@doronbehar

doronbehar Jan 11, 2020
Contributor

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, ... }: {

This comment has been minimized.

@doronbehar

doronbehar Jan 10, 2020
Contributor

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.

This comment has been minimized.

@rnhmjoj

rnhmjoj Jan 10, 2020
Author Contributor

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 ~)

This comment has been minimized.

@doronbehar

doronbehar Jan 11, 2020
Contributor

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?

This comment has been minimized.

@rnhmjoj

rnhmjoj Jan 15, 2020
Author Contributor

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>

This comment has been minimized.

@doronbehar

doronbehar Jan 10, 2020
Contributor

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.

This comment has been minimized.

@doronbehar

doronbehar Feb 10, 2020
Contributor

@rnhmjoj Could you please take care of this suggestion?

This comment has been minimized.

@rnhmjoj

rnhmjoj Feb 10, 2020
Author Contributor

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.

This comment has been minimized.

@doronbehar

doronbehar Feb 10, 2020
Contributor

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

@rnhmjoj rnhmjoj force-pushed the rnhmjoj:urxvt branch from 16c22b0 to 0228710 Jan 15, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Jan 29, 2020

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 added 6 commits Jan 8, 2020
@rnhmjoj
Copy link
Contributor Author

@rnhmjoj rnhmjoj commented Feb 10, 2020

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

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Feb 10, 2020

@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 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 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
Member

@worldofpeace worldofpeace commented Feb 10, 2020

@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 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
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
rxvt-unicode on aarch64-linux Success
Details
rxvt-unicode on x86_64-darwin Success
Details
rxvt-unicode on x86_64-linux Success
Details
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace 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.

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

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Feb 10, 2020

👍 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 rnhmjoj:urxvt branch Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.