google / vim-maktaba

Consistent Vimscript
Apache License 2.0
591 stars 42 forks source link

Symlinked plugin with addon-info.json "name" may be added to the runtimepath twice #61

Open glts opened 10 years ago

glts commented 10 years ago

This happens on an old MacVim 7.3, but not on newer versions of Vim.

Create a plugin with an addon-info.json file with an explicit "name", for example ~/code/vim-linked/addon-info.json:

{
    "name": "linked.vim",
    "version": "0.0.0",
    "dependencies": {
        "maktaba": {}
    }
}

Create a symlink to the plugin location, then start Vim with a vimrc that #GetOrInstalls the symlink location. (Here I used locations with different leaf dir names, but the same thing happens when the leaf dir names are the same.)

set nocompatible
source ~/.vim/maktaba/maktaba/bootstrap.vim
call maktaba#plugin#GetOrInstall('~/.vim/maktaba/linked')

Result. &rtp:

~/.vim,
~/code/vim-linked/,
~/.vim/maktaba/linked/,
/Applications/MacVim.app/Contents/Resources/vim/vimfiles,
/Applications/MacVim.app/Contents/Resources/vim/runtime,
/Applications/MacVim.app/Contents/Resources/vim/vimfiles/after,
~/.vim/after,
~/.vim/maktaba/maktaba

:echo maktaba#plugin#RegisteredPlugins():

['linked.vim', 'maktaba']
dbarnett commented 10 years ago

This should be fixed by #60 (almost ready to submit). I'm changing all the maktaba internal stuff to not touch rtp, so it will only be updated once, by your explicit call to maktaba#plugin#GetOrInstall.

That's a bit of a workaround. The detection would theoretically still fail to realize that '~/code/vim-linked/' and '~/.vim/maktaba/linked/' are the same path. Might still be worth investigating this bug independently and figuring out why maktaba doesn't understand these are the same path.

glts commented 10 years ago

Ah, cool.

FYI, I've tried the 'register' branch, and while the runtimepath problem goes away, my MacVim now starts with uncaught exceptions. Setup is the same as above.

Error detected while processing function maktaba#plugin#Enter..maktaba#plugin#Flag:
line    5:
E605: Exception not caught: ERROR(NotFound): Flag "plugin" not defined in plugin "linked.vim"
Error detected while processing function maktaba#plugin#Enter:
line   22:
E171: Missing :endif

Though this is probably a user error. I'll check later.

dbarnett commented 10 years ago

Hmm… I can't reproduce with vim 7.3.429. Followed your directions exactly. What version is your MacVim?

You can debug if you do :func /RegisterPlugin and then take that function name (e.g., <SNR>16_RegisterPlugin) and add breakadd func <SNR>NN_RegisterPlugin to your vimrc. I don't see any branch in the code where it would source instant files without first having defined the flags, but maybe it's somehow checking the wrong path for the plugin/ directory at the top of s:InitPlugin not finding it, or l:already_installed is messing up somehow?

glts commented 10 years ago

This is on a plain Vim 7.3 (no patches), which for a long time had been the latest supported MacVim 7.3 on Mac OS X 10.6.

I had some trouble getting debugging to work, but here is the plugin object as returned by maktaba#plugin#Get after starting up. The setup is exactly as given above.

{
  'AddonInfo': function('maktaba#plugin#AddonInfo'),
  'Flag': function('maktaba#plugin#Flag'),
  'GenerateHelpTags': function('maktaba#plugin#GenerateHelpTags'),
  'HasDir': function('maktaba#plugin#HasDir'),
  'HasFiletypeData': function('maktaba#plugin#HasFiletypeData'),
  'HasFlag': function('maktaba#plugin#HasFlag'),
  'IsLibrary': function('maktaba#plugin#IsLibrary'),
  'Load': function('maktaba#plugin#Load'),
  'MapPrefix': function('maktaba#plugin#MapPrefix'),
  'Source': function('maktaba#plugin#Source'),
  '_addon_info': {
    'author': 'author',
    'dependencies': {'maktaba': {}},
    'name': 'linked.vim',
    'version': '0.0.0',
  },
  '_entered': {
    'autoload': [],
    'ftplugin': {},
    'instant': [],
    'plugin': [],
  },
  'flags': {},
  'globals': {},
  'location': '/Users/glts/code/vim-linked/',
  'logger': { ... },
  'name': 'linked.vim',
}

Flags are indeed not defined, and the location is the dereferenced symlink. On a current Vim the location is /Users/glts/.vim/maktaba/linked/.

glts commented 10 years ago

Phhhh, this is almost certainly to do with the resolve problem prior to patch 7.3.194, discussed in #18 and #31.

https://github.com/google/maktaba/blob/7b25c31c1769576802ff49baf1ca88392c0d4a1a/autoload/maktaba/plugin.vim#L507-L525

dbarnett commented 10 years ago

Ah, okay, so this a bug independent of #60 and the register branch. We need working path resolving for all supported vim versions. Let's try to bisect what version of vim is affected and find a workaround (or find an implementation that works in both old and new vim).

glts commented 10 years ago

I wouldn't call this independent of #60 and the 'register' branch. The bug is in the new code, in fact on line 522 of the part I highlighted.

Changing

" If plugin is symlinked, register resolved path as custom location to avoid
" conflicts.
let l:resolved_location = s:Fullpath(resolve(a:plugin.location))

to

" If plugin is symlinked, register resolved path as custom location to avoid
" conflicts. As above, fnamemodify() is required to strip the trailing slash.
let l:resolved_location = s:Fullpath(resolve(fnamemodify(a:plugin.location, ':p:h')))

fixes the problem. The problem description is exactly the same as that given in the comment 15 lines above. I'm too lazy to open a pull req, can you fix it?

I don't know your reasoning behind requiring the trailing slashes in paths, but it does make life a little harder. Cheers.

dbarnett commented 10 years ago

Oh, I misunderstood then.

Should be fixed on the resolve branch now if you want to double-check I fixed it properly.

I don't know your reasoning behind requiring the trailing slashes in paths, but it does make life a little harder.

The reason we prefer explicit trailing slashes is that it makes the path unambiguously refer to a directory, which helps in path manipulation code like maktaba#path#Dirname to not get confused. It reduces the need for going to the filesystem to determine whether paths are supposed to refer to a file or directory, and also makes things behave better when referring to dirs that don't exist (yet).

glts commented 10 years ago

A-ok.