wbthomason / packer.nvim

A use-package inspired plugin manager for Neovim. Uses native packages, supports Luarocks dependencies, written in Lua, allows for expressive config
MIT License
7.83k stars 266 forks source link

Do not put compiled loader in stdpath('config') #201

Open clason opened 3 years ago

clason commented 3 years ago

Currently, packer puts the compiled loader script in stdpath('config')/plugins. Since I sync stdpath('config') across multiple machines, this often leads to conflicts on :PackerSync.

Since the script is generated from user config, I believe it would be more appropriate to put it in stdpath('cache') or stdpath('data') (where rplugin.vim is placed, for example). In particular, the latter would be easy to handle without new logic by just putting it in, say .local/share/nvim/site/pack/loader/start/packer/plugin/ instead of .config/nvim/plugin. (I tested this.)

(Vim packages allow multiple parallel pack/**/start directories, all of which are automatically visible to vim. In fact, I believe the original idea for packages was that each package would have its own pack/<package>/start and pack/<package>/opt directory, where start serves as a replacement for autoload.)

kkharji commented 3 years ago

@clason I believe changing compile_path in packer setup function is all you need todo. But I agree the default should be out of user config

wbthomason commented 3 years ago

I'm happy to make this change, but I need to think about/figure out the right way to change over from the old default. In particular, we don't want two packer_compiled files; we need to either move or delete the old file. This should probably happen via a shim that checks if there's a file in the old location and, if so, prints a warning and deletes the file. We can remove this shim after a month or two.

clason commented 3 years ago

Agreed, but I don't see the need to be particularly clever about it -- it's not that the old location is not working. Wouldn't it be enough for PackerCompile to check if packer_compiled exists in the old location; if so, delete it; and then write it in the new location? (This check can be removed after a while, of course.) Or is that what you meant by "shim"?

wbthomason commented 3 years ago

That is exactly what I was suggesting with the "shim", yes. Sorry, that comment was >50% me thinking aloud about the right way to do this.

francisco285 commented 3 years ago

I think this is a bigger issue now that #14686 is merged, because people will try to use stdpath('config')/plugin/*.lua files instead of requiring them in init.lua and it will break their configs.

clason commented 3 years ago

No, #14686 should have zero (breaking) implication for packer (otherwise we wouldn't have merged it). In fact, I am using stdpath('config')/plugin/*.lua with packer plugins and it works just fine.

However, there is one positive implication for packer: it should now be possible to have a packer_compiled.lua!

francisco285 commented 3 years ago

Your packer_compiled.vim won't necessarily be loaded after all your other lua files in the plugin/ directory, unless you move it to something like after/plugin/.

Let's say we use the following init.lua file in ~/.config/nvim:

-- vim: expandtab:shiftwidth=2:tabstop=2:softtabstop=2
vim.cmd('set noundofile')
vim.cmd('set nowritebackup')
vim.cmd('set noswapfile')
vim.cmd('set nowritebackup')
vim.cmd('set clipboard=unnamed,unnamedplus')
vim.cmd('set wildignorecase')
vim.cmd('set ignorecase')
vim.cmd('set hidden')

Global = {
  colorscheme = 'tokyonight'
}

local install_path = vim.fn.expand(vim.fn.stdpath('data') .. '/site/pack/packer/opt/packer.nvim')

if vim.fn.empty(vim.fn.glob(install_path)) > 0 then
  vim.cmd('!git clone https://github.com/wbthomason/packer.nvim ' .. install_path)
end

vim.cmd([[packadd packer.nvim]])

require('packer').startup(
  function(use)
    use { 'wbthomason/packer.nvim', opt = true }

    use {
      'folke/tokyonight.nvim',
      config = function()

        -- Using the global variable here makes the generated packer_compile.vim try to use it as well
        if Global.colorscheme == 'tokyonight' then
          vim.cmd('colorscheme tokyonight')
        end
      end
    }
  end
)

If we use it in ~/.config/nvim/init.lua and run :PackerCompile, everything works (assuming you didn't already have a packer_compiled created with some error), if we restart neovim, everything still works.

If we put it in ~/.config/nvim/plugin/init.lua, open neovim, run :PackerCompile, everything works (because the global variable was already set on startup), but if we restart neovim, packer breaks: image

This happens because plugin/init.lua is loaded after packer_compiled.vim, packer_compiled.vim tries to use global "Global" and fails because it is only set in plugin/init.lua

Now that it is possible, I assume some people will want to move their lua files to plugin/ and remove those requires in init.lua. Loading lua files by using require() in init.lua makes sure those files will all be loaded before plugin/* files, but if we make neovim auto-source them in plugin/ with packer_compiled.vim still in there, packer_compiled.vim will be executed before.

I may be wrong about it, but hopefully this makes some sense :sweat_smile:

clason commented 3 years ago

So this is a problem due to the load order "vim first, then Lua"? And shouldn't people be aware of the startup sequence anyway and expect that this breaks?

clason commented 3 years ago

But I believe all of this is orthogonal to the original issue, which is where to put plugin/packer_compiled.vim in the standard directories. I suggest you open a new issue about this if you think that this is actual breakage (as in, code that used to work before now doesn't).

francisco285 commented 3 years ago

So this is a problem due to the load order "vim first, then Lua"?

Not exactly, if it was packer_compiled.lua it would probably still break if we had a file named plugin/z.lua instead of plugin/init.lua.

And shouldn't people be aware of the startup sequence anyway and expect that this breaks?

Yes, but the point is that changing the default directory would be good because of the reasons described above and also because it would prevent the problem I described from happening with the default options

I suggest you open a new issue about this if you think that this is actual breakage (as in, code that used to work before now doesn't).

That makes sense, I commented here because I think solving this issue will solve the other one as well, but I'll open another issue just in case :+1: .

clason commented 3 years ago

Oh, you mean because moving packer_compiled out of the user config plugin directory into another rtp plugin directory would automatically put it after all the user configs? That makes sense (and since I do that anyway via the setup option, this may be why I haven't noticed any problems so far).

francisco285 commented 3 years ago

Yep, that's right. It should be ok to use any directory in rtp that is auto-sourced after user config directories. I used after/plugin as an example, but to be honest I don't think it would be good because it is still one of the user directories and users would still have problems if they try to use lua files in there.

shadmansaleh commented 3 years ago

This would fix the issue as pack/start/*/plugin/**/*.(lua|vim) is executed after user ones . But I think real solution would be to get rid of packer_compiled.vim all togather . Compilation adds a ton of compexcity for very little value . Also since it's planned to make users load their packer config all time this should be quite feasible .

wbthomason commented 3 years ago

As @shadmansaleh says, I'm trying (as fast as I have time) to move toward a single breaking change making users load their configs in their init.lua files. There will still be a "compiled" file, containing some precomputed paths which need to be sourced as well as the initialization for lazy-loaders (there is a startup time benefit to this, and it's very simple with auto-recompilation to avoid the need to manually rebuild), but it will live in stdpath('data') or stdpath('cache') by default.

wbthomason commented 3 years ago

Incidentally, if anyone has time to help with those changes, it's welcome. Otherwise, I'm getting to them as quickly as I can, which is unfortunately still pretty slowly right now.

hgaiser commented 2 years ago

I was wondering if there was an update on this issue? I was trying to move my packer_compiled.lua to stdpath('data') for the reasons mentioned earlier, but it fails to load when I start nvim. I added:

packer.init {
    compile_path = require('packer.util').join_paths(vim.fn.stdpath('data'), 'plugin', 'packer_compiled.lua')
}

Before running packer.startup. This doesn't seem to work, I'm a bit lost on why that is the case. If I configure it to stdpath('config') instead, it does work, curiously enough.

Regardless (didn't mean to hijack this issue), I was hoping there was an update :).

clason commented 2 years ago

You need to put it somewhere Neovim will actually find it on startup, e.g.,

  compile_path = vim.fn.stdpath 'data' .. '/site/pack/loader/start/packer.nvim/plugin/packer.lua',
hgaiser commented 2 years ago

You need to put it somewhere Neovim will actually find it on startup, e.g.,

Thanks! That worked. Could you explain how this works? I had assumed that because I set that path, packer would know where to find it too. How come it needs to be placed in certain places? I'm just curious and want to learn about how these things tie together :).

clason commented 2 years ago

It's irrelevant if packer can find it; Neovim itself needs to find it, and it only looks in specific places (:h startup).