Skip to content
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

Feature: Lockfile support #1009

Open
EdenEast opened this issue Aug 11, 2022 · 18 comments · May be fixed by #1010
Open

Feature: Lockfile support #1009

EdenEast opened this issue Aug 11, 2022 · 18 comments · May be fixed by #1010

Comments

@EdenEast
Copy link
Contributor

EdenEast commented Aug 11, 2022

This started as a idea's discussion post. Creating a feature issue instead to make it an actual feature request.

Describe the feature

Like many other users I store my neovim's configuration in source control. I can use my configuration on many different
systems. The problem is that my configuration can only be validated to work with my local plugins at their current git
revisions. The state of the local plugins should also be commitable. As neovim's ecosystem is constantly changing this
would prevent an update from breaking my config and causing me to maintain it. Now I can choose when I have time to
maintain my config.

Why not snapshots?

  • Setting snapshot config option calls snapshot rollup every time neovim start
    • This preforms a git fetch every for every plugin in the snapshot file ever time neovim is started
  • Not integrated into packer commands like install and update
  • Snapshot file not formatted for diffing changes and not sorted (not idempotent).
    • Having an PackerSnapshotDone autocmd would at least let us call jq to sort and pretty print.
      This would make snapshot's diff understandable and idempotent.

Snapshots feel more like a git stash. A way of taking a temporary snapshot of my current state while experimenting.
Something that I will eventually throw away. It is it's own system tacked onto packer and not integrated into it.

Proposal

Packer should provide a lockfile option. When enabled, packer would apply this lockfile to git plugin type's
installer and updater functions. This would integrate the lockfile (if enabled) into the commands install,
update and sync.

The lockfile is a lua file that returns a table mapping plugins to their commit hashes. The lockfile entries are also
sorted so that multiple calls are idempotent.

Configuration

local config_defaults = {
  -- ...
  lockfile = {
    -- Apply lockfile on packer commands (Defaults to false for an opt-in feature)
    enable = false,
    -- Default lockfile filepath. Defaults to config location as it should be commited into dotfiles
    path = join_paths(stdpath("config"), "lockfile.lua"),
    -- Update lockfile after an upgrade call
    update_on_upgrade = true,
  },
}

Commands

A new command would be added called upgrade. This would temporally disable the lockfile and then execute an update.
Once the update is finished the lockfile will also be updated if update_on_upgrade is enabled.

The other new command that would be added is lockfile. This command will update the lockfile with the current state of
your local plugins. The internal lockfile table would also be re-loaded. This is useful is someone sets
update_on_upgrade to false and only wants to update the lockfile once they have tested the upgrade is stable. If it is
not then they can just call update and the lockfile will be reapplied.

Applying lockfile

The lockfile is applied if:

  • The plugin name is found in lockfile table
  • The plugin is a git type (not local)
  • There is no tag key

The lockfile is also checked and applied to any plugin spec defined in requires key.

Updating lockfile

Since local plugin types are ignored by the lockfile, if an entry exists in the current lockfile that value is reused.

Example lockfile

-- Automatically generated by packer.nvim
return {
  ["LuaSnip"] = { commit = "c599c56", date = "1659863559" },
  ["alpha-nvim"] = { commit = "d688f46", date = "1658591867" },
  ["cmp-buffer"] = { commit = "62fc67a", date = "1655319089" },
  ["cmp-emoji"] = { commit = "19075c3", date = "1632834658" },
  ["cmp-nvim-lsp"] = { commit = "affe808", date = "1652705110" },
  ["cmp-nvim-lua"] = { commit = "d276254", date = "1633919304" },
}

Commit dates

With the lockfile applied to both the install and update commands we can take advantage of actually knowing the
commit we want. Along with the commit hash we can also save the commit's date. This would then allow the use of
--shallow-since instead of --depth 999999. This makes clones and updates of repositories leaner both on
network and diskspace. Getting the commit's date in unix time is simple:

git show -s --format="%ct" HEAD

Additional thoughts

  • Should the upgrade command perform the same operations as update or sync?
    • Should this be controlled with a config option? (should it call compile?)
@danielo515
Copy link

I see some projects implementing this in userland, for example, lunar-vim and, more recently, myself in my own config. This is something I really want, because there is nothing more frustrating than running packer sync to get new plugins installed and old ones removed just to discover that another 50+ plugins were updated... and that one of them is totally breaking my config...

@wom
Copy link

wom commented Aug 23, 2022

This. I run same dots on multiple machines; and it's hard to verify same environment if I have to bootstrap a new VM quickly (and it's a PITA to manually grab hashes). Packer Seems the logical place to handle it (as outlined here).

@rwjblue
Copy link

rwjblue commented Aug 23, 2022

I see some projects implementing this in userland, for example, lunar-vim and, more recently, myself in my own config.

Exactly. I've done the same in my vim config.

See code for how it works.

Example docs (including how to add a new plugin without updating all the other hashes, updating all plugins, rolling back to "last known good" snapshot, etc).

@EdenEast
Copy link
Contributor Author

EdenEast commented Sep 1, 2022

@wbthomason, with the discussion here and your first draft of the v2 roadmap, I am wondering your thoughts on this feature request. With a rewrite it is also the time to look at packer's current features mainly snapshots. Or add this as a feature to add in stage 5. Would like to know your thoughts in general on a lockfile in packer.

@wbthomason
Copy link
Owner

@EdenEast thanks for the feature request, PR, and ping. I still need to review the PR, but my thoughts from this request and discussion are:

  • This seems like a feature the community wants
  • This feature is orthogonal to the main thrust of stage 1 of the rewrite plan, moving away from compilation. This could serve as an argument for implementing the feature now (since it won't interfere and will be easy to pull in) or waiting
  • Honestly, this feature seems like an improved version of snapshots, though I admit I never really groked why people use snapshots

Given this, and the fact that there's already a PR, I'm leaning toward merging a version of this in sooner than later. I think the best course of action might be to talk to the main stakeholders with snapshots and see if they'd be willing to replace the snapshot functionality with this feature, to prevent creep and duplication.

In somewhat more detail:

  • I'm hesitant to add another command. What about adding arguments to PackerSync/PackerUpdate to do the same thing, e.g. PackerSync no_lockfile or something?
  • I buy that there should be some new function/probably a corresponding command for regenerating the lockfile
  • I'm slightly leery of the semantics of applying the lockfile as proposed. It seems that, for users who aren't aware of these details, this could cause confusing situations in which plugins which they expect to be updated are not. If we use this approach, we should at least add an informational output about which plugins are locked (e.g. to the status output); otherwise, maybe the lockfile should always apply for the plugins it contains, regardless of separately specified tag, etc. (or, better still, the lockfile should apply and issue an error if the specified tag is different from the locked version, etc.)

I'll try to take a look over the PR in the next few days; please ping me if I haven't commented on it by early next week. Thanks!

@EdenEast
Copy link
Contributor Author

EdenEast commented Sep 2, 2022

Thanks for your initial thoughts on this feature. I too could not grok snapshots in the context of a package manager. I found that people were trying to use snapshots as a lockfile but the feature was not designed in such a way. Adding a lockfile in some form to the operation of packer made more sense to me and easier for others to understand how it works (a lockfile is a known and understood concept)

I would like to merge a simple (MVP) version of a lockfile before really trying to rewrite packer, as the lockfile needs to be integrated into the internals of packer. With that said I would like to get a simple interface for users and functionality that they would understand.

I'm hesitant to add another command. What about adding arguments to PackerSync/PackerUpdate to do the same thing, e.g. PackerSync no_lockfile or something?
I buy that there should be some new function/probably a corresponding command for regenerating the lockfile

I used the term upgrade from other package managers. This was an attempt to differentiate between updating your local plugin store to the lockfile and updating the local store to the latest remote changes. Also used sync and update as examples as sync is just an update and compile.

  • I would propose that in the future rewrite that there would be only two updating commands. One that updates regardless of lockfile, and one that syncs with the lockfile. Should compile on update would be then moved to a config setting. If someone disables the lockfile they would produce the same result.

  • Another idea is to add this all to a lockfile command. This lockfile command would take arguments like regenerate to recreate the lockfile based on the local plugin store or update which would act update (or sync) the local plugin store with the plugins remote. (I think the current proposal is better than this option as I feel it is more clear and understandable)

I'm slightly leery of the semantics of applying the lockfile as proposed

I was not on handling the lockfile with tag and branch keys. I felt that if someone put a branch that they would want to track a different branch but still be able to lock at a commit. So that was fine but tag was different. If someone has a tag and a lockfile entry I was not sure how to handle this. I like the idea of issuing an error if the commit is not the same as the tag specified. That would make the most sense.

Adding lockfile information to packer's status is a good idea to help users understand the current packer state.

@danielo515
Copy link

danielo515 commented Sep 2, 2022 via email

@wbthomason
Copy link
Owner

@danielo515: You can already specify a single package/list to upgrade with update() or sync(). I think that + lockfile regeneration should be enough to upgrade subsets of the lockfile at a time? And yes, specifying lockfile output location is important - for new lockfile creation, this should be an explicit parameter.

@EdenEast
Copy link
Contributor Author

EdenEast commented Sep 3, 2022

I can add an optional path argument to the lockfile command to generate to the filepath specified.

@danielo515
Copy link

danielo515 commented Sep 3, 2022 via email

@EdenEast
Copy link
Contributor Author

EdenEast commented Sep 3, 2022

With this change I would also need some way of setting which lockfile to apply. This could be added as an argument to any install /update command or the lockfile command would have subcommands. The lockfile command could have two subcommands generate and (set / apply). Not sure on which method would be preferred. (Initially I lean towards an optional argument to install / update commands)

@wbthomason
Copy link
Owner

I strongly prefer an optional argument over more commands. Maybe something like this:

  • A config setting defines the default lockfile path
  • The lockfile creation function/command accepts an optional path to override the config value
  • A config setting defines whether lockfiles should be used by default or not
  • In install/update, if either the "auto-lockfile" setting is set and a lockfile exists at the default path OR an optional path to a lockfile is passed to the install/update functions, the lockfile is applied.

Does that seem reasonable?

@danielo515
Copy link

danielo515 commented Sep 3, 2022 via email

@wbthomason
Copy link
Owner

@danielo515 Can you elaborate on the benefit there vs. having a default lockfile setting?

@danielo515
Copy link

danielo515 commented Sep 4, 2022 via email

@EdenEast
Copy link
Contributor Author

EdenEast commented Sep 6, 2022

Ok with the current conversation I propose a new command workflow. This will remove the upgrade command and instead add optional flags to the existing install, update and sync commands:

" Updating without applying lockfile
PackerUpdate --nolockfile

" Updating a specific plugin without applying lockfile
PackerUpdate --nolockfile plenary.nvim

" Generating a lockfile to some other path
PackerLockfile --path="/some/other/path.lua"

" Updating plugins and applying a specific lockfile
PackerUpdate --lockfile="/some/other/path.lua"

" Updating a specific plugin with a specific lockfile
PackerUpdate --lockfile="/some/other/path.lua" plenary.nvim

The same interface would be applied to PackerInstall and PackerSync. I still think there needs to be one new command for regenerating the lockfile.

@EdenEast
Copy link
Contributor Author

@wbthomason and @lewis6991, seeing that there is now a new project called rewrite and there and there is lockfile support there, should this be the tracking issue for that or another one be created?

@lewis6991
Copy link
Collaborator

It's still very early days on the rewrite, though I'm now accepting feedback if you want to test.

I don't mind how you want to track it, but it's not really top priority for me right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants