search

MarcWeber / vim-addon-manager

manage and install vim plugins (including their dependencies) in a sane way. If you have any trouble contact me. Usually I reply within 24 hours
Other
660 stars 59 forks source link

plugin after/ paths should be placed before user's after/ path on &rtp #165

Closed dbarnett closed 9 years ago

dbarnett commented 9 years ago

Vim's runtimepath should have the following categories in order:

  1. User's vim files.
  2. Plugin vim files.
  3. System vim files.
  4. System after/ files.
  5. Plugin after/ files.
  6. User's after/ files.

This lets the user run both first and last, and similarly gives plugin files priority over system files. See maktaba#rtp#Add, which tries to preserve that ordering.

VAM seems to always put plugin after/ paths at the very end, preempting the user's after/ path. The end of my runtimepath looks like:

  '/usr/share/vim/vim74/',
  '/usr/share/vim/vimfiles/after/',
  '/var/lib/vim/addons/after/',
  '/home/dbarnett/.vim/after/',
  '/home/dbarnett/.vim/vim-addons/vim-addon-manager/',
  '/home/dbarnett/.vim/vim-addons/UltiSnips/after/']

It has the wrong precedence and also confuses other runtimepath manipulation utilities like maktaba#rtp#Add that use heuristics to try to insert plugin after/ files in the right order. The vim-addon-manager entry itself also ends up too late in the order after the recommended VAM setup.

VAM should take more care to maintain a good runtimepath order.

MarcWeber commented 9 years ago

You have to think diffently about VAM's reccomendation about .vimrc and what VAM does. So maybe you want to prefix rtp by VAM's runtime path so that VAM can be loaded at all. Maybe everything is fine then for your case. Its hard if not impossible for VAM to know which after path should be latest. Maybe the best way would be adding another hook to allow the user to override the "add rtp" implementation.

dbarnett commented 9 years ago

I realize there's some challenge/ambiguity making it work properly, but I don't think the user will generally know better than their plugin manager how to handle rtp.

For the vim-addon-manager entry itself, I think it's fine that it's initially added in the wrong place, but VAM could detect and correct that wrong ordering in initialization.

MarcWeber commented 9 years ago

Eventually you should start talking about the "real" problem you have. What are you using maktaba#rtp#Add for which is causing your trouble?

dbarnett commented 9 years ago

The concrete problem is when a plugin defines say after/ftplugin/cpp.vim and you define ~/.vim/after/ftplugin/cpp.vim, and suddenly your settings are being overridden and it's impossible for you to fix.

My "real" problem is just that I want to recommend VAM to friends at work, but can't stomach adding another gotcha for me to check in every troubleshooting session. There's a delicate balance of layers of user/plugin/system overrides and arbitrarily changing precedence on those groups can have seriously unpredictable results. My runtimepath looks fine when I use just maktaba to manage it but after I install any plugin using VAM, that and all subsequent runtimepath modifications have the wrong precedence.

MarcWeber commented 9 years ago

Well - VAM's idea also is "why not package your own ~/.vim as plugin" - and then its no longer that easy to understand what a plugin and what your after directories are.

Anyway: I see two fixes 1) allow maktaba to manage the rtp, eg introduce a setting "maktaba_managed_rtp" or such 2) fix your case: try to keep ~/.vim/after last, because its likely to fix most of your issues 3) duplicate work of maktaba or ship it with VAM. 4) adding a "keep_rtp_last" option allowing the user to specify some /after directories which must be put last, one item could be $HOME/.vim/after then. This would be "easier to use" than implementing the custom hook.

I guess that 4) would be most usable because it would not depend on loading order of plugins

Do you think it would make your case "usable"? We could even make it default to [$HOME/.vim (or _vim)/after] directory

dbarnett commented 9 years ago

Yes, I think option 4 is a good approach provided it includes reasonable defaults (hopefully including the defaults for other common platforms as well).

Thanks for being accommodating of my corner cases!

ZyX-I commented 9 years ago

Vim's runtimepath should have the following categories in order:

The problem is that none of the following categories exist in &rtp. Path manager and specifically VAM can or can be modified to tell you the following things when parsing &runtimepath:

  1. It is the path added by VAM.
  2. It is the path ending with {directory_separator}after.
  3. It is some other path that was there before VAM was first called.
  4. It is some other path that was added later.

. I think this is enough to resolve your issue: just when modifying &rtp use the following order: {1}\{2}, ({34})\{2}, {1}&{2}, ({34})&{2}. I.e. do the following in vam#ActivateAddons:

  1. Get list of paths out of &rtp.
  2. Remove all directories managed by VAM out of there.
  3. Split after/ paths out of the result.
  4. Add new VAM paths to the global list of paths managed by VAM.
  5. Add new VAM after paths to the global list of paths managed by VAM.
  6. Call a hook function with the following arguments: list of non-after non-VAM paths, list of non-after VAM paths, list of after VAM paths, list of after non-VAM paths.
    1. The only thing default implementation of this function does is joining these lists together in order given and assigns the result to &rtp. To make the implementation simpler all paths in all lists should be already escaped (otherwise unescaping logic for all paths from &rtp is needed).

. I do not like other solutions because

  1. User after directory being the last one is a valid request. Worse, you can create a logic that will keep it the last one. (This logic will also keep system after directory the last one, but I do not see much harm in this since it is mostly empty.)
  2. There are already too much options and very bloated documentation. I do not think this option will be used by anybody, but @dbarnett.
  3. I do not like solutions which lead to unnecessary code duplication.
  4. Given hook with arguments like this one may easily implement first variant if he needs.
dbarnett commented 9 years ago

I like the concept of VAM being detecting paths it didn't add and avoiding modifications to them, but it sounds like you're suggesting that (by default) system paths should have higher precedence than VAM plugins. That sounds pretty extreme.

I agree there's a cost to adding a new option and it may not be the right approach. But I strongly disagree that this is a "fringe feature", unless you're arguing after/ files are in general (and if so, why does VAM bother to add them to runtimepath?).

I do not think this option will be used by anybody, but @dbarnett.

What do you mean by "be used"? If you mean changing this "keep_rtp_last" setting from it's default, I don't even plan on doing that, because the default $HOME/.vim/after is all I need, along with most other users. I'd be just as fine with just changing this "default" to a hard-coded list in VAM as long as it includes a good set of reasonable guesses, but if you did want to make it a setting it would be a nice acknowledgement of the fact that it's a guess, and an escape hatch for users with weird nonstandard config paths that happen to notice (after a long horrible troubleshooting session) that their settings are being overridden by a rogue plugin file, so they have some option besides just uninstalling the plugin.

MarcWeber commented 9 years ago

I'm tired of talking. Can you give this patch a try? http://mawercer.de/tmp/vam.patch Note that you have to set the option in the .vimrc as commented

ZyX-I commented 9 years ago

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512

On November 19, 2014 8:00:42 PM EAT, David Barnett notifications@github.com wrote:

I like the concept of VAM being detecting paths it didn't add and avoiding modifications to them, but it sounds like you're suggesting that (by default) system paths should have higher precedence than VAM plugins. That sounds pretty extreme.

Ah, this was an accident. Should swap first two arguments. (Maybe better to have non-VAM non-after (nVnA), non-VAM after (nVA), VnA, VA in function arguments and VnA, nVnA, VA, nVA in the result).

I agree there's a cost to adding a new option and it may not be the right approach. But I strongly disagree that this is a "fringe feature", unless you're arguing after/ files are in general (and if so, why does VAM bother to add them to runtimepath?).

I do not think this option will be used by anybody, but @dbarnett.

What do you mean by "be used"? If you mean changing this "keep_rtp_last" setting from it's default, I don't even plan on doing that, because the default $HOME/.vim/after is all I need, along with most other users. I'd be just as fine with just changing this "default" to a hard-coded list in VAM as long as it includes a good set of reasonable guesses, but if you did want to make it a setting it would be a nice acknowledgement of the fact that it's a guess, and an escape hatch for users with weird nonstandard config paths that happen to notice (after a long horrible troubleshooting session) that their settings are being overridden by a rogue plugin file, so they have some option besides just uninstalling the plugin.

Problem is that they will not know this option unless they read source code, are proficient at guessing how this option should be searched for or have memorized our documentation.

Reply to this email directly or view it on GitHub: https://github.com/MarcWeber/vim-addon-manager/issues/165#issuecomment-63673988

-----BEGIN PGP SIGNATURE----- Version: APG v1.1.1

iQJNBAEBCgA3BQJUbOGoMBwfMDI7PjIgHTg6PjswOSAQOzU6QTA9NEA+MjhHIDxr cC1wYXZAeWFuZGV4LnJ1PgAKCRBu+P2/AXZZIj6wD/0b8+rDbWHNbrOfxTsHPK3B DJvHpKLZMS4Tu4wRd1d88iRg/i27/G+4r2H+9LSYW75acPVGCPMenZ5ID4F2kN15 Jb9luZuiI7rY1tckg+KHQPVyY2THZClEamEetdUSXTBkDX4kv4DadzeH7fzEwtLU 9tBGdEkr5tIWuTpBSFNvVXngLa/OnI2IQlDCt0/9iY61o0ibNMkQqJ3Wx5153rYu yC9zB2QgaGwhAQNstXvH9LTh52ppZQ/EzC2FztBWFRXAy2MXP8OP0uKbHsYzvuuJ t/2a0cwgI+MHjpv07XT7ie67nncQJcacMcmDlkte5xe4Ov3PR0N+0If3MjzPeyWf Q1VRaRayIBk+2Z+0mFNDwCT+61ZMmcTL7dYHAGBc20XSxZsR8TWZUqgmb/14S2V2 +YFR3i6eDQ5fECSZ7x38D2JbJcy1tsBOBDTu+3WLnncYnF5URqloF6LkQpeG8oco RPHsI3JGm82pNzuHwzDlsV9Ih9aOV1W4EudYjPwpogemqDWQrwdgTtRLqbLqToa9 40IEPhD+V/TyIXRn6ow5S+4ADia0pAJVrL4TQoboavntbsV9rwPEhJ8zF6x8U1XF 0Z6pxlH53Q4mpgT/rvSnBcFsbYb7NH6anN9u+VOjMRlUmBcfMO4FkIcB0DTHuVnZ 2jF5rHa4qjUr+IKzCGjwtw== =fM1g -----END PGP SIGNATURE-----

MarcWeber commented 9 years ago

http://mawercer.de/tmp/vam.patch now adds this option to README.md. Vim is broken in many ways, eg see this: http://vim-wiki.mawercer.de/wiki/topic/in-which-way-does-vim-suck.html I don't want to be more perfect than VIM is. ZyX, do you mind me submitting this patch? Then we can close this discussion and move on.

dbarnett commented 9 years ago

Patch seems to work.

FWIW, if rtp manipulation code is performance critical in VAM (parts of it are for maktaba), I saw some potential optimizations in the patch code like only evaluating $HOME once, using \V (very nomagic) on the regex instead of using escape(), and looping over rtp entries once instead of twice for the filter().

MarcWeber commented 9 years ago

I've pushed the patch I proposed. If ZyX comes up with something better he's welcome to reopen and propose another fix. @dbarnett I don't want to over optimise VAM code. neovim might change a lot (by using lua as default language) anyway.

dbarnett commented 9 years ago

@MarcWeber Sure. I figured VAM planned to support classic vim for a while even if neovim changes everything soon (and the regex thing was more about safety), but point taken.

Anyway, thanks for the fix. I'll consider it subject to change, but would appreciate a ping here if it does change.