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

Jump to bottom

kakoune: rework plugin support #91792

Merged
merged 2 commits into from Nov 1, 2020
Merged

kakoune: rework plugin support #91792

merged 2 commits into from Nov 1, 2020

Conversation

jcpetruzza
Copy link
Contributor

@jcpetruzza jcpetruzza commented Jun 29, 2020 •
edited

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    . So we set the newly added KAKOUNE_RUNTIME
    environment variable.

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.

@ofborg ofborg bot requested a review from vrthra June 29, 2020 22:03
@joefiorini
Copy link
Contributor

@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
Merged
10 tasks
eraserhd
eraserhd suggested changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@eraserhd eraserhd left a comment

Choose a reason for hiding this comment

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

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.

@eraserhd eraserhd mentioned this pull request Aug 4, 2020
Merged
@jcpetruzza
Copy link
Contributor Author

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.

@eraserhd
Copy link
Contributor

@jcpetruzza I assume most people are currently using kakoune, rather than kakoune-unwrapped, so this alias won't help them. I merged this PR locally, and I needed to change quite a few things. See here: eraserhd/dotfiles@a9e8c5d

I get the naming thing, and that's really unfortunate.

<rant/>Names are the principle method of abstraction in functional programming, and they should tell the caller their contract and hide the implementation details if possible.... hrmmm... <rant/>

Ok, how about kakoune-without-plugins and kakoune, with a deprecated alias kakoune-unwrapped for kakoune-without-plugins and a deprecated function wrapper wrapKakoune?

(Also, this PR now has conflicts from the recent release.)

@eraserhd
Copy link
Contributor

@jcpetruzza Hey, this would be nice to get merged... what do you think of the package renaming? Also, there's now a merge conflict to resolve.

@jcpetruzza
Copy link
Contributor Author

Yes, it'd be good to get this merged; it's been on my TODO list for a while, sorry! I'll be afk for the next two weeks, happy to look into it after that!

Notice that nix-community/home-manager#1356 recently added support for kakoune plugins (so this now works via home-manager) but it requires kakaoune-unwrapped to work, so removing it will require some coordination.

I still think we should deprecate it, and use kakoune for plain kak, it's the most obvious name. Using kakoune-with-plugins makes it more easy to discover. We can probably make it so that kakoune.plugins [...] also works, to make all backwards compatible.

@jcpetruzza
Copy link
Contributor Author

@eraserhd, I've updated the PR. Notable changes:

  • I tried to preserved backwards compatibility, so kakoune-unwrapped is still there, and kakoune can be overriden with either { plugins = [ foo ]; } or { configure = { plugins = [ foo ]; }; } as before.
  • No longer uses the hack of copying the binary, but now relies on KAKOUNE_RUNTIME (thanks for that, btw! 🙂)
  • I "fixed" the issue with :doc not working with plugins you found. It seems kak doesn't like the doc files to be symlinks. It looks like a bug to me, but worked it around for now by copying the content of the files.

Let me know what you think?

@joefiorini If you are able to try the new version on macOS again, that'd be great!

@eraserhd
Copy link
Contributor

Code looks good to me. I'm currently bisecting Darwin breakages this morning, so I haven't tested yet. Will post back when that's resolved.

@eraserhd
Copy link
Contributor

eraserhd commented Oct 26, 2020 •
edited

Took me a while to get a chance to look at it, but it is working nicely for me!

@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-already-reviewed/2617/269

@jcpetruzza
kakoune: rework plugin support
5503893 
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 derivation that includes the plugins simply by doing a
`symlinkJoin` of `kakoune-unwrapped` 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. By default kakoune expects the `autoload` directory to be in
     `../share/kak/autoload` relative to the location of the `kak` binary.
     We need to set the `KAKOUNE_RUNTIME` to point the symlinked
     share/kak for this to work.
@jcpetruzza
kakoune: Fix :doc when using plugins
464804b 
So, kakoune takes the documentation shown for `:doc` from
`$KAKOUNE_RUNTIME/share/kak/doc`. Unfortunately, it seems
that it will ignore files that are symlinks. This is arguably
a bug in kakoune, we workaround it for now by copying the
content of the docfiles.
@kevincox kevincox merged commit ce3a2e6 into NixOS:master Nov 1, 2020
berbiche added a commit to berbiche/home-manager that referenced this pull request Nov 6, 2020
@berbiche
kakoune: fix tests
4cc1b77 
Following NixOS/nixpkgs#91792
@berbiche berbiche mentioned this pull request Nov 6, 2020
Merged
7 tasks
malte-v pushed a commit to malte-v/home-manager that referenced this pull request Feb 24, 2021
@berbiche
kakoune: fix tests
d0d8e08 
Following NixOS/nixpkgs#91792
aakropotkin pushed a commit to aakropotkin/home-manager that referenced this pull request Feb 28, 2021
@berbiche @aakropotkin
kakoune: fix tests
94df693 
Following NixOS/nixpkgs#91792
cab404 pushed a commit to cab404/home-manager that referenced this pull request Apr 23, 2021
@berbiche @cab404
kakoune: fix tests
ec2d69f 
Following NixOS/nixpkgs#91792
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevincox kevincox kevincox left review comments

@eraserhd eraserhd eraserhd approved these changes

@vrthra vrthra Awaiting requested review from vrthra

Assignees
No one assigned
Labels
8.has: clean-up 8.has: documentation 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jcpetruzza @joefiorini @eraserhd @nixos-discourse @kevincox @ryantm