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)luarocks: make rocks path configurable; fixes #672 #957

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

msva
Copy link

@msva msva commented Jul 3, 2022

Well, actually, it makes all paths configurable (just in case), but main reason was to make rocks path configurable, as I have ~/.cache on tmpfs (and it is not persistent across reboots. Moreover, actually, cache_dir by itself, wherever it placed, is not guaranteed to be persistent). So, it was impossible for me to properly use rocks functionality with hardcoded rocks path...

Copy link
Owner

@wbthomason wbthomason left a comment

Choose a reason for hiding this comment

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

Thanks for this; it's a good improvement. I would like to request a small change: the configuration logic can be cleaned up a bit by making the paths fields in the packer.luarocks config table, and using vim.tbl_extend.

local hererocks_install_dir = util.join_paths(rocks_path, lua_version.dir)
local shell_hererocks_dir = vim.fn.shellescape(hererocks_install_dir)
local config = nil
local cache_path = nil
Copy link
Owner

Choose a reason for hiding this comment

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

Slight nitpick: if we're making these externally configurable, let's group them together in a table to make them more manageable within this file. Really, I suppose they should just be fields in the config table, and we should use vim.tbl_extend with the appropriate merge behavior.

Copy link
Author

@msva msva Aug 5, 2022

Choose a reason for hiding this comment

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

Well, I'll try to invent something here, but it wouldn't be so easy: most parameters requires values of previously-defined value.

Current behaviour is just to check if it is defined in config, and use defined value if it is, and some default value if don't. And, as it is executed line-by-line, if some "early" parameter was defined in config, it will affect all next-defined things.

And if we'll just use a table, this magic won't work, as we cant call neither other fields in the table of "defaults", nor values from not-yet-merged resulting table.

So, I'll still need to check values from _config, or invent some other way.

// by the way, I found that cache_path here is never used in anu way except being a part of rocks_path, so I removed it as useless.

Hope, you'll not object that change.

@wbthomason
Copy link
Owner

Also sorry for taking so long to get to this! I haven't had much time/energy for packer lately.

@msva
Copy link
Author

msva commented Aug 5, 2022

Well, I'll try to invent something here, but it wouldn't be so easy: most parameters requires values of previously-defined value.

Current behaviour is just to check if it is defined in config, and use defined value if it is, and some default value if don't. And, as it is executed line-by-line, if some "early" parameter was defined in config, it will affect all next-defined things.

And if we'll just use a table, this magic won't work, as we cant call neither other fields in the table of "defaults", nor values from not-yet-merged resulting table.

So, I'll still need to check values from _config, or invent some other way.

// by the way, I found that cache_path here is never used in anu way except being a part of rocks_path, so I removed it as useless.

Hope, you'll not object that change.

@msva
Copy link
Author

msva commented Aug 18, 2022

@wbthomason ping? (direct ping in case you're overwhelmed by notifications)

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

Successfully merging this pull request may close these issues.

3 participants