vimPlugins.fzf-vim: automatically add fzfWrapper #52685

timokau · 2018-12-22T15:55:31Z

Motivation for this change

fzf-vim doesn't work without "regular" fzf. Currently that makes it necessary to manually add both fzf-vim and fzfWrapper.

I don't think the symlinkJoin approach is ideal, but it is the best I could come up with (not being very familiar with the vim plugin infrastructure). Just adding "fzfWrapper" to the dependencies of fzf-vim didn't work.

CC @andsild

Things done

Ma27 · 2019-02-17T10:29:52Z

Just adding "fzfWrapper" to the dependencies of fzf-vim didn't work.

Can you recheck, please? I just tried this with the following expression on master (dee617c):

with import ./. { };

vim_configurable.customize {
  name = "vim";
  vimrcConfig.vam = {
    knownPlugins = pkgs.vimPlugins;
    pluginDictionaries = [
      { name = "fzfWrapper"; }
      { name = "fzf-vim"; }
    ];
  };
}

With this VIM the commands :Files, :GFiles and :GFiles? work fine.

EDIT: I misunderstood your comment I guess, I'll check if this is still the case as well :)

Ma27 · 2019-02-17T10:34:42Z

I wrongly assumed that you meant the list of VIM plugins (i.e. in vim_configurable) with dependencies. In fact you're right though, adding fzf-vim should be sufficient, anything else is fairly counter-intuitive. I'll rebase your commit against master. If this is sufficient I'd merge :)

Ma27 · 2019-02-17T10:37:53Z

Hmm although the build itself works fine, the evaluation of a vim_configurable expression as pasted above fails with the following error:

error: attribute 'rtp' missing, at /home/ma27/Projects/nixpkgs/pkgs/misc/vim-plugins/vim-utils.nix:246:54
(use '--show-trace' to show detailed location information)

timokau · 2019-02-19T10:19:47Z

Makes sense, since buildVimPlugin isn't used and that adds the rtp attribute which is used by the vam and vim-plug implementations.

Maybe we can come up with a proper propagatedBuildInputs or something like that.

timokau · 2019-02-19T10:21:11Z

Thinking about it again: Shouldn't adding fzfWrapper to fzf-vim's dependencies be pretty much equivalent to manually adding fzfWrapper to the list of vim plugins?

timokau · 2019-02-19T11:50:40Z

Actually testing it again now, it does work to simply add fzfWrapper to the dependencies of fzf-vim. I probably opened this PR before looking deeper into the plugin system and fixing the dependency management for the native plugin implementation (#52767).

Can you confirm that this also works for you @Ma27?

Ma27

Unfortunately this doesn't work for me (with fixed evaluation), :Files still breaks in a vim configured by vim_configurable with fzf-vim loaded as plugin.

pkgs/misc/vim-plugins/overrides.nix

timokau · 2019-02-19T17:04:21Z

It works with packages but fails with vam. Adding fzf-vim to the dependencies generates this vam vimrc:

let g:nix_plugin_locations = {}
let g:nix_plugin_locations['fzf-vim'] = "/nix/store/fkp4swn162iq7pp309srkp6z0sgl2fk6-vimplugin-fzf-vim-2018-12-11/share/vim-plugins/fzf-vim"
let g:nix_plugin_locations['fzf'] = "/nix/store/a9qnz59i7xgjlh57kg5jybqmdizqd3pb-vimplugin-fzf-0.17.5/share/vim-plugins/fzf"

let g:nix_plugin_locations['vim-addon-manager'] = "/nix/store/1xh8pflfs94f0cmii7cfq5j11k6by2aw-vimplugin-vim-addon-manager-2018-07-27/share/vim-plugins/vim-addon-manager"
[...]
call add(l, {'name': 'fzf-vim'})

while adding it manually to the dictionaries generates this:

let g:nix_plugin_locations = {}
let g:nix_plugin_locations['fzf-vim'] = "/nix/store/fkp4swn162iq7pp309srkp6z0sgl2fk6-vimplugin-fzf-vim-2018-12-11/share/vim-plugins/fzf-vim"
let g:nix_plugin_locations['fzfWrapper'] = "/nix/store/a9qnz59i7xgjlh57kg5jybqmdizqd3pb-vimplugin-fzf-0.17.5/share/vim-plugins/fzf"
[...]
call add(l, {'name': 'fzf-vim'})
call add(l, {'name': 'fzfWrapper'})

So adding it to the deps uses the pname, while adding it manually uses the attribute name. Also, the call line isn't properly generated.

fzfWrapper is needed for fzf-vim to work.

timokau · 2019-02-19T17:11:21Z

This is probably something I messed up in my refactor. I pushed a commit that works at least in this case. However with vam, I really don't know what I'm doing. I removed the "vam plugin -> toNix" thing completely and replaced it by a simple name dictionary. That is probably not valid in some cases.

Ma27 · 2019-02-19T22:10:54Z

WIth the latest patches the fzf-vim problem is solved with vam as well. I also tested your change against my rather complex ViM setup, so there's a certain chance that your vam change is correct :)

Pinging @Mic92 as he worked quite much on the vim-plugins sub-package.

Mic92 · 2019-02-22T09:48:52Z

I wonder if we should reduce the number of plugin manager we support?
Are there use cases for having all of them?

Mic92 · 2019-02-22T09:53:58Z

Building and running all the vim tests seems to work:

$ nix-build -A vimUtils

Ma27 · 2019-02-22T09:58:49Z

I wonder if we should reduce the number of plugin manager we support?

No idea admittedly. I've been using vam for ~2 years now and I never had a reason to change that.

Mic92 · 2019-02-22T10:53:38Z

pkgs/misc/vim-plugins/overrides.nix

@@ -27,6 +27,9 @@ self: super: {
    dependencies = with super; [ vim-addon-manager ];
  };

+  # Mainly used as a dependency for fzf-vim. Wraps the fzf program as a vim
+  # plugin, since part of the fzf vim plugin is included in the main fzf
+  # program.
  fzfWrapper = buildVimPluginFrom2Nix {


Do we need to expose it then to the user?

Just having fzf-vim there would create less confusion what the right package is.

I think it makes sense to keep it because

Backwards compatibility. No need to break the configuration of some users who have added fzfWrapper.

According to upstream's readme:

fzf in itself is not a Vim plugin, and the official repository only provides the basic wrapper function for Vim and it's up to the users to write their own Vim commands with it

So that sounds like fzfWrapper is intended to be used by end-users. To be honest I doubt that there are many users that do this, but it makes sense to support.

In that case I'd keep fzfWrapper for now (at least) and do a proper deprecation phase.

timokau · 2019-02-22T19:44:13Z

I wonder if we should reduce the number of plugin manager we support?
Are there use cases for having all of them?

With my refactors of the plugin infrastructure, I've noticed that pretty much every corner case is used by somebody (https://xkcd.com/1172/). While I wouldn't be opposed to remove stuff, there should be a proper deprecation period and I don't think we have deprecation quite figured out yet.

Also there would be some bikeshedding to be done to select the "one true" implementation. VAM and "vim packages" are probably the only serious contenders. I'd argue that vim packages are the way to go nowadays (native upstream support, provides everything we need), but I'm sure some disagree. Also most "legacy" configurations will be VAM.

Ma27 · 2019-02-25T00:40:44Z

While I wouldn't be opposed to remove stuff, there should be a proper deprecation period and I don't think we have deprecation quite figured out yet.

Did you encounter any behavioral differences? Or is it possible that your changes in the plugin attr set breaks functionality?

VAM and "vim packages" are probably the only serious contenders. I'd argue that vim packages are the way to go nowadays (native upstream support, provides everything we need), but I'm sure some disagree. Also most "legacy" configurations will be VAM.

If this simplifies things and the migration is not too hard, I'd say 👍 Unless I'm missing something, there shouldn't a big advantage from VAM.

Do you want to open an issue to discuss about this?

timokau · 2019-02-25T11:06:44Z

While I wouldn't be opposed to remove stuff, there should be a proper deprecation period and I don't think we have deprecation quite figured out yet.

Did you encounter any behavioral differences? Or is it possible that your changes in the plugin attr set breaks functionality?

Behavioral differences between what? I haven't tested the VAM version thoroughly (i.e. I just made sure it starts and :Files works). Its very possible that the change from toNix p to just the name breaks functionality. I don't know what that call is used for. It seemed like in practice, only the name ends up in there anyways. But I haven't done the research to confirm that.

I've used the old symlinkJoin version with vim packages since I've opened this PR, without any issues or behavioral differences.

VAM and "vim packages" are probably the only serious contenders. I'd argue that vim packages are the way to go nowadays (native upstream support, provides everything we need), but I'm sure some disagree. Also most "legacy" configurations will be VAM.

If this simplifies things and the migration is not too hard, I'd say +1 Unless I'm missing something, there shouldn't a big advantage from VAM.

Do you want to open an issue to discuss about this?

I've opened #56338. I do not intend to remove anything myself though, as I don't want to deal with the inevitable fallout of people complaining.

Ma27 · 2019-02-25T12:56:14Z

Behavioral differences between what? I haven't tested the VAM version thoroughly (i.e. I just made sure it starts and :Files works). Its very possible that the change from toNix p to just the name breaks functionality. I don't know what that call is used for. It seemed like in practice, only the name ends up in there anyways. But I haven't done the research to confirm that.

I wanted to know if you observed any differences (or breakage) after changing the toNix expression.

I can have a more detailed look at this at the end of the week, but I'm not sure about the consequences of this change as well ATM.

lrworth · 2019-03-07T22:22:41Z

Just dropping by. I haven't tried this branch, but in nixpkgs-unstable right now, it looks like there's a missing dependency on actual fzf, so :Files prompts to download a binary. nix-env -i fzf fixes it.

Ma27

Just dropping by. I haven't tried this branch, but in nixpkgs-unstable right now, it looks like there's a missing dependency on actual fzf, so :Files prompts to download a binary. nix-env -i fzf fixes it.

You're right, this probably got unnoticed, at least I have fzf installed on my system. @timokau I assume this could be fixed by simply patching the Vim script's sources?

Judging from the sources, I currently don't think that this actually breaks a user's setup, we simply add transitive dependencies (if not specified manually in pluginDictionaries), so if we hit an edge case, it's probably because of wrongly specified dependencies in overrides.nix.

I tested this with my historically-grown Vim config and didn't encounter further issues. Also I tested several plugins with dependencies (fzf-vim, vim-bazel, ensime-vim) and the patch seems fine.

I'd suggest we merge this for now (not sure if this should be backported) and wait for further feedback.

Ma27 · 2019-03-25T00:57:33Z

pkgs/misc/vim-plugins/vim-utils.nix

@@ -291,7 +291,7 @@ let

        " tell vam about which plugins to load when:
        let l = []
-        ${lib.concatMapStrings (p: "call add(l, ${toNix p})\n") vam.pluginDictionaries}
+        ${lib.concatMapStrings (p: "call add(l, {'name': '${p.pname}'})\n") plugins}


With the removal of that reference, toNix is not needed anymore.

zimbatm · 2019-08-07T10:00:45Z

Is anything holding back this PR?

Ma27 · 2019-08-07T11:26:14Z

Ouch, I totally forgot about that one, sorry :/

I fixed this locally and didn't encounter the problem since then. From my perspective this appears fine, @Mic92 @timokau opinions?

zimbatm · 2019-08-07T11:56:04Z

No worries, I just hit the same issue today so finding this PR was quite helpful actually.

What will happen to users who already have both plugins in their list, will it de-duplicate?

Ma27 · 2019-08-07T11:57:22Z

I'm not entirely sure anymore as it's some time ago that I had a closer look at this. Let me check what happens tonight on my setup ok? :)

timokau · 2019-08-07T16:57:25Z

I don't remember the details with the different plugin managers, but if this works for everyone (or at least doesn't break anything), we can merge. Can't believe it has been 8 months already, didn't seem that long ago.

Personally I've been using the original symlinkJoin approach in my vim packages based configuration and kind of forgot about it.

Ma27

As requested by @zimbatm I tested if this works with the old configuration (it appears to deduplicate) and with a vim config that only uses fzf-vim. As this is a pretty confusing bug and I guess that it took most of us to find out the cause (or this PR) I guess that it's a good thing to merge this now 👍

Ma27 · 2019-08-11T15:16:44Z

@timokau thanks!

GrahamcOfBorg added 6.topic: vim 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 22, 2018

timokau force-pushed the fzf-vim-dep branch from db330e5 to 3ff7474 Compare February 19, 2019 11:54

Ma27 reviewed Feb 19, 2019

View reviewed changes

pkgs/misc/vim-plugins/overrides.nix Outdated Show resolved Hide resolved

timokau added 2 commits February 19, 2019 18:09

vimPlugins.fzf-vim: add fzfWrapper dependency

d55e92d

fzfWrapper is needed for fzf-vim to work.

vimPlugins: fix vam dependency handling

15bb628

timokau force-pushed the fzf-vim-dep branch from 3ff7474 to 15bb628 Compare February 19, 2019 17:10

Mic92 reviewed Feb 22, 2019

View reviewed changes

timokau mentioned this pull request Feb 25, 2019

Decide on a single vim plugin infrastructure #56338

Open

Ma27 reviewed Mar 25, 2019

View reviewed changes

Ma27 approved these changes Aug 11, 2019

View reviewed changes

Ma27 merged commit 12a7cc0 into NixOS:master Aug 11, 2019

timokau deleted the fzf-vim-dep branch August 11, 2019 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vimPlugins.fzf-vim: automatically add fzfWrapper #52685

vimPlugins.fzf-vim: automatically add fzfWrapper #52685

timokau commented Dec 22, 2018

Ma27 commented Feb 17, 2019 •

edited

Loading

Ma27 commented Feb 17, 2019

Ma27 commented Feb 17, 2019

timokau commented Feb 19, 2019

timokau commented Feb 19, 2019

timokau commented Feb 19, 2019

Ma27 left a comment

timokau commented Feb 19, 2019 •

edited

Loading

timokau commented Feb 19, 2019

Ma27 commented Feb 19, 2019

Mic92 commented Feb 22, 2019

Mic92 commented Feb 22, 2019

Ma27 commented Feb 22, 2019

Mic92 Feb 22, 2019

Mic92 Feb 22, 2019

timokau Feb 22, 2019

Ma27 Feb 25, 2019

timokau commented Feb 22, 2019

Ma27 commented Feb 25, 2019

timokau commented Feb 25, 2019

Ma27 commented Feb 25, 2019

lrworth commented Mar 7, 2019 •

edited

Loading

Ma27 left a comment

Ma27 Mar 25, 2019

zimbatm commented Aug 7, 2019

Ma27 commented Aug 7, 2019

zimbatm commented Aug 7, 2019

Ma27 commented Aug 7, 2019

timokau commented Aug 7, 2019

Ma27 left a comment

Ma27 commented Aug 11, 2019

vimPlugins.fzf-vim: automatically add fzfWrapper #52685

vimPlugins.fzf-vim: automatically add fzfWrapper #52685

Conversation

timokau commented Dec 22, 2018

Motivation for this change

Things done

Ma27 commented Feb 17, 2019 • edited Loading

Ma27 commented Feb 17, 2019

Ma27 commented Feb 17, 2019

timokau commented Feb 19, 2019

timokau commented Feb 19, 2019

timokau commented Feb 19, 2019

Ma27 left a comment

Choose a reason for hiding this comment

timokau commented Feb 19, 2019 • edited Loading

timokau commented Feb 19, 2019

Ma27 commented Feb 19, 2019

Mic92 commented Feb 22, 2019

Mic92 commented Feb 22, 2019

Ma27 commented Feb 22, 2019

Mic92 Feb 22, 2019

Choose a reason for hiding this comment

Mic92 Feb 22, 2019

Choose a reason for hiding this comment

timokau Feb 22, 2019

Choose a reason for hiding this comment

Ma27 Feb 25, 2019

Choose a reason for hiding this comment

timokau commented Feb 22, 2019

Ma27 commented Feb 25, 2019

timokau commented Feb 25, 2019

Ma27 commented Feb 25, 2019

lrworth commented Mar 7, 2019 • edited Loading

Ma27 left a comment

Choose a reason for hiding this comment

Ma27 Mar 25, 2019

Choose a reason for hiding this comment

zimbatm commented Aug 7, 2019

Ma27 commented Aug 7, 2019

zimbatm commented Aug 7, 2019

Ma27 commented Aug 7, 2019

timokau commented Aug 7, 2019

Ma27 left a comment

Choose a reason for hiding this comment

Ma27 commented Aug 11, 2019

Ma27 commented Feb 17, 2019 •

edited

Loading

timokau commented Feb 19, 2019 •

edited

Loading

lrworth commented Mar 7, 2019 •

edited

Loading