Closed 39555 closed 3 days ago
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 |
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%
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.)
Cool thanks! I will look at that
@reubeno I have questions
I've noticed that in bash kill -l
can 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?
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
trap
is case insensitive (e.g trap int
works), while kill -l
is not. Should kill
also be case insensitive?
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've noticed that in bash
kill -l
can print selected signals e.g.,kill -l HUP 2 TERM
would print [snipped]while
trap -l
always prints all the signals. Should thetrap -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.
- Maybe we should also print
EXIT
,DEBUG
andERR
? Bash is highly inconsistent in this regard. For example, when you print all withkill -l
, Bash doesn't show these signals, but if you typekill -l 0
, it printsEXIT
. Furthermore, when you typekill -l 33
it shows nothing but when you typekill -l ERR
it shows33
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
.
trap
is case insensitive (e.gtrap int
works), whilekill -l
is not. Shouldkill
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
- when
kill -l
bash prints full names (e.gSIGINT
) but whenkill -l 2
, it printsINT
. Should it be alwaysSIGINT
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.
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.
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' -- onlyEXIT' and
DEBUG' are accepted.
@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
)?
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();
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
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.
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.
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).)
There should be additional signals for linux that are not listed in nix::Signal...
There is ongoing pr for realtime signals in nix https://github.com/nix-rust/nix/pull/2451
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)?
Yeah. For now just realtime signals are missing (the linux failed test shows them)
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?
Your latest changes are looking good to me. Please let us know when you're ready for final review before merging?
@reubeno I'm ready
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 theEXIT
signal, except for thetrap
builtin https://www.gnu.org/software/bash/manual/html_node/Bourne-Shell-Builtins.html