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(lua): add vim.system() #23827

Merged
merged 1 commit into from Jun 7, 2023
Merged

Conversation

lewis6991
Copy link
Member

@lewis6991 lewis6991 commented May 30, 2023

Problem

Handling system commands in Lua is tedious and error-prone:

  • vim.fn.jobstart() is vimscript and comes with all limitations attached to typval.
  • vim.loop.spawn is too low level

Also see: #17620 (comment)

Solution

Add vim.system().

local on_exit = function(code, signal, stdout, stderr)
  print(code)
  print(signal)
  print(stdout)
  print(stderr)
end

-- Asynchronous
vim.system({'echo', 'hello'}, nil, on_exit)

-- Synchronous
local stdout = vim.system({'echo', 'hello'}):wait().stdout
system({cmd}, {opts}, {on_exit})                                *vim.system()*
    Run a system command

    See |uv.spawn()| for more details.

    Parameters: ~{cmd}      (string[]) Command to execute
      • {opts}     (SystemOpts|nil) Options:
                   • cwd: (string) Set the current working directory for the
                     sub-process.
                   • env: table<string,string> Set environment variables for
                     the new process. Inherits the current environment with
                     `NVIM` set to |v:servername|.
                   • clear_env: (boolean) `env` defines the job environment
                     exactly, instead of merging current environment.
                   • stdin: (string|string[]|boolean) If `true`, then a pipe
                     to stdin is opened and can be written to via the
                     `write()` method to SystemObj. If string or string[] then
                     will be written to stdin and closed. Defaults to `false`.
                   • stdout: (boolean|function) Handle output from stdout.
                     When passed as a function must have the signature
                     `fun(err: string, data: string)`. Defaults to `true`
                   • stderr: (boolean|function) Handle output from stdout.
                     When passed as a function must have the signature
                     `fun(err: string, data: string)`. Defaults to `true`.
                   • text: (boolean) Handle stdout and stderr as text.
                     Replaces `\r\n` with `\n`.
                   • timeout: (integer)
                   • detach: (boolean) If true, spawn the child process in a
                     detached state - this will make it a process group
                     leader, and will effectively enable the child to keep
                     running after the parent exits. Note that the child
                     process will still keep the parent's event loop alive
                     unless the parent process calls |uv.unref()| on the
                     child's process handle.
      • {on_exit}  (function|nil) Called when subprocess exits. When provided,
                   the command runs asynchronously. Receives SystemCompleted
                   object, see return of SystemObj:wait().

    Return: ~
        SystemObj Object with the fields:
        • pid (integer) Process ID
        • wait (fun(timeout: integer|nil): SystemCompleted)
          • SystemCompleted is an object with the fields:
            • code: (integer)
            • signal: (integer)
            • stdout: (string), nil if stdout argument is passed
            • stderr: (string), nil if stderr argument is passed

        • kill (fun(signal: integer))
        • write (fun(data: string|nil)) Requires `stdin=true`. Pass `nil` to
          close the stream.
        • is_closing (fun(): boolean)

Todo

  • Add tests

Future work

  • pty support
  • /dev/null support. (Can pass an empty handler for the same effect)
  • Execution via a shell

@github-actions github-actions bot added refactor changes that are not features or bugfixes lsp labels May 30, 2023
@lewis6991 lewis6991 force-pushed the feat/systemlua branch 11 times, most recently from 3c81b0f to 878caf5 Compare May 31, 2023 11:16
@lewis6991 lewis6991 changed the title refactor: add _system.lua module for system commands feat(lua): add vim.system() May 31, 2023
@lewis6991 lewis6991 mentioned this pull request May 31, 2023
2 tasks
@lewis6991 lewis6991 added refactor changes that are not features or bugfixes lua stdlib and removed refactor changes that are not features or bugfixes lsp labels May 31, 2023
@lewis6991 lewis6991 force-pushed the feat/systemlua branch 3 times, most recently from 603c8d0 to 7060ffe Compare May 31, 2023 11:34
@clason
Copy link
Member

clason commented May 31, 2023

Is there a way to pipe stuff to stdin (like Vimscript's system(cmd, string))?

@lewis6991
Copy link
Member Author

Yes, stdin can be a string or string[].

@lewis6991 lewis6991 marked this pull request as ready for review May 31, 2023 11:48
@lewis6991 lewis6991 force-pushed the feat/systemlua branch 3 times, most recently from 49c8942 to 9831ec0 Compare May 31, 2023 13:13
@lewis6991
Copy link
Member Author

Windows test failures are germane (stupid Windows line endings...)

Probably needs something like c65e220/runtime/lua/vim/lsp/util.lua#L458C29-L459

(which seems like something that could be refactored -- e.g., as part of a vim.string library).

I don't think vim.system should be doing anything special with line endings. One example is in Gitsigns \r needs to be preserved since the system output is used to diff against the buffer text so we need to take the output verbatim.

For now, I think we just need to make the tests handle outputs with \n or \r\n. There might be an argument that the default handler strips \r and users must use custom handlers to preserve them.

@clason
Copy link
Member

clason commented Jun 6, 2023

I don't think vim.system should be doing anything special with line endings. One example is in Gitsigns \r needs to be preserved since the system output is used to diff against the buffer text so we need to take the output verbatim.

That is a strong argument (also since it's easier to strip than to restore). Is this something that should/could be mentioned somewhere, or is this too niche or obvious in the context?

@clason
Copy link
Member

clason commented Jun 6, 2023

Is this low-hanging fruit now, or better left for a follow-up PR?

-- Run a system command and timeout after 30 seconds.
local function system(cmd, ...)
local args = { ... }
local args_count = vim.tbl_count(args)
local stdin = (args_count > 0 and args[1] or '')
local stderr = (args_count > 1 and args[2] or false)
local ignore_error = (args_count > 2 and args[3] or false)
local opts = {
add_stderr_to_output = stderr,
output = '',
stderr = '',
on_stdout = system_handler,
on_stderr = system_handler,
on_exit = system_handler,
}
local jobid = vim.fn.jobstart(cmd, opts)
if jobid < 1 then
local message = 'Command error (job='
.. jobid
.. '): `'
.. shellify(cmd)
.. '` (in '
.. vim.fn.string(vim.fn.getcwd())
.. ')'
error(message)
shell_error_code = 1
return opts.output
end
if not is_blank(stdin) then
vim.api.nvim_chan_send(jobid, stdin)
end
local res = vim.fn.jobwait({ jobid }, 30000)
if res[1] == -1 then
error('Command timed out: ' .. shellify(cmd))
vim.fn.jobstop(jobid)
elseif shell_error() and not ignore_error then
local emsg = 'Command error (job='
.. jobid
.. ', exit code '
.. shell_error_code
.. '): `'
.. shellify(cmd)
.. '` (in '
.. vim.fn.string(vim.fn.getcwd())
.. ')'
if not is_blank(opts.output) then
emsg = emsg .. '\noutput: ' .. opts.output
end
if not is_blank(opts.stderr) then
emsg = emsg .. '\nstderr: ' .. opts.stderr
end
error(emsg)
end
-- return opts.output
local _ = ...
return vim.trim(vim.fn.system(cmd))
end

@lewis6991
Copy link
Member Author

I would say it is just hanging fruit.

I did have a go at refactoring it but the code has some niche options that I'm not sure are meaningful to keep. And tbh, the whole file could do with some cleanup so probably better to do it in one go (in a follow up PR).

@clason
Copy link
Member

clason commented Jun 7, 2023

external_deps test is straight-up broken (@dundargoc) across PRs, so after squashing this should be good to go.

@lewis6991 lewis6991 force-pushed the feat/systemlua branch 2 times, most recently from 0194751 to a4dfcca Compare June 7, 2023 09:34
@lewis6991
Copy link
Member Author

lewis6991 commented Jun 7, 2023

For the \r\n problem, python has a text=true argument which I've copied here (default false). Note: before text they had universal_newlines, so they've done the churn for us.

Note vim.fn.system() also has this:

	Result is a String, filtered to avoid platform-specific quirks:
	- <CR><NL> is replaced with <NL>
	- NUL characters are replaced with SOH (0x01)

Lua can happily handle embedded NUL chars so we don't need to do that.

So now the following are roughly equivalent.

local out = vim.fn.system({'echo', 'hello'})
local out = vim.system({'echo', 'hello'}, {text=true}):wait().stdout

vim.system is a bit more verbose than I planned (with :wait(), text=true and .stdout), but it's probably a good middle-ground given its versatility.

@lewis6991 lewis6991 force-pushed the feat/systemlua branch 2 times, most recently from 7509ddd to b920c36 Compare June 7, 2023 11:00
runtime/lua/vim/lsp/rpc.lua Outdated Show resolved Hide resolved
@lewis6991 lewis6991 force-pushed the feat/systemlua branch 2 times, most recently from 7b1a6b8 to f9be445 Compare June 7, 2023 12:23
Problem:

  Handling system commands in Lua is tedious and error-prone:
  - vim.fn.jobstart() is vimscript and comes with all limitations attached to typval.
  - vim.loop.spawn is too low level

Solution:

  Add vim.system().
  Partly inspired by Python's subprocess module
  Does not expose any libuv objects.
@lewis6991 lewis6991 merged commit c0952e6 into neovim:master Jun 7, 2023
11 of 12 checks passed
@lewis6991 lewis6991 deleted the feat/systemlua branch June 7, 2023 12:52
@A-Lamia
Copy link

A-Lamia commented Aug 2, 2023

vim.system does not work on windows 10 using:
:=vim.system({'echo', 'hello'}, {text=true}):wait().stdout
is this just me or can anyone replicate ?

NVIM: v0.10.0-dev-751+gd086bc1e8
image

@lewis6991
Copy link
Member Author

The message seems like it's indicating echo doesn't exist.

@A-Lamia
Copy link

A-Lamia commented Aug 2, 2023

@lewis6991 it does though, tested on CMD and PS, isn't echo a native command lol ?

EDIT: you are right though fd seems to return something but not echo, I'm very confused :)

@lewis6991
Copy link
Member Author

The tests added in this PR cover echo on windows so the issue must be something specific to your environment.

@A-Lamia
Copy link

A-Lamia commented Aug 2, 2023

@lewis6991 didn't work on my second desktop but worked on my laptop, so yeah it's probably something on my side, do these commands all run on default CMD ?

@lewis6991
Copy link
Member Author

No idea, vim.system is just a wrapper around vim.uv.spawn which is provided by libuv.

@mfussenegger
Copy link
Member

uv.spawn doesn't run the process in a shell. You'll have to run something like vim.system({"cmd.exe", "/C", "echo"}):wait() if you need one.

(And please stop using this PR as support forum. You're notifying all reviewers. Use matrix if you've general usage questions. )

@neovim neovim locked as off-topic and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lua stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants