[Feature request] Support package snapshot #298 #370

c3n21 · 2021-05-24T09:05:37Z

~~It's just a first implementation and I only tested it with spec(1), namely:~~

> spec can be a table with a function as its first element and config overrides as another element:
packer.startup({function() use 'tjdevries/colorbuddy.vim' end, config = { ... }})

~~## Usage~~
~~### Make snapshot~~
~~To make a snapshot you just have to run :PackerSnapshot <snapshot name>~~

~~### Restore snapshot~~
~~1. To restore a snapshot you have to add snapshot key in the config table you pass to packer.startup.~~
~~So, if I want to restore a snapshot named foo I would have something like this:~~

~~packer.startup(~~ ~~{function() use 'tjdevries/colorbuddy.vim' end, ~~ ~~config = { snapshot='foo' })~~

~~You can also specify the default directory in which retrieve and save snapshots with the following:~~
~~packer.startup(~~ ~~{function() use 'tjdevries/colorbuddy.vim' end, ~~ ~~config = { snapshot='foo', snapshot_path='path/to/snapshots' })~~

By default the snapshot's location is in ~/.config/nvim/plugin

~~2. Source the file :luafile %~~
~~3. :PackerUpdate~~
~~4. Restart NeoVim and enjoy~~

~~If you want to update you will need to remove snapshot from config table, and then source the file and update.~~

Final Implementation

Usage

Make snapshot

To make a snapshot you just have to run :PackerSnapshot <snapshot name>; you can also append single plugins name to selectively snapshot only the plugins you want like :PackerSnapshot <snapshot name> <plugin_1> <plugin_2>

Restore snapshot

To rollback to a snapshot you have two ways:

Add config.snapshot = 'some snapshot'. You can specify either the snapshot name which will be fetched inside config.snapshot_path (the default snapshot directory which is ~/.cache/nvim/packer but you can change it) or an absolute path.
So, if I want to restore a snapshot named foo I would have something like this:

packer.startup( {function() use 'tjdevries/colorbuddy.vim' end, config = { snapshot='foo' })

In this way, every time you boot up NeoVim it will always load that snapshot.

You can also run :PackerRollback which updates your installed plugins' commit to the snapshot you specified. ~~After that you just need to run :PackerUpdate and it will revert back~~

Delete snapshot

Just run PackerDelete <snapshot name> , you can either provide only the snapshot name or an absolute path.

Misc:

I've also added a packer.snapshot_complete() so that PackerRollback and PackerDelete can prompt which snapshots are available.
The snapshot format is a Lua table serialized using vim.inspect
packer.rollback hard resets the git repo of the given plugin

Known issues:

~~After running the update, the window will show error saying that you are in detached HEAD mode~~

c3n21 · 2021-05-24T09:06:48Z

Resolve #298

akinsho

@c3n21 this looks great thanks for taking the time to implement this. I haven't tried it locally or anything just left some comments regarding some comments and a question or two.

Could you also maybe add some tests for this new functionality as well in tests dir under snapshot_spec.lua or something to validate that they are being created and read correctly. There are a few other tests there which should help guide you. Also there are docs on plenary's repo

I should probably say that given this is whole new thing ultimately @wbthomason will have to review when he's free.

akinsho · 2021-05-25T10:31:37Z

lua/packer.lua

+  local f_snapshot, err = io.open(filename, 'r+')
+
+  if err ~= nil then
+    print(err)


probs a lingering debug print

akinsho · 2021-05-25T10:32:53Z

lua/packer/install.lua

@@ -32,6 +32,9 @@ local function install_plugin(plugin, display_win, results)
 end

 local function do_install(_, plugins, missing_plugins, results)
+    for index, value in ipairs(plugins) do
+        print(fmt("index: %s, value: %s", index, value))


Is this intentional or left over from the initial implementation

akinsho · 2021-05-25T10:33:39Z

lua/packer/snapshot.lua

+
+local function cfg(_config) config = _config end
+--[[
+property: 1


What does this apply to? Is this an emmy lua annotation for something?

lua/packer.lua

akinsho · 2021-05-25T10:36:13Z

lua/packer.lua

+    filename = util.join_paths(config.snapshot_path, filename)
+    await(snapshot(filename, plugins))
+    await(a.main)
+    --local delta = string.gsub(vim.fn.reltimestr(vim.fn.reltime(start_time)), ' ', '')


Is this left over from some profiling you were doing?

akinsho · 2021-05-25T10:37:32Z

CHANGELOG.md

@@ -0,0 +1,22 @@
+# CHANGELOG


This will be one for @wbthomason to decide but packer hasn't previously maintained a changelog so not sure if we need/should be adding this here since it isn't particularly discoverable. I think this should probably just go in the main docs 🤷🏾

I actually favor adding a changelog and linking to it from the main docs; the README is too long as it is.

I would prefer to have changes in the changelog be date and commit-stamped, though.

To clarify: we also ought to document this feature in the main docs, but I like having a changelog too. Also, the docs should describe what a snapshot does/does not save.

c3n21 · 2021-05-25T11:18:27Z

@c3n21 this looks great thanks for taking the time to implement this. I haven't tried it locally or anything just left some comments regarding some comments and a question or two.

Could you also maybe add some tests for this new functionality as well in tests dir under snapshot_spec.lua or something to validate that they are being created and read correctly. There are a few other tests there which should help guide you. Also there are docs on plenary's repo

I should probably say that given this is whole new thing ultimately @wbthomason will have to review when he's free.

Thank you for taking your time and review my PR, really appreciated.

As soon as I have some free time I will clean up the mess I left in the code and create the tests.

wbthomason

Thanks, as @akinsho said this looks like a great start on the feature. I agree with @akinsho's comments and left some questions of my own on the changes, mostly around documentation and deciding how snapshots will interact with "fresh" plugin specification values.

wbthomason · 2021-05-25T20:21:32Z

CHANGELOG.md

@@ -0,0 +1,22 @@
+# CHANGELOG


I would prefer to have changes in the changelog be date and commit-stamped, though.

lua/packer.lua

wbthomason · 2021-05-25T20:23:36Z

CHANGELOG.md

@@ -0,0 +1,22 @@
+# CHANGELOG


To clarify: we also ought to document this feature in the main docs, but I like having a changelog too. Also, the docs should describe what a snapshot does/does not save.

lua/packer.lua

lua/packer/plugin_types/git.lua

lua/packer/result.lua

lua/packer/plugin_types/git.lua

c3n21 · 2021-05-27T22:41:27Z

Just implemented PackerRollback.
Next I would like to:

add the completion for PackerRollback
add the tests and it should be good to go

c3n21 · 2021-05-31T19:37:29Z

@wbthomason @akinsho I've been working on the tests for the snapshot feature, but I can't get it to work with plugin_utils_spec.lua and packer_snapshot_spec.lua (which is the one I've created)

Could you give me some advice or point out where am I doing wrong?

akinsho · 2021-05-31T19:58:16Z

@c3n21 are you saying the tests don't run? or that you can't get them to pass? anyway re. running them you need to use PlenaryBustedDirectory tests/ {minimal_init = 'tests/minimal.vim'} I advise this over the file only test since that will pick up your init.vim and could be causing unpredictable results.

Regarding the tests themselves from what I can see it appears you might be nesting describe blocks inside the tests themselves which won't work

akinsho · 2021-05-31T19:59:44Z

Oh and a side note about the tests can you call them just snapshot_spec.lua I don't think the prefix is needed and I mean to try and change any of the existing test files that are prefixed that way since they should just use the filename + _spec.lua as the test name

tests/packer_snapshot_spec.lua

c3n21 · 2021-06-01T08:11:33Z

@akinsho

I've made removed the describe block inside the it block as you said and at least it runs now.

However, if I add some async code the test gives me this result

We had an unexpected error: { "...packer/start/plenary.nvim/lua/plenary/async_lib/util.lua:314: Blocking on future failed ...acker/st art/plenary.nvim/lua/plenary/async_lib/async.lua:20: expected a single return value\n\nstack traceback:\n\t...packer/start/plenary.nvim/l ua/plenary/async_lib/util.lua:319: in function <...packer/start/plenary.nvim/lua/plenary/async_lib/util.lua:303>\n" } { errs = {}, fail = {}, fatal = { "...packer/start/plenary.nvim/lua/plenary/async_lib/util.lua:314: Blocking on future failed ...acker/start/plenary.nvim/lua/p lenary/async_lib/async.lua:20: expected a single return value\n\nstack traceback:\n\t...packer/start/plenary.nvim/lua/plenary/async_lib/u til.lua:319: in function <...packer/start/plenary.nvim/lua/plenary/async_lib/util.lua:303>\n" }, pass = {} }

but if I remove all the async code (remove await and a. from describe and it it runs but it makes an empty snapshot.
That's probably because it doesn't install the test plugin, but I can't figure out how to make it work.

akinsho · 2021-06-01T08:38:41Z

@c3n21 the async testing has kind of been borrowed from plenary but packer has it's own async implementation so I think it's a little rough and obscure used together. When I've seen this error in the past it's usually meant that there is an error occurring in your test somewhere and this is how it is bubbling up. It could be the usage/syntax also I'll take a look and see if there are any incorrect usages

akinsho · 2021-06-01T08:43:19Z

@c3n21 I can't see any clear errors. One thing to keep in mind is that it's probably a good idea to test as close to your changes as possible for the time being i.e. just call your functions and assert they do the correct thing rather than trying to setup all of packer. A lot of those code paths are untested so you could easily get blocked by a completely separate issue that is unrelated to your change.

c3n21 · 2021-06-01T09:35:28Z

@c3n21 I can't see any clear errors. One thing to keep in mind is that it's probably a good idea to test as close to your changes as possible for the time being i.e. just call your functions and assert they do the correct thing rather than trying to setup all of packer. A lot of those code paths are untested so you could easily get blocked by a completely separate issue that is unrelated to your change.

The problem is that I need to be sure that at least one plugin is installed using packer so that I can snapshot it, check the snapshot format etc, so it's quite mandatory to call packer.install()

Without that it's pointless...

Atm I'm only checking if the snapshot have been created or not

akinsho

It looks like snapshot is only really dependent on list_missing_plugins, so rather than trying to run install which I think will have quite a lot of side effects you could try to just mock that function. I think plenary has a few mock utilities but I don't know if those will work with a required module so another thing you could do is in the setup of your test you could replace require with fake_require and check if the value is that module and return a fake module

function test_require(module_name)
  if module_name == "plugin_utils" then
    return {list_missing_plugins = function() return {value_I_want} end}
  end
  return require(module_name)
end

-- Inside describe
_G.require = test_require

tests/packer_snapshot_spec.lua

c3n21 · 2021-06-10T06:07:20Z

Sorry for the absence but I need to take some exams, so I'll keep working on this once I'll be done (by the end of the month)

Just letting you know that I'm not dead and didn't give up about this feature :)

c3n21 · 2021-08-02T19:41:13Z

Update:

finally I've managed to pull it off.

I have updated the first message of this thread so check it out for the "basic usage".

I've also updated the documentation about the snapshot feature and added all the tests I could think of.

There is a "bug" though I already documented in the first comment

c3n21 · 2021-08-28T18:24:55Z

@wbthomason is this feature still needed? Because it still needs some improvement but if it's not needed I'm not going to improve this

akinsho · 2021-08-29T11:41:12Z

@c3n21 @.wbthomason is away atm and I haven't really been following things too closely. I don't think the need for this has changed though since it's driven entirely by all the people who would want to use it which I don't think has changed and no alternative has been implemented

c3n21 · 2021-08-29T13:42:52Z

@c3n21 @.wbthomason is away atm and I haven't really been following things too closely. I don't think the need for this has changed though since it's driven entirely by all the people who would want to use it which I don't think has changed and no alternative has been implemented

Ok, I'll mention you for the review when I'll be done

c3n21 · 2021-08-30T14:12:53Z

@akinsho it should be good now. I've added the docs and updated the README.md.

wbthomason · 2021-08-30T19:57:01Z

Hi @c3n21! Thanks for your continued work on this, and apologies for my lack of response; as @akinsho mentioned, I was gone.

For reviewing/testing: do you have any suggested operations which would be good to validate that everything is working?

c3n21 · 2021-08-30T20:20:10Z

Hi @c3n21! Thanks for your continued work on this, and apologies for my lack of response; as @akinsho mentioned, I was gone.

For reviewing/testing: do you have any suggested operations which would be good to validate that everything is working?

Hi and welcome back!
I've already provided some tests for git.get_rev (which fails and I don't know why), git.revert (not revert_last, mine does a hard reset of the git repo to git.commit),snapshot(), packer.delete().

For testing you could:

Make a snapshot with PackerSnapshot <snapshot name>;
PackerUpdate and ensure that git rev-parse --short HEAD of the plugins updated in ~/.local/share/nvim/site/pack/packer/start/ is different with the one reported inside ~/.cache/nvim/packer/<snapshot name>;
PackerRollback <snapshot name> (you can use TAB to autocomplete the snapshot name) and rollback to and check with git rev-parse --short HEAD that the package have been resetted properly.
PackerDelete <snapshot name> (TAB if you want autocompletion) if you don't need that snapshot anymore

You can also specify in the config, e.g. config = {snapshot = <snapshot name>} when you run packer.startup() which calls packer.rollback(config.snapshot) under the hood, so it should be the same as doing it manually.

c3n21 · 2021-09-22T15:59:32Z

Hi @wbthomason ! Did you manage to review this feature?

c3n21 · 2021-11-22T18:48:39Z

I've updated my PR and now it seems to pass all the tests.
I've also cleaned up my fork's history since it was a bit cluttered.
Could you take a look when you have some time? @wbthomason

wbthomason · 2022-02-05T19:25:06Z

Taking a look now. Thanks for the work @c3n21, guidance @shadmansaleh, and patience on everyone's parts.

wbthomason

This looks mostly good to go. However:

We should resolve the currently failing tests
packer.snapshot.delete does not seem to work for me - I get an error at snapshot.lua:189.
We should probably make a function analogous to packer.snapshot for snapshot deletion - i.e. at the top-level and with the async aspect wrapped appropriately.

lua/packer.lua

wbthomason · 2022-02-05T19:49:22Z

lua/packer.lua

@@ -794,6 +805,106 @@ packer.plugin_complete = function(lead, _, _)
  return completion_list
 end

+---Snapshots installed plugins
+---@param snapshot_name string absolute path or just a snapshot name
+packer.snapshot = function(snapshot_name, ...)


Nitpick: I think we should either print an error or choose a sane default if snapshot_name is nil.

c3n21 · 2022-02-14T14:45:03Z

Right now I'm having some issues with the async code as mentioned here

If anyone could help me figure out what's going out it would be great.

wbthomason · 2022-02-14T19:17:15Z

@c3n21 I fixed the merge conflict and am re-running the tests; I'll look at the trace and see if I can figure out what's going on.

wbthomason · 2022-02-14T19:35:28Z

@c3n21 Tests seem good to go now. Anything remaining before merge that you know of?

c3n21 · 2022-02-14T20:05:06Z

@c3n21 Tests seem good to go now. Anything remaining before merge that you know of?

I've made a minor refactor to make the code inside snapshot.create() shorter and a bit more modular.
If you don't like it just reset it and merge it as it is if it's all good to go.

Thanks for your time!

* when taking a snapshot, if `snapshot_name` exists the user will be asked if they want to overwrite the existing snapshot * better error handling * packer.snapshot() will default to '%Y-%m-%d' if no snapshot name is provided

snapshot.delete is not async

Dockerfile: * added non-root user `test` * added entrypoint to automatically run `make test` inside archlinux container * copy packer.nvim directly into the container Makefile: * added run and run-tests to quickly run tests inside the container

c3n21 · 2022-02-15T17:21:31Z

@wbthomason I've polished a bit the code with the advices you gave me above.
I've also added an updated version of the Dockerfile and Makefile I've used during my tests which may be useful in the future.
In the Dockerfile I've created a test user and copied packer.nvim root into the container to simulate a common use case (I don't think we always run nvim as root).

Also I think this might be useful because rollback hard resets the repo, and using it on the host machine could reset all the work not pushed, and running tests is as easy as running make run-tests.

wbthomason · 2022-02-25T16:25:26Z

Thanks again, @c3n21! I think we're good to merge.

smhc · 2022-02-26T05:57:49Z

Thanks @c3n21 - good to see this merged. Thanks for all your work on it.

I've been using it since the .lua snapshot generation and have now switched to the more recent incarnation which has .json generation. I do notice that a side effect of async and vim.encode_json is that the .json file generated is not ordered or stable. This is not ideal for checking into source control and easily seeing what changed - I would expect this snapshot file to be source controlled by many people.

I couldn't find a simple solution so I simply use 'jq --sort-keys' on the file afterwards - but if anyone knows a simple fix for packer to do it automatically I can raise a PR.

shadmansaleh · 2022-02-26T07:09:14Z

The output isn't table because lua tables aren't stable . We can use list and sort the list before converting it to json.

c3n21 · 2022-02-26T07:40:22Z

Thanks @c3n21 - good to see this merged. Thanks for all your work on it.

I've been using it since the .lua snapshot generation and have now switched to the more recent incarnation which has .json generation. I do notice that a side effect of async and vim.encode_json is that the .json file generated is not ordered or stable. This is not ideal for checking into source control and easily seeing what changed - I would expect this snapshot file to be source controlled by many people.

I couldn't find a simple solution so I simply use 'jq --sort-keys' on the file afterwards - but if anyone knows a simple fix for packer to do it automatically I can raise a PR.

What about sorting the table before encoding? That should be consistent isn't it?

EDIT:

Just saw the reply

arathunku · 2022-02-27T08:17:29Z

I've solved keys sorting by additionally processing the final file, given that it's a json dump.

echo $(jq --sort-keys . packages) > packages
prettier --parser json --write packages

It's not hooked into vim but into my script for updating plugins with --headless mode.
Easy on eyes diffs :)

EDIT: now I see @smhc had already suggested that 🤦

smhc · 2022-02-27T10:45:54Z

Yes I use something like:

    packer.snapshot_format = function(fn)
      local fullpath=vim.fn.stdpath('config')..'/'..fn
      local tmp=vim.fn.stdpath('config')..'/temp_'..fn
      os.execute('jq --sort-keys . '..fullpath..'> '..tmp)
      os.rename(tmp, fullpath)
    end

The jq format seems as good as it can get, I don't think there's any need to run 'prettier'. I'd prefer to chain it with 'packer.snapshot' however I couldn't get the 'await', 'and_then' stuff working correctly, and without that there is race conditions causing empty files etc. So I just run it manually afterwards.

akinsho reviewed May 25, 2021

View reviewed changes

wbthomason requested changes May 25, 2021

View reviewed changes

wbthomason mentioned this pull request May 27, 2021

[Feature request] Support package snapshot #298

Closed

akinsho reviewed May 27, 2021

View reviewed changes

lua/packer/plugin_types/git.lua Outdated Show resolved Hide resolved

c3n21 marked this pull request as draft May 27, 2021 17:03

akinsho reviewed May 31, 2021

View reviewed changes

tests/packer_snapshot_spec.lua Outdated Show resolved Hide resolved

akinsho reviewed Jun 1, 2021

View reviewed changes

tests/packer_snapshot_spec.lua Outdated Show resolved Hide resolved

tests/packer_snapshot_spec.lua Outdated Show resolved Hide resolved

c3n21 marked this pull request as ready for review August 2, 2021 19:38

c3n21 requested a review from wbthomason August 4, 2021 15:34

c3n21 requested a review from akinsho August 30, 2021 15:52

wbthomason requested changes Feb 5, 2022

View reviewed changes

vuki656 mentioned this pull request Feb 6, 2022

Write a script that copies the nvim config at the machine start and system falls back to it if the current config fails vuki656/nvim-config#124

Closed

Merge branch 'master' into snapshot

63db854

wbthomason added 2 commits February 14, 2022 13:31

Update Neovim versions for testing

abde04d

Attempt to fix snapshot tests by using proper async function

e36f8ea

refactor(snapshot)

431a9dc

c3n21 and others added 8 commits February 14, 2022 21:08

refactor(snapshot)

85f5afe

chore: format with stylua

5ead3e4

refactor(snapshot)

f840ff9

* when taking a snapshot, if `snapshot_name` exists the user will be asked if they want to overwrite the existing snapshot * better error handling * packer.snapshot() will default to '%Y-%m-%d' if no snapshot name is provided

chore: format with stylua

c42345b

refactor(snapshot)

31c7cf2

* when taking a snapshot, if `snapshot_name` exists the user will be asked if they want to overwrite the existing snapshot * better error handling * packer.snapshot() will default to '%Y-%m-%d' if no snapshot name is provided

refactor(snapshot)

89d9b1a

snapshot.delete is not async

refactor(snapshot): improved async logic of rollback

b4f45e7

build

4864933

Dockerfile: * added non-root user `test` * added entrypoint to automatically run `make test` inside archlinux container * copy packer.nvim directly into the container Makefile: * added run and run-tests to quickly run tests inside the container

wbthomason merged commit 40cbd5c into wbthomason:master Feb 25, 2022

FelixKratz mentioned this pull request Apr 30, 2022

Let users know when *BREAKING CHANGES* occur before running :NvChadUpdate NvChad/NvChad#984

Closed

younger-1 mentioned this pull request May 1, 2022

Feat: add PackerSnapshotDone autocmd #898

Open

frdwhite24 mentioned this pull request May 8, 2022

Find a way to make my build more stable (dependency version manager) frdwhite24/nvim#2

Closed

[Feature request] Support package snapshot #298 #370

[Feature request] Support package snapshot #298 #370

Conversation

c3n21 commented May 24, 2021 • edited Loading

Final Implementation

Usage

Make snapshot

Restore snapshot

Delete snapshot

Misc:

Known issues:

c3n21 commented May 24, 2021

akinsho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c3n21 commented May 25, 2021

wbthomason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c3n21 commented May 27, 2021

c3n21 commented May 31, 2021

akinsho commented May 31, 2021

akinsho commented May 31, 2021

c3n21 commented Jun 1, 2021

akinsho commented Jun 1, 2021

akinsho commented Jun 1, 2021

c3n21 commented Jun 1, 2021

akinsho left a comment

Choose a reason for hiding this comment

c3n21 commented Jun 10, 2021

c3n21 commented Aug 2, 2021 • edited Loading

c3n21 commented Aug 28, 2021

akinsho commented Aug 29, 2021

c3n21 commented Aug 29, 2021

c3n21 commented Aug 30, 2021

wbthomason commented Aug 30, 2021

c3n21 commented Aug 30, 2021 • edited Loading

c3n21 commented Sep 22, 2021

c3n21 commented Nov 22, 2021

wbthomason commented Feb 5, 2022

wbthomason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c3n21 commented Feb 14, 2022

wbthomason commented Feb 14, 2022

wbthomason commented Feb 14, 2022

c3n21 commented Feb 14, 2022

c3n21 commented Feb 15, 2022 • edited Loading

wbthomason commented Feb 25, 2022

smhc commented Feb 26, 2022

shadmansaleh commented Feb 26, 2022 • edited Loading

c3n21 commented Feb 26, 2022 • edited Loading

arathunku commented Feb 27, 2022 • edited Loading

smhc commented Feb 27, 2022

c3n21 commented May 24, 2021 •

edited

Loading

c3n21 commented Aug 2, 2021 •

edited

Loading

c3n21 commented Aug 30, 2021 •

edited

Loading

c3n21 commented Feb 15, 2022 •

edited

Loading

shadmansaleh commented Feb 26, 2022 •

edited

Loading

c3n21 commented Feb 26, 2022 •

edited

Loading

arathunku commented Feb 27, 2022 •

edited

Loading