Skip to content

Conversation

passcod
Copy link
Member

@passcod passcod commented Jun 27, 2025

Implementation for #22

Migration for tokio: replace Box::into_pin(child.wait()) with child.wait()

@TheLostLambda
Copy link
Contributor

I'll give this a try in just a moment! I am currently forced to be on Windows, so hopefully I won't run into that stack overflow, but I'll let you know if I do!

Additionally, and maybe this is best of a separate PR, I've run into the case where I think I'll need post_spawn to provide a command: &mut Command argument to get my wrapper to function properly! I'm very happy to elaborate, but essentially I use the new std::io::pipe to merge stdout and stderr into one stream that I write to a logfile with this code:

fn pre_spawn(&mut self, command: &mut Command, _core: &StdCommandWrap) -> io::Result<()> {
    let mut logfile = File::create(&self.path)?;
    let (mut rx, tx) = pipe()?;

    self.thread = Some(thread::spawn(move || {
        io::copy(&mut rx, &mut logfile).unwrap();
    }));

    command.stdout(tx.try_clone()?).stderr(tx);
    Ok(())
}

But that thread will never return / be joined if anything is holding onto a copy of tx, so I really need post_spawn to let me call:

command
    .stdout(Stdio::null())
    .stderr(Stdio::null());

Or similar, or else my program completely deadlocks when I try to join the thread in .wait():

fn wait(&mut self) -> io::Result<ExitStatus> {
    let exit_status = self.inner.wait();

    if let Some(thread) = mem::take(&mut self.thread) {
        thread.join().expect("logfile thread panicked");
    }

    exit_status
}

Let me know if it's possible to change the signature of post_spawn to fn post_spawn(&mut self, _command: &mut Command, _child: &mut Child, _core: &StdCommandWrap) -> Result<()> — that will make it possible to mutate Command for one spawn, then change it back right after — a niche, but I think very useful pattern!

@TheLostLambda
Copy link
Contributor

TheLostLambda commented Jun 28, 2025

Furthermore, if we can get things to a functional state you're happy with, I'm more than happy to expand on the "Custom Wrappers" part of the documentation and can include my LogFile wrapper as an example. It's pretty short overall and I think it would hit all of the key points (except, perhaps, .extend(), but I could try to shoehorn that in if that's important!).

@TheLostLambda
Copy link
Contributor

TheLostLambda commented Jun 28, 2025

@passcod Thanks for all of the great work so far! I think the fix for the failing Windows tests is: 1fc90cb

@passcod passcod marked this pull request as ready for review June 30, 2025 00:01
@passcod
Copy link
Member Author

passcod commented Jun 30, 2025

expand on the "Custom Wrappers" part of the documentation

that would be very welcome!

@TheLostLambda
Copy link
Contributor

expand on the "Custom Wrappers" part of the documentation

that would be very welcome!

That's done now in #24 !

@passcod passcod merged commit bcfd7fd into main Jul 23, 2025
13 checks passed
@passcod passcod deleted the child-wrapping-ux branch July 23, 2025 02:21
@passcod passcod restored the child-wrapping-ux branch July 23, 2025 02:22
@passcod passcod deleted the child-wrapping-ux branch July 23, 2025 02:34
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.

2 participants