feat(tools): allow specifying paths instead of auto installing#488
feat(tools): allow specifying paths instead of auto installing#488olisikh wants to merge 4 commits into
Conversation
|
I am currently using this and is semi-required for a nice working setup of nvim-java with NixOS (from what I can tell). It works well (although I haven't tested the vscode extensions since I leave those disabled). |
s1n7ax
left a comment
There was a problem hiding this comment.
Strongly agree with the motivation — many users (Nix, NixOS, sandboxed envs) want externally managed tooling. Tests are good. Some things to address:
Manager() re-instantiated on every install — lua/pkgm/resolve.lua install
Manager():install(...) constructs a new Manager (re-reading specs, re-logging init) for each package, whereas get_install_dir calls Manager:get_install_dir(...) statically. Either share a single instance, or — preferred — switch both to the static call style consistently. Mixing the two is confusing.
Silent fallback to nonexistent config dir — lua/java-core/ls/servers/jdtls/cmd.lua get_jdtls_config_path
If neither config_<plat> nor config_<os> directory exists, the function returns config_path (which is bogus) and jdtls fails later with a cryptic error. Please err.throw with a useful message naming both attempted paths.
get_lombok_path may return empty string
vim.fn.glob(...) returns '' when no match — the caller then constructs -javaagent: and jdtls silently runs without lombok. Worth detecting and erroring with "lombok jar not found at ".
auto_install = false + jdk.path = nil for JDK
For the JDK section in config.lua there's no auto_install = false path documented (and the README example shows JDK without auto_install toggling). If a user sets jdk.auto_install = false and forgets jdk.path, get_jdk_home will call Manager:get_install_dir for an uninstalled package. Either: (a) treat JDK identically to other tools (require either auto_install or path), or (b) document explicitly that jdk.auto_install = false requires jdk.path.
Cache key strategy — lua/java-core/utils/lsp.lua get_jdtls_cache_conf_path
The sha256-suffix-when-jdtls-root-set / legacy-name-when-not approach is reasonable for back-compat, but it does mean users on auto-install never benefit from per-version isolation. If switching JDTLS versions is a real workflow (it is for development), consider always hashing. Not a blocker; flag it for future cleanup.
Doc nit
README/doc says path makes nvim-java "not install the tool" — but for tools where enable = false is also possible, the interaction (enable=false + path=...) is undefined. A one-line clarification helps.
Overall the design is right; mostly hardening the error paths.
s1n7ax
left a comment
There was a problem hiding this comment.
Strongly support the motivation — Nix/sandboxed users want this. Tests look good. A few hardening asks inline.
| err.throw(('nvim-java: %s auto_install disabled and no path configured'):format(name)) | ||
| end | ||
|
|
||
| return Manager():install(name, config.version) |
There was a problem hiding this comment.
Manager() constructs a fresh instance (re-reading specs, re-logging init) on every install call, while line 39 uses the class-static form Manager:get_install_dir(...). Pick one — either share a single instance across the module, or call both statically. Mixed styles will trip up the next reader.
| return os_config_path | ||
| end | ||
|
|
||
| return config_path |
There was a problem hiding this comment.
Silent fallback to a nonexistent directory: if neither config_<plat> nor config_<os> exists, we return config_path (which we just confirmed doesn't exist) and jdtls fails later with a cryptic error. Please err.throw here naming both attempted paths so the user knows what to check.
| return config.lombok.path | ||
| end | ||
|
|
||
| local lombok_root = M.get_install_dir('lombok', config.lombok) |
There was a problem hiding this comment.
vim.fn.glob(...) returns '' when no match — caller then constructs -javaagent: and jdtls runs silently without lombok. Worth detecting empty and err.throw('lombok jar not found at <pattern>').
There was a problem hiding this comment.
added extra check:
if lombok_jar == '' then
err.throw('nvim-java: lombok jar not found at ' .. lombok_jar_pattern)
end
|
|
||
| ---@param config java.Config | ||
| ---@return string | ||
| function M.get_jdk_home(config) |
There was a problem hiding this comment.
JDK handling diverges from the other tools: the resolver here only checks has_path then falls through to Manager:get_install_dir. There's no can_auto_install check, so if a user sets jdk.auto_install = false and forgets jdk.path, they'll get a confused install-dir lookup for something never installed. Either align with install()'s contract, or document explicitly that jdk.auto_install = false requires jdk.path.
| local cache_root = M.get_jdtls_cache_root_path() | ||
| local conf_path = path.join(cache_root, 'config') | ||
| local cache_key = get_cache_key(jdtls_root) | ||
| local conf_dir_name = cache_key and ('config_' .. cache_key) or 'config' |
There was a problem hiding this comment.
Hashed-when-set / legacy-name-when-not is reasonable for backward compat, but it means auto-install users never get per-version isolation when they bump JDTLS. Worth a follow-up to always hash so version bumps don't pick up stale config state. Not a blocker.
There was a problem hiding this comment.
Hmm, I think this one is fine, if get_jdtls_root() returns a unique path (which I think it does), i.e. ~/.local/share/nvim/nvim-java/packages/jdtls/1.55.0, then get_cache_key() would always generate a new value.
- declare manage at module scope once and reuse - throw if plugin config is inconsistent - update docs to explain path parameter has no effect when tool is disabled
This PR adds extra configuration property per each tool called
path.If the
pathis specified,nvim-javatries to resolve the tool by the path, if it can't andauto_install = true, it would download it, ifauto_install = falsean error would be shown.The cache path is computed with extra hash to avoid cache collisions when switching tools. If java is auto-installed, the old cache path strategy is used (no hashing for backward compatiblity).
Some (or all?) jdtls distributions do not have
config_mac_armfile, onlyconfig_macmaking jdtls to fail instantly.Instead of failing instantly another attempt is made to resolve config by OS name.
I believe this is an important change for people who do not like their tools downloaded randomly from the internet.
This change should not have any breaking changes, but don't take my words for granted. I have tested this change and it works well.
Would be happy to hear back and adjust if necessary, thanks a bunch!