xmake-io / xmake

🔥 A cross-platform build utility based on Lua
https://xmake.io
Apache License 2.0
9.89k stars 775 forks source link

os.exec misbehaves with programs trapping SIGINT #4889

Closed dkaszews closed 6 months ago

dkaszews commented 6 months ago

Xmake Version

xmake v2.8.9+20240321

Operating System Version and Architecture

Ubuntu 22.04 aarch64

Describe Bug

I am using xmake to compile RISC-V code and have created a phony target to run it with spike simulator. To exit spike, you need to first enter interactive mode with Ctrl+C, then run quit. Under xmake, the Ctrl+C just kills xmake, so spike gets detached from but kept alive, making it spam I/O errors and impossible to exit without kill as anything you type goes into shell.

To reproduce, use the test script below:

$ ./trap.sh
Still alive
^CType "quit" to exit
quit

$ xmake run trap
Still alive
^CType "quit" to exit
$
trap.sh: line 5: read: read error: 0: Input/output error
Still alive
trap.sh: line 5: read: read error: 0: Input/output error
Still alive
trap.sh: line 5: read: read error: 0: Input/output error
Still alive
quit
Command 'quit' not found
trap.sh: line 5: read: read error: 0: Input/output error
Still alive
trap.sh: line 5: read: read error: 0: Input/output error
Still alive
...

Expected Behavior

Xmake should trap SIGINT for the duration of os.run and wait for the spawned process to exit on its own, or send it SIGTERM/SIGKILL on repeated offences (configurable?). Other programs which trap SIGKILL include:

Project Configuration

xmake.lua

target("trap")
    set_kind("phony")
    -- TODO: Ctrl-C does not kill inner process
    on_run(function (target)
        os.exec("$(projectdir)/trap.sh")
    end)

trap.sh

#!/usr/bin/env bash
function handler { echo 'Type "quit" to exit'; }
trap handler SIGINT
while true; do
    read -t 3 -p $'Still alive\n' x || continue
    echo $x | grep -q 'quit' && break
done

Additional Information and Error Logs

Traping signals in Lua can be seen here: https://stackoverflow.com/a/34409274/2894535

waruqi commented 6 months ago

Normally, all child processes and xmake belong to the same process group, and they all receive the SIGINT signal, even if xmake doesn't catch it, and all child processes and xmake are exited at the same time.

Therefore, by default, xmake should not trap for SIGINT and then force all child processes to exit, but should let the child processes handle the SIGINT signal themselves.

Then the sub-process you're executing now, which is a special case, caught SIGINT on its own and didn't execute immediately to exit. It needs the user to perform some interaction to continue to exit. I can provide lua's SIGINT signal registration interface, and user can handle how to exit the child process according to their needs.

Also, it might be better if the child process could go to exit by executing other interactive commands instead of SIGINT. like gdb, lldb. we can use exclusive argument to ignore SIGINT.

os.execv("lldb", {}, {exclusive = true}) 
ruki-2:test ruki$ xmake r -d
(lldb) target create "/private/tmp/test/build/macosx/x86_64/release/test"
Current executable set to '/private/tmp/test/build/macosx/x86_64/release/test
' (x86_64).
(lldb) ^C
(lldb) ^C
(lldb) ^C
(lldb) quit
waruqi commented 6 months ago

or you can try this patch, I added os.signal api, user can register signal handler to handle SIGINT. https://github.com/xmake-io/xmake/pull/4890

xmake update -s dev

test.lua

import("core.base.process")

function main()
    local proc
    os.signal(os.SIGINT, function (signo)
        print("sigint")
        if proc then
            proc:kill()
        end
    end)
    proc = process.open("./trap.sh")
    if proc then
        proc:wait()
        proc:close()
    end
end
ruki$ xmake l test.lua
Still alive
Still alive
^CType "quit" to exit
sigint
dkaszews commented 6 months ago

@waruqi Looks good, will have to test it whether it suits my usage. Agreed it would be better if program offered a way to interact with it other than SIGINT, but sometimes you just have to work with what you have.

One more suggestion: it could be useful to provide ability to reset a signal handler to default by calling os.signal(os.SIGINT, nil). On Unix it is done with signal(SIGINT, SIG_DFL);, on Windows with SetConsoleCtrlHandler(NULL, FALSE);.

dkaszews commented 6 months ago

I tested it, adding os.signal(os.SIGINT, function() end) works great - xmake is not killed, the signal is forwarded to script (unlike with { exclusive = true }). I can create a pull request implementing signal resetting if you want.

waruqi commented 6 months ago

I tested it, adding os.signal(os.SIGINT, function() end) works great - xmake is not killed, the signal is forwarded to script (unlike with { exclusive = true }). I can create a pull request implementing signal resetting if you want.

you can try.

os.signal(os.SIGINT, handler)
os.signal(os.SIGINT, os.SIGDFL) -- clear
os.signal(os.SIGINT, os.SIGIGN) -- ignore
dkaszews commented 6 months ago

I implemented it in #4895 , verified with modified xmake.lua:

target("trap")
    set_kind("phony")
    on_run(function (target)
        print('SIGINT trapped and forwarded, will print "trap" and "Type quit to exit"')
        os.signal(os.SIGINT, function() print('trap'); end)
        os.execv("$(projectdir)/trap.sh", {})

        print('SIGINT ignored, will do nothing')
        os.signal(os.SIGINT, os.SIGIGN)
        os.execv("$(projectdir)/trap.sh", {})

        print('SIGINT default, will kill xmake and break running script')
        os.signal(os.SIGINT, os.SIGDFL)
        os.execv("$(projectdir)/trap.sh", {})
    end)
waruqi commented 6 months ago

We can use signal module now. https://github.com/xmake-io/xmake/pull/4908

import("core.base.process")
import("core.base.signal")

function main()
    local proc
    signal.register(signal.SIGINT, function (signo)
        print("sigint")
        if proc then
            proc:kill()
        end
    end)
    proc = process.open("./trap.sh")
    if proc then
        proc:wait()
        proc:close()
    end
end
signal.register
signal.ignore
signal.reset
dkaszews commented 6 months ago

@waruqi I just got back to trying it out, cannot get it to work inside xmake.lua. I modified the previous test script, but am getting error: attempt to index a nil value (global 'signal'). If I try to add import("core.base.signal") as per your example, then I get an error: ./xmake.lua:2: global 'import' is not callable (a nil value), ditto for require.

target("trap")
    set_kind("phony")
    on_run(function (target)
        print('SIGINT trapped and forwarded')
        signal.register(signal.SIGINT, function() print('trap'); end)
        os.execv("$(projectdir)/trap.sh", {})

        print('SIGINT ignored')
        signal.ignore(signal.SIGINT)
        os.execv("$(projectdir)/trap.sh", {})

        print('SIGINT default')
        signal.reset(signal.SIGINT)
        os.execv("$(projectdir)/trap.sh", {})
    end)
waruqi commented 6 months ago

you should call import in on_run