fix: work in progress macos support #8

mightyiam · 2023-11-12T15:14:04Z

Co-authored-by: alex source@proof.construction
Co-authored-by: echolinq echolinq@protonmail.com
Co-authored-by: warren2k 846021+warren2k@users.noreply.github.com

mightyiam · 2023-11-12T15:15:22Z

@doy thank you for this crate. We're trying to use it and we need macos support. So we've been trying to figure that out. I will comment on the individual changes. Could you please allow it to run?

Co-authored-by: alex <source@proof.construction> Co-authored-by: echolinq <echolinq@protonmail.com> Co-authored-by: warren2k <846021+warren2k@users.noreply.github.com>

mightyiam

@warren2k @proofconstruction @adrianEffe

mightyiam · 2023-11-12T15:16:32Z

src/pty.rs

@@ -15,7 +15,6 @@ impl Pty {
    /// unable to put it into non-blocking mode.
    pub fn new() -> crate::Result<Self> {
        let pty = crate::sys::Pty::open()?;
-        pty.set_nonblocking()?;


Apparently, macos won't let ya do this prior to obtaining a pts.

mightyiam · 2023-11-12T15:17:13Z

src/pty.rs

-        Ok(Pts(self.0.get_ref().pts()?))
+        let pts = Pts(self.0.get_ref().pts()?);
+        self.0.get_ref().set_nonblocking()?;
+        Ok(pts)


So we do it here. And we're not certain whether this is idempotent enough to not be an issue with multiple calls.

i think this should be fine to call multiple times

mightyiam · 2023-11-12T15:18:09Z

tests/basic.rs

+    let pts = pty.pts().unwrap();
    pty.resize(pty_process::Size::new(24, 80)).unwrap();
    let mut child = pty_process::blocking::Command::new("cat")
-        .spawn(&pty.pts().unwrap())
+        .spawn(&pts)
        .unwrap();


Similar to setting nonblocking, other operations also seem to be disallowed prior to obtaining a pts.

mightyiam · 2023-11-12T15:19:51Z

tests/basic.rs

-    pty.write_all(&[4u8]).unwrap();
-    let status = child.wait().unwrap();
-    assert_eq!(status.code().unwrap(), 0);
+    child.kill().unwrap();


On macos, this ^D doesn't seem to kill the cat. 🤷 ??

If we care, we better figure out why. If we don't, we may as well allow the child to be dropped implicitly, I suppose.

i'd rather figure out what's going on here - ^D does work to terminate a cat process when run normally on macos, so this crate (or test) is doing something wrong here, and i don't want to just sidestep that.

i think this one is actually related to your change above - you're keeping the pts alive longer than before, which apparently prevents the wait from succeeding. if you instead do something like

let mut child = { let pts = pty.pts().unwrap(); pty.resize(pty_process::Size::new(24, 80)).unwrap(); pty_process::blocking::Command::new("cat") .spawn(&pts) .unwrap() };

^D should work again.

Hrm... perhaps. Assuming this is true, wouldn't a user be prone to making a similar mistake such as we made in this test?

When spawning a new process, the Pts is borrowed. Is this because it's meant for reuse? If not, can it instead by passed by value so that spawn() can drop it?

Pts is meant for reuse (probably in UNIX in general) as demonstrated in

pty-process/tests/behavior.rs

Lines 3 to 29 in 74c223d

#[test]

fn test_multiple() {

let pty = pty_process::blocking::Pty::new().unwrap();

let pts = pty.pts().unwrap();

pty.resize(pty_process::Size::new(24, 80)).unwrap();

let mut child = pty_process::blocking::Command::new("echo")

.arg("foo")

.spawn(&pts)

.unwrap();

let mut output = helpers::output(&pty);

assert_eq!(output.next().unwrap(), "foo\r\n");

let status = child.wait().unwrap();

assert_eq!(status.code().unwrap(), 0);

let mut child = pty_process::blocking::Command::new("echo")

.arg("bar")

.spawn(&pts)

.unwrap();

assert_eq!(output.next().unwrap(), "bar\r\n");

let status = child.wait().unwrap();

assert_eq!(status.code().unwrap(), 0);

}

So, alternatively, what does dropping the Pts do... or what does not dropping it do to prevent the cat child process from exiting on a signal?

It seems that blocking::Pts doesn't have a Drop impl and neither does sys::Pts. It would be std::os::fd::OwnedFd that needs to be dropped in order for cat to terminate on a signal. On macos, at least.

Which kind of makes me think that it may be that cat is terminating and something else does not. But that's just a hunch. We'd have to take a closer look. We can start with the wait method.

This library's spawn method returns a plain std::process::Child. So it is std::process::Child::wait that is blocking forever even though a signal was sent to cat.

One option: cat is not receiving the signal.
Another option: cat is receiving the signal and terminating—what else would it do?—but for another reason the wait method keeps blocking. Something that has to do with that non-dropped Pts.

mightyiam · 2023-11-12T15:22:55Z

tests/basic.rs

-    child.kill().await.unwrap()
+    #[cfg(not(target_os = "macos"))]
+    child.kill().await.unwrap();


On macos this seems to hang. Why?

similarly, this isn't doing anything obviously wrong, so i'd like to understand what is broken here before just disabling it.

mightyiam · 2023-11-12T15:24:02Z

tests/basic.rs

-    pty_w.write_all(&[4u8]).await.unwrap();
-    let status = child.wait().await.unwrap();
-    assert_eq!(status.code().unwrap(), 0);
+    child.kill().await.unwrap()


Similar to test_cat_blocking.

samuela · 2023-11-25T02:59:48Z

FWIW I tested this example resize usage on the changes from this PR, but found that it doesn't seem to make a difference. Not sure if you view that as in scope for this particular PR or not.

mightyiam · 2023-11-25T13:13:03Z

@samuela I'm not sure what the restrictions on macos are. From what we've seen so far, resizing before creating a client is not tolerated. Which makes the abstraction provided by this crate less appealing on macos.

samuela · 2023-11-25T17:30:42Z

Thanks @mightyiam! Only calling resize after spawning a child seems to fix the problem for me

samuela · 2023-11-28T07:13:06Z

Ok I can now confirm that as long as I wait until after spawning a process, everything seems to be fine with this PR. Without this PR, it doesn't work regardless of order. So it's definitely a step forward IMHO.

mightyiam · 2023-12-02T00:32:38Z

@samuela I'd love the crate author's feedback, as well, but haven't been able to get it yet.

doy · 2023-12-03T07:16:09Z

thanks for looking into this! these tests have been failing on macos for a while, and i don't have a mac so i'm glad you're looking into it.

doy · 2023-12-03T22:29:10Z

1895e6c this commit appears to get the test suite working on the macos github action workers - can you try this out and see if it works for your case?

mightyiam · 2023-12-09T15:19:17Z

We just had a session where we made some experiments and learned a few things. We will move toward a PR tomorrow. Thank you for that wip commit. We will look at it tomorrow, as well.

samuela · 2023-12-16T01:40:20Z

Hi @mightyiam , just curious if you created that PR? I'd be curious to try it!

mightyiam · 2023-12-16T01:43:28Z

Next session scheduled 7 hours from now.

samuela · 2024-01-24T05:03:16Z

Hi @doy @mightyiam friendly ping on this. I've been running on this PR for a few weeks now and it's worked great so far.

mightyiam · 2024-01-24T09:26:06Z

@samuela we've extracted #9 as an isolated piece of valuable change that could be easily accepted. After which we intend to rebase and proceed in some reasonable way.

I also offered @doy my co-maintaining of this project.

fix: work in progress macos support

7889630

Co-authored-by: alex <source@proof.construction> Co-authored-by: echolinq <echolinq@protonmail.com> Co-authored-by: warren2k <846021+warren2k@users.noreply.github.com>

mightyiam force-pushed the macos_draft_pr branch from 8d25795 to 7889630 Compare November 12, 2023 15:21

mightyiam commented Nov 12, 2023

View reviewed changes

mightyiam mentioned this pull request Nov 12, 2023

Error "Inappropriate ioctl for device" on macOS #7

Open

samuela mentioned this pull request Jan 2, 2024

publish to crates.io samuela/sshenanigans#1

Open

samuela mentioned this pull request Mar 13, 2024

ci all features #9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: work in progress macos support #8

fix: work in progress macos support #8

mightyiam commented Nov 12, 2023

mightyiam commented Nov 12, 2023

mightyiam left a comment

mightyiam Nov 12, 2023

mightyiam Nov 12, 2023

doy Dec 3, 2023

mightyiam Nov 12, 2023

mightyiam Nov 12, 2023

doy Dec 3, 2023

doy Dec 3, 2023

mightyiam Dec 9, 2023

warren2k Dec 9, 2023

mightyiam Dec 9, 2023

mightyiam Dec 9, 2023

mightyiam Dec 9, 2023

mightyiam Nov 12, 2023

doy Dec 3, 2023

mightyiam Nov 12, 2023

samuela commented Nov 25, 2023

mightyiam commented Nov 25, 2023

samuela commented Nov 25, 2023

samuela commented Nov 28, 2023

mightyiam commented Dec 2, 2023

doy commented Dec 3, 2023

doy commented Dec 3, 2023

mightyiam commented Dec 9, 2023

samuela commented Dec 16, 2023

mightyiam commented Dec 16, 2023

samuela commented Jan 24, 2024

mightyiam commented Jan 24, 2024

	#[test]
	fn test_multiple() {
	let pty = pty_process::blocking::Pty::new().unwrap();
	let pts = pty.pts().unwrap();
	pty.resize(pty_process::Size::new(24, 80)).unwrap();

	let mut child = pty_process::blocking::Command::new("echo")
	.arg("foo")
	.spawn(&pts)
	.unwrap();

	let mut output = helpers::output(&pty);
	assert_eq!(output.next().unwrap(), "foo\r\n");

	let status = child.wait().unwrap();
	assert_eq!(status.code().unwrap(), 0);

	let mut child = pty_process::blocking::Command::new("echo")
	.arg("bar")
	.spawn(&pts)
	.unwrap();

	assert_eq!(output.next().unwrap(), "bar\r\n");

	let status = child.wait().unwrap();
	assert_eq!(status.code().unwrap(), 0);
	}

fix: work in progress macos support #8

Are you sure you want to change the base?

fix: work in progress macos support #8

Conversation

mightyiam commented Nov 12, 2023

mightyiam commented Nov 12, 2023

mightyiam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuela commented Nov 25, 2023

mightyiam commented Nov 25, 2023

samuela commented Nov 25, 2023

samuela commented Nov 28, 2023

mightyiam commented Dec 2, 2023

doy commented Dec 3, 2023

doy commented Dec 3, 2023

mightyiam commented Dec 9, 2023

samuela commented Dec 16, 2023

mightyiam commented Dec 16, 2023

samuela commented Jan 24, 2024

mightyiam commented Jan 24, 2024