Add the ability to set stdin for a Process

[svenier]

This adds a new optional field to Process objects of the bytes to be passed into the command's stdin. This has the unfortunate side effect of forcing the command to run locally because the Remote Execution API doesn't support stdin (yet).

This is in support of #22918

[benjyw]

Thanks for this contribution! Some comments so far.

[benjyw]

Looks good overall. Let us know when this is out of Draft state and ready for a full review.

[benjyw]

Code looks great, a clarification on a comment would be useful. Thanks!

[svenier]

Comments addressed. Question: Should the AI disclosure stay in the PR description above (which I think I read ends up in the commit message) or should it move to just a comment on the discussion?

[benjyw]

Comments addressed. Question: Should the AI disclosure stay in the PR description above (which I think I read ends up in the commit message) or should it move to just a comment on the discussion?

Moving it to a comment makes sense.

[benjyw]

Looks great! Just one minor nit on one of the comments. Let me know when this is ready and I'll trigger one last CI run, and then merge on green.

[svenier]

AI Disclosure: Changes were made by an AI agent and review by multiple humans. Unfortunately, none of them are Rust experts, so some special attention is warranted there.

[svenier]

@benjyw All set! Thanks for your guidance on this.

[benjyw]

One more quick question.

[tdyas]

Regarding REAPI support, Pants already has the ability to generate a wrapper script around remote executions. Stdin support would just be another item for the wrapper script to handle. See maybe_make_wrapper_script.

[svenier]

Regarding REAPI support, Pants already has the ability to generate a wrapper script around remote executions. Stdin support would just be another item for the wrapper script to handle. See maybe_make_wrapper_script.

Oh, hello! Can this be an improvement in a future version or should I flip this back to a draft so I don't merge it with the remote execution restriction in place?

[tdyas]

Regarding REAPI support, Pants already has the ability to generate a wrapper script around remote executions. Stdin support would just be another item for the wrapper script to handle. See maybe_make_wrapper_script.

Oh, hello! Can this be an improvement in a future version or should I flip this back to a draft so I don't merge it with the remote execution restriction in place?

In my view, it would depend on which release this PR ends up in. If it goes out as is in 2.31, then users would be able to observe a difference in behavior between local processes and remote execution processes. My vote would be to avoid that if we can.

But if it will go out in 2.32, then deferring the remote execution support to a separate PR seems fine.

[benjyw]

@tdyas how complicated would it be to enhance that script? I'm guessing "not very"?

[tdyas]

@tdyas how complicated would it be to enhance that script? I'm guessing "not very"?

Very straightforward:

1. Modify maybe_make_wrapper_script to take an Option<String> with the stdin content filename to use. Add "has stdin content" as an additional reason to trigger use of the wrapper script. See

   pants/src/rust/process_execution/src/lib.rs

   Line 1222 in f9a2641

   if caches_fragment.is_none() && sandbox_root_fragment.is_none() {

   for the code which short-circuits use of the wrapper script if there is no need to do so.

2. Modify the exec in the wrapper script to redirect from the stdin file if stdin filename is configured.

3. Persist the stdin bytes to the CAS by updating wrapper_script_digest_opt to include a file for stdin content if there is such content. Use the filename that was passed in to maybe_make_wrapper_script.

[svenier]

Regarding REAPI support, Pants already has the ability to generate a wrapper script around remote executions. Stdin support would just be another item for the wrapper script to handle. See maybe_make_wrapper_script.

Oh, hello! Can this be an improvement in a future version or should I flip this back to a draft so I don't merge it with the remote execution restriction in place?

In my view, it would depend on which release this PR ends up in. If it goes out as is in 2.31, then users would be able to observe a difference in behavior between local processes and remote execution processes. My vote would be to avoid that if we can.

But if it will go out in 2.32, then deferring the remote execution support to a separate PR seems fine.

@cburroughs already asked to hold this until 2.32.

[svenier]

This is probably going to require a change to the UI since currently the Process object just gets Bytes for stdin, not a Digest.

Since that's a big of a big change, I'll flip this back to a draft and send out pings when I get the changes done.

[tdyas]

This is probably going to require a change to the UI since currently the Process object just gets Bytes for stdin, not a Digest.

Since that's a big of a big change, I'll flip this back to a draft and send out pings when I get the changes done.

Bytes in the API is fine. Use the Store API to write the Bytes into the Digest with store_file_bytes as the existing wrapper script code does.

[svenier]

Bytes in the API is fine. Use the Store API to write the Bytes into the Digest with store_file_bytes as the existing wrapper script code does.

Sure, but if the Bytes were originally a file to begin with, it'd save a bunch of hassle to just pass the original Digest through.

[cburroughs]

Thanks for the contribution. We've just branched for 2.31.x, so merging this pull request now will come out in 2.32.x, please move the release notes updates to docs/notes/2.32.x.md if that's appropriate.