Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add library with function to spawn a process connected to a pseudo-terminal #742

Merged
merged 8 commits into from
Mar 25, 2024

Conversation

Rinzwind
Copy link
Contributor

This pull request adds a small library with a function to spawn a process connected to a pseudo-terminal. As discussed in pavel-krivanek/terminal issue #2 and PTerm issue #58, it’s not entirely possible to replicate this function in-image using posix_spawn. The function calls setsid and ioctl with TIOCSCTTY in the spawned process. For the setsid call, the attribute flag POSIX_SPAWN_SETSID can be passed to posix_spawn (nonstandard but supported by the GNU C Library and macOS). But for the ioctl call there is, as far as I know, no corresponding attribute or file action that can be passed.

The new File Browser that is being adopted (see: Pharo pull request #16084) offers a context menu item ‘Open terminal here’. That currently does not allow using PTerm (PTerm issue #62), but it would be nice if it did and if the VM made that convenient to use by providing the necessary supporting library.

@guillep
Copy link
Member

guillep commented Feb 12, 2024

Hi @Rinzwind, @pavel-krivanek nice!

The build had a hiccup, I relaunched it.
I see no objections to this.

It would be nice however to see some comments in the code.
Maybe copy-pasting part of the PR description is enough?

@Rinzwind
Copy link
Contributor Author

I added a comment, fixed the compilation error on Linux (“error: implicit declaration of function 'ptsname'”), and improved the error checks in ‘tty_spawn’.

I’m not sure whether the library should be given a more distinct name. There’s an existing ‘libtty’.

…’ to use ‘_exit’ instead of ‘exit’, and ‘dprintf’ instead of ‘printf’, to avoid flushing the open streams copied, buffers included, from the parent process.
@Rinzwind
Copy link
Contributor Author

Can this be merged? I had to make one more change (commit 6fa0c9e) but I think it’s ready now. I wrote some tests for which I will open a Pharo pull request once ‘libtty’ is available in the VM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants