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

feat: manage luarocks installation as rockspec dependency #340

Merged
merged 3 commits into from
Jun 7, 2024
Merged

Conversation

mrcjkb
Copy link
Member

@mrcjkb mrcjkb commented May 17, 2024

Closes #336.

Existing installations will have to unset vim.g.rocks_nvim.luarocks_binary for this to take effect (Linux and macOS only).

@mrcjkb mrcjkb marked this pull request as draft May 17, 2024 17:37
Copy link
Contributor

Review Checklist

Does this PR follow the Contribution Guidelines? Following is a partial checklist:

Proper conventional commit scoping:

  • For example, fix(installer): some installer bugfix

  • Pull request title has the appropriate conventional commit prefix.

If applicable:

  • Tested
    • Tests have been added.
    • Tested manually (steps in PR description).
  • Updated documentation.

@mrcjkb
Copy link
Member Author

mrcjkb commented May 17, 2024

The bootstrapping guide still hard codes a luarocks install. Not sure how to solve that.

@vhyrro
Copy link
Collaborator

vhyrro commented May 18, 2024

Code written thus far looks good to me apart from the removal NTB mentioned.

In my opinion, we should rewrite a large portion of the installation script to perform a temporary installation into /tmp for UNIX systems. The bootstrapped luarocks should only serve as a temporary vessel for the one actually installed into the tree. To make the experience smooth, the temporary luarocks should be passed the same --tree as the location where the new luarocks will be installed; the freshly installed luarocks should also use this same tree therefore as well.

I think we should try to tackle the problem in one big swoop here, it's quite likely we'll need to mark these changes as breaking and bump to 3.0? I can imagine that the vim.g.rocks might need some slight changes.

@mrcjkb
Copy link
Member Author

mrcjkb commented May 19, 2024

I've updated the installer and bootstrap scripts to install to a temp directory (not tested yet).

I think we should try to tackle the problem in one big swoop here, it's quite likely we'll need to mark these changes as breaking and bump to 3.0? I can imagine that the vim.g.rocks might need some slight changes.

I don't think actually breaks any existing installations. And it might be nice to still be able to override the luarocks_binary in the config (e.g. when using nix).
But removing the luarocks install from existing installations would most likely be a manual process, so a major version bump might be in order. This would also give us an opportunity to remove the deprecated API.

@mrcjkb
Copy link
Member Author

mrcjkb commented May 19, 2024

I can't figure out why the nix build of rocks.nvim fails with

┃        > Missing dependencies for rocks.nvim scm-1:
┃        >    luarocks (not installed)
┃        >
┃        > rocks.nvim scm-1 depends on lua >= 5.1 (5.1-1 provided by VM: success)
┃        > rocks.nvim scm-1 depends on luarocks (not installed)
┃        > Warning: Failed searching manifest: Failed downloading https://luarocks.org/manifest-5.1 - no downloader tool available, please in…
┃        >
┃        > Error: Could not satisfy dependency luarocks: No results matching query were found for Lua 5.1.
┃        For full logs, run 'nix log /nix/store/arbpyscrxmvx2xyj9hhv77rmwrzf28b4-luajit2.1-rocks.nvim-scm-1.drv'.

@teto do you have an idea? Could this be an issue with build-luarocks-package.nix?
Maybe lua51Packages.luarocks doesn't have a rock tree that can be picked up by luarocks?

It looks like the rocks_dir doesn't point to the full nix store path, like it does for other rocks:

  {
    ["name"] = "dep-0",
    ["rocks_dir"] = "luarocks-3.11.0-rocks",
    ["root"] = "/nix/store/rzafl4br5rpmxs36srcj5vs7r03h785k-luarocks-3.11.0"
  },

@mrcjkb
Copy link
Member Author

mrcjkb commented May 30, 2024

NixOS/nixpkgs#316009

@mrcjkb
Copy link
Member Author

mrcjkb commented May 30, 2024

This one is technically ready, but I would like to wait for a luarocks release that has a fix for luarocks/luarocks#1661

@mrcjkb mrcjkb force-pushed the luarocks branch 3 times, most recently from 342e763 to 9e353a7 Compare May 30, 2024 23:44
@mrcjkb
Copy link
Member Author

mrcjkb commented May 30, 2024

...and of course it's broken on Windows 😠

@mrcjkb mrcjkb force-pushed the luarocks branch 8 times, most recently from 5c8e24b to e53c0e2 Compare May 31, 2024 11:35
@mrcjkb
Copy link
Member Author

mrcjkb commented May 31, 2024

So... on Windows, luarocks installs a rocks_path/bin/luarocks.bat binary when installing the luarocks package.
This doesn't seem to work with vim.system, as it doesn't accept unix-like CLI args.

I'm done with this for Windows 😠. We have a few options:

  • Request that the luarocks rock install a proper luarocks executable, equivalent to what you get on Windows when installing luarocks regularly.
  • Tell Windows users to use a system installation and keep it up-to-date
  • Implement a special case for the installer (which doesn't have proper Windows support yet), where we don't install luarocks into a temp directory on Windows.

@teto
Copy link
Member

teto commented May 31, 2024

sry I completely missed your ping here. The nix issue will be fixed with NixOS/nixpkgs#316221 hopefully

(dropping this here https://github.com/oploadk/localua as it looked of interest).

@mrcjkb mrcjkb marked this pull request as ready for review June 3, 2024 15:01
@vhyrro
Copy link
Collaborator

vhyrro commented Jun 7, 2024

Code LGTM, although I've yet to actually try this on my machine, so that'll be the next step :)

Copy link
Collaborator

@vhyrro vhyrro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a crude manual test but it seems to function nicely :)

@mrcjkb mrcjkb enabled auto-merge (squash) June 7, 2024 17:50
@mrcjkb mrcjkb merged commit b74b36f into master Jun 7, 2024
9 of 10 checks passed
@mrcjkb mrcjkb deleted the luarocks branch June 7, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] pin luarocks version / manage luarocks installation
4 participants