Use fs2-io library for file and process handling#23
Conversation
To ensure exactly the right operations are executed on blocking threads and to reduce the maintenance burden, swap in fs2-io for 90% of the I/O-related code. Also use fs2 for spawning external processes and hashing.
rochala
left a comment
rochala
left a comment
There was a problem hiding this comment.
Great PR, thanks for doing this. 2 small comments left.
| process.stderr.through(fs2.text.utf8.decode).compile.string | ||
| ).parMapN(ProcessResult.apply) | ||
| }.adaptError { case e: java.io.IOException => | ||
| val cmd = command.headOption.getOrElse("") |
There was a problem hiding this comment.
Claude review:
Bug in ProcessRunner.adaptError (ProcessRunner.scala)
def run(command: String, args: List[String], ...) = {
...
}.adaptError { case e: java.io.IOException =>
val cmd = command.headOption.getOrElse("") // BUGcommand is now a String, not List[String]. String.headOption returns Option[Char], so cmd will be a single Char
(the first character of the binary name). The error message would print Failed to start 'n': ... instead of
Failed to start 'nonexistent-command-xyz': .... Should just be command directly.
| IO.blocking { | ||
| val millMarker = millMarkers.find(m => Files.exists(dir.resolve(m))) | ||
| val hasSbt = Files.exists(dir.resolve("build.sbt")) | ||
| val hasScalaBuild = Files.isDirectory(dir.resolve(".scala-build")) |
There was a problem hiding this comment.
Lets restore .scala-build detection. It is duplicated for now with the fallback but in the future if we want to add support for any other build tool, the fallback may no longer apply
There was a problem hiding this comment.
Re-added and refactored the code using findM for better readability.
# Conflicts: # lib/src/cellar/build/MillBuildTool.scala # lib/src/cellar/build/ProjectClasspathProvider.scala # lib/src/cellar/build/SbtBuildTool.scala # lib/src/cellar/handlers/ProjectHandler.scala # lib/test/src/cellar/ProjectAwareIntegrationTest.scala
rochala
left a comment
rochala
left a comment
There was a problem hiding this comment.
LGTM, thanks for the contribution :D
2 participants
To ensure exactly the right operations are executed on blocking threads (which doesn't seem to be the case yet, see #21) and to reduce the maintenance burden, swap in
fs2-iofor 90% of the I/O-related code. The remaining 10% are deeply involved with virtual file systems and ZIP files, hence I left them as-is for now.Also use fs2 for spawning external processes and hashing to profit from its safe and easy-to-use APIs.
Tested locally, everything passed just fine. In my brief test, also the hashes were consistent with the previous implementation (which is generally expected as fs2 delegates to
MessageDigest).This PR is a proposal, I'm totally fine if it ends up getting discarded. I just fell into the rabbit hole of "let me quickly try to optimise
ProcessRunner" 🫣.