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.87k stars 266 forks source link

feat: Lockfile support #1010

Open EdenEast opened 2 years ago

EdenEast commented 2 years ago

Implementation of feature request #1009.

Resolves: #1009

Items from issue discussion:

smhc commented 2 years ago

Couldn't this be achieved by the existing PR https://github.com/wbthomason/packer.nvim/pull/849 ?

wbthomason commented 2 years ago

@smhc: First off, I'm sorry for missing your PR for so long! I was too busy for packer for a while, and it looks to have come in during that time.

Second: @EdenEast has laid out some of the justification for lockfiles over snapshots here: https://github.com/wbthomason/packer.nvim/issues/1009#issue-1336330500. Personally, I find the arguments of avoiding the snapshot rollup on every start and having more diff-friendly output compelling, but I'll also admit that I do not use and am unlikely to ever use either snapshots or lockfiles, so I may be missing some important points.

Perhaps there's a way we can combine the work from the two PRs?

wbthomason commented 2 years ago

Also @EdenEast do you mind fixing the conflicts before I review?

smhc commented 2 years ago

Second: @EdenEast has laid out some of the justification for lockfiles over snapshots here: https://github.com/wbthomason/packer.nvim/issues/1009#issue-1336330500.

The changes I made under my PR effectively resolve those issues. The snapshot file is loaded on startup and used as the 'commit' key for each plugin. Running 'upgrade' will ignore this key if it was set via the snapshot file. This allows upgrades even when a snapshot/lock file is in use.

It doesn't support pretty-print, but I added that myself independently via a custom mechanism through jq.

My changes aren't necessarily production quality, they were more just intended as a prototype to to explain how I think the snapshot stuff should work.

I'm happy to throw away my PR. However my 2c is that the snapshot behaviour should just be fixed (similar to how my PR fixes it) instead of an additional lockfile feature. I think the way I modified the snapshot functionality leaves it backwards compatible with how people may have been using it anyway.

If this PR improves the way clones are performed and has pretty printing that would be a useful addition.

wbthomason commented 2 years ago

@smhc: Ok, thanks. I need to read through both PRs in detail. I will note that I do not favor having both lockfiles and snapshots (as mentioned on #1009); a prerequisite for merging this PR would be replacing the snapshot feature.

EdenEast commented 2 years ago

In the context of a package/plugin manager a lockfile is a known concept and is understandable for users. The snapshot feature only partially converts this aspect. Having an integrated lockfile feature instead of a side snapshot feature would make the most sense. In the current issue we are talking about how to preserve the functionalitty of the snapshots for people that still want to save them. People where mostly trying to use the snapshots as a lockfile anyways. Guessing that there will be a deprecation period for the snapshot feature. @wbthomason can speak more on how it would be migrated (I can see the feature depricated when this is merged and removed by the v2 rewrite for example).

EdenEast commented 2 years ago

Converted this pr back to a draft while I implement the changes discussed in the feature request issue. Also have to update it with the recent PRs introduced.

EdenEast commented 2 years ago

@wbthomason I updated this pr with the latest changes from master and the discussion in the issue and should be ready for a first pass review.

List of changes:

EdenEast commented 2 years ago

Here is a minimal_init.lua file that I was using to test this branch:

minimal_init.lua ```lua -- `minimal_init.lua` used for reproducible configuration -- Open with `nvim --clean -u minimal_init.lua` local is_windows = vim.fn.has 'win32' == 1 local function join(...) local sep = is_windows and '\\' or '/' return table.concat({ ... }, sep) end local function script_path() local str = debug.getinfo(2, 'S').source:sub(2) return str:match '(.*/)' end local cwd = script_path() local root_path = join(is_windows and os.getenv 'TEMP' or '/tmp', 'nvim') local package_root = join(root_path, 'site', 'pack') local root_plugin_path = join(package_root, 'packer') local packer_install_path = join(root_plugin_path, 'start', 'packer.nvim') local packer_compiled_path = join(root_path, 'plugin', 'packer_compiled.lua') local packer_lockfile_path = join(root_plugin_path, 'start', 'packer.nvim', 'lockfile.lua') vim.opt.packpath = join(root_path, 'site') vim.opt.runtimepath:prepend(root_path) vim.g.loaded_remote_plugins = 1 vim.opt.ignorecase = true if vim.fn.isdirectory(packer_install_path) == 0 then local dirname = vim.fs.dirname(packer_install_path) vim.fn.system { 'mkdir', '-p', dirname } vim.fn.system { 'ln', '-s', cwd, packer_install_path } vim.cmd.packadd 'packer.nvim' end function _G.reload() for pack, _ in pairs(package.loaded) do if vim.startswith('packer', pack) then package.loaded[pack] = nil end end vim.cmd 'source %' print 'reloaded' end vim.keymap.set('n', '', function() reload() end) vim.keymap.set('n', '', function() vim.cmd 'PackerUpdate' end) vim.keymap.set('n', '', function() vim.cmd 'PackerUpdate --nolockfile' end) local packer = require 'packer' local use = packer.use packer.init { compile_path = packer_compiled_path, package_root = package_root, log = { level = 'info' }, lockfile = { enable = true, path = packer_lockfile_path, update_on_upgrade = true, }, } use(cwd) use { 'christoomey/vim-tmux-navigator' } use { 'EdenEast/nightfox.nvim', as = 'nightfox', config = function() vim.cmd.colorscheme 'nightfox' end, } use { 'rcarriga/nvim-notify', config = function() vim.notify = require 'notify' end, } use { 'sonph/onehalf', rtp = 'vim', } ```
EdenEast commented 2 years ago

Reminder poke to @wbthomason for a review.😄

mutlusun commented 1 year ago

Thank you for your work!

I have tested your changes using my local config and so far everything works fine. The lockfile is created, taken into account when running PackerUpdate and the changes written to the lockfile (if I passed --nolockfile).

Two small observations:

Again, thanks for your work! This makes my setup much easier (unfortunately working with different nvim version on different clients)!

EdenEast commented 1 year ago

Thanks for testing the PR! --nolockfile option only disables the lockfile from being applied for that update command. The configuration option lockfile.regen_on_update determines if the lockfile should be regenerated on the update command. This is defaulted to false. If you did not override this value then calling update should not regenerate the lockfile. Was this not the case for you?

As for the display, ya if a package is downgraded it does say that it has changed. I might be able to check how many commits behind the current commit is to origin. That might be something that would clarify the change. Would have to see how to show that in the display.

EdenEast commented 1 year ago

Added the number of commits ahead or behind the new commit is from the prev one.

Screen Shot 2022-10-24 at 9 38 10 PM
mutlusun commented 1 year ago

Thanks for your feedback and the clarification! Indeed, I set regen_on_update to true :smile:

mutlusun commented 1 year ago

Is there anything I can do to help merging this PR? I would really like to see this feature in packer :)

EdenEast commented 1 year ago

This feature is ready to be merged into packer. Currently it only needs a review by the maintainer @wbthomason. As with any PR actually using it and testing it always helps.

Maybe asking for others to test the changes against their configuration would help with catching weird edge cases.

meuter commented 1 year ago

Hi @EdenEast, I have also been trying to roll out my own lockfile support on top of packer but my "solution" feels hackish at best. I think you may be right, it needs to be part of packer itself. I must say this PR is excaltly what I needed! Thanks for you contribution. I tried it in my config and it works exceptionally well. Can't wait for this to be merged 🥳 !

And of course, @wbthomason: Thanks for a great plugin BTW!

EdenEast commented 1 year ago

Note: CI failed on nightly with a one word removal in readme. I cant retry the job.

EdenEast commented 1 year ago

I documented the optional arguments for install and update commands in the help docs.

shaunsingh commented 1 year ago

Hi, in my use case I use the headless mode of packer.nvim to create a cli to manage my packages The other commands emit a PackerComplete event when they're finished, so I can use it like this

nvim --headless -c 'autocmd User PackerComplete quitall' -c 'lua require[[packages]]' -c 'lua require("packer").sync()'

And it quits the headless instance once packer is done syncing

However, with lockfile it hangs forever nvim --headless -c 'autocmd User PackerComplete quitall' -c 'lua require[[packages]]' -c 'lua require("packer").lockfile()'

I think it makes sense to emit PackerComplete once the lock file is done being generated?

meuter commented 1 year ago

I think it makes sense to emit PackerComplete once the lock file is done being generated?

I use PackerUpdate for this and have the regen_on_update = true options:

nvim --headless -c 'autocmd User PackerComplete quitall' -c 'PackerUpdate --nolockfile"

(But I agree that it does make sense to emit PackerComplete on PackerLockfile as well)

EdenEast commented 1 year ago

I did not add PackerComplete because packer did not operate over or change the local plugin store. I will emit the autocommand at the end and @wbthomason can let me know if it should be the generic PackerComplete or something like PackerLockfileComplete like now compile has its own.

EdenEast commented 1 year ago

After thinking about this I feel that there should be a different autocmd for the lockfile to distinguish between a packer command that operates on the plugin store and a command that deals with internal packer states like compile and now lockfile.

I have added a new autocmd called PackerLockfileDone which is emitted after a lockfile command. I would like to know @wbthomason thoughts on this.

iverberk commented 1 year ago

I'd like to mention that I've been using this functionality without issues and look forward to seeing it merged. Nice work!

EdenEast commented 1 year ago

I am not sure on the plan for this PR. This PR might be merged in as it is now into the current version of packer ()v1) or I will have to port this functionality to v2 of packer once the core of it is stabilized.

I will keep this PR up to date with the master branch (v1). Anyone can use this branch until the feature is merged into either v1 or v2.

iverberk commented 1 year ago

@EdenEast thanks for keeping it up to date so that we can use this nice functionality. I'm quite happy with how things are setup with v1 and don't expect to benefit much from a rewrite (as an end-user), although I understand the motivation for it.

EdenEast commented 1 year ago

@wbthomason could this pr be merged into packer. Version 2 will be some time before it is set as the main branch. This feature is done and can be used by users now. This feature will be added back into v2 when it is ready to be released. I know that there are a lot of people waiting for this feature to be added to packer.