There are two problems with patch 3/5:
1. The intended way to kill a process and all its descendents recursively
is through process group. Zig's high-level abstraction for spawning
child processes currently does not support process group.
2. Semantically correct signals for termination does not work on the
spawned process and its descendents. The only ones I found working
are SIGUSR{1,2} which is a hack relying on their default action.
Due to the first issue, the patch series when applied is supposed
to be built with Zig's standard library as patched here:
https://github.com/ziglang/zig/pull/20556
I will keep that PR compatible with the latest patch series sent here.
To build Mepo with the patched std lib, first acquire Zig 0.13.0's lib
(your distro would install it in $PREFIX/lib/zig, or clone/download Zig
from upstream and look into lib), then apply the following:
https://github.com/ziglang/zig/pull/20556.diff
Then build Mepo with the additional flag --zig-lib-dig:
zig build --zig-lib-dir /path/to/zig-lib-dir
Nguyễn Gia Phong (5):
Move get_env_vars to api/shellpipe_async.zig
Derive shellpipe_sync from *_async
Create button and keybind to cancel sync shellpipe
Switch to alpha blend mode
Gray out UI during sync shellpipe
src/Mepo.zig | 54 +++++++++++------
src/api/shellpipe_async.zig | 55 ++++++++++++++----
src/api/shellpipe_sync.zig | 113 ++++++++++++++++++------------------
src/blit/blit.zig | 43 +++++++++-----
src/types.zig | 1 +
src/util/utilsdl.zig | 2 +-
6 files changed, 167 insertions(+), 101 deletions(-)
--
2.45.1
[RFC PATCH mepo 1/5] Move get_env_vars to api/shellpipe_async.zig
Hey Phong -
Thanks for your work on implementation & sending, comments on the two
points below:
> 1. The intended way to kill a process and all its descendents recursively> is through process group. Zig's high-level abstraction for spawning> child processes currently does not support process group.
I see that the pgid PR was finally merged in the last few days which
is great news. Thank you for opening the PR on Zig & figuring out the
correct way this should be done in Zig's stdlib.
I've rebased your changes & added a few minor fixups on the branch
`cnx-shellpipe-plus-rebase`. On that branch I also did a bit of work to
extract the shell exec implementation to `utilprocess.run_subprocess`.
I think the way forward (atleast for now) is likely just to sub
out the implementation for utilprocess.run_subprocess to rather use
C's fork/setpgid/exec since mepo builds against libc in anycase. By
extracting to a separate helper function we can keep this integration
(zig vs c way) relatively isolated. By using C subprocess spawning we
should be able to get pgid spawning without waiting for zig to roll a
minor or patch release; and can switch back to the zig stdlib way of
doing things when a new zig release is versioned.
> 2. Semantically correct signals for termination does not work on the> spawned process and its descendents. The only ones I found working> are SIGUSR{1,2} which is a hack relying on their default action.
In my testing SIGKILL on the spawned process works fine & consistently
so I just switched over to that.
Really great work, thank you for taking this on. This will be a huge UX
improvement once merged.
Miles
On 2024-07-23 at 18:33-04:00, Miles Alan wrote:
> > 2. Semantically correct signals for termination does not work on the> > spawned process and its descendents. The only ones I found working> > are SIGUSR{1,2} which is a hack relying on their default action.>> In my testing SIGKILL on the spawned process works fine & consistently> so I just switched over to that.
I don't think SIGKILL is appropriate for job cancelation as it prevents
the terminated process from freeing resources, e.g. temporary file
in mepo_ui_menu_route_mobroute.sh (which BTW is not cleaned up).
I tried to asked about the swallowed SIGTERM on the Zig Matrix room
(basically the only system programming chat room I participate in)
and did not get any answer. I will try to come up with a min reprod
to see if it's Zig-specific, and either way ask somewhere that's
_not instant_ messaging. Not understanding why it behaves so oddly
really bugs me.
On 2024-07-23 at 18:33-04:00, Miles Alan wrote:
> I've rebased your changes & added a few minor fixups on the branch> `cnx-shellpipe-plus-rebase`. On that branch I also did a bit of work to> extract the shell exec implementation to `utilprocess.run_subprocess`.>> I think the way forward (atleast for now) is likely just to sub> out the implementation for utilprocess.run_subprocess to rather use> C's fork/setpgid/exec since mepo builds against libc in anycase. By> extracting to a separate helper function we can keep this integration> (zig vs c way) relatively isolated. By using C subprocess spawning we> should be able to get pgid spawning without waiting for zig to roll a> minor or patch release; and can switch back to the zig stdlib way of> doing things when a new zig release is versioned.
Thanks, and that sounds like a good plan to me. If you have yet
to work on the libc-based impl I would like to take a stab at it.
I took a quick look at that branch and wanna do some nitpicking as well!
On 2024-07-21 at 10:33-04:00, Miles Alan wrote:
>Remove continual stdout functionality from async_shellpipe>---> run_shellpipe(mepo, unique_handle_id, ...> _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);
[snip]
> pub fn run_shellpipe(mepo: *Mepo, async_id: ?i8, cmd: []const u8) !void {
[snip]
> if (async_id) |unique_handle_id| {
[snip]
>+ defer _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);
It seems the thread ID is removed twice here by accident
(BTW unlike Go's, defer in Zig is scoped at the block level).
I also noticed that QueueHashMap.swapRemove is always used
to just remove the item, i.e. the return value is never used
in the current codebase.
There are also some trailing whitespaces
at the function signature of utilprocess.run_subprocess.
On Fri, Jul 26, 2024, at 12:37 AM, Nguyễn Gia Phong wrote:
> On 2024-07-23 at 18:33-04:00, Miles Alan wrote:>> > 2. Semantically correct signals for termination does not work on the>> > spawned process and its descendents. The only ones I found working>> > are SIGUSR{1,2} which is a hack relying on their default action.>>>> In my testing SIGKILL on the spawned process works fine & consistently>> so I just switched over to that.>> I don't think SIGKILL is appropriate for job cancelation as it prevents> the terminated process from freeing resources, e.g. temporary file> in mepo_ui_menu_route_mobroute.sh (which BTW is not cleaned up).>> I tried to asked about the swallowed SIGTERM on the Zig Matrix room> (basically the only system programming chat room I participate in)> and did not get any answer. I will try to come up with a min reprod> to see if it's Zig-specific, and either way ask somewhere that's> _not instant_ messaging. Not understanding why it behaves so oddly> really bugs me.
I do agree SIGKILL is less ideal as it won't allow for resource
cleanup. And yeah.. I still have a bit of work to do on the
mepo_ui_menu_route_mobroute.sh script mentioned wrt. cleanup.
I still think SIGKILL is preferable to the old SIGUSR1/2, but if we
can figure a way to make SIGTERM (which is what it should actually be)
work reliably I'm all for it. I am also quite curious if this is a zig issue
or if a libc would have the same behavior as well.
> On 2024-07-23 at 18:33-04:00, Miles Alan wrote:>> I've rebased your changes & added a few minor fixups on the branch>> `cnx-shellpipe-plus-rebase`. On that branch I also did a bit of work to>> extract the shell exec implementation to `utilprocess.run_subprocess`.>>>> I think the way forward (atleast for now) is likely just to sub>> out the implementation for utilprocess.run_subprocess to rather use>> C's fork/setpgid/exec since mepo builds against libc in anycase. By>> extracting to a separate helper function we can keep this integration>> (zig vs c way) relatively isolated. By using C subprocess spawning we>> should be able to get pgid spawning without waiting for zig to roll a>> minor or patch release; and can switch back to the zig stdlib way of>> doing things when a new zig release is versioned.>> Thanks, and that sounds like a good plan to me. If you have yet> to work on the libc-based impl I would like to take a stab at it.>
Absolutely, feel free to take a stab at the libc based subprocess spawning
implementation. I hadn't started on it as I'm working on a few other
things at the moment.
> I took a quick look at that branch and wanna do some nitpicking as well!>> On 2024-07-21 at 10:33-04:00, Miles Alan wrote:>>Remove continual stdout functionality from async_shellpipe>>--->> run_shellpipe(mepo, unique_handle_id, ...>> _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);> [snip]>> pub fn run_shellpipe(mepo: *Mepo, async_id: ?i8, cmd: []const u8) !void {> [snip]>> if (async_id) |unique_handle_id| {> [snip]>>+ defer _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);>> It seems the thread ID is removed twice here by accident> (BTW unlike Go's, defer in Zig is scoped at the block level).>> I also noticed that QueueHashMap.swapRemove is always used> to just remove the item, i.e. the return value is never used> in the current codebase.>> There are also some trailing whitespaces> at the function signature of utilprocess.run_subprocess.
Thanks for the catch on the extra defer - should be fixed now.
As for swapRemove: if returns false, that means it was just a nop
(key didn't exist). And IIRC in the majority of these cases; nop'ing
resultingly from this as well is the correct behavior (so yea it seems
the return value is never used). I suppose we could remove the return
value altogether, though it's just a proxy to AutoArrayHashMap. If
there are places you find where it shouldn't be a nop from key missing /
behavior is incorrect do let me know.
And thanks for the reminder on whitespace - I need to get back into the
habit of running zig fmt; my editor doesn't show trailing whitespace
currently.