-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
Automatically pump os.proc
streams when SystemStreams
are redirected
#3275
Conversation
public InputPumper(InputStream src, | ||
OutputStream dest, | ||
public InputPumper(Supplier<InputStream> src, | ||
Supplier<OutputStream> dest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making these lazily-initialized is necessary to avoid infinite recursion when initializing the pumper <-> stream <-> subprocess data structure, which is circular
I think this should be ready for review @lefou |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Due to lack of test coverage, we should manually test this at least once in a BSP session.
Tested the BSP integration via intellij on example/scalabuilds/9-realistic. Seems to load the project successfully with some |
Depends on com-lihaoyi/os-lib#283
This moves the subprocess stream handling logic out of
Jvm.spawnSubprocess
and makes it apply to allos.proc
invocations, greatly reducing the room for error. With this,Jvm.spawnSubprocess
becomes a very thin wrapper aroundos.proc.spawn
. We also rely directly on OS-Lib's own pumper threads to pump to our destination, rather than having them pump into in-memory buffers and then spawning our own pumper threads to pump from those buffers to the destinationI spent some time looking into how to do the stdout/err handling at the process level, but couldn't find any reasonable mechanism to do so that allows us to preserve the ordering of the stdout/stderr. This is the original motivation to squishing it into one stream via
ProxyOutputStream
/ProxyStreamPumper
and is important because otherwise you find e.g. stack traces out of order with printlns, which makes debugging very difficult. Might be possible using some socket/fifo/pipe cleverness, but not as part of this PRAdded an integration test to assert on the subtleties of stdout, stderr, and their inherited alternatives. This PR is required for the test to pass