LuaLS / lua-language-server

A language server that offers Lua language support - programmed in Lua
https://luals.github.io
MIT License
3.29k stars 305 forks source link

Fix lua-language-server --check (#2683) #2775

Closed Freed-Wu closed 1 month ago

Freed-Wu commented 2 months ago

Fix #2683

tomlau10 commented 2 months ago

https://github.com/LuaLS/lua-language-server/pull/2775/files#r1696338679

I use archlinux's lua-language-server. shell is zsh.

Hi, I just reproduced this using archlinux (docker) as stated in your environment

$ docker run -it --rm archlinux bash
[root@80e6e013091f /]# pacman -Sy --noconfirm lua-language-server
:: Synchronizing package databases...
 core downloading...
 extra downloading...
resolving dependencies...
looking for conflicting packages...

Package (1)                New Version  Net Change  Download Size

extra/lua-language-server  3.9.3-1       18.29 MiB       2.84 MiB

Total Download Size:    2.84 MiB
Total Installed Size:  18.29 MiB

:: Proceed with installation? [Y/n] 
:: Retrieving packages...
 lua-language-server-3.9.3-1-x86_64 downloading...
checking keyring...
checking package integrity...
loading package files...
checking for file conflicts...
:: Processing package changes...
installing lua-language-server...
:: Running post-transaction hooks...
(1/1) Arming ConditionNeedsUpdate...
[root@80e6e013091f /]# lua-language-server --check
subprocess::spawn: (system:2)No such file or directory

exec /usr/lib/lua-language-server/bin/lua-language-server -E /usr/lib/lua-language-server/main.lua \ --logpath="$DEFAULT_LOGPATH" --metapath="$DEFAULT_METAPATH" \ "$@"


---

Now it is clear that the `-E` is inserted in this launch script 😕
And this launch script feels **wrong** to me.
- There should not be `-E /usr/lib/lua-language-server/main.lua`
- It should be simply `/usr/lib/lua-language-server/bin/lua-language-server "$@"` as suggested in LuaLS Q&A here: https://luals.github.io/#other-install
> If you plan to use a symbolic link to point to the language server, you will want to instead use a wrapper script like the one below. This is required because the language server expects to find files in the current working directory.

```bash
#!/bin/bash
exec "<path-to-directory>/bin/lua-language-server" "$@"

You can verify this by just executing the full binary path:

[root@80e6e013091f /]# /usr/lib/lua-language-server/bin/lua-language-server --check
The argument of CHECK must be a string, but got boolean

No -E xxx whatever is required, and it works as expected. Therefore the root cause is the incorrect launch script of archlinux package, but not LuaLS itself. @Freed-Wu

tomlau10 commented 2 months ago

Oh I just found that lua-language-server can be used as lua interpreter, it even supports a -e option during bootstrap 😂 https://github.com/LuaLS/lua-language-server/blob/2a48dee3f216992150f05e288fa6a439c864f0b8/make/bootstrap.lua#L1-L10

Now I agree that we just can't assume arg[-1] is the executable 👍 From the bootstrap logic, it allows the executable to be used like a lua interpreter, and there might be other cli options before the main script.


I assume that the executable path will always be the most negative index that give a non nil value in arg[] ? Would it be better to use while loop and find the most negative index as the exe name? Maybe something like this:

-- exe name will be at the most negative index argument
local exe
local minIndex = -1
while arg[minIndex] do
    exe = arg[minIndex]
    minIndex = minIndex - 1
end

This can handle some extreme cases where in someday, someone modifies the launch script to eg. exec /usr/lib/lua-language-server/bin/lua-language-server -A -B -C -D -E /usr/lib/lua-language-server/main.lua ... (for whatever unknown reason), the --check will still works on all platforms 🤣

fidgetingbits commented 2 months ago

fwiw I was consistently running into this bug on NixOS and this PR fixes it for me.

CppCXY commented 2 months ago

If you are ready, please update the changelog. I don't have time to test on other platforms.

sumneko commented 1 month ago

I don't have time to test, but I believe that this pull request has been completed.