feat(tools): allow specifying paths instead of auto installing#488
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.
- 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
76b5728 to
a16ae90
Compare
a16ae90 to
180e258
Compare
|
Thanks for the contribution <3 |
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!