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

chore: prefactor process manager #7034

Merged
merged 8 commits into from
Jan 22, 2024

Conversation

chris-olszewski
Copy link
Member

@chris-olszewski chris-olszewski commented Jan 19, 2024

Description

This PR lays some ground work for PTY support by hiding implementation details of building the command from users of the ProcessManager. The public interface for ProcessManager and Child after this PR no longer use any part of tokio::process.

This PR adds our own Command struct that is intended to be used with ProcessManager::spawn instead of the raw command builder type which in the future might be portable_pty::CommandBuilder. This approach offers a few benefits:

  • It allows us to limit the API we expose for command building to only options supported by both regular child processes and child processes hooked up to a pseudo-terminal.
  • Commands can be decoupled from the spawning strategy
  • Allows us to avoid running into the orphan rule

⚠️ Behavior change: we will now not have our child processes inherit our stdin. This was never intended behavior, but an oversight of us not explicitly calling stdin(Stdio::piped()) or stdin(Stdio::null()).

Testing Instructions

Existing test suite and 👀

Each commit should be easily reviewable on its own.

Closes TURBO-2070

Copy link

vercel bot commented Jan 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-basic-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2024 4:55pm
examples-designsystem-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2024 4:55pm
examples-gatsby-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2024 4:55pm
examples-kitchensink-blog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2024 4:55pm
examples-native-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2024 4:55pm
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2024 4:55pm
examples-svelte-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2024 4:55pm
examples-tailwind-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2024 4:55pm
examples-vite-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2024 4:55pm
rust-docs ❌ Failed (Inspect) Jan 22, 2024 4:55pm
turbo-site ✅ Ready (Inspect) Visit Preview Jan 22, 2024 4:55pm

Copy link
Contributor

github-actions bot commented Jan 19, 2024

🟢 Turbopack Benchmark CI successful 🟢

Thanks

Copy link
Member Author

@chris-olszewski chris-olszewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick notes

pub async fn wait_with_piped_outputs<W: Write>(
&mut self,
mut stdout_pipe: W,
mut stderr_pipe: Option<W>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independently capturing stderr wasn't an used outside of tests and can't be easily done with processes that are hooked up to a pseudo-terminal (possible, but involves redirecting stderr to a file and streaming that). In preparation we remove this feature.

Comment on lines +120 to +124
.stdin(if open_stdin {
Stdio::piped()
} else {
Stdio::null()
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a behavior change as we will no longer have our children inherit our stdin

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(but that matches the Go eversion)

@chris-olszewski chris-olszewski marked this pull request as ready for review January 19, 2024 23:32
@chris-olszewski chris-olszewski requested a review from a team as a code owner January 19, 2024 23:32
Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor API concerns, but otherwise LGTM.

crates/turborepo-lib/src/process/command.rs Show resolved Hide resolved
crates/turborepo-lib/src/process/command.rs Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jan 20, 2024

🟢 CI successful 🟢

Thanks

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

Successfully merging this pull request may close these issues.

None yet

3 participants