Yeah, it looks like we only use ptsname_r() from the main loop, so we should be re-entrant safe. Mind cooking up a patch for me?

Created attachment 339078 [details] [review]
terminal: fallback to ptsname() if ptsname_r() is not available

We are technically safe from race-conditions here in Builder specific
code today, as we only call ptsname_r() from the main loop. So switching
to the MT-unsafe ptsname() is fine. However, I'd like to always use the
re-entrant safe POSIX functions when possible.

This falls back to ptsname() when ptsname_r() isn't available, such as on
BSD variants.

Attachment 339078 [details] pushed as 6e03588 - terminal: fallback to ptsname() if ptsname_r() is not available

Hi Christian.

I totally missed your second message and actually came up with a similar patch as well...
Anyway, I did not answer before because while the terminal works, it outputs this:

/bin/ksh: No controlling tty (open /dev/tty: Device not configured)
/bin/ksh: warning: won't have full job control

Does posix_openpt() work on BSD? I see in Vte's configure.ac it checks for both posix_openpt() and BSD's openpty(). However, I don't see openpty() being used during pty master creation. We rely on Vte to create the pty master for us, so we only do the grantpt(), unlockpt(), ptsname() dance.

I don't have a BSD machine available, so I'll need you to do some digging in gb-terminal-view.c:gb_terminal_respawn() to see where the error is in the flow. Although if we actually spawn the process, that means we didn't detect a failure in our setup pty flow. So perhaps we are just missing something else that is needed on various BSDs.

Created attachment 339137 [details] [review]
setup TIOCSCTTY ioctl on tty fd

Can you check and see if this fixes job control? If so, we will add plumbing to IdeSubprocessLauncher to setup TTY plumbing (as we need to do it differently with Flatpak vs pain subprocesses).

(In reply to Christian Hergert from comment #5)
> Does posix_openpt() work on BSD? I see in Vte's configure.ac it checks for

Yes it does work.
AFAIK Vte3 on BSD uses posix_openpt/grantpt/... and not openpty by default.
All are supported.

> both posix_openpt() and BSD's openpty(). However, I don't see openpty()
> being used during pty master creation. We rely on Vte to create the pty
> master for us, so we only do the grantpt(), unlockpt(), ptsname() dance.
> 
> I don't have a BSD machine available, so I'll need you to do some digging in
> gb-terminal-view.c:gb_terminal_respawn() to see where the error is in the
> flow. Although if we actually spawn the process, that means we didn't detect
> a failure in our setup pty flow. So perhaps we are just missing something
> else that is needed on various BSDs.

Ack, I'll have a look.
Thanks Christian.

(In reply to Christian Hergert from comment #6)
> Created attachment 339137 [details] [review] [review]
> setup TIOCSCTTY ioctl on tty fd
> 
> Can you check and see if this fixes job control? If so, we will add plumbing
> to IdeSubprocessLauncher to setup TTY plumbing (as we need to do it
> differently with Flatpak vs pain subprocesses).

Unfortunately, adding the TIOCSCTTY ioctl did not help...

Review of attachment 339137 [details] [review]:

::: plugins/terminal/gb-terminal-view.c
@@ +272,3 @@
     IDE_GOTO (failure);
 
+  ioctl (tty_fd, TIOCSCTTY, tty_fd);

two things:

1) this ioctl is in the wrong place. presumably you're calling setsid() somewhere if there's no controlling terminal.  It's got to be after the setsid() call

2) the third argument is a boolean (something like steal_ctty_from_someone_else). it should be FALSE, since TRUE only works for root.

We do call setsid() so that we can more reliably kill all of the grandchildren if we force_exit() the parent. (Ideally we'd have some cgroup for this to be more reliable in the future, but this is the best we can do for now).

I guess we can do something similar to what flatpak is doing in the session helper. If isatty(stdin), then call the ioctl() after we have setsid().

Created attachment 339675 [details] [review]
subprocess-launcher: setup IOCSCTTY on stdin_fd after setsid()

This ensures that after our forked process becomes the session leader we
setup the TTY with an ioctl() of TIOCSCTTY ensuring the terminal the
controlling terminal.

This only affects processes that are spawned as children of our process,
as the Flatpak process helper deals with this in the session helper.

Antoine, mind giving master a test (which has commit 7b68bdcb3e2925dff94c1fea555557e2e5470662)?

Hi Christian. Thanks for following on this, unfortunately that did not fix the issue. I'm still getting a non controlling tty.

It looks like we weren't calling setpgid() after our setsid(), which could potentially be an issue on BSD. I think it was fine on Linux due to how tasks are implemented.

If this doesn't work, is there a *BSD VM somewhere with GNOME included that I could use build/test?

I already tried that as well but that did not help either :-/
I must say I am a bit puzzled...
I could set you up with a VM but the problem is that GNOME will not work on it since there's no accelerated display on !bare-metal.

can you see if the TIOCSCTTY ioctl 

1) is running
2) if yes, is failing?
3) if yes, what the error code is?

Review of attachment 339675 [details] [review]:

::: libide/subprocess/ide-subprocess-launcher.c
@@ +80,3 @@
+
+  if (isatty (priv->stdin_fd))
+    ioctl (priv->stdin_fd, TIOCSCTTY, 0);

wait if this is in the child, shouldn't that just STDIN_FILENO ?

Ah yes, because GSpawnChildSetupFunc should not be called until right before exec(), which means that we've already remapped stdin_fd to STDIN_FILENO.

Created attachment 339835 [details] [review]
subprocess: use proper fileno for ioctl()

We were trying to setup the TTY on stdin_fd which is no longer valid since
our child setup func is called after all the fd remapping has occurred
(and right before exec()).

seems plausible to me, Antoine, does this one work?

\o/ Yes we have a winner :-)
Thanks a lot guys.

great!