Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds multiplex sandbox support and improves argument parsing #52

Merged
merged 8 commits into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .bazelrc_shared
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ build --tool_java_runtime_version="remotejdk_21"
build --strategy=ScalaCompile=worker
build --worker_max_instances=4
build --worker_sandboxing
build --experimental_worker_multiplex_sandboxing
build --verbose_failures

test --test_output=all
Expand Down
9 changes: 8 additions & 1 deletion rules/private/phases/phase_coverage_jacoco.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,14 @@ def phase_coverage_jacoco(ctx, g):
outputs = [in_out_pair[1] for in_out_pair in in_out_pairs],
executable = code_coverage_configuration.instrumentation_worker.files_to_run.executable,
input_manifests = worker_input_manifests,
execution_requirements = _resolve_execution_reqs(ctx, {"supports-workers": "1"}),
execution_requirements = _resolve_execution_reqs(
ctx,
{
"supports-multiplex-workers": "1",
"supports-workers": "1",
"supports-multiplex-sandboxing": "1",
},
),
arguments = [args],
)

Expand Down
10 changes: 7 additions & 3 deletions rules/private/phases/phase_zinc_compile.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,17 @@ def phase_zinc_compile(ctx, g):
execution_requirements_tags = {
"supports-multiplex-workers": "1",
"supports-workers": "1",
"supports-multiplex-sandboxing": "1",
}

# Disable sandboxing if incremental compilation features are going to be used
# because they require stashing files outside the sandbox that Bazel isn't
# aware of.
# Disable several things if incremental compilation features are going to be used
# because incremental compilation require stashing files outside the sandbox that
# Bazel isn't aware of and is less deterministic than ideal.
if zinc_configuration.incremental:
execution_requirements_tags["no-sandbox"] = "1"
execution_requirements_tags["no-cache"] = "1"
execution_requirements_tags["no-remote"] = "1"
execution_requirements_tags["supports-multiplex-sandboxing"] = "0"

# todo: different execution path for nosrc jar?
ctx.actions.run(
Expand Down
9 changes: 8 additions & 1 deletion rules/private/phases/phase_zinc_depscheck.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,14 @@ def phase_zinc_depscheck(ctx, g):
outputs = [deps_check],
executable = deps_configuration.worker.files_to_run.executable,
input_manifests = worker_input_manifests,
execution_requirements = _resolve_execution_reqs(ctx, {"supports-workers": "1"}),
execution_requirements = _resolve_execution_reqs(
ctx,
{
"supports-multiplex-workers": "1",
"supports-workers": "1",
"supports-multiplex-sandboxing": "1",
},
),
arguments = [deps_args],
)
deps_checks[name] = deps_check
Expand Down
9 changes: 8 additions & 1 deletion rules/scala/private/doc.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,14 @@ def scaladoc_implementation(ctx):
ctx.actions.run(
arguments = [args],
executable = ctx.attr._runner.files_to_run.executable,
execution_requirements = _resolve_execution_reqs(ctx, {"supports-workers": "1"}),
execution_requirements = _resolve_execution_reqs(
ctx,
{
"supports-multiplex-workers": "1",
"supports-workers": "1",
"supports-multiplex-sandboxing": "1",
},
),
input_manifests = input_manifests,
inputs = depset(
src_jars + srcs + [zinc_configuration.compiler_bridge],
Expand Down
1 change: 1 addition & 0 deletions rules/scala_proto/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ scala_binary(
deps = [
"//src/main/scala/higherkindness/rules_scala/common/args",
"//src/main/scala/higherkindness/rules_scala/common/error",
"//src/main/scala/higherkindness/rules_scala/common/sandbox",
"//src/main/scala/higherkindness/rules_scala/common/worker",
"@annex//:net_sourceforge_argparse4j_argparse4j",
"@annex_proto//:com_github_os72_protoc_jar",
Expand Down
45 changes: 32 additions & 13 deletions rules/scala_proto/private/ScalaProtoWorker.scala
Original file line number Diff line number Diff line change
@@ -1,31 +1,52 @@
package annex.scala.proto

import higherkindness.rules_scala.common.args.ArgsUtil
import higherkindness.rules_scala.common.args.ArgsUtil.PathArgumentType
import higherkindness.rules_scala.common.args.implicits._
import higherkindness.rules_scala.common.error.AnnexWorkerError
import higherkindness.rules_scala.common.sandbox.SandboxUtil
import higherkindness.rules_scala.common.worker.WorkerMain
import java.io.{File, PrintStream}
import java.nio.file.{Files, Paths}
import java.nio.file.{Files, Path, Paths}
import java.util.Collections
import net.sourceforge.argparse4j.ArgumentParsers
import net.sourceforge.argparse4j.impl.Arguments
import net.sourceforge.argparse4j.inf.ArgumentParser
import net.sourceforge.argparse4j.inf.{ArgumentParser, Namespace}
import protocbridge.{ProtocBridge, ProtocRunner}
import scala.jdk.CollectionConverters._
import scalapb.ScalaPbCodeGenerator

object ScalaProtoWorker extends WorkerMain[Unit] {

private class ScalaProtoRequest private (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@jadenPete jadenPete Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

val isGrpc: Boolean,
val outputDir: Path,
val protoPaths: List[Path],
val sources: List[Path],
)

private object ScalaProtoRequest {
def apply(workDir: Path, namespace: Namespace): ScalaProtoRequest = {
new ScalaProtoRequest(
isGrpc = namespace.getBoolean("grpc"),
outputDir = SandboxUtil.getSandboxPath(workDir, namespace.get[Path]("output_dir")),
protoPaths = SandboxUtil.getSandboxPaths(workDir, namespace.getList[Path]("proto_paths")),
sources = SandboxUtil.getSandboxPaths(workDir, namespace.getList[Path]("sources")),
)
}
}

private val argParser: ArgumentParser = {
val parser = ArgumentParsers.newFor("proto").addHelp(true).fromFilePrefix("@").build
parser
.addArgument("--output_dir")
.help("Output dir")
.metavar("output_dir")
.`type`(Arguments.fileType.verifyCanCreate)
.`type`(PathArgumentType.apply())
parser
.addArgument("--proto_paths")
.nargs("*")
.`type`(Arguments.fileType.verifyIsDirectory)
.`type`(PathArgumentType.apply())
.setDefault_(Collections.emptyList)
parser
.addArgument("--grpc")
Expand All @@ -35,28 +56,26 @@ object ScalaProtoWorker extends WorkerMain[Unit] {
.help("Source files")
.metavar("source")
.nargs("*")
.`type`(Arguments.fileType.verifyCanRead.verifyIsFile)
.`type`(PathArgumentType.apply())
.setDefault_(Collections.emptyList)
parser
}

override def init(args: Option[Array[String]]): Unit = ()

protected def work(ctx: Unit, args: Array[String], out: PrintStream): Unit = {
val namespace = argParser.parseArgs(args)
val sources = namespace.getList[File]("sources").asScala.toList
val protoPaths = namespace.getList[File]("proto_paths").asScala.toList
protected def work(ctx: Unit, args: Array[String], out: PrintStream, workDir: Path): Unit = {
val workRequest = ScalaProtoRequest(workDir, ArgsUtil.parseArgsOrFailSafe(args, argParser, out))

val scalaOut = namespace.get[File]("output_dir").toPath
val scalaOut = workRequest.outputDir
Files.createDirectories(scalaOut)
val outOptions = if (namespace.getBoolean("grpc")) {
val outOptions = if (workRequest.isGrpc) {
"grpc:"
} else {
""
}
val params = List(s"--scala_out=${outOptions}${scalaOut}")
::: protoPaths.map(dir => s"--proto_path=${dir.toString}")
::: sources.map(_.getPath.toString)
::: workRequest.protoPaths.map(dir => s"--proto_path=${dir.toString}")
::: workRequest.sources.map(_.toString)

class MyProtocRunner[ExitCode] extends ProtocRunner[Int] {
def run(args: Seq[String], extraEnv: Seq[(String, String)]): Int = {
Expand Down
9 changes: 8 additions & 1 deletion rules/scala_proto/private/core.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,14 @@ def scala_proto_library_implementation(ctx):
tools = compiler_inputs,
input_manifests = input_manifests,
progress_message = "Compiling %{label} protobuf into Scala source",
execution_requirements = _resolve_execution_reqs(ctx, {"supports-workers": supports_workers}),
execution_requirements = _resolve_execution_reqs(
ctx,
{
"supports-multiplex-workers": supports_workers,
"supports-workers": supports_workers,
"supports-multiplex-sandboxing": supports_workers,
},
),
arguments = [args],
)

Expand Down
1 change: 1 addition & 0 deletions rules/scalafmt/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ scala_binary(
scala = "//src/main/scala:bootstrap",
visibility = ["//visibility:public"],
deps = [
"//src/main/scala/higherkindness/rules_scala/common/sandbox",
"//src/main/scala/higherkindness/rules_scala/common/worker",
"//src/main/scala/higherkindness/rules_scala/workers/common",
"@annex//:net_sourceforge_argparse4j_argparse4j",
Expand Down
9 changes: 8 additions & 1 deletion rules/scalafmt/private/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,14 @@ def build_format(ctx):
input_manifests = runner_manifests,
inputs = [ctx.file.config, src],
tools = runner_inputs,
execution_requirements = _resolve_execution_reqs(ctx, {"supports-multiplex-workers": "1", "supports-workers": "1"}),
execution_requirements = _resolve_execution_reqs(
ctx,
{
"supports-multiplex-workers": "1",
"supports-workers": "1",
"supports-multiplex-sandboxing": "1",
},
),
mnemonic = "ScalaFmt",
)
manifest_content.append("{} {}".format(src.short_path, file.short_path))
Expand Down
42 changes: 32 additions & 10 deletions rules/scalafmt/scalafmt/ScalafmtRunner.scala
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
package annex.scalafmt

import higherkindness.rules_scala.common.args.ArgsUtil
import higherkindness.rules_scala.common.args.ArgsUtil.PathArgumentType
import higherkindness.rules_scala.common.sandbox.SandboxUtil
import higherkindness.rules_scala.common.worker.WorkerMain
import higherkindness.rules_scala.workers.common.Color
import java.io.{File, PrintStream}
import java.nio.file.Files
import java.nio.file.{Files, Path}
import net.sourceforge.argparse4j.ArgumentParsers
import net.sourceforge.argparse4j.impl.Arguments
import net.sourceforge.argparse4j.inf.{ArgumentParser, Namespace}
import org.scalafmt.Scalafmt
import org.scalafmt.config.ScalafmtConfig
import org.scalafmt.sysops.FileOps
Expand All @@ -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 (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@jadenPete jadenPete Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

val configFile: Path,
val inputFile: Path,
val outputFile: Path,
)

protected[this] def work(worker: Unit, args: Array[String], out: PrintStream): Unit = {
private[this] object ScalafmtRequest {
def apply(workDir: Path, namespace: Namespace): ScalafmtRequest = {
new ScalafmtRequest(
configFile = SandboxUtil.getSandboxPath(workDir, namespace.get[Path]("config")),
inputFile = SandboxUtil.getSandboxPath(workDir, namespace.get[Path]("input")),
outputFile = SandboxUtil.getSandboxPath(workDir, namespace.get[Path]("output")),
)
}
}

private[this] val argParser: ArgumentParser = {
val parser = ArgumentParsers.newFor("scalafmt").addHelp(true).defaultFormatWidth(80).fromFilePrefix("@").build
parser.addArgument("--config").required(true).`type`(Arguments.fileType)
parser.addArgument("input").`type`(Arguments.fileType)
parser.addArgument("output").`type`(Arguments.fileType)
parser.addArgument("--config").required(true).`type`(PathArgumentType.apply())
parser.addArgument("input").`type`(PathArgumentType.apply())
parser.addArgument("output").`type`(PathArgumentType.apply())
parser
}

protected[this] def init(args: Option[Array[String]]): Unit = {}

val namespace = parser.parseArgsOrFail(args)
protected[this] def work(worker: Unit, args: Array[String], out: PrintStream, workDir: Path): Unit = {
val workRequest = ScalafmtRequest(workDir, ArgsUtil.parseArgsOrFailSafe(args, argParser, out))

val source = FileOps.readFile(namespace.get[File]("input").toPath())(Codec.UTF8)
val source = FileOps.readFile(workRequest.inputFile)(Codec.UTF8)

val config = ScalafmtConfig.fromHoconFile(namespace.get[File]("config").toPath()).get
val config = ScalafmtConfig.fromHoconFile(workRequest.configFile).get
@tailrec
def format(code: String): String = {
val formatted = Scalafmt.format(code, config).get
Expand All @@ -50,7 +72,7 @@ object ScalafmtRunner extends WorkerMain[Unit] {
}
}

Files.write(namespace.get[File]("output").toPath, output.getBytes)
Files.write(workRequest.outputFile, output.getBytes)
}

}
15 changes: 9 additions & 6 deletions src/main/scala/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,9 @@ configure_zinc_scala(
# IntelliJ libraries, so we can get a Scala SDK on sync.
scala_library(
name = "scala-sdk",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain a bit more why this is needed? I have a couple questions:

  • Does IntelliJ look in the classpath of generated JARs to find a Scala SDK upon sync?
    • If so, why do we need a bogus target like this when virtually every Scala target will have the standard library bundled with it?
  • Why didn't we have a Scala 3 SDK before?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't have a Scala 3 SDK before because I forgot to add it.

The IntelliJ Scala + Bazel integration could use some improvements. Happy to chat more about this offline because I've got a fork of it that would work well with your toolchain work.

deps_used_whitelist = [
"@annex//:org_scala_lang_scala3_library_3",
],
deps_used_whitelist = compiler_classpath_2_13,
scala = ":zinc_3",
deps = [
"@annex//:org_scala_lang_scala3_library_3",
],
deps = compiler_classpath_2_13,
)

compiler_classpath_3 = [
Expand All @@ -91,6 +87,13 @@ runtime_classpath_3 = [
"@annex//:org_scala_lang_tasty_core_3",
]

scala_library(
name = "scala-sdk-3",
deps_used_whitelist = compiler_classpath_3,
scala = ":zinc_3",
deps = compiler_classpath_3,
)

configure_bootstrap_scala(
name = "bootstrap_3",
compiler_classpath = compiler_classpath_3,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package higherkindness.rules_scala

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can we merge these two package statements into one? I always thought multiple package statements was a bad Scala feature.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done like this everywhere else in the codebase. I don't support it, but this is consistent with everywhere else. If we want to change it, I'm open to it, but let's keep consistency everywhere.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize it was done like this elsewhere. Let's keep it for now, then.

package common.args

import common.error.AnnexWorkerError
import java.io.{PrintStream, PrintWriter}
import java.nio.file.{Path, Paths}
import net.sourceforge.argparse4j.helper.HelpScreenException
import net.sourceforge.argparse4j.inf.{Argument, ArgumentParser, ArgumentParserException, ArgumentType, Namespace}
import scala.language.reflectiveCalls

object ArgsUtil {

/**
* Safely handles expected errors that pop up during parsing arguments. The argparse4j calls System.exit when things
* go wrong, and we don't want the worker to exit. Instead we exit the work request and print to the output stream.
*/
def parseArgsOrFailSafe(args: Array[String], parser: ArgumentParser, out: PrintStream): Namespace = {
try {
parser.parseArgs(args)
} catch {
case e: HelpScreenException =>
parser.handleError(e, new PrintWriter(out))
throw new AnnexWorkerError(0)
case e: ArgumentParserException =>
parser.handleError(e, new PrintWriter(out))
throw new AnnexWorkerError(1)
}
}

case class PathArgumentType() extends ArgumentType[Path] {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Shouldn't this be an object instead of a case class?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this needs to be a class and that argparse4j needs a new instance of the argument type. At least that's how the file argument type works https://github.com/argparse4j/argparse4j/blob/6c0f8590f7408025daa5f3b234e914cf805808a6/main/src/main/java/net/sourceforge/argparse4j/impl/Arguments.java#L275

override def convert(parser: ArgumentParser, arg: Argument, value: String): Path = {
Paths.get(value)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ scala_library(
scala = "//src/main/scala:bootstrap",
visibility = ["//visibility:public"],
deps = [
"//src/main/scala/higherkindness/rules_scala/common/error",
"@annex//:net_sourceforge_argparse4j_argparse4j",
],
)
Expand Down
Loading
Loading