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

kakoune: rework plugin support #91792

Open
wants to merge 1 commit into
base: master
from

Conversation

@jcpetruzza
Copy link
Contributor

jcpetruzza commented Jun 29, 2020

Motivation for this change

The previous implementation of plugin-support for the kakoune derivation
was based on generating, at build time, a plugins.kak file that would
source all .kak files in the list of plugins, and wrap the kak binary
in a script that would add some command-line arguments so that this
file gets loaded on start-up. The main problem with this approach
is that the plugins' code get executed after the user's configuration
file is loaded, so effectively one cannot automatically activate/configure
these plugins.

The idiomatic way of loading plugins is ensuring they end up installed
somwhere under share/kak/autoload. Because plugins are already being
packaged to have their code in share/kak/autoload/plugins/<name-of-plugin>,
we can obtain a kakoune-with-plugins derivation simply by doing a
symlinkJoin of kakoune and all the requested plugins.

For this to work, we need to fix two issues:

  1. By default, kakoune makes share/kak/autoload a symbolic link to
    share/kak/rc, which contains all builtin definitions. We need
    to patch this to put the symlink under share/kak/autoload/rc, so that
    the join works.

  2. kakoune actually expects the autoload directory to be in
    ../share/kak/autoload relative to the location of the kak binary
    (and this can't currently be overriden, say, with environment
    variables), so we can't rely on a symlink for the kak binary and
    need to copy it instead.

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.
The previous implementation of plugin-support for the kakoune derivation
was based on generating, at build time, a `plugins.kak` file that would
source all .kak files in the list of plugins, and wrap the `kak` binary
in a script that would add some command-line arguments so that this
file gets loaded on start-up. The main problem with this approach
is that the plugins' code get executed *after* the user's configuration
file is loaded, so effectively one cannot automatically activate/configure
these plugins.

The idiomatic way of loading plugins is ensuring they end up installed
somwhere under `share/kak/autoload`. Because plugins are already being
packaged to have their code in `share/kak/autoload/plugins/<name-of-plugin>`,
we can obtain a `kakoune-with-plugins` derivation simply by doing a
`symlinkJoin` of `kakoune` and all the requested plugins.

For this to work, we need to fix two issues:

  1. By default, kakoune makes `share/kak/autoload` a symbolic link to
     `share/kak/rc`, which contains all builtin definitions. We need
     to patch this to put the symlink under `share/kak/autoload/rc`, so that
     the join works.

  2. kakoune actually expects the `autoload` directory to be in
     `../share/kak/autoload` relative to the location of the `kak` binary
     (and this can't currently be overriden, say, with environment
     variables), so we can't rely on a symlink for the `kak` binary and
     need to copy it instead.
@joefiorini
Copy link
Contributor

joefiorini commented Jul 28, 2020

@jcpetruzza FWIW, I have been using Kakoune on macOS installed with this patch for weeks now, it fixed a problem I had with the previous setup and seems to be working great. What is needed to get this merged?

@jcpetruzza jcpetruzza mentioned this pull request Aug 3, 2020
7 of 10 tasks complete
Copy link
Contributor

eraserhd left a comment

I think I (the original author of the plugin support being changed here) like this approach.

👍 Using symlinkJoin.
👍 Copying the kak binary instead of the big and potentially fragile wrapper script.
👎 Renaming the packages.

Renaming the packages will break existing users. kakoune-unwrapped was the name introduced by the original PR, based on a request from @teto and precedent with neovim. I'm happy to go along with a name change if Nix has decided on a de-facto naming convention for this pattern, but otherwise it seems arbitrary.

@eraserhd
Copy link
Contributor

eraserhd commented Aug 3, 2020

Also, I'm not sure what's going on, but the :doc command no longer autocompletes help pages, even though it does manage to find them. I'm not sure why.

@jcpetruzza
Copy link
Contributor Author

jcpetruzza commented Aug 8, 2020

Thanks for the feedback, @eraserhd!

I'm not sure what is the convention for naming packages in nixpkgs, tbh. My understanding was that the -unwrapped packages made sense when there were other versions of the same packages based on wrapping the binary with a shell script to select certain options. That way, the -unwrapped would serve as an entrypoint for that. Since with this change kak would no longer be based on a wrapper script, I though keeping the convention could be misleading. For what is worth, the convention of package and package-with-plugins is used elsewhere in nixpkgs too, e.g. with gimp.

The point about backwards compatibility is a very good one. If we were to go with kakoune/kakoune-with-plugins, we could perhaps makekakoune-unwrapped an alias to kakoune and mark it as deprecated.

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

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