Persistent/worker scala compiler #91

ianoc · 2016-09-13T05:00:53Z

Adds support for the persistant/worker Scala compiler. All the code for doing the protobufs is just lifted from the bazel example/tests for this. Thanks to @johnynek we've added in all the features via this route to get the build passing.

commit d3ed208 Merge: 61b3b57 cd24529 Author: Ian O'Connell <[email protected]> Date: Mon Sep 12 21:21:53 2016 -0700 Merge pull request #3 from ianoc/oscar/worker-resources Add resource support, minor cleanups commit cd24529 Author: Oscar Boykin <[email protected]> Date: Mon Sep 12 18:05:01 2016 -1000 Add resource support, minor cleanups commit 61b3b57 Merge: 9bf9087 d0d6658 Author: P. Oscar Boykin <[email protected]> Date: Mon Sep 12 16:53:40 2016 -1000 Merge pull request #2 from ianoc/oscar/javac_scala_worker Add javac support for mixed targets commit d0d6658 Author: Oscar Boykin <[email protected]> Date: Mon Sep 12 14:29:18 2016 -1000 Add javac support for mixed targets commit 9bf9087 Merge: 617885e 30b0efc Author: Ian O'Connell <[email protected]> Date: Mon Sep 12 16:25:56 2016 -0700 Merge pull request #1 from ianoc/oscar/resident-compiler Factor out ScalaCInvoker option parsing commit 30b0efc Author: Oscar Boykin <[email protected]> Date: Mon Sep 12 13:22:56 2016 -1000 address review commit b28dcd1 Author: Oscar Boykin <[email protected]> Date: Mon Sep 12 12:35:14 2016 -1000 Factor out ScalaCInvoker option parsing commit 617885e Author: Ian O Connell <[email protected]> Date: Sun Sep 11 21:43:59 2016 -0700 Adding commit 821ab21 Author: Ian O Connell <[email protected]> Date: Sun Sep 11 19:47:11 2016 -0700 wip commit f6bebe6 Author: Ian O Connell <[email protected]> Date: Sun Sep 11 19:38:41 2016 -0700 wip commit 4ab6810 Author: Ian O Connell <[email protected]> Date: Sun Sep 11 19:33:02 2016 -0700 wip commit cdc4995 Author: Ian O Connell <[email protected]> Date: Sun Sep 11 17:55:40 2016 -0700 wip commit 51b9251 Author: Ian O Connell <[email protected]> Date: Sun Sep 11 17:45:45 2016 -0700 WIP commit e4b53aa Author: Ian O Connell <[email protected]> Date: Sun Sep 11 17:29:09 2016 -0700 WIP commit 1cc779c Author: Ian O Connell <[email protected]> Date: Sun Sep 11 17:16:05 2016 -0700 Wip commit 1f9600c Author: Ian O Connell <[email protected]> Date: Sun Sep 11 14:13:33 2016 -0700 WIP commit d2dce89 Author: Ian O Connell <[email protected]> Date: Sun Sep 11 14:11:14 2016 -0700 Undo commit ec05677 Author: Ian O Connell <[email protected]> Date: Sun Sep 11 14:06:17 2016 -0700 WIP commit dcf82b4 Author: Ian O Connell <[email protected]> Date: Sun Sep 11 12:19:19 2016 -0700 Some formatting so my editor is less cranky commit f11926a Author: Ian O Connell <[email protected]> Date: Sun Sep 11 12:12:29 2016 -0700 WIP -- have the jar creator code inlined commit 0a1ad35 Author: Ian O Connell <[email protected]> Date: Sat Sep 10 21:01:44 2016 -0700 wip commit 3a0c1ab Author: Ian O Connell <[email protected]> Date: Sat Sep 10 20:58:10 2016 -0700 WIP commit 15d02b1 Author: Ian O Connell <[email protected]> Date: Sat Sep 10 18:10:28 2016 -0700 Make deploy jar name easier to grep for commit d3823aa Author: Ian O Connell <[email protected]> Date: Tue Aug 9 11:40:40 2016 -0700 Use bind's for twitter scrooge so local repo's can override scrooge versions commit f11e814 Merge: f82800b 4146c9e Author: Ian O Connell <[email protected]> Date: Thu Jul 28 16:28:02 2016 -0700 Merge branch 'master' of github.com:bazelbuild/rules_scala commit f82800b Author: Ian O Connell <[email protected]> Date: Thu Jul 28 15:32:14 2016 -0700 Fix repl test commit 9c3f37c Author: Ian O Connell <[email protected]> Date: Thu Jul 28 15:30:46 2016 -0700 Update test rules

bazel-io · 2016-09-13T05:00:54Z

Can one of the admins verify this patch?

ianoc · 2016-09-13T05:02:47Z

This points at the bazel repo itself at a sha to get the protobuf code for the worker. Took this route following the changes over at https://github.com/bazelbuild/dash/pull/13 . Should we be trying to use bazel tools here or have the WORKSPACE's have a bazel checkout?

ittaiz · 2016-09-13T05:45:50Z

Super exciting! @ianoc do you have any rough numbers on how this impacts build times?

ianoc · 2016-09-13T05:48:34Z

Numbers are as you might imagine a bit all over the place depending on the nature/shape of your build targets (how many files and so on). But in a reasonable collection of targets it was 2.5mins -> 1min. On one target thats got some macros in it (its just 2 scala files...) 15sec -> 9sec -> ... -> 2.8sec. [Several timings i did over changing some code to re-run it, so the jit would get fully warmed and it went down over a few builds to the 2.8 sec range]

johnynek · 2016-09-13T06:34:26Z

scala/scala.bzl


 _implicit_deps = {
  "_ijar": attr.label(executable=True, default=Label("@bazel_tools//tools/jdk:ijar"), single_file=True, allow_files=True),
  "_scala": attr.label(executable=True, default=Label("@scala//:bin/scala"), single_file=True, allow_files=True),
-  "_scalac": attr.label(executable=True, default=Label("@scala//:bin/scalac"), single_file=True, allow_files=True),
+  "_scalac": attr.label(cfg=HOST_CFG, executable=True, default=Label("//src/java/io/bazel/rulesscala/scalac"), allow_files=True),


I don't know what HOST_CFG does in this context. I know that it means use the host configuration but what does that mean in this context? Can you add a comment?

I don't either ¯_(ツ)_/¯ ! Lifted from example when i was trying to make this work. Happy to drop if it all still runs. I'll check

Well, I don't know if you noticed, but we have been compiling targets
twice. Bazel does this for the host and target compilations. I don't see
why we need to do that with JVM code like this. I wonder if this flag (in
the right spot) could change that behavior.
On Tue, Sep 13, 2016 at 06:22 ianoc [email protected] wrote:

In scala/scala.bzl
#91 (comment):

_implicit_deps = {
"_ijar": attr.label(executable=True, default=Label("@bazel_tools//tools/jdk:ijar"), single_file=True, allow_files=True),
"_scala": attr.label(executable=True, default=Label("@scala//:bin/scala"), single_file=True, allow_files=True),

"_scalac": attr.label(executable=True, default=Label("@scala//:bin/scalac"), single_file=True, allow_files=True),

"_scalac": attr.label(cfg=HOST_CFG, executable=True, default=Label("//src/java/io/bazel/rulesscala/scalac"), allow_files=True),

I don't either ¯_(ツ)_/¯ ! Lifted from example when i was trying to make
this work. Happy to drop if it all still runs. I'll check

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/rules_scala/pull/91/files/1b42b87a93989fa22db6657ce861a33c06b3eb03#r78593996,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEJdt-9PlCp4-1NFCORlwZMGzzqx19xks5qps23gaJpZM4J7Sy_
.

Do you mean we should want to add or remove? I've seen us compiling targets on different actions again, but haven't noticed us just doing the same thing twice over. But i'd believe it..

johnynek · 2016-09-13T06:54:54Z

Can we break this into two PRs? One to restyle the python/bzl and then a second to add the worker? It's a bit hard to review with such an important change along with the restyling.

ianoc · 2016-09-13T16:11:20Z

Does using https://github.com/bazelbuild/rules_scala/pull/91/files?w=1 clean it up enough or still rather the 2 prs?

johnynek · 2016-09-13T17:03:15Z

scala/scala.bzl

+    all_srcjars = set(srcjars + list(dep_srcjars))
+    # look for any plugins:
+    plugins = _collect_plugin_paths(ctx.attr.plugins)
+    plugin_arg = ",".join(list(plugins))


I'm a little worried about this. Are we sure plugins can't have , in them? This whole array encoding with , is worrisome. I wish there was a standard method to escape a set of characters and then unescape them in another language.

comma's are relatively common, so wouldn't surprise me if its in some one of their args :/. one option would be to json encode the list itself i suppose?

johnynek · 2016-09-13T17:09:58Z

scala/scala.bzl

+Classpath: {cp}
+Files: {files}
+EnableIjar: {enableijar}
+ijarOutput: {ijar_out}


seems like maybe we should make this IjarOutput to be consistent, right? Same below.

Also, can we sort this list since order does not matter?

johnynek · 2016-09-13T17:13:29Z

scala/scala.bzl

+        mnemonic="Scalac",
+        progress_message="scala %s" % ctx.label,
+        execution_requirements={"supports-workers": "1"},
+        arguments=list(ctx.attr.jvm_flags) + ["@" + argfile.path],


can we add a comment here like:

when we run with a worker, the `@argfile.path` is removed and passed line by line as arguments in the protobuf. In that case, the rest of the arguments are passed to the process that starts up and stays resident. In either case (worker or not), they will be jvm flags which will be correctly handled since the executable is a jvm app that will consume the flags on startup.

This confused me a bit, and I may forget why it works.

johnynek · 2016-09-13T17:14:48Z

scala/scala.bzl

-  rjars += [ctx.outputs.jar]
-  rjars += _collect_jars(ctx.attr.runtime_deps).runtime
+def _collect_jars(targets):
+    """Compute the runtime and compile-time dependencies from the given targets"""  # noqa


what's with the noqa?

Stops linters caring about lines

johnynek · 2016-09-13T17:22:18Z

src/java/io/bazel/rulesscala/scalac/ScalaCInvoker.java

+import static java.nio.charset.StandardCharsets.UTF_8;
+
+/**
+ * A class for creating Jar files. Allows normalization of Jar entries by setting their timestamp to


I don't think this comment is up to date.

johnynek · 2016-09-13T17:34:21Z

okay, looks great. I'm going to run a quick test on a couple of repos and if they are green, I say we ship it. This is exciting.

johnynek · 2016-09-13T18:19:21Z

Only thing missing, in my opinion, is to update the README to explain:

you need bazel 0.3.1 or later.
you need to add bazel to your WORKSPACE as well
how to run with persistent worker (--strategy=Scalac=worker), and how to add that to you .bazelrc to get it automatically for build and test.

This is really great.

ianoc · 2016-09-13T18:36:32Z

updated the readme

johnynek · 2016-09-13T18:37:42Z

README.md

+```python
+--strategy=Scalac=worker
+```
+to your command line, or to enable by default for building/testing add it to your .bazelrc.


to do this do you need to make lines like:

build --strategy=Scalac=worker test --strategy=Scalac=worker

or can you just add --strategy=Scalac=worker?

uhhh, i could be totally wrong, so lets just put that in

johnynek · 2016-09-13T18:56:01Z

@philwo you might want to take a look. I'm going to merge this. It seems 2-6x faster depending on the target size (big wins for really small targets). Thanks for the work on this!

I would say it is a real bummer to have to check out all of bazel to get an easy to access java jar to use here (the generated from protobuf). Would be nice to have a smaller download (maybe a stand-alone protobuf + java library that lives outside of bazel and that bazel can depend on).

johnynek · 2016-09-14T18:57:32Z

@philwo one note: it would be nice if we could consume a json stream, not a protobuf, which could have a lighter weight coupling between workers and bazel. Then we would not need to depend on the protobufs in the bazel repo and have this giant checkout. I guess we could copy the file, but that also seems hacky.

philwo · 2016-09-15T11:37:10Z

@ianoc @johnynek Thank you so much for this contribution & merging it! This is a really big win for Bazel.

I agree that the protobuf being hidden in the Bazel repository is annoying in a case like this. I'll figure something out.

* upstream/master: use strings rather than HOST/DATA_CFG Add cfg attribute on executable labels Switch to using shorter stack traces. Catch throwables from type errors make thrift targets quieter (bazelbuild#94) Fix jvm_flag support (bazelbuild#93) Make compile timing optional, with default false Update README.md Persistent/worker scala compiler (bazelbuild#91) fix typo in README.md (bazelbuild#89) Fix for thrift rule issue that manifests with remote repos (bazelbuild#83) improve the deploy jar creation (bazelbuild#85) Use bind's for twitter scrooge so local repo's can override scrooge and thrift versions (bazelbuild#84)

ianoc-stripe added 2 commits September 12, 2016 21:28

Handle passing through jvm options correctly, fix up merges from master

2aaff85

googlebot added the cla: yes label Sep 13, 2016

ianoc-stripe added 3 commits September 12, 2016 22:03

Set bazel version to a tag

ac31676

Bump bazel version

b763cde

Remove cat

1b42b87