Process piping

[szymon-rd]

Resolve #187

This PR adds a way to pipe processes concisely and conveniently. Previously, creating pipelines behaving similarly to Unix's was impossible (or hard). Broken pipe error was handled by the parent process, so pipeline like yes | head -n 5 would run forever, and the app itself crash/throw error. Additionally, there was no way to pass arguments like pipefail.

All this is done with custom handler threads and not the JVM 9 API. Java's API is also incapable of handling Broken pipe.

On Windows, the broken pipe handling is not supported - there is no broken pipe error.

Pull request: #200

[szymon-rd]

@lihaoyi Could you run the workflow, please? Btw, why does it require approval? It may be better for future contributors to disable the requirement for approval. (especially in cases like this one, where locally everything works and I'm trying to get it to work on gh actions)

[lefou]

Approval is a security measure. Once, you have contributed (at least one commit to git main branch), no further approval is needed.

[lefou]

If you want to reproduce CI issues, you could also push (or create a pull request) to your own fork.

[szymon-rd]

Tests are still a bit flaky on the CI (~1 in 20 runs fail), next commit should finally fix that :)

edit: Actually, it may have been one-off, I really can't reproduce it

[szymon-rd]

However, I think it is ready for review aside from that minor issue (@lefou @lihaoyi)

[lihaoyi]

Is it possible to use a smaller more ubiquitous process in these test cases here? I don't want to have to install the whole of scala-cli just to run unit tests locally, especially when it seems some combination of simple unix tools (cat, sleep, bash -c, etc.) should be able to exercise it.

If necessary, we can even build Scala executable jar files as part of the build.sc and run them using java -jar here, which would be better than using Scala-CLI since it would not require any third-party install

[szymon-rd]

We can do that, but I wonder if that's not a pattern we would want to exercise more. As os-lib is a library for handling processes, maybe a tool that allows easy creation of programs is useful. Especially given that it will be just scala very soon.

The benefit of these tests is that they can run on all platforms. On Windows, it's quite hard to find a command like tail. If I digged a bit more, I would probably be able to work around it. But I would rather create the jars, I think.

[lihaoyi]

lets go with test jars then. Mill makes submodules cheap, we can easily have a few submodules to create jars for testing purposes. e.g. Mills own codebase has dozens of submodules to create test jars for it's codesig analyzer (https://github.com/com-lihaoyi/mill/tree/main/main/codesig/test/cases/callgraph)

Even if scala-cli is going to be scala, as of now os-lib doesn't require scala to be installed at all, so better to avoid additional unmanaged dependencies unless they're truly necessary. Even test dependencies.

Maybe we want to have some easier way of creating sub-processes with Scala code, but that's probably a separate discussion from this specific PR.

[szymon-rd]

Could you please provide an example of a decent simple workflow generating jars like this, for a project like os-lib (or maybe even here)? I am unfamiliar with Mill, the configuration of the project you provided is quite complex, and my version will probably be a bit overcomplicated, leading to more work in the review-fixes cycle.

[lihaoyi]

Something like this should work

diff --git a/build.sc b/build.sc
index 80437ae..c28ab6b 100644
--- a/build.sc
+++ b/build.sc
@@ -58,6 +58,10 @@ trait MiMaChecks extends Mima {
   )
 }
 
+object testJar1 extends JavaModule
+object testJar2 extends JavaModule
+object testJar3 extends JavaModule
+
 trait OsLibModule
     extends CrossScalaModule
     with PublishModule
@@ -84,7 +88,12 @@ trait OsLibModule
     def ivyDeps = Agg(Deps.utest, Deps.sourcecode)
 
     // we check the textual output of system commands and expect it in english
-    def forkEnv = super.forkEnv() ++ Map("LC_ALL" -> "C")
+    def forkEnv = super.forkEnv() ++ Map(
+      "LC_ALL" -> "C",
+      "TEST_JAR_1_ASSEMBLY" -> testJar1.assembly().path.toString,
+      "TEST_JAR_2_ASSEMBLY" -> testJar2.assembly().path.toString,
+      "TEST_JAR_3_ASSEMBLY" -> testJar3.assembly().path.toString,
+    )
   }
 }

You can then put the source code for your test jars in testJar1/src/TestJar1.java, and use the assemblies in your test suite via os.proc("java", "-jar", sys.env("TEST_JAR_1_ASSEMBLY"))).call()

[lihaoyi]

Extra backtick here after Pipefail

[lihaoyi]

Some nitpicks, but otherwise looks good I think

[szymon-rd]

@lihaoyi Done I think

[szymon-rd]

And - Are we going to release M1 for 1.0 now?

[lihaoyi]

Lemme take another review pass at this and I'll merge it if there aren't any other issues to discuss

As for 1.0, I support we could. The library has been pretty stable the last 1-2 years, so once this merges it might be time to put a ribbon on it and call it stable. @lefou WDYT?

[lefou]

As for 1.0, I support we could. The library has been pretty stable the last 1-2 years, so once this merges it might be time to put a ribbon on it and call it stable. @lefou WDYT?

Let's cut a 0.9.2 first. In the end, we managed to stay binary compatible. Also, to get the new features / API out as soon as possible.

For 1.0, I'd like to do some more polishing, like bumping Scala versions and dropping older Scala.js (and Scala Native) versions as a consequence. I'd rather avoid supporting one platform in 1.0 and drop it in 1.0.1, so better, we have the 0.9.2.

[szymon-rd]

It's still 0.x, so semver doesn't apply here. But shouldn't it be 0.10.0? We are releasing a new, backward-compatible feature.

[lefou]

It's still 0.x, so semver doesn't apply here. But shouldn't it be 0.10.0? We are releasing a new, backward-compatible feature.

We're using early semver, which means, we handle version compatibility like this: 0.major.minor. So a 0.9.2 is perfectly fine, as it is semantically a minor release, it adds something but does not remove anything. I'd avoid a 0.10 (a early major), as we want to release a 1.0.0 soon.