Skip to content

Added output configurations. Related to #3170 #3543

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

Dralt03
Copy link

@Dralt03 Dralt03 commented Mar 8, 2025

Exposed a Configuration for the user to select how they want to deal with sterr and stdout for streams in /io/shared/src/main/scala/fs2/io/process/ProcessesBuilder.scala

@Dralt03 Dralt03 changed the title Added output configurations Added output configurations. Related to #3170 Mar 8, 2025
@armanbilge armanbilge self-requested a review March 8, 2025 15:14
@armanbilge armanbilge marked this pull request as draft March 9, 2025 23:26
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Nice start! Do you think you can also make the necessary changes to the Processes implementations?

/** Starts the process and returns a handle for interacting with it.
* Closing the resource will kill the process if it has not already terminated.
*/
final def spawn[F[_]: Processes]: Resource[F, Process[F]] =
Processes[F].spawn(this)
}

sealed trait ProcessOutputMode
object ProcessOutputMode {
Copy link
Member

Choose a reason for hiding this comment

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

These look like the right set of options! We need to support stdin, stdout, and stderr to be configured separately.

@Dralt03
Copy link
Author

Dralt03 commented Mar 11, 2025

I think this should do it but do let me know if there is more to improve on this

@Dralt03 Dralt03 marked this pull request as ready for review March 16, 2025 18:19
} { process =>
F.delay(process.isAlive())
.ifM(
evalOnVirtualThreadIfAvailable(
Copy link
Member

Choose a reason for hiding this comment

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

Deleted by mistake?

Copy link
Author

Choose a reason for hiding this comment

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

Must've been deleted while solving merge conflicts adding them back

Comment on lines 83 to 89
sealed trait StreamOutputMode
object StreamOutputMode {
case object Pipe extends StreamOutputMode
case object Inherit extends StreamOutputMode
case object Ignore extends StreamOutputMode
case class FileOutput(path: Path) extends StreamOutputMode
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sealed trait StreamOutputMode
object StreamOutputMode {
case object Pipe extends StreamOutputMode
case object Inherit extends StreamOutputMode
case object Ignore extends StreamOutputMode
case class FileOutput(path: Path) extends StreamOutputMode
}
sealed abstract class StreamRedirect
object StreamRedirect {
case object Pipe extends StreamRedirect
case object Inherit extends StreamRedirect
case object Discard extends StreamRedirect
final case class File(path: Path) extends StreamRedirect
}

env =


val spawnOptions = js.Dynamic.literal {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using js.Dynamic can you update the SpawnOptions face to add the additional fields?

Comment on lines +73 to +74
/** @see [[outputMode]] */
def withOutputConfig(outputConfig: ProcessOutputConfig): ProcessBuilder
Copy link
Member

Choose a reason for hiding this comment

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

I think separating it out into three config methods for stdin/stdout/stderr (like the JDK process builder) will make the API easier to use.

@Dralt03 Dralt03 requested a review from armanbilge March 19, 2025 08:59
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this PR! Tests currently are not passing on JVM or JS. (Native is broken for reasons not related to you.)

@Dralt03
Copy link
Author

Dralt03 commented Apr 12, 2025

@armanbilge The tests for rootJS are currently not passing for the merged option. Do you have an idea on how we can proceed with that. I'm thinking about setting the stdio to ["pipi", "pipe", 1] to show if it should be merged but I'm not sure if that would work so I need some help with that

@armanbilge
Copy link
Member

I think Node.js might not support that feature, so we can just implement it with FS2 Stream#merge.

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