Adds multiplex sandbox support and improves argument parsing #52

jjudd · 2024-08-26T19:52:23Z

Primary reviewer here is @jadenPete. I added @gregghz in case they're interested in taking a look.

jadenPete · 2024-08-26T20:31:53Z

rules/scala_proto/private/ScalaProtoWorker.scala

 import protocbridge.{ProtocBridge, ProtocRunner}
 import scala.jdk.CollectionConverters._
 import scalapb.ScalaPbCodeGenerator

 object ScalaProtoWorker extends WorkerMain[Unit] {

+  private class ScalaProtoRequest private (


Nit: Should this be a case class (this applies to the other configuration classes created in this commit)?

I don't believe so because you get this error message related to Scala 3 migration:

src/main/scala/higherkindness/rules_scala/workers/common/CommonArguments.scala:15: error: access modifiers for `copy` method are copied from the case class constructor under Scala 3 (or with -Xsource-features:case-apply-copy-access) Scala 3 migration messages are issued as errors under -Xsource:3. Use -Wconf or @nowarn to demote them to warnings or suppress. Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=scala3-migration, site=higherkindness.rules_scala.workers.common.CommonArguments case class CommonArguments private ( ^ 1 error

Oh, I think you need to remove the private constructor modifier too.

Agreed. I want the constructor to be private, though, which is why this is a class and not a case class.

I only want people to be able to construct these objects by passing in a namespace and a working directory, so someone doesn't accidentally construct one incorrectly.

That makes sense. I think this is fine as-is, then.

jadenPete · 2024-08-26T20:45:34Z

src/main/scala/higherkindness/rules_scala/common/sandbox/SandboxUtil.scala

+  def getSandboxFile(workDir: Path, file: File): File = {
+    workDir.resolve(file.toPath).toFile
+  }
+  def getSandboxPaths(workDir: Path, paths: JList[Path]): List[Path] = {


Nit: Do we need these getSandboxPaths methods? Writing a method for every collection seems a bit unnecessary when the caller could just as easily call getSandboxPath for every element.

I originally was doing that, but found it to be enough boiler plate that I wrote these as convenience methods.

jadenPete · 2024-08-26T20:46:24Z

rules/scalafmt/scalafmt/ScalafmtRunner.scala

@@ -14,20 +18,38 @@ import scala.io.Codec

 object ScalafmtRunner extends WorkerMain[Unit] {

-  protected[this] def init(args: Option[Array[String]]): Unit = {}
+  private[this] class ScalafmtRequest private (


Nit: I think we should avoid using private[this] because it doesn't offer many guarantees over private and will be removed in Scala 3 (this applies to the other uses of private[this] and protected[this] in this commit.

I think there's a performance advantage, however small, in Scala 2 to using private[this] vs private https://users.scala-lang.org/t/what-does-passing-this-after-access-modifier-mean/3838/5

Something that might have a performance impact (though I think the JIT should be pretty good at eliminating this anyway), is that a private[this] val gets compiled to a field only. While private val gets compiled to a field and an accessor method.

Ah, good to know. This was just a nit, so I'm fine with keeping it either way.

jadenPete · 2024-08-26T20:48:26Z

src/main/scala/BUILD

@@ -3,6 +3,7 @@ load(
    "configure_bootstrap_scala",
    "configure_zinc_scala",
    "scala_library",
+    # "bar_toolchain",


Can we remove this comment?

Yep. Nice catch.

jadenPete · 2024-08-26T20:48:53Z

src/main/scala/BUILD

@@ -67,17 +68,15 @@ configure_zinc_scala(

 # Scala 3

+# Adding this, so we make sure to have a Scala library in the
+# IntelliJ libraries, so we can get a Scala SDK on sync.
 # Adding this, so we make sure to have a Scala library in the


It looks like this comment was duplicated.

Yep. Fixing.

jadenPete · 2024-08-26T21:33:04Z

src/main/scala/higherkindness/rules_scala/workers/deps/DepsRunner.scala

-    def pathsForLabel(depLabel: String): Seq[String] = {
+    val label = workRequest.label
+    val directDepLabels = workRequest.directDepLabels
+    val groupLabelToJarPaths = workRequest.groups.map { group =>


Nit: Can we inline label and directDepLabels? Also, should groupLabelToJarPaths be a lazy val on DepsRunnerRequest?

I've inlined those two. I did not not change that val to be lazy due to the performance penalty of lazy and the locking that comes with it.

jadenPete · 2024-08-26T21:34:32Z

src/main/scala/higherkindness/rules_scala/workers/deps/DepsRunner.scala

+      .readAllLines(workRequest.usedDepsFile)
+      .asScala
+      .view
+      // Use the read mapper on the used classpath entries in order to keep


Out of curiosity, could you explain what the read mapper does?

Read/write mapper is what we use to serialize and deserialize paths in a machine independent way.

If you didn't do mapping you'd end up with something like /home/foo/project/src/scala/bar/Qux.scala, but that's machine dependent. We only care about the src/scala/bar/Qux.scala bit.

The write mappers write things to disk in a machine independent way.

The read mappers know how to read those things from disk and translate them to actual paths on your machine.

Most of this is defined in src/main/scala/higherkindness/rules_scala/workers/common/AnnexMapper.scala

That clarifies things. Thanks!

jadenPete · 2024-08-26T21:37:39Z

src/main/scala/higherkindness/rules_scala/workers/zinc/compile/Deps.scala

Could you fill me in on why this changes was necessary?

Normalize does this:

Returns a path that is this path with redundant name elements eliminated.

I realized we weren't normalizing paths, so it would in theory be possible to get a path that points to the same spot, but the string doesn't match up. I wanted to eliminate that potential bit of non-determinism by normalizing the absolute paths.

Gotcha. I wasn't sure if it was necessary as a consequence of introducing multiplexed, sandboxed workers, or if it was just good practice.

Good point. This is unrelated to multiplex sandboxing. Just something I encountered while working on the sandboxing and applied everywhere. It really should have been its own commit. My bad.

jadenPete · 2024-08-26T21:40:53Z

src/main/scala/higherkindness/rules_scala/workers/zinc/compile/ZincRunner.scala

    val parser = ArgumentParsers.newFor("zinc-worker").addHelp(true).build
    parser.addArgument("--persistence_dir", /* deprecated */ "--persistenceDir").metavar("path")
    parser.addArgument("--use_persistence").`type`(Arg.booleanType)
    parser.addArgument("--extracted_file_cache").metavar("path")
    // deprecated
    parser.addArgument("--max_errors")
-    parser.parseArgsOrFail(args.getOrElse(Array.empty))
+    val namespace = parser.parseArgsOrFail(args.getOrElse(Array.empty))


Should we use ArgsUtil.parseArgsOrFailSafe here?

Not here, no. This is called with the worker is being initialized. It's ok for the worker to die here. If the worker dies here it just fails to start. It's also preferable for it to die as I'm not sure what else to do here.

If that worker dies on a work request, that's bad. Recovery is just telling Bazel that work request failed and then waiting for the next request to come in.

Ah, that makes sense.

jadenPete · 2024-08-26T21:42:30Z

src/main/scala/higherkindness/rules_scala/common/worker/WorkerMain.scala

@@ -13,11 +14,14 @@ trait WorkerMain[S] {

  protected[this] def init(args: Option[Array[String]]): S


This method seems rarely used in subclasses of WorkerMain because argument parsing (which calls ArgsUtil.parseArgsOrFailSafe) now requires access to the output stream, which this method doesn't provide. Should we refactor it to provide that and make more places use init in a meaningful way?

I think this is still useful for when you need to initialize the worker.

init happens once when the worker starts. work happens on every work request.

Agree that it is called in frequently and a good tradeoff may be providing a default implementation of init, but I'm not sure off the top of my head how to accomplish that because of the generics.

I was mainly concerned with us re-parsing arguments in work in a lot of places. I wonder if removing the args parameter in work would help to enforce that we're doing argument parsing in init.

Argument parsing happens in work, though. In work, we parse the arguments for each work request.

In init we parse args for the worker as a whole. I think the only place we have a valid use case for this is the Zinc incremental compilation stuff, which I'd like to remove. Beyond that, I don't see a good reason to keep init around.

In another PR I can remove the incremental compilation piece and we should be able to remove init.

Ah, I was conflating command-line arguments vs. the arguments supplied with each worker task. Carry on.

…traction These could/should probably be two separate commits, but the improvement to argument parsing was inspired by the requirements of multiplex sandboxing. The work was so intertwined that I'm leaving it as one PR. Good news is that if we need to move to another parsing framework in the future, it should be much easier because parsing is no longer spread throughout the work functions.

This is useful for when workers fail to run and have an error message to help with debugging. Otherwise you just have empty output.

jjudd requested review from gregghz and jadenPete August 26, 2024 19:52

jjudd force-pushed the multiplex-sandbox branch 2 times, most recently from 0c9b83d to e3e76db Compare August 26, 2024 20:04

jadenPete approved these changes Aug 26, 2024

View reviewed changes

jjudd added 8 commits August 26, 2024 16:43

Support more worker types for depscheck

e65caef

Disable cache and remote when doing incremental compilation

1b9afd1

Print AnnexWorkerErrors when they have messages or causes

e438833

This is useful for when workers fail to run and have an error message to help with debugging. Otherwise you just have empty output.

Support multiplex sandboxing for scalafmt

6c5c350

Support more worker types for scala_proto

3faf1c2

Support more worker types for scaladoc

5e5b0ee

Support more worker types for jacoco

80843d2

jjudd force-pushed the multiplex-sandbox branch from e3e76db to 80843d2 Compare August 26, 2024 23:52

jjudd merged commit fee073f into lucid-master Aug 27, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds multiplex sandbox support and improves argument parsing #52

Adds multiplex sandbox support and improves argument parsing #52

jjudd commented Aug 26, 2024 •

edited

Loading

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024 •

edited

Loading

jjudd Aug 26, 2024

jadenPete Aug 27, 2024

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024 •

edited

Loading

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024

jjudd Aug 26, 2024

jadenPete Aug 26, 2024

		@@ -13,11 +14,14 @@ trait WorkerMain[S] {

		protected[this] def init(args: Option[Array[String]]): S

Adds multiplex sandbox support and improves argument parsing #52

Adds multiplex sandbox support and improves argument parsing #52

Conversation

jjudd commented Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadenPete Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadenPete Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjudd commented Aug 26, 2024 •

edited

Loading

jadenPete Aug 26, 2024 •

edited

Loading

jadenPete Aug 26, 2024 •

edited

Loading