Use a scala compiler plugin to throw errors on missing direct-deps #243

natansil · 2017-07-04T11:28:49Z

This PR is the first phase in implementing #235.

Description

It keeps the current end-user behavior of failing a build due to missing direct-deps.
This behavior is achieved in a different way. Instead of providing the Scala compiler only direct deps, it also provides it indirect-deps.
But an additional step is run by a scala compiler plugin called dependency-analyzer which is provided with build target information (direct jars, indirect jars and indirect target ).
The plugin throws an error if it concludes that certain used jars are not directly dependent upon by the bazel target

A new feature is to provide the user with the information of which target label is missing in the build file.
For example:

ERROR: /Users/natans/Work/rules_scala/test_expect_failure/missing_direct_deps/BUILD:4:1: scala //test_expect_failure/missing_direct_deps:transitive_dependency_user failed: Process exited with status 1 [sandboxed].
error: Target '//test_expect_failure/missing_direct_deps:direct_dependency' is used but isn't explicitly declared, please add it to the deps

Known limitations with the current PR:

the Scala compiler plugin is built using an older version of rules Scala (bootstrapping build) in order to avoid cyclic dependency
the plugin tests are not hermetic as they use Coursier to fetch artifacts from the internet.

Next planned work

switch the plugin tests to be E2E tests in order to avoid the current pitfalls (coursier, jar paths concatenation)
add support for scala_binary (currently only works on scala_library)
enable toggling between warn/error/off modes
change the plugin such that it "knows" when the compilation unit actually used the jar, and not just when the compiler transitively needs it
in addition to throwing errors, the plugin should output buildoser commands to fix the BUILD files

Thanks to @nadavwe who contributed a lot to this feature

bazel-io · 2017-07-04T11:28:51Z

Can one of the admins verify this patch?

natansil · 2017-07-04T11:39:00Z

Hi @kchodorow , we want to add a scala compiler plugin to the rules-scala repo, but we cannot build it directly with scala_library as the latter uses the plugin, and so causes a cyclic dependency.
To bypass the problem we 'bootstrap' plugin build using an previous commit of rules-scala we fetch using http_archive, but we get the following warning:

WARNING: /private/var/tmp/_bazel_natans/76d980e70234552ca74184efcaae9f88/external/io_bazel_rules_scala_for_plugin_bootstrapping/WORKSPACE:1: Workspace name in /private/var/tmp/_bazel_natans/76d980e70234552ca74184efcaae9f88/external/io_bazel_rules_scala_for_plugin_bootstrapping/WORKSPACE (@io_bazel_rules_scala) does not match the name given in the repository's definition (@io_bazel_rules_scala_for_plugin_bootstrapping); this will cause a build error in future versions.

Is there some other way you can think of that resolves the cyclic dependency here?

ittaiz · 2017-07-05T05:11:58Z

@johnynek I'll do a detailed code review but I'd appreciate your general feedback about this direction.

johnynek

In general, this is very exciting. I'd have two main requests:

can we keep the bootstrap compiler local to this repo (by parameterizing the skylark rules and then making a scala_library_bootstrap rule or something which can produce jars that scala_library can depend on).
can we remove all the whitespace changes from this diff to make reviewing less error prone/burdensome.

johnynek · 2017-07-05T19:07:46Z

WORKSPACE

@@ -1,5 +1,16 @@
 workspace(name = "io_bazel_rules_scala")

+
+####### previous rules scala for building of scala compiler plugin
+rules_scala_version="3070353d06bb726141e6cea241e5d6e355a4d14f" # update this as needed


I don't like this approach here. It makes it more difficult to have your own branch of rules.

Could we instead have a bootstrap compiler mode that is very simple, but possibly slow that does not depend on fancy features? Or maybe just an option we can pass into the skylark which can use the direct/indirect approach at all?

removed http_archive of old rules_scala version.
added bootstrapping version of scala_library rule

johnynek · 2017-07-05T19:08:46Z

plugin/src/main/resources/scalac-plugin.xml

@@ -0,0 +1,4 @@
+<plugin>
+  <name>dependency-analyzer</name>
+  <classname>plugin.src.main.scala.io.github.retronym.dependencyanalyzer.DependencyAnalyzer</classname>


what is this used for?

It's a scala compiler plugin descriptor that contains the entry point for the plugin.
See http://www.scala-lang.org/old/node/140

johnynek · 2017-07-05T19:09:31Z

plugin/src/main/scala/io/github/retronym/dependencyanalyzer/DependencyAnalyzer.scala

@@ -0,0 +1,100 @@
+package plugin.src.main.scala.io.github.retronym.dependencyanalyzer


should we use a different package name when we are doing this? It seems weird to have a copy of the code without putting it in a new package.

johnynek · 2017-07-05T19:12:44Z

plugin/src/main/scala/io/github/retronym/dependencyanalyzer/DependencyAnalyzer.scala

+import scala.tools.nsc.plugins.{Plugin, PluginComponent}
+import scala.tools.nsc.{Global, Phase}
+
+class DependencyAnalyzer(val global: Global) extends Plugin with Compat {


is this exactly the same code with no changes? If so, can we add a comment to that effect and if not explain where it is different?

We have referred this question to Google open source office

johnynek · 2017-07-05T19:16:10Z

plugin/src/test/scala/io/github/retronym/dependencyanalyzer/TestUtil.scala

+    else s"-P:dependency-analyzer:$name:${values.mkString(":")}"
+  }
+
+  object Coursier {


this test is doing network ops, no? That seems suboptimal to me if so.

@johnynek That is correct, we noted this issue in the description...
This coming week, a couple of Wix developers are working on this feature.
Their first task is to move the tests in this unit test class to the the E2E test run by the test_run script. this way using Coursier will not be neccessary

As this is is the only issue waiting to be resolved in the coming week, we would appreciate it if you can give your OK to merge this PR, in order for the new developers to work on a new branch out of master

johnynek · 2017-07-05T19:16:35Z

plugin/src/test/scala/io/github/retronym/dependencyanalyzer/DependencyAnalyzerSpec.scala

+      s"error on $target should not appear in errors!")
+  }
+
+  implicit class `decode bazel lables`(targetLabel: String) {


lables is misspelled.

fixed spelling

johnynek · 2017-07-05T19:20:00Z

scala/scala.bzl

@@ -644,6 +679,8 @@ scala_library = rule(
  attrs={
      "main_class": attr.string(),
      "exports": attr.label_list(allow_files=False),
+      "enable_dependency_analyzer": attr.bool(default=True, mandatory=False),
+      "dependency_analyzer_plugin": attr.label(default=Label("//plugin/src/main:dependency_analyzer"), allow_files=_jar_filetype, mandatory=False),


I think you may need a full @io_bazelbuild_rules_scala//plugin/... name here since it may not be expanded correctly in remote repos.

added repo prefix

johnynek · 2017-07-05T19:24:28Z

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

+
+    String[] pluginParams;
+    if (ops.enableDependencyAnalyzer) {
+      pluginParams = pluginParamsInUse;


can we move 197 here to make it easier to verify that when we don't use the dependency analyzer we won't use the options.

johnynek · 2017-07-05T19:35:59Z

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

@@ -280,38 +301,39 @@ private static final Path newArgFile(CompileOptions ops, String[] javaSources, P
  private static void removeTmp(Path tmp) throws IOException {
    if (tmp != null) {
      Files.walkFileTree(tmp, new SimpleFileVisitor<Path>() {
-         @Override


side note; all the cosmetic changes are making this hard to review. I have to carefully read each line to make sure it does not contain a semantic change. Could we possibly have a minimal change with no cosmetic changes and send a second PR that has the formatting changes? This kind of separation significantly reduces the review difficulty.

Totally agree. removed formatting changes

johnynek · 2017-07-05T19:36:26Z

test_expect_failure/missing_direct_deps/A.scala

+package test_expect_failure.missing_direct_deps
+
+object A {
+	def foo = {


pretty wild scala style here. :)

:) Anything you want me to change?

johnynek · 2017-07-05T19:38:56Z

cc @retronym who may be interested to see his code show up here.

ittaiz · 2017-07-06T12:31:14Z

test this please (dear ci bot)

ittaiz · 2017-07-07T13:32:06Z

plugin/src/main/scala/io/github/retronym/dependencyanalyzer/DependencyAnalyzer.scala

+class DependencyAnalyzer(val global: Global) extends Plugin {
+
+  val name = "dependency-analyzer"
+  val description =


this is no longer the description right?

ittaiz · 2017-07-07T13:37:41Z

plugin/src/main/scala/io/github/retronym/dependencyanalyzer/DependencyAnalyzer.scala

+        for (usedJar <- usedJars;
+             usedJarPath = usedJar.path;
+             target <- indirect.get(usedJarPath)
+             if !direct.contains(usedJarPath)) {


is this idiomatic? shouldn't the guard be in the previous line? maybe @dkomanov knows

moved up to the previous line

ittaiz · 2017-07-07T18:56:19Z

scala/scala.bzl

+
+    if (hasattr(ctx.attr, 'enable_dependency_analyzer')
+        and ctx.attr.enable_dependency_analyzer
+        and hasattr(ctx.attr, 'dependency_analyzer_plugin')):


why do we need this third check? Is there a situation where enable_dependency_analyzer exists but dependency_analyzer_plugin doesn't?

It is not needed. removed it.

ittaiz · 2017-07-07T18:58:24Z

scala/scala.bzl

+    compiler_classpath = ":".join([j.path for j in all_jars])
+    direct_jars = ",".join([j.path for j in cjars])
+
+    valid_jar_paths = []


what does this mean? when is a jar path invalid?

This was added in order to pass non-related failing tests.

On retrospect, this was incorrect and there was a missing test on 'file' deps, e.g. @com_google_guava_guava_21_0//jar:file.
added a test, and deleted these lines

ittaiz · 2017-07-07T19:00:34Z

scala/scala.bzl

-      "manifest": "%{name}_MANIFEST.MF",
-      },
+      "enable_dependency_analyzer": attr.bool(default=True, mandatory=False),
+      "dependency_analyzer_plugin": attr.label(default=Label("@io_bazel_rules_scala//plugin/src/main:dependency_analyzer"), allow_files=_jar_filetype, mandatory=False),


should this be a private attribute? are actual users supposed to be able to configure this? I think not so it should probably be internal (_dependency_analyzer_plugin)

changed to internal

ittaiz · 2017-07-07T19:01:38Z

scala/scala.bzl

+  outputs=library_outputs,
+)
+
+scala_library_for_plugin_bootstrapping = rule(


as much as I hate comments maybe one is needed here since the attribute trick might be hard to grasp at first and second look

comment added

ittaiz · 2017-07-07T19:02:58Z

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

+
+
+    String[] pluginParams;
+    if (ops.enableDependencyAnalyzer) {


extract method?

ittaiz · 2017-07-07T19:05:04Z

scala/scala.bzl

@@ -162,9 +162,31 @@ def _compile(ctx, cjars, dep_srcjars, buildijar):
    all_srcjars = set(srcjars + list(dep_srcjars))
    # look for any plugins:
    plugins = _collect_plugin_paths(ctx.attr.plugins)
+
+    if (hasattr(ctx.attr, 'enable_dependency_analyzer')
+        and ctx.attr.enable_dependency_analyzer


i think this (the ability to turn off dependency analyzer) doesn't have a test, right?
even if this is covered by existing tests (which I'm not sure) there's definitely no test which encodes the feature, right? WDYT about having the next pair also add that test?

The feature to turn off dependency analyzer has not been added yet. This if statement is used because compile method is used by scala_binary which does not support the plugin yet, and also the bootstrapping version of scala_library. What test do you suggest can be added here?

the test will be to use the command line flags and see that we don't have the dependency analyzer.
But we'll need to wait for 0.5.3 or 0.6.0 for that change.
If you can add an internal issue about it and we'll add it later on

added issue to rules_scala #248

ittaiz · 2017-07-07T19:07:06Z

test_run.sh

+    echo "'bazel build of scala_library with missing direct deps should have failed."
+    exit 1
+  fi
+  echo "$output"


Is this echo (the first one) helpful for debugging? not against it just trying to understand

yes. If there is an error, the output of the build will be shown

ittaiz · 2017-07-07T19:14:18Z

@natansil the build fails after you rebased from master. Did it pass locally?

natansil · 2017-07-09T07:39:34Z

@ittaiz, no I merged from github directly. Not doing that again... :)
Anyway, that is fixed now...

googlebot · 2017-07-09T14:55:15Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-07-09T16:14:59Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

nadavwe · 2017-07-09T16:21:31Z

as the other commiter, I'm OK with the commits that were done.

natansil · 2017-07-09T16:28:08Z

@ittaiz, finished with the CR changes. please review them as well

ittaiz · 2017-07-09T17:55:48Z

scala/scala.bzl

@@ -382,6 +382,13 @@ def collect_srcjars(targets):
            srcjars += [target.srcjars.srcjar]
    return srcjars

+def update_jars_to_labels(jars2labels, target, java_provider):
+  for jar in (java_provider.compile_jars + java_provider.transitive_runtime_jars):
+    if jar.path not in jars2labels:


i guess this is to eliminate d showing up in both c and d right? if so maybe a named inner method, a comment or both would help clarify this

correct, added several inner methods that hopefully convey the intent

ittaiz · 2017-07-09T17:56:26Z

scala/scala.bzl

+def update_jars_to_labels(jars2labels, target, java_provider):
+  for jar in (java_provider.compile_jars + java_provider.transitive_runtime_jars):
+    if jar.path not in jars2labels:
+      if hasattr(target, "jars_to_labels") and jar.path in target.jars_to_labels:


can you explain why this fixes the bug?

The bug was that an incorrect target was given in the error message to the user. If we have A, B, and C, (A has B in deps, B has C in deps) than by the time A was built, the dependency information it receives was already aggregated and attached to the target B.

In order to retrieve information attached to target C, another provider (jars_to_labels) was needed to be returned by scala_library in order for targets such as C be exposed when building A

ittaiz · 2017-07-09T18:14:40Z

@natansil added two (hopefully last) comments

johnynek

This is coming along great. One thing I am not seeing is how we ensure we use ijars for the indirect dependencies. If we use the runtime jars, we are going to cause a recompilation any time any aspect of any runtime dep changes. I don't think we want that as it will mean that we recompile everything downstream of all changes all the time. I think we need to be careful here.

johnynek · 2017-07-09T19:38:29Z

scala/scala.bzl

+
+    if (hasattr(ctx.attr, 'enable_dependency_analyzer')
+        and ctx.attr.enable_dependency_analyzer):
+      enable_dependency_analyzer = ctx.attr.enable_dependency_analyzer


isn't this always true here? Can we just write enable_dependency_analyzer = True here?

johnynek · 2017-07-09T19:41:50Z

scala/scala.bzl

@@ -221,12 +245,13 @@ SourceJars: {srcjars}
    # _jdk added manually since _java doesn't currently setup runfiles
    # _scalac, as a java_binary, should already have it in its runfiles; however,
    # adding does ensure _java not orphaned if _scalac ever was not a java_binary
-    ins = (list(cjars) +
+    ins = (list(all_jars) +


this is only all_jars if dependency analysis is on, isn't it?

Also, in that case, if any of the runtime jars change at all, you will have to recompile. Won't this result in a lot of rebuilding? It seems like we want the ijars for the transitive dependencies, but not the runtime jars for the transitive dependencies. Is this actually what is happening, if so can we comment here to remind the reader?

fixed. passing only transitive compile_time ijars.
passing only direct deps if dependency analysis is off.

johnynek · 2017-07-09T19:44:14Z

scala/scala.bzl

-      "ijar": "%{name}_ijar.jar",
-      "manifest": "%{name}_MANIFEST.MF",
-      },
+      "enable_dependency_analyzer": attr.bool(default=True, mandatory=False),


if this is here, it means we can't use it with scala_binary, scala_test or scala_macro_library right. Any reason for the split?

We decided to start first with scala_library, the pair continuing work this week will add this also to scala_binary, scala_test and scala_macro_library, etc...

ittaiz · 2017-07-10T16:00:31Z

test this please (ci bot)

natansil · 2017-07-11T13:38:46Z

@johnynek , please review after change of passing ijars instead of jars

ittaiz · 2017-07-11T14:09:50Z

test this please (Ci bot)

johnynek · 2017-07-12T02:18:54Z

scala/scala.bzl

            compile_jars += java_provider.compile_jars
+            transitive_cjars += java_provider.compile_jars


can we not access the transitive ijars with the java provider?

Java provider only exposes compile_jars and transitive_runtime_jars.
I've opened an issue for transitive_compile_time_jars to be exposed

johnynek · 2017-07-12T02:19:35Z

scala/scala.bzl


    runtime_dep_jars =  _collect_jars(ctx.attr.runtime_deps + extra_runtime_deps)
    transitive_rjars += runtime_dep_jars.transitive_runtime_jars

    jars2labels.update(runtime_dep_jars.jars2labels)
-    return struct(compile_jars = cjars, transitive_runtime_jars = transitive_rjars, jars2labels=jars2labels)
+    return struct(compile_jars = cjars, transitive_runtime_jars = transitive_rjars, jars2labels=jars2labels, transitive_cjars = transitive_cjars)


for consistency with transitive_runtime_jars should we call this transitive_compile_jars?

@johnynek WDYT about having transitive_cjars be named compile_jars because intent wise that's what they are. The jars which we're going to compile with. Implementation wise they are transitive but I'm not sure that's the focus.
And I'd rename the existing compile_jars to be direct_jars since that's the only reason they're still needed. For the plugin to know if the used jar is direct or indirect.
WDYT?

The java provider means compile_jars to be direct though, no? I'd prefer to keep terminology with them, even if it is not ideal.

transitive_compile_jars to me seems most consistent with bazel's naming. Do you disagree?

johnynek · 2017-07-12T02:21:36Z

have you tested this internally on a real repo yet?

jvican · 2017-07-12T11:46:20Z

Note: to get the biggest benefit of the compiler plugin that has been ported from the Classpath Shrinker, I recommend using Scala 2.11.11 that has improvements to the stub error messages that allows build tool authors to write bots to remove classpath entries automatically. This is what they are doing in Pants (EDITED).

natansil · 2017-07-12T14:06:10Z

@johnynek

have you tested this internally on a real repo yet?

We have started the process today, I will update when we have results

…yzer based on classpath shrinker

…mon_attr)

…all cjars to transitive_cjars in all cases

…dency changes internally

…d caveat on recompilations due to transitive dependencies changes

ittaiz · 2017-07-25T08:26:52Z

test this please

ittaiz · 2017-07-25T09:04:30Z

Thank you so much @natansil! Appreciate the work along the long journey.

cushon · 2017-08-01T20:33:07Z

We haven't seen appreciable overhead with the Strict Java Deps plugin. Would it be possible to put together a benchmark that demonstrates the slowdown you're seeing? Also, is it possible that the different with scala is caused by a longer classpath using strict deps than with manually curated deps?

googlebot added the cla: yes label Jul 4, 2017

johnynek reviewed Jul 5, 2017

View reviewed changes

natansil force-pushed the dependency_analyzer branch from bb3a1e2 to 08c7a7c Compare July 7, 2017 14:49

ittaiz suggested changes Jul 7, 2017

View reviewed changes

googlebot added cla: no and removed cla: yes labels Jul 9, 2017

natansil force-pushed the dependency_analyzer branch from 9888a86 to c72cb05 Compare July 9, 2017 16:14

ittaiz reviewed Jul 9, 2017

View reviewed changes

johnynek reviewed Jul 9, 2017

View reviewed changes

johnynek reviewed Jul 12, 2017

View reviewed changes

natansil mentioned this pull request Jul 12, 2017

the shrinker will warn on provided indirect targets if they are used … scalacenter/classpath-shrinker#3

Closed

natansil and others added 23 commits July 25, 2017 10:46

when strict mode is disabled, pass only direct jars to scala compiler

b5b2975

add third_party aread and licensing statement for the dependency_anal…

df50732

…yzer based on classpath shrinker

make 'collection of [labels of jars]'' code more comprehensible

9b95403

extract to method

ffa505c

add scalac plugin license file

c9a1826

enable dependency analyser for scala_binary

f2cba75

error/warn/off toggle for dependency analyzer

2654194

cosmetics

abe4c99

remove coursier from tests. pre-fetch needed jars

81062ab

match file name to class name

6c5f695

make _scala_binary_common a bit clearer

f49631f

clarify name: provided_cjars -> compiler_classpath_jars

4e619ed

fix warning E2E test issue

d67195c

enable strict-deps check for all rules (move dep_analyzer_mode to com…

41e0bff

…mon_attr)

don't collect jars if mode=off

36dfa9b

add special jars to transitive_cjars to scala_test instead of adding …

4348623

…all cjars to transitive_cjars in all cases

Oscar CR changes

7c8c5f8

add test that no compilation triggers of target when transitive depen…

e5a091b

…dency changes internally

add buildozer command action to error/warn log for strict deps

4df8b0f

make it clear the strict_deps mode attribut will be replaced soon. ad…

6d368ea

…d caveat on recompilations due to transitive dependencies changes

no_recompilation_on_internal_change CR changes

8a696a6

buildozer commands CR changes

25a6266

expand on recompilation explanation

41b8fa5

natansil force-pushed the dependency_analyzer branch from 72ae9aa to 41b8fa5 Compare July 25, 2017 07:54

ittaiz merged commit aaa6c7d into bazelbuild:master Jul 25, 2017

nadavwe mentioned this pull request Aug 13, 2017

using Classpath shrinker to warn on indirect targets wix-incubator/rules_scala#1

Closed

jadenPete mentioned this pull request Dec 9, 2024

Use Zinc for dependency checking #1668

Open

		@@ -0,0 +1,100 @@
		package plugin.src.main.scala.io.github.retronym.dependencyanalyzer

		compile_jars += java_provider.compile_jars
		transitive_cjars += java_provider.compile_jars

Use a scala compiler plugin to throw errors on missing direct-deps #243

Use a scala compiler plugin to throw errors on missing direct-deps #243

Conversation

natansil commented Jul 4, 2017

Description

Known limitations with the current PR:

Next planned work

bazel-io commented Jul 4, 2017

natansil commented Jul 4, 2017

ittaiz commented Jul 5, 2017

johnynek 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

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

natansil Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

johnynek commented Jul 5, 2017

ittaiz commented Jul 6, 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

ittaiz commented Jul 7, 2017

natansil commented Jul 9, 2017

googlebot commented Jul 9, 2017

googlebot commented Jul 9, 2017

nadavwe commented Jul 9, 2017

natansil commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natansil Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

ittaiz commented Jul 9, 2017

johnynek 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

natansil Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

ittaiz commented Jul 10, 2017

natansil commented Jul 11, 2017

ittaiz commented Jul 11, 2017

Choose a reason for hiding this comment

natansil Jul 6, 2017 •

edited

Loading

natansil Jul 10, 2017 •

edited

Loading

natansil Jul 10, 2017 •

edited

Loading

natansil Jul 12, 2017 •

edited

Loading

jvican commented Jul 12, 2017 •

edited

Loading