enforce stdout/stderr contents in .local example/integration tests

[lihaoyi]

This should allow us to do debugging or iteration on example/integration in .local mode, which is a lot faster than .fork or .server

Even after #2425 landed, the .local integration tests are still much faster to run than .fork and .server, with publishing taking ~20s down from ~60s before which is still a very annoying wait. Better if we can avoid that, using .local in the common development workflow and leaving .fork and .server for CI to catch rare edge-case bugs

We basically needed to be a bit more robust in spawning subprocesses, allowing the InputPumper machinery to work for any redirected streams rather than being hardcoded to work with the PipedInputStream that MillServerMain uses.

I had to fix an issue in ScalaJSModule#run to make example.web[3-scalajs-module].local.test pass with this additional enforcement. It appears we were not properly pumping the JSEnv subprocess stdout/stderr. This would cause the outputs to disappear when run repeatedly in client-server mode, since the output would be inherited by first MillClient process that spawned the server, and not subsequent clients that simply connect to it

[lihaoyi]

@lefou this should be ready to review i think, CI looks good

[lefou]

I like the fix but I'm not completely sold by exposing the original streams. There are probably reasons to swap them and with this change, we leak the original streams. If some code is using them, we may see unexpected behavior/error. Could we instead just store a hash or provide a isOriginalInStream(stream)?

[lefou]

I don't think this works reliable, as objects are only instantiated when their class is loaded/accessed (I think). It works for us, as we use SystemStream from MillMain, which is "early enough", but there is no guarantee this still works, if we change MillMain later.

Either we set this from outside, or we add some notes about these potential issue.

[lihaoyi]

So my idea was that this is guaranteed to work on the condition that we only use SystemStreams.withStreams to do the redirects. I updated all the existing code to do so, and is probably a best practice anyway in order to keep the logic consistent throughout the codebase.

Now I can't guarantee that someone in future won't add code that runs before ours that redirects stuff manually using System.setOut or Console.withOut, but as long as we stick with the convention of using SystemStreams.withStreams, then this should work robustly. And since we're the application, we should be able to guarantee that no code runs before ours (code that runs after is not a problem since we'll already have saved SystemStreams.original

[lihaoyi]

@lefou I actually wanted to expose the original streams intentionally for debugging purposes.

Sometimes when I want to print stuff and the stream redirects are screwed up, I have to do something like os.proc("cat", os.temp("foo\n")).call(stdout = os.Inherit) to get my debugging lines to the console. Exposing the original streams would let me just do mill.api.SystemStreams.original.out.println, which while a bit long is definitely easier to remember than jumping through hoops with os.proc

[lefou]

@lefou I actually wanted to expose the original streams intentionally for debugging purposes.

Sometimes when I want to print stuff and the stream redirects are screwed up, I have to do something like os.proc("cat", os.temp("foo\n")).call(stdout = os.Inherit) to get my debugging lines to the console. Exposing the original streams would let me just do mill.api.SystemStreams.original.out.println, which while a bit long is definitely easier to remember than jumping through hoops with os.proc

One reason we added the top-level stream redirection is the way our BSP server is implemented. Stdin and stdout are reserved for the protocol layer and anything printed there would break the protocol. I do see the value when debugging stuff, but the risk to break something is real. Maybe we can only leak the original stderr channel, which is more unlikely to be used for "protocol" stuff?

[lihaoyi]

@lefou sure, I can do that. Will update the PR

[lihaoyi]

Made the discussed changes, hopefully CI is happy

[lefou]

Looks good to me. I'd like to have explicit return types in public API.

[lihaoyi]

Only failure seems flaky, going to merge this