-
-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Lockfile support #1010
base: master
Are you sure you want to change the base?
feat: Lockfile support #1010
Conversation
d6cde5a
to
e9c523d
Compare
2642894
to
54ff9ee
Compare
54ff9ee
to
aa14512
Compare
Couldn't this be achieved by the existing PR #849 ? |
@smhc: First off, I'm sorry for missing your PR for so long! I was too busy for Second: @EdenEast has laid out some of the justification for lockfiles over snapshots here: #1009 (comment). 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? |
Also @EdenEast do you mind fixing the conflicts before I review? |
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. |
f2d5378
to
b32a78b
Compare
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). |
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. |
6136ac6
to
865ef7e
Compare
@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:
|
865ef7e
to
2b30bc9
Compare
Here is a minimal_init.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', '<F1>', function()
reload()
end)
vim.keymap.set('n', '<F2>', function()
vim.cmd 'PackerUpdate'
end)
vim.keymap.set('n', '<F3>', 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',
} |
Reminder poke to @wbthomason for a review.😄 |
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 Two small observations:
Again, thanks for your work! This makes my setup much easier (unfortunately working with different nvim version on different clients)! |
Thanks for testing the PR! 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. |
This change introduces a way to pass optional arguments to packer commands. Optional arguments are denoted by `--`. There are two types of optional arguments: `option` and `flags`. Option contains `=` with the attached value Ex: --path=/some/path Flags are standalone and set their value to true. Ex: --nolockfile ({ nolockfile = true }) This is used to add `--path` argument to the `lockfile` command.
There was an issue with a rebased change that was resolved.
If the status command is the first packer command run then the lockfile has not been loaded yet. This would then end up not populating the `lockfile` key in the status command.
Co-authored-by: pynappo <[email protected]>
08790b3
to
191d37b
Compare
I'd like to mention that I've been using this functionality without issues and look forward to seeing it merged. Nice work! |
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. |
@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. |
@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. |
[packer.nvim](https://github.com/wbthomason/packer.nvim) has some anti-features and bugs that were a deal breaker for me: - Bootstrapping is not straightforward - Snapshots are fundamentally broken, i.e. if I removed a plugin I tried to restore a snapshot packer would not work. - Luarocks install doesn't work on macos - Packer compilation step is annoying and sometimes makes config files out of sync with the current setup, which makes debugging and plugin development awkward [lazy.nvim](https://github.com/folke/lazy.nvim) doesn't have a compilation step, doesn't require [impatient.nvim](https://github.com/lewis6991/impatient.nvim) for speeding up modules initialization, has a straightforward bootstrap process and in general has a better design than [packer.nvim](https://github.com/wbthomason/packer.nvim). SEE: wbthomason/packer.nvim#814 SEE: wbthomason/packer.nvim#1010 SEE: wbthomason/packer.nvim#180
[packer.nvim](https://github.com/wbthomason/packer.nvim) has some anti-features and bugs that were a deal breaker for me: - Bootstrapping is not straightforward - Snapshots are fundamentally broken, i.e. if I removed a plugin I tried to restore a snapshot packer would not work. - Luarocks install doesn't work on macos - Packer compilation step is annoying and sometimes makes config files out of sync with the current setup, which makes debugging and plugin development awkward [lazy.nvim](https://github.com/folke/lazy.nvim) doesn't have a compilation step, doesn't require [impatient.nvim](https://github.com/lewis6991/impatient.nvim) for speeding up modules initialization, has a straightforward bootstrap process and in general has a better design than [packer.nvim](https://github.com/wbthomason/packer.nvim). SEE: wbthomason/packer.nvim#814 SEE: wbthomason/packer.nvim#1010 SEE: wbthomason/packer.nvim#180
Implementation of feature request #1009.
Resolves: #1009
Items from issue discussion: