Add Hocon toolchain, worker support, update rules_scala_annex and Bazel#34
Add Hocon toolchain, worker support, update rules_scala_annex and Bazel#34jjudd wants to merge 1 commit into
Conversation
jjudd
force-pushed
the
jjudd-annex-toolchain-update
branch
from
93f5471 to
f4bcb00
Compare
| import higherkindness.rules_scala.common.worker.{WorkTask, WorkerMain} | ||
| import java.io.{File, FileWriter, PrintStream} | ||
| import java.nio.file.{Files, Path, Paths} | ||
| import org.rogach.scallop.* |
There was a problem hiding this comment.
We use argparse4j in rules_scala_annex, scopt in our private monorepo, and scallop here. Out of curiosity, why'd you opt for that framework instead of argparse4j or scopt?
There was a problem hiding this comment.
Scallop was already used by the existing code. I didn't change that bit. This diff makes it look like this file is entirely new, but it's about 80% copied from the old implementation.
| override def init(args: Option[Array[String]]): Unit = () | ||
|
|
||
| protected def work(task: WorkTask[Unit]): Unit = { | ||
| // WorkerMain does not expand param files, so we do it here. |
There was a problem hiding this comment.
You don't have to do this in this PR, but we should really consider doing this automatically in WorkerMain.
There was a problem hiding this comment.
Yeah. It's something we do often enough it'd be nice if it did it automagically. Not something I want to do as part of this PR, though.
|
|
||
| writeConfig(finalConfig, renderOptions, opts.output(), opts.header) | ||
| } catch { | ||
| case NonFatal(e) => |
There was a problem hiding this comment.
I believe WorkerMain already catches exceptions in work, so this can be removed.
There was a problem hiding this comment.
I don't think there's any harm in including it, though. If you haven't read through WorkerMain or your'e an LLM that doesn't know about it, then it would likely look like an error to throw uncaught exceptions.
| val builder = Set.newBuilder[String] | ||
| val source = scala.io.Source.fromFile(file) | ||
| try { | ||
| for (line <- source.getLines()) { |
There was a problem hiding this comment.
Can you not just do source.getLines().toSet?
There was a problem hiding this comment.
I understand you copied this from the old Compiler, so feel free to ignore this comment if it's out of scope.
There was a problem hiding this comment.
I can try and see if it causes any trouble.
tmccombs
left a comment
tmccombs
left a comment
There was a problem hiding this comment.
I don't understand all of the bazel stuff, but it looks ok to me
| load(":create-toolchain.bzl", "create_hocon_toolchain") | ||
|
|
||
| toolchain_type( | ||
| name = "toolchain_type", |
There was a problem hiding this comment.
I'm not very familiar with bazel toolchains, but this seems like an odd name to me.
| """Defines and configures a Hocon toolchain. | ||
|
|
||
| Args: | ||
| name: Name of the generated `toolchain` target. Register it with `register_toolchains`. |
| "@hocon_maven//:org_scala_lang_scala_compiler", | ||
| "@hocon_maven//:org_scala_lang_scala_library", | ||
| "@hocon_maven//:org_scala_lang_scala_reflect", | ||
| compiler_classpath_3 = [ |
There was a problem hiding this comment.
is it really worht adding the "_3" suffixes to these?
|
|
||
| _scalafmt() { | ||
| find hocon-compiler -name '*.scala' -exec $scalafmtbin --config "$PWD/.scalafmt.conf" "$PWD/{}" "$PWD/{}" \; | ||
| find hocon-compiler-cli -name '*.scala' -exec $scalafmtbin --config "$PWD/.scalafmt.conf" "$PWD/{}" "$PWD/{}" \; |
There was a problem hiding this comment.
Is there a significance of the "-cli" suffix?
| output = header_file, | ||
| content = ctx.attr.header, | ||
| ) | ||
| args.add("-h", header_file) |
There was a problem hiding this comment.
nit: it might be better to use a different option for this. lik --header or -H, sicne -h is conventionally used for printing help info.
| val base = opt[File]().map(SandboxUtil.getSandboxFile(workDir, _)) | ||
| val output = opt[File](required = true).map(SandboxUtil.getSandboxFile(workDir, _)) | ||
| val include = opt[List[File]](default = Some(Nil)).map(_.map(SandboxUtil.getSandboxFile(workDir, _))) | ||
| val envKeyLists = opt[List[File]]( | ||
| "env-key-lists", | ||
| default = Some(Nil), | ||
| ).map(_.map(SandboxUtil.getSandboxFile(workDir, _))) |
There was a problem hiding this comment.
Would it be worth making a local helper:
val inSandbox = (file: File) => SandboxUtil.getSandboxFile(workDir, file)so all these could be written as
_.map(inSandbox)?
| // top-level command, so render the full help from the builder (as Scallop's default does). | ||
| val helpText = | ||
| if (subcommand.isEmpty) builder.getFullHelpString() | ||
| else builder.findSubbuilder(subcommand).get.getFullHelpString() |
There was a problem hiding this comment.
do we have any subcommands?
| val includes = opts.include() | ||
| val configParser = new ConfigParser(includes, opts.optionalInclude()) | ||
| val baseConfig = opts.base.toOption.map(configParser.parse) | ||
| val mainConfig = configParser.parse(opts.src()) |
There was a problem hiding this comment.
is it worth doing a throwIfInterrupted between parsing and merging?
|
|
||
| set -euo pipefail | ||
|
|
||
| cd "$(bazel info workspace 2> /dev/null)" |
There was a problem hiding this comment.
is this actually necessary, since we run the test using an absolute label?
| ) | ||
|
|
||
| default_java_toolchain( | ||
| name = "repository_default_toolchain_21", |
There was a problem hiding this comment.
Could we upgrade to 25?
maybe in a separate PR?
3 participants
No description provided.