feat: implement kill -l

[39555]

Utilize nix::signal::Signal to print the all signals.

Note that 0 - EXIT is missing for now because it is not posix, and I didn't find any documentation for the EXIT signal, except for the trap builtin https://www.gnu.org/software/bash/manual/html_node/Bourne-Shell-Builtins.html

If a sigspec is 0 or EXIT, arg is executed when the shell exits.

[github-actions]

Performance Benchmark Report

Benchmark name	Baseline (μs)	Test/PR (μs)	Delta (μs)	Delta %
expand_one_string	3.41 μs	3.41 μs	-0.00 μs	⚪ Unchanged
instantiate_shell	60.31 μs	59.74 μs	-0.57 μs	⚪ Unchanged
instantiate_shell_with_init_scripts	31314.42 μs	31246.11 μs	-68.31 μs	⚪ Unchanged
parse_bash_completion	2781.39 μs	2760.68 μs	-20.71 μs	⚪ Unchanged
parse_sample_script	4.25 μs	4.19 μs	-0.05 μs	⚪ Unchanged
run_echo_builtin_command	91.21 μs	90.05 μs	-1.16 μs	⚪ Unchanged
run_one_builtin_command	110.03 μs	107.92 μs	-2.12 μs	⚪ Unchanged
run_one_external_command	1960.99 μs	1975.12 μs	14.13 μs	⚪ Unchanged
run_one_external_command_directly	1016.45 μs	1016.05 μs	-0.40 μs	⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
brush-core/src/builtins/kill.rs	🔴 0%	🟠 64.62%	🟢 64.62%
brush-core/src/builtins/trap.rs	🟠 69.05%	🟠 66.67%	🔴 -2.38%
brush-core/src/completion.rs	🔴 19.93%	🔴 19.96%	🟢 0.03%
brush-core/src/shell.rs	🟢 77.87%	🟢 78.01%	🟢 0.14%
brush-core/src/sys/unix/signal.rs	🟠 68.48%	🟠 65.48%	🔴 -3%
brush-core/src/traps.rs	🟠 50%	🟢 94.59%	🟢 44.59%
Overall Coverage	🟢 73.47%	🟢 73.77%	🟢 0.3%

Minimum allowed coverage is 70%, this run produced 73.77%

[reubeno]

Thanks for taking a look at kill -l, @39555 . I really appreciate you going back through some of these unimplemented facilities and making them solid 😃

Had you noticed the implementation of trap -l (and the other trap functions)? We already have an implementation of that (trap -l), and would be ideal to share common code. What do you think about factoring out some common shell signal parsing/enumeration logic that can be reused across the two?

(One note -- right now the kill builtin is cfg(unix) whereas the trap one isn't.)

[39555]

Cool thanks! I will look at that

[39555]

@reubeno I have questions

1. I've noticed that in bash kill -lcan print selected signals e.g., kill -l HUP 2 TERM would print:

1
INT
15

while trap -l always prints all the signals. Should the trap -l also be able to print selected signals?

2. Maybe we should also print EXIT, DEBUG and ERR? Bash is highly inconsistent in this regard. For example, when you print all with kill -l, Bash doesn't show these signals, but if you type kill -l 0, it prints EXIT. Furthermore, when you type kill -l 33 it shows nothing but when you type kill -l ERR it shows 33

3. trap is case insensitive (e.g trap int works), while kill -l is not. Should kill also be case insensitive?

4. when kill -l bash prints full names (e.g SIGINT) but when kill -l 2, it prints INT. Should it be always SIGINT or should I mimic the bash behavior?

[reubeno]

1. I've noticed that in bash kill -lcan print selected signals e.g., kill -l HUP 2 TERM would print
   [snipped]

while trap -l always prints all the signals. Should the trap -l also be able to print selected signals?

Yes, I think it would be best for trap -l to behave like bash's implementation does. In theory, I suppose that some script could use it to decode signal numbers / names. Seems unlikely, but couldn't hurt to be consistent there.

2. Maybe we should also print EXIT, DEBUG and ERR? Bash is highly inconsistent in this regard. For example, when you print all with kill -l, Bash doesn't show these signals, but if you type kill -l 0, it prints EXIT. Furthermore, when you type kill -l 33 it shows nothing but when you type kill -l ERR it shows 33

I see the inconsistencies as well; I'm assuming some of it maybe intentional, and other aspects may come down to the way the shared code happened to be structured.

For kill, EXIT, DEBUG, and ERR don't really make sense. They aren't really signals that can be sent to a job or process. They're virtual signals that can be trapped via trap. I'd vote for us avoiding duplicating much logic between trap and kill, and where possible without too much effort, hiding these 3 virtual signals from kill.

3. trap is case insensitive (e.g trap int works), while kill -l is not. Should kill also be case insensitive?

I see trap call out its case insensitivity; did you see kill -l being case sensitive? From my quick tests on a few versions of bash, I saw it behave in a similar case insensitive fashion:

bash$ kill -l int
2
bash$ kill -l iNt
2
bash$ kill -l INT
2
bash$ kill -l SiGiNT
2
bash$ kill -l SIGINT
2
bash$ kill -l sigint
2

4. when kill -l bash prints full names (e.g SIGINT) but when kill -l 2, it prints INT. Should it be always SIGINT or should I mimic the bash behavior?

I think it makes sense to mimic the bash behavior for kill -l <signal>, just on the extreme off-chance that some caller would notice the difference. For kill -l (listing all of them), I don't think I have a strong preference one way or the other.

[39555]

1. Interesting, the value of ERR and DEBUG is based on NSIG macro and there is no way to determine them on bsdlike systems except that it is defines as 32. On macos ERR = 33, DEBUG = 32, on linux there is libc::SIGRTMAX and ERR = ibc::SIGRTMAX + 1, Debug = libc::SIGRTMAX.

2. Also I found a line in the bash change log for the version 2.01.1. Fixed that in brush.

s. A fix was made so that you can no longer trap SIGEXIT' or SIGDEBUG' --
only EXIT' and DEBUG' are accepted.

[39555]

@reubeno Should I try to use libc::SIGRTMAX and libc::SIGN where possible and fallback to the 32 or leave DEBUG, ERR without numbers (than it would be kill -l ERR -> ERR: invalid signal specification)?

[39555]

tokio defines that range as https://github.com/jorendorff/tokio/blob/d51f16855bce90c3c73ae199c7d6bdb340297e99/tokio/src/signal/unix.rs#L31

        // There are reliable signals ranging from 1 to 33 available on every Unix platform.
        #[cfg(not(target_os = "linux"))]
        let possible = 0..=33;

        // On Linux, there are additional real-time signals available.
        #[cfg(target_os = "linux")]
        let possible = 0..=libc::SIGRTMAX();

[39555]

wzsh has an interesting implementation https://github.com/wez/wzsh/blob/9573218001c4109ed162ad664c87ba033c7f4e2c/src/exitstatus.rs#L54, it just defines NSIG as an arbitrary large number1024, thereforeERR would be 1025

[reubeno]

What would your recommendation be? I think the requirements here are that the numbers for these virtual signals don't conflict with any of the real signals. They're not expected to be portable across systems, to my knowledge.

[39555]

What would your recommendation be? I think the requirements here are that the numbers for these virtual signals don't conflict with any of the real signals. They're not expected to be portable across systems, to my knowledge.

Fоr now, I made it without numbers. It is pretty accurate, kill just prints a name instead:

kill -l INT
2
kill -l DEBUG
DEBUG

in kill -l and trap -l ERR and DEBUG are not listed, similar to Bash.

[reubeno]

Sounds good to me. I'll try to review this in the next day or so. (I'd gotten a bit busy looking at an incoming issue report that seemed more severe (syntax parse failures).)

[reubeno]

Changes generally look good to me; just a few comments related to testing. Thanks for putting in the work to get trap and kill to agree on things. I think that will pay off over time.

[reubeno]

Was kill -l 8 intentionally omitted? For that matter, what about going with a for loop for better maintainability/readability?

[reubeno]

Any other ideas on how to get some test coverage for kill -l?

I'm not sure how much we want to assume about the format of bash's output, but something like this would extract a "flat" list from its implementation of kill -l:

siglist=($(kill -l | sed -e "s/[[:digit:]]\+)//g"))
for s in ${siglist[@]}; do echo $s; done

[39555]

@reubeno should we also hide EXIT from the printed list? The test is currently failing because of EXIT

[reubeno]

I don't have a strong preference on that. If we want to allow it to be different, we can always intentionally have the test grep -v / exclude it so it doesn't show up in a delta.

[39555]

There should be additional signals for linux that are not listed in nix::Signal...

[39555]

There is ongoing pr for realtime signals in nix nix-rust/nix#2451

[reubeno]

There is ongoing pr for realtime signals in nix nix-rust/nix#2451

Is that something we could integrate with later (without preventing us from moving forward with this set of changes now)?

[39555]

Yeah. For now just realtime signals are missing (the linux failed test shows them)

[reubeno]

Yeah. For now just realtime signals are missing (the linux failed test shows them)

Sounds like something we could track with a TODO follow-up and exclude from the diff (via grep) for now?

[reubeno]

Your latest changes are looking good to me. Please let us know when you're ready for final review before merging?

[39555]

@reubeno I'm ready