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: support extending the default luarocks config #452

Merged
merged 10 commits into from
Jul 5, 2024

Conversation

mrcjkb
Copy link
Member

@mrcjkb mrcjkb commented Jul 2, 2024

Stacked on #442

This adds a solution for #443 by making it easier to defend the default luarocks config.

You can add extra directories to search for binaries via

vim.g.rocks_nvim = {
  -- ...
  ---@type string | table file path or config table to extend the default with
  luarocks_config = {
    external_deps_dirs = { "/some/path" },
  },
}

See https://github.com/luarocks/luarocks/wiki/Config-file-format for details.

Copy link
Contributor

github-actions bot commented Jul 2, 2024

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 mrcjkb force-pushed the extra_luarocks_config branch 5 times, most recently from a477b45 to 1e258d8 Compare July 2, 2024 23:00
.gitignore Outdated Show resolved Hide resolved
@teto
Copy link
Member

teto commented Jul 3, 2024

unrelated to this PR but I noticed some redundancy between lua/rocks/config/init.lua and lua/rocks/config/internal.lua RocksOpts and RocksConfig : aren't these the same ? can we do something about the doc redundancy ?

@mrcjkb mrcjkb force-pushed the extra_luarocks_config branch from 1e258d8 to fba7170 Compare July 3, 2024 14:33
@mrcjkb
Copy link
Member Author

mrcjkb commented Jul 3, 2024

unrelated to this PR but I noticed some redundancy between lua/rocks/config/init.lua and lua/rocks/config/internal.lua RocksOpts and RocksConfig : aren't these the same ? can we do something about the doc redundancy ?

config/init and RocksOpts are exposed to the user. Each field of RocksOpts is optional (can be nil), allowing users to omit fields without getting lua-language-server warning diagnostics.

config/internal and RocksConfig are not exposed to the user. None of the fields of RocksConfig are optional, giving us better type safety.

It's a bit of boilerplate, but worth it imo.

@mrcjkb mrcjkb requested a review from teto July 3, 2024 22:00
@mrcjkb
Copy link
Member Author

mrcjkb commented Jul 3, 2024

I've made writing/loading the luarocks config async + on demand. That ought to minimise the impact on startup time.
Also, if we need to update the config at runtime (maybe for parallel installs?), this is the better approach.

@mrcjkb mrcjkb force-pushed the extra_luarocks_config branch 2 times, most recently from 5c78881 to b524d3b Compare July 3, 2024 22:08
@teto
Copy link
Member

teto commented Jul 3, 2024

I dont know what to think here. The luarocks_config as a string/table makes things more complex to understand.

If it's a string, then we just load the file without merging.
If it's a table, then we merge and write the file (but can't decide the location anymore).

Also it's user-facing so if it ends being a bad idea, it's hard to rewind.

One of the problems is that our luarocks config depends on vim.g.rocks_nvim.rocks_path, that seems like the only reason for merging the configs. We could mention that the first rock_tree is where to install stuff ? but that might be too complex for users.

If I take the nix scenario, I can generate the variables/rockspecs and pass it as vim.g.rocks_nvim.luarocks_config table. Right now, I had to copy the defaults from rocks.nvim into my generated luarocks config file so in that regards, the PR improves things.

@mrcjkb mrcjkb force-pushed the extra_luarocks_config branch from b524d3b to fe5e7da Compare July 3, 2024 22:17
@mrcjkb
Copy link
Member Author

mrcjkb commented Jul 3, 2024

If it's a string, then we just load the file without merging.
If it's a table, then we merge and write the file (but can't decide the location anymore).

The string is to prevent this from being a breaking change.
If we decide we don't want to support it anymore, we can start off with a vim.deprecate and then remove support for it being a string in version 3.

Edit: Added a deprecation notice as discussed.

Base automatically changed from install_args to master July 3, 2024 22:27
@mrcjkb mrcjkb force-pushed the extra_luarocks_config branch 7 times, most recently from f8833c4 to a56c9ed Compare July 4, 2024 16:19
@mrcjkb mrcjkb force-pushed the extra_luarocks_config branch from a56c9ed to 25bbb2b Compare July 4, 2024 16:19
@mrcjkb mrcjkb force-pushed the extra_luarocks_config branch from 25bbb2b to 034c112 Compare July 4, 2024 16:26
@mrcjkb
Copy link
Member Author

mrcjkb commented Jul 4, 2024

How on earth did the integration tests get flaky again after a rebase?

@mrcjkb mrcjkb force-pushed the extra_luarocks_config branch 4 times, most recently from 00393ec to 38f8dad Compare July 4, 2024 16:46
plugin/rocks.lua Outdated Show resolved Hide resolved
@mrcjkb
Copy link
Member Author

mrcjkb commented Jul 5, 2024

Merging, as tests are succeeding and all issues have been addressed.

@mrcjkb mrcjkb enabled auto-merge (rebase) July 5, 2024 15:03
@mrcjkb mrcjkb merged commit 7da4f42 into master Jul 5, 2024
10 checks passed
@mrcjkb mrcjkb deleted the extra_luarocks_config branch July 5, 2024 15:03
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.

3 participants