zio / zio-process

A simple ZIO library for interacting with external processes and command-line programs
https://zio.dev/zio-process
Apache License 2.0
64 stars 13 forks source link

Release new version #399

Open pablf opened 6 months ago

pablf commented 6 months ago

A new version release would be helpful to implement full native and scalajs support for projects with zio-process as dependency.

reibitto commented 5 months ago

I still haven't looked at everything or tried the Scala.js and Scala Native support myself. I noticed there were a few caveats and things that differ from the plain JVM version. There may be some issues that I need to resolve. But maybe I could release a milestone or release candidate first so that anyone who wants to can test it and report issues. Granted we have snapshot releases already, but it's slightly less convenient consuming that in a project. And if I do a RC release more people will see it in their notifications.

I notice CI is red currently so I might need to do something about that first. Maybe some flaky tests.

Sorry for not noticing this earlier. I've been busy with a bunch of stuff lately. I'll hopefully be able to start this next week. I haven't done much on this project in a while.

pablf commented 5 months ago

A RC sounds good. I didn't notice the failing CI, but seems to be a flaky test because it was passing in the PR. I'll try to take a look. I think the problem might be in the interface connecting Node Streams and Java Streams.

reibitto commented 2 weeks ago

Hi @pablf, I'm preparing to do an official release after this PR goes in #430 but Scala.js is still an issue with flakiness. I was able to duplicate the problem and have a reproduction case that fails 100% of the time. In my PR I created the following test:

test("streaming entire result from stdout should wait for completion") {
  val zio = for {
    process <- Command(s"zio-process/shared/src/test/bash/echo-repeat-short.sh").run
    _       <- ZIO.sleep(5.seconds) // TODO: This should not be needed. If you remove the sleep it fails.
    stdout  <- process.stdout.string
  } yield stdout

  assertZIO(zio)(equalTo("iteration: 1\niteration: 2\niteration: 3\n"))
}

echo-repeat-short.sh is just a script that prints 3 lines with a 1 second delay between each:

for i in {1..3}; do echo "iteration: $i"; sleep 1; done

So it seems like reading from stdout is returning immediately rather than streaming it in and waiting. I looked at the implementation for Scala.js and I'm not seeing where the problem exactly is. I can keep looking but I figured since you're familiar with this you might be able to immediately recognize what the problem is. I'm guessing it's a very simple fix and a wait call or promise is needed somewhere but I'm not 100% sure. Do you have any ideas?

pablf commented 2 weeks ago

@reibitto From looking at this, it would seem like a promise would need to be added somewhere, maybe in the data modeling streams (ProcessStream I think?, the type of process.stdout) could be easier, like a custom-made completed. I think that I tried adding several promises in different places trying to make it work and so it might be not be completely clear where it can be added. I'll check if there is a more appropriate place, but please reach with any other questions or ideas. In general, it might be also an issue of the js java streams implementation.

pablf commented 2 weeks ago

@reibitto one question, is there a difference between your echo-repeat-short.sh and echo-repeat.sh? Because the latter has simply a longer duration right?