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

Bulk install fails to write to rocks.toml #28

Closed
sarmong opened this issue May 7, 2024 · 5 comments · Fixed by nvim-neorocks/rocks.nvim#323 or nvim-neorocks/rocks.nvim#324
Closed
Assignees

Comments

@sarmong
Copy link

sarmong commented May 7, 2024

If you call vim.cmd("Rocks install " .. plugin) synchronously for many plugins in a row the following happens.

  1. One plugin finishes installing and writes to rocks.toml
  2. Another plugin finishes and writes to the old version of the plugin, thus removing plugin 1.
  3. Etc.

If I try to bulk install 100 plugins, about 3 gets added to rocks.toml

@mrcjkb
Copy link
Member

mrcjkb commented May 7, 2024

Oof, that is going to be tricky to solve. We use asynchronous write operations when writing to rocks.toml

I guess a potential solution would be:

  • Keep track of the rocks.toml in-memory, along with a last modified date.
  • If we write to rocks.toml ourselves, update the last modified date of the cached config.
  • If we read from rocks.toml, check the last modified date, and compare it with the cache. Invalidate the cache if the rocks.toml's last modified date is more recent (indicating that the file has been edited manually).

@sarmong What is your use case for bulk installs?

@sarmong
Copy link
Author

sarmong commented May 8, 2024

I am just discovering rocks.nvim and to see how my current config will work with it, I just fed the table with all of my plugins into Rocks install and it failed.

What I did now is just ran a macro to generate toml declarations for each plugin. The problem with this approach is I don't have the rev field (as I get after running Rocks install). Even after running sync or update the rev doesn't get added.

Some more issues:

  • If I do the same, but with rocks (not git), another problem gets added to this one - most rocks just fail to install at all.
  • I can't install anything if rocks-git is not installed. For example, if I have a table with a list of git plugins and rocks-git as the first item, none of the git plugins will be found as rocks-git hasn't loaded yet.

I understand that this is due to async nature of it. But maybe there should be a separation between a rocks plugin and a neovim plugin (or a general rock). Thus we can be sure that rocks plugins are installed first and foremost because other things depend on it.

Should I open separate issues in the core for these?

I really like the idea and the concept of this plugin manager, but the migration experience from another manager is rather frustrating. I also sometimes get neovim terminated with 139 error (iirc) after running sync, however, I haven't traced the exact problem yet.

@mrcjkb
Copy link
Member

mrcjkb commented May 8, 2024

Interesting. I have been thinking of creating a migration script for popular plugin managers.
Either as an addition to the installer, or as a plugin that you can install with your previous plugin manager and then dispose of after the migration.

If it uses :Rocks sync, it won't have the issue of missing rocks.nvim modules, because :Rocks sync already takes care of that.

Should I open separate issues in the core for these?

I'm not too sure about supporting bulk installs beyond maybe fixing the concurrency issues. It would add quite a bit of complexity that would be redundant with the migration tool.

@sarmong
Copy link
Author

sarmong commented May 8, 2024

Interesting. I have been thinking of creating a migration script for popular plugin managers.

IMHO, a generic approach that accepts { name: 'plugin-name', rev: 'hash|tag' } (as a single table or a list) seems to me much more future-proof. Getting list of packages from any package manager is an easy task.

Instead of a separate plugin that will need to be 'disposed', it would be as easy as copying a lua code and sourcing it. Something like this:

local plugins = vim.tbl_map(function(p)
    return { name: p.source, rev = p. version }
end, MiniDeps.get_current_session())

rocks.install(plugins)

-- or even
for _, p in ipairs(plugins)
  rocks.install(p)
end

I'd say it not only useful as a migration tool (although, IMHO migration experience is vital for the adoption of the plugin), but also there may be cases when you need to install many packages at once. Say, if you install nvim-cmp, you also would want to install a bunch of completion sources along with it. So, running Rocks install hrsh7th/nvim-cmp hrsh7th/cmp-buffer ray-x/cmp-treesitter can be a common thing to do.

Right now you can't use this command, and if a user tries to run multiple Rocks install (obviously without waiting for the previous one to finished), they will end up trying to debug why not all plugins are in rocks.toml.

I haven't look into the implementation of the plugin yet, can rocks.toml just be locked from writing for a period of time? I guess I might be missing something, but wouldn't this flow work?

  1. Plugin finishes installing
  2. If rocks.toml is locked, wait until predefined timeout.
  3. If timeout - delete installed plugin and notify the user of an error.
  4. If rocks.toml is unlocked - lock it
  5. Write to it
  6. Unlock

If there are problems with that - then a quick fix to improve UX would be to immediately error out on Rocks install if the previous installation hasn't finished yet.

@mrcjkb
Copy link
Member

mrcjkb commented May 8, 2024

Adding support for bulk installations to :Rocks install could be tricky, because
it already supports the following signatures:

  • install {rock}
  • install {rock} {version}
  • install {rock} {opt1}={x} {opt2}={y} ...
  • install {rock} {version} {opt1}={x} {opt2}={y} ...

as well as (added by rocks-git):

  • install {owner}/{repo}
  • install {remote}:{owner}/{repo}
  • install {...} {opt1}={x} {opt2}={y} ...

I fear that adding additional signatures like

  • install {rock1} {rock2} ...
  • install {rock1}:{version} {rock2}:{version} ...
  • install {owner1}:{repo1} {owner2}:{repo2} ...

would significantly increase complexity and overwhelm users.

When we prevent race conditions in :Rocks install, sync, prune, etc. (nvim-neorocks/rocks.nvim#324), a command/signature for bulk installation will be obsolete. I don't think the slight reduction in characters typed warrants the extra complexity, especially since you can just use the command history and replace the plugin name.

As for a lua API (see :h rocks.api), there is already a require("rocks.api").install function, which installs a single rock and an optional version (which can also be rev={revision}).
It also takes an optional callback that is invoked after the installation has completed. But there's currently no guarantee that rocks.toml will be written by then. I have a fix for that in the pipeline.

When the two fixes have been merged, bulk installations will be possible.
See this test case.

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