added scala_toolchain with scalacopts support #364

or-shachar · 2017-12-11T16:16:29Z

WIP for #170
Initial implementation of scala_toolchain
BREAKING change - when merged we must register scala toolchain by either:
A) a call to register_scala_toolchains must happen, just like in rules_go, in order to load default toolchain.
B) defining custom scala_toolchain and registering it

Open questions:

How to test it? given we can't alter the WORKSPACE file of rules_scala. In ideal case - we want to load the toolchain defined in //test_expect_failure/scalacopts_from_toolchain:failing_scala_toolchain.
Is that okay to introduce breaking change for this feature? What is the alternative.

Still need to document

bazel-io · 2017-12-11T16:16:31Z

Can one of the admins verify this patch?

ittaiz · 2017-12-11T17:36:26Z

Test this please

…

On Mon, 11 Dec 2017 at 18:16 bazel.io machine account < ***@***.***> wrote: Can one of the admins verify this patch? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#364 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF6wCpfXk0gaDsuyBJq1YX7Wl4lKAks5s_VVggaJpZM4Q9pYk> .

ittaiz · 2017-12-11T17:47:25Z

How to test it? Easy! The e2e testing lib :) I have a passing test in a stash. I'll show you tomorrow.
Re breaking change- I think it's an option but we'd like to understand if that's the only option.
Additionally I think that if we're making a breaking change we need to understand if every additional feature there will be another breaking change (i.e. Will using scala version from it break everyone again)?
@katre is there a way for a rule to register a default toolchain?

katre

Thanks, this looks good, only one comment.

katre · 2017-12-11T17:57:34Z

scala/BUILD

+
+toolchain(
+    name = 'scala_toolchain',
+      toolchain_type = '@io_bazel_rules_scala//scala:scala_toolchain.bzl',


toolchain_type should be a label to a target (generally a target of rule toolchain_type()), not an extension.

@katre maybe the docs should contain a snippet of my_toolchain_type so that implementors can see an example?

Yes, that's a good idea, I'll add it soon.

thanks! concretely given the lack of docs, any chance for a small snippet here? so the devs can follow up on your comment

sure:

in scala/BUILD, add:

toolchain_type(name = 'toolchain_type')

Then use '@io_bazel_rules_scala//scala:toolchain_type' as the toolchain_type parameter everywhere.

ittaiz · 2017-12-11T18:26:42Z

WORKSPACE

@@ -80,3 +80,6 @@ filegroup(
 )
 """
 )
+
+load("//scala:scala.bzl", "register_scala_toolchains")
+register_scala_toolchains()


@katre is there a way to not mandate this call by every one of our users? like a default toolchain

Unfortunately, no. Bazel needs to know the available toolchains, WORKSPACE is the place for this to happen.

Eventually, when fully recursive workspace loading exists, you could place the register_scala_toolchains() call inside the WORKSPACE for the rules_scala repository, and that would be loaded, but for now it has to be in the root WORKSPACE.

I see.
Two questions:

java_toolchain doesn't mandate this because it's on a different implementation, right?

once such a toolchain is declared can more properties be added with default values so that people won't break?

Correct, java_toolchain doesn't use this mechanism (I'm adding support this month!) Once it does, java toolchains will need to be registered, with at least one being registered by default by Bazel's initialization system.

Yes, you can add further properties to the scala_toolchain. You can either include a default value in the scala_toolchain rule definition (possibly with a warning), or have the rules handle missing properties, whichever makes sense for your rule implementation.

ittaiz · 2017-12-11T18:39:54Z

To summarize the thread from the comments:
We can't have a default toolchain but once we do this breaking change we can add other properties with default values so only break people once.

johnynek · 2017-12-11T18:49:04Z

scala/scala_toolchain.bzl

+scala_toolchain = rule(
+  _scala_toolchain_impl,
+  attrs = {
+    'scalacopts': attr.string_list(),


could we move the scala_library and scala_compiler jars here as well? Since, ultimately, that is how we will be able to switch from 2.11 to 2.12, etc...

Also, we might have default plugins here (since scala native and scala_js use plugins).

ittaiz · 2017-12-11T19:04:00Z

Oscar, I asked Or and Idan to start with a minimal PR to test the waters and to chunk it down (they have ~2 more days to work on it). I definitely agree we want the scala version to be in here and that is one of the big motivations for this work. We want to move very soon to 2.12 and the whole branch thing doesn't scale. Re scala library and scala_compiler_jars- I also thought about it but now I'm not sure about the cons. IIUC if we do the granularity of scala version with those then if I want to have a custom toolchain for custom scalac opts will I need to supply all the jar labels myself? I'd rather just say 2.11.7

…

On Mon, Dec 11, 2017 at 8:49 PM P. Oscar Boykin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In scala/scala_toolchain.bzl <#364 (comment)> : > @@ -0,0 +1,12 @@ +def _scala_toolchain_impl(ctx): + toolchain = platform_common.ToolchainInfo( + scalacopts = ctx.attr.scalacopts, + ) + return [toolchain] + +scala_toolchain = rule( + _scala_toolchain_impl, + attrs = { + 'scalacopts': attr.string_list(), could we move the scala_library and scala_compiler jars here as well? Since, ultimately, that is how we will be able to switch from 2.11 to 2.12, etc... Also, we might have default plugins here (since scala native and scala_js use plugins). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#364 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF4lncNGjUjgzwgxBxQLtRd0MKxb7ks5s_XkhgaJpZM4Q9pYk> .

or-shachar · 2017-12-12T16:54:32Z

@ittaiz @damienmg - I am a thrilled to start using the integration testing framework (https://github.com/bazelbuild/bazel-integration-testing) :-)

I tried to use it to test out scala toolchain. I first wanted to see that I can compile scala easily. I think I got close but I failed to understand how to build the WORKSPACE file within the test. I want to somehow load current codebase of io_bazel_rules_scala as external repository using local_repository annotation. But what do I put under path? Do I need to copy/link files? The build currently fail with message:

ERROR: error loading package '': Encountered error while reading extension file 'scala/scala.bzl': no such package '@io_bazel_rules_scala//scala': /private/var/tmp/_bazel_ors/d956edd8fd2ed13432bc93c4e3b899a9/bazel-sandbox/8545224450491333639/execroot/io_bazel_rules_scala/_tmp/664cbc6d835417a9336252bb55af018e/37b39d51376b3b47b0789c75ea46aada/external/io_bazel_rules_scala must be an existing directory.

johnynek · 2017-12-12T18:35:31Z

test this please

johnynek · 2017-12-12T18:43:16Z

test/toolchain/ScalaToolchainTest.scala

+import org.specs2.mutable.{Before, SpecificationWithJUnit}
+import org.specs2.specification.{BeforeAll, Scope}
+import scala.collection.JavaConverters._
+


I'm a little concerned that we are using specs for core testing here. Prior to this, all of our tests used scalatest and we only tested specs to exercise specifically specs. Can we have one consistent way of testing (which prior to this was scalatest)?

johnynek · 2017-12-12T18:45:46Z

test_run.sh

@@ -208,6 +208,10 @@ test_scala_junit_test_can_fail() {
  action_should_fail test test_expect_failure/scala_junit_test:failing_test
 }

+test_scalaopts_from_scala_toolchain() {


we aren't calling this are we?

Also, I thought the point of using the bazel testing framework was to stop adding things to this giant script. Can we not express this test as something that we can call with bazel test ?

True. I must've missed it when cleaning the test in favor to the integration test framework

or-shachar · 2017-12-13T17:21:31Z

pardon for many commits. We must squash this PR once we decide to merge it.
eventually we failed to utilize the e2e framework as it currently requires too many manual wiring of internal and external dependencies.

@damienmg , @ittaiz - we will list our challenges and open an issue on the integration repository.

The current state is okay but not optimal: the toolchain we load globally emits warnings on unused variables. The scala_library of our test adds another scalacopts that turns warnings to fatal. That's why we expect it to fail. Very implicit, but best I got until now.

Tomorrow I'll try to make the build fail on class-name length, and try to find out if there's any importance to order of scalac options and if so - how to test that I prioritised the opts given in the libraries over the toolchain.

johnynek · 2017-12-14T01:10:59Z

WORKSPACE

@@ -80,3 +80,6 @@ filegroup(
 )
 """
 )
+
+# we load toolchain that fails the build on unused warnings
+register_toolchains("//test_expect_failure/scalacopts_from_toolchain:failing_scala_toolchain")


does this need to be in the WORKSPACE? Is that the only place to register new toolchains?

Is it the case that you can only have 1 version of each toolchain in play in a given repo? Is that how it works?

according to https://docs.bazel.build/versions/master/toolchains.html#overview

When a target requests a toolchain, Bazel checks the list of registered toolchains and creates a dependency from the target to the first matching toolchain it finds. To find a matching toolchain, Bazel does the following:

Looks through the registered toolchains, first from the --experimental_extra_toolchains flag, then from the registered_toolchains calls in the project's WORKSPACE file.

For each registered toolchain, Bazel performs the following checks:

a. Does the toolchain match the requested toolchain type? If not, skip it.

b. Do the toolchain's execution and target constraints match the constraints stated in the project's execution and target platforms? If yes, the toolchain is a match.

Because Bazel always selects the first matching toolchain, order the toolchains by preference if you expect the possibility of multiple matches.

You can register toolchains via a flag, or in the WORKSPACE. First match is selected.

I just think it is going to confuse me and others that the only registered toolchain is on a path with "expect_failure" which just does not seem correct.

I agree and I think this is because we couldn't utilize bazel e2e testing :(
You do understand that this is the toolchain for a test right? It's not what users are supposed to use.
@or-shachar is there one that users are supposed to use?
@johnynek maybe making the comment clearer it's for testing?
@or-shachar why not register it via a command line flag of the specific test?

johnynek · 2017-12-14T01:12:09Z

scala/BUILD

+)
+
+toolchain(
+    name = 'scala_toolchain',


this is confusing to me. Does this string just coincidentally match the load or the item on line 4? Is this a different namespace? Can we possible not have such a name collision to avoid any confusion?

You are absolutely right. I see what we can do to avoid it

It's just a target name, and doesn't need to match anything, and is referenced from the register_toolchains call in the WORKSPACE.

johnynek · 2017-12-14T01:15:15Z

scala/scala.bzl

@@ -955,7 +960,8 @@ scala_library = rule(
  attrs={
      } + _implicit_deps + _common_attrs + library_attrs + _resolve_deps,
  outputs=library_outputs,
-  fragments = ["java"]
+  fragments = ["java"],
+ toolchains = ['@io_bazel_rules_scala//scala:scala_toolchain_type']


weird indentation here.

johnynek · 2017-12-14T01:15:25Z

scala/scala.bzl

@@ -965,7 +971,8 @@ scala_library_for_plugin_bootstrapping = rule(
  implementation=_scala_library_impl,
  attrs= _implicit_deps + library_attrs + _resolve_deps + _common_attrs_for_plugin_bootstrapping,
  outputs=library_outputs,
-  fragments = ["java"]
+  fragments = ["java"],
+ toolchains = ['@io_bazel_rules_scala//scala:scala_toolchain_type']


weird indentation here.

johnynek · 2017-12-14T01:15:32Z

scala/scala.bzl

@@ -975,7 +982,8 @@ scala_macro_library = rule(
      "exports": attr.label_list(allow_files=False),
      } + _implicit_deps + _common_attrs + _resolve_deps,
  outputs= common_outputs,
-  fragments = ["java"]
+  fragments = ["java"],
+ toolchains = ['@io_bazel_rules_scala//scala:scala_toolchain_type']


johnynek · 2017-12-14T01:21:22Z

scala/BUILD

+      visibility = ["//visibility:public"]
+    )
+
+toolchain_type(


this is confusing to me. What is this doing? Is this declaring a new toolchain type?

Peeve: once I noticed this it really annoyed me: why does skylark use side-effecting functions with a name parameter rather than just let bindings:

scala_toolchain_type = toolchain_type(visibility = ["//visibility:public"])

ugh...

anyway.

The syntax is a bit awkward. I agree.

Yes, this declares a new toolchain type, which is currently just a label with no other data associated.

johnynek · 2017-12-14T16:32:56Z

scala/BUILD

+
+#call scala toolchain
+scala_toolchain(
+      name = 'scala_toolchain_impl',


I think this is a collision too. I think we can name this scala_default_toolchain_impl or something to not confused with the toolchain rule impl.

katre · 2017-12-14T17:17:53Z

WORKSPACE

@@ -80,3 +80,6 @@ filegroup(
 )
 """
 )
+
+# we load toolchain that fails the build on unused warnings
+register_toolchains("//test_expect_failure/scalacopts_from_toolchain:failing_scala_toolchain")


You can register toolchains via a flag, or in the WORKSPACE. First match is selected.

katre · 2017-12-14T17:18:30Z

scala/BUILD

+      visibility = ["//visibility:public"]
+    )
+
+toolchain_type(


Yes, this declares a new toolchain type, which is currently just a label with no other data associated.

katre · 2017-12-14T17:19:35Z

scala/BUILD

+)
+
+toolchain(
+    name = 'scala_toolchain',


It's just a target name, and doesn't need to match anything, and is referenced from the register_toolchains call in the WORKSPACE.

katre · 2017-12-14T17:20:57Z

scala/BUILD

+    )
+
+toolchain_type(
+    name = "scala_toolchain_type",


Nit: Just "toolchain_type" is sufficient, since "scala" is already in the package name, and it'll make your toolchain type slightly smaller.

Ie,
@io_bazel_rules_scala//scala:scala_toolchain_type
vs
@io_bazel_rules_scala//scala:toolchain_type

This will also be consistent with the go, rust, cc, and jdk toolchain types.

can we change to just toolchain_type as katre suggests?

johnynek · 2017-12-15T20:16:54Z

@katre question: in scala, there are multiple backends: js, native, jvm and different versions that are incompatible (2.10, 2.11, 2.12, ....)

Are toolchains a way we build for multiple targets? For instance, I want to build for both jvm and js? So, I guess my question is: could I make a macro that builds the same rule with two different toolchains?

Critically, not all scala rules can be built with scalajs because all the transitive dependencies need to be either scalajs or js (you can't depend on a jvm library in scalajs). Similarly for scala-native (it can depend only on pure scala or C libraries). How would you suggest we tackle this issue?

katre · 2017-12-15T20:29:18Z

It depends on how the results would be used. I would probably model the different result types as different platforms: scala_jvm, scala_js, scala_native_x86. Each of these would then have a constraint for the scala compiler backend:

In scala/constraints/BUILD.bazel"

constraint_setting(name = "compiler_backend")
constraint_value(name="compiler_jvm", constraint-setting=":compiler_backend")
constraint_value(name="compiler_js", constraint-setting=":compiler_backend")
constraint_value(name="compiler_native", constraint-setting=":compiler_backend")

In scala/platforms/BUILD.bazel:

platform(name="scala_jvm",
  constraint_values = ["//scala/constraints:compiler_jvm"])
platform(name="scala_js",
  constraint_values = ["//scala/constraints:compiler_js"])
platform(name="scala_native_x86",
  constraint_values = [
    "//scala/constraints:compiler_native",
    "@bazel_tools//platforms:x86_64",
  ])

And, finally, in scala/toolchains/BUILD.bazel:

scala_toolchain(name = "toolchain_jvm",
   other config)
toolchain(name="register_toolchain_jvm",
  toolchain_type = "...:toolchain_type",
  exec_compatible_with = [],
  target_compatible_with = ["//scala/constraints:compiler_jvm"],
  toolchain = ":toolchain_jvm")
# Repeat for JS and native

Then users can select which toolchain by passing "--platforms=@io_bazel_rules_scala//scala/platforms:scala_jvm" or "...//platforms:scala_js".

The rules and toolchain would need logic to tell if a target cannot be built with that toolchain, and to emit an error if the wrong source is given.

If jvm vs js vs native output isn't what the user should be selecting with the --platforms, we can work out what the right conceptual model should be.

ittaiz · 2017-12-15T21:26:09Z

This model doesn't allow cross building right? Being able to build the same graph for scala 2.11 and 2.12 for example

…

On Fri, 15 Dec 2017 at 22:29 katre ***@***.***> wrote: It depends on how the results would be used. I would probably model the different result types as different platforms: scala_jvm, scala_js, scala_native_x86. Each of these would then have a constraint for the scala compiler backend: In scala/constraints/BUILD.bazel" constraint_setting(name = "compiler_backend") constraint_value(name="compiler_jvm", constraint-setting=":compiler_backend") constraint_value(name="compiler_js", constraint-setting=":compiler_backend") constraint_value(name="compiler_native", constraint-setting=":compiler_backend") In scala/platforms/BUILD.bazel: platform(name="scala_jvm", constraint_values = ["//scala/constraints:compiler_jvm"]) platform(name="scala_js", constraint_values = ["//scala/constraints:compiler_js"]) platform(name="scala_native_x86", constraint_values = [ "//scala/constraints:compiler_native", ***@***.***_tools//platforms:x86_64", ]) And, finally, in scala/toolchains/BUILD.bazel: scala_toolchain(name = "toolchain_jvm", other config) toolchain(name="register_toolchain_jvm", toolchain_type = "...:toolchain_type", exec_compatible_with = [], target_compatible_with = ["//scala/constraints:compiler_jvm"], toolchain = ":toolchain_jvm") # Repeat for JS and native Then users can select which toolchain by passing ***@***.***_bazel_rules_scala//scala/platforms:scala_jvm" or "...//platforms:scala_js". The rules and toolchain would need logic to tell if a target cannot be built with that toolchain, and to emit an error if the wrong source is given. If jvm vs js vs native output isn't what the user should be selecting with the --platforms, we can work out what the right conceptual model should be. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#364 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF_vvjv8rffAB2jTuZeDlE3b3WYRhks5tAtafgaJpZM4Q9pYk> .

katre · 2017-12-15T21:28:51Z

Currently, bazel can't support that. It's on our roadmap (you'd add the scala versions as constraints in the platforms and toolchains, and specify multiple target platforms with the "--platforms" flag), but we're not ready to start investigating the work, primarily because it will cause Bazel's internal data structures to double or more in size.

ittaiz · 2017-12-15T22:51:34Z

Sounds good. Especially since one can use macros to generate duplicate targets

…

On Fri, 15 Dec 2017 at 23:28 katre ***@***.***> wrote: Currently, bazel can't support that. It's on our roadmap (you'd add the scala versions as constraints in the platforms and toolchains, and specify multiple target platforms with the "--platforms" flag), but we're not ready to start investigating the work, primarily because it will cause Bazel's internal data structures to double or more in size. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#364 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFxgsAjSyvoxjXGTMYhwvmVY5wfOUks5tAuSUgaJpZM4Q9pYk> .

johnynek · 2017-12-19T19:14:21Z

This seems mergeable (as a first step). Is that right?

or-shachar · 2017-12-30T16:36:08Z

@johnynek - sorry for neglecting this thread. Don't merge it yet. I want to update it a bit to follow the suggestions to better naming and then we can merge. If I won't be able to do it in the next few hours I'll surely do it by this coming Tuesday

Could not understand how to load current repo as external repo within the test

…ain" This reverts commit d7eed19.

katre · 2018-01-02T15:10:39Z

I replied on #4372, take a look there.

or-shachar · 2018-01-03T09:30:02Z

close to rerun travis :-(

or-shachar · 2018-01-03T09:30:12Z

open to rerun travis :-(

bazel-io · 2018-01-03T09:30:14Z

Can one of the admins verify this patch?

ittaiz · 2018-01-03T09:55:06Z

test this please (ci bot)

use cli flag to load the toolchain that should fail the test

- added "default" prefix to make it clear that its default_toolchain - moved the toolchain_type to the top level

ittaiz · 2018-01-04T09:55:07Z

Test this please

katre

This looks great, thanks!

or-shachar · 2018-01-04T16:07:49Z

@katre thanks a lot! :-)
@ittaiz @johnynek - will be happy for your feedback as well. I also updated the docs.
Ready to merge

johnynek · 2018-01-04T19:30:58Z

👍

I'll let @ittaiz do the honors to make sure you don't have any follow up concerns.

closes #170

ittaiz · 2018-01-04T22:07:16Z

README.md

+scala_register_toolchains()
+```
+[read more here](#scala_toolchain)
+# register default scala toolchain


why do we need this section? it looks like the two lines above and it doesn't seem formatted in the "view" mode

ittaiz · 2018-01-04T22:14:52Z

@or-shachar Thanks! We've been waiting a long time for this :)
As @johnynek said next up is scala sdk. @nadavwe this is a big step towards scala 2.12 support in master.
Thanks @katre for your support!

katre · 2018-01-04T22:40:04Z

Thanks to you guys for picking up the work, I'm always happy to help out.

googlebot added the cla: yes label Dec 11, 2017

katre reviewed Dec 11, 2017

View reviewed changes

ittaiz reviewed Dec 11, 2017

View reviewed changes

johnynek reviewed Dec 11, 2017

View reviewed changes

johnynek reviewed Dec 12, 2017

View reviewed changes

johnynek reviewed Dec 14, 2017

View reviewed changes

katre reviewed Dec 14, 2017

View reviewed changes

johnynek mentioned this pull request Dec 22, 2017

Configurable tooling tweag/rules_haskell#27

Merged

or-shachar added 6 commits December 30, 2017 18:48

WIP - added scala_toolchain with scalacopts support - tests fail

ba42f7f

[FAIL] use bazel-integration-testing framework to test toolchain

7caf47c

Could not understand how to load current repo as external repo within the test

Revert "[FAIL] use bazel-integration-testing framework to test toolch…

43ef090

…ain" This reverts commit d7eed19.

falling back to sh test instead

001dfa7

added newline

29f5ca1

better classname

2c8df83

katre mentioned this pull request Jan 2, 2018

toolchains flag --experimental_extra_toolchains is not supported bazelbuild/bazel#4372

Closed

removing prefix from toolchain flag

077156e

or-shachar closed this Jan 3, 2018

or-shachar reopened this Jan 3, 2018

or-shachar added 4 commits January 4, 2018 11:07

load default (empty) toolchain on WORKSPACE

d2eed4f

use cli flag to load the toolchain that should fail the test

removed scala_ prefix from toolchain type and implementation

69e70cf

fixed merge conflicts

4028bb7

re-organized toolchain BUILD file

597e639

- added "default" prefix to make it clear that its default_toolchain - moved the toolchain_type to the top level

katre approved these changes Jan 4, 2018

View reviewed changes

Updated README.md

e091a8a

or-shachar changed the title ~~WIP - added scala_toolchain with scalacopts support - tests fail~~ added scala_toolchain with scalacopts support Jan 4, 2018

johnynek mentioned this pull request Jan 4, 2018

extend scala_toolchain to include the scala-sdk #380

Closed

ittaiz reviewed Jan 4, 2018

View reviewed changes

updated READEME.md

2c4e804

ittaiz merged commit bc2940d into bazel-contrib:master Jan 4, 2018

This was referenced Jan 4, 2018

added back mistakenly deleted paragraph #381

Merged

rule's "scalacopts" should get higher priority than toolchain "scalacopts" #382

Open

ittaiz mentioned this pull request Mar 19, 2018

Ease ability of rules to test themselves with this library bazelbuild/bazel-integration-testing#32

Open

hchauvin mentioned this pull request Apr 23, 2018

Executable and runfiles "teleportation" bazelbuild/bazel-integration-testing#64

Open

ittaiz mentioned this pull request Jan 18, 2020

scala_toolchain repository rule? #170

Closed

added scala_toolchain with scalacopts support #364

added scala_toolchain with scalacopts support #364

Conversation

or-shachar commented Dec 11, 2017

Open questions:

bazel-io commented Dec 11, 2017

ittaiz commented Dec 11, 2017 via email

ittaiz commented Dec 11, 2017

katre left a 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

ittaiz commented Dec 11, 2017

Choose a reason for hiding this comment

ittaiz commented Dec 11, 2017 via email

or-shachar commented Dec 12, 2017

johnynek commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

or-shachar commented Dec 13, 2017

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

johnynek commented Dec 15, 2017

katre commented Dec 15, 2017

ittaiz commented Dec 15, 2017 via email

katre commented Dec 15, 2017

ittaiz commented Dec 15, 2017 via email

johnynek commented Dec 19, 2017

or-shachar commented Dec 30, 2017

katre commented Jan 2, 2018

or-shachar commented Jan 3, 2018

or-shachar commented Jan 3, 2018

bazel-io commented Jan 3, 2018

ittaiz commented Jan 3, 2018

ittaiz commented Jan 4, 2018

katre left a comment

Choose a reason for hiding this comment

or-shachar commented Jan 4, 2018

johnynek commented Jan 4, 2018

Choose a reason for hiding this comment

ittaiz commented Jan 4, 2018

katre commented Jan 4, 2018