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

Directly forward execution output to stdout #370

Merged
merged 5 commits into from
Feb 13, 2024
Merged

Conversation

marzipankaiser
Copy link
Contributor

@marzipankaiser marzipankaiser commented Jan 24, 2024

Fixes #336 (since I was looking into the Process API r.n. anyway).

Issue #336 is caused by the Process being started using .!! which:

Starts the process represented by this builder, blocks until it exits, and returns the output as a String. Standard error is sent to the console. If the exit code is non-zero, an exception is thrown.

We now use a custom ProcessLogger and also connect stdin.
This makes this mostly work, apart from potential race conditions between input and output. (Note: .!< would work well in that regard but breaks the tests.)

Issue #336 is caused by the Process being started using `.!!` which:
```
Starts the process represented by this builder, blocks until it exits, and returns the output as a String. Standard error is sent to the console. If the exit code is non-zero, an exception is thrown.
```

This uses `.!<` instead, which:
```
Starts the process represented by this builder, blocks until it exits, and returns the exit code. Standard output and error are sent to the console. The newly started process reads from standard input of the current process.
```

Note: This should also allow reading from stdin now (but I have not
tested this yet).
@marzipankaiser
Copy link
Contributor Author

Apparently this is used in some way during testing. Am investigating.

They were broken because we apparently check test output by running via
the normal eval and capturing the output from `C.config.output()`.

This breaks stdin for executed programs again, but this never worked
before anyway.
@marzipankaiser
Copy link
Contributor Author

This is still suboptimal, but it does allow for input and output, and at least streams the outputs on a line-by-line basis (So, a flush from effekt won't work)

effekt/jvm/src/main/scala/effekt/Runner.scala Outdated Show resolved Hide resolved

override def out(s: => String): Unit = {
C.config.output().emitln(s)
System.out.flush()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to flush each time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can probably drop this; it is left over from trying to get the ordering right when printing not-full-lines.
emitln should flush anyway.

@b-studios
Copy link
Collaborator

It's great that you made progress here. But I would have hoped to just have a thin wrapper that acts exactly like programming against the host native interface.

Co-authored-by: Jonathan Immanuel Brachthäuser <jonathan@b-studios.de>
@marzipankaiser
Copy link
Contributor Author

This was mostly a product of me reading into the process API docs anyway.

If I understand that correctly, then we'd need to change how tests are run (because they currently check the output in C.config). IMHO, we might want to run tests differently anyway (esp. w.r.t. stdin and stdout), but I'm not sure how big of a change this would be. - Then, using .!< should do exactly what we'd want here (just use stdin/out/err for in/out/err), no?

@b-studios b-studios merged commit 29b3e9a into master Feb 13, 2024
2 checks passed
@b-studios b-studios deleted the fix/issue-336 branch February 13, 2024 17:03
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.

println output should not be buffered during execution
2 participants