Use a provider for jars2labels #534

johnynek · 2018-06-23T23:17:54Z

part of #450 to modernize our rules.

We still have extra_information, which I will take a stab at next.

johnynek · 2018-06-23T23:34:22Z

@ittaiz please take a look.

johnynek · 2018-06-24T03:48:07Z

the intellij test failure seems unrelated. Added #536.

johnynek · 2018-06-24T03:49:08Z

scala/private/common.bzl

-  if _provider_of_dependency_contains_label_of(dependency, jar):
-    jars2labels[jar.path] = dependency.jars_to_labels[jar.path]
+    path = jar.path
+    if path not in jars2labels:


this seems to introduce order dependency: if you find an unknown provider first, but later it is seen as a direct dependency, you'll only see the unknown, right?

not for direct but if you find an unknown and then another sub graph finds it as direct and afterwards a third target depends on both of these then indeed it can miss the known label.

We can fix it by also checking if the path starts with Unknown label (I'd rather we not do it on this PR)

This should not happen now that we're moving everything to scala_import/scala_import_external

We're considering dropping this mechanism of passing the jars2labels in favor of what java_rules are doing (reading the labels of "offending" jars from the manifest and then using (offline) bazel query to understand which target exactly to add [for example support exports])

ittaiz · 2018-06-25T19:36:31Z

looked a bit, not sure I understand the change. I'll take a look soon because I think I'm just too tired...

andyscott · 2018-06-25T20:46:45Z

scala/private/common.bzl

@@ -38,7 +42,7 @@ def _collect_jars_when_dependency_analyzer_is_off(dep_targets):
  return struct(
      compile_jars = depset(transitive = compile_jars),
      transitive_runtime_jars = depset(transitive = runtime_jars),
-      jars2labels = {},
+      jars2labels = _empty_jars2labels,


Inlining this seems better for readability, IMHO. Is there a performance benefit to reusing the same empty provider instance?

Well, I don't know, but I to me, I felt the opposite: it appeared clearer to me (here is an empty value) vs inlining JarsToLabels(jars_to_labels = {}) which someone has to read an understand as an empty value.

Next, it also seemed it could be faster.

But if you would strongly prefer the change, I will make it. Please let me know.

johnynek · 2018-06-29T22:32:24Z

@ittaiz any chance you could take a look at this?

I’m out this week but jars2labels is the last barrier to move to the modern style. I’d really like to solve this in the next two weeks.

ittaiz · 2018-07-01T12:07:25Z

Will take a look this week

ittaiz · 2018-07-01T12:07:46Z

@johnynek actually, any chance you can resolve the conflicts?

johnynek · 2018-07-09T21:30:38Z

@ittaiz I have merged master.

johnynek · 2018-07-10T16:36:56Z

@ittaiz okay, I think I have fixed it. I had forgotten to update scala_import. Seems to be working locally for me. Let's see if CI passes.

johnynek · 2018-07-10T16:59:20Z

green!

ittaiz · 2018-07-10T17:44:56Z

scala/private/common.bzl

@@ -91,33 +96,23 @@ def filter_not_sources(deps):

 def add_labels_of_jars_to(jars2labels, dependency, all_jars, direct_jars):
  for jar in direct_jars:
-    _add_label_of_direct_jar_to(jars2labels, dependency, jar)
+    jars2labels[jar.path] = dependency.label


why did you remove the private methods?
I thought they added a bit of a declarative feeling in an otherwise very imperative code

so, I found it pretty hard to follow these very tiny methods. They were generally only called one time, and they didn't encapsulate much: they were just rewriting in english what the function was doing.

I think we ideally want some logical chunk, which isn't huge, maybe 5-10 lines, or something that is called many times. Here we often had 1 call, and really a lot of methods below the logical grouping, in my view.

hmm, so I subscribe to a different style. I factor much less how much it's called and the size and much more the readability of the story.
Keep it on same level of abstraction. For example:

for jar in direct_jars: _add_label_of_direct_jar_to(jars2labels, dependency, jar) for jar in all_jars: _add_label_of_indirect_jar_to(jars2labels, dependency, jar)

How should we proceed?
Currently we can get into a sort of tug war where whenever one of us refactors they change to how they like it.

I am kind of liberal in what I accept, so I tend to accept code written by others as long as I don't have strong objections, but at the same time, I have super limited time and for some, probably foolish, reason I have been trying to get this modernization done. I'd rather not sink more time in to write the code in a style I don't agree with, for code we don't use, if no one on your team has the time to help us move forward on this.

I would say: let's merge this it is a 80 line change that retires the second to last custom provider.

I have sunk a ton of time into this, and Stripe does not even use it at all. If you want to structure it differently, I don't mind, you can follow up with another PR as long as you keep the provider around. I don't think it is really fair for one of us to task the other to implement the style they prefer. The one doing the work should get the lion's share of the say on style questions we don't agree about.

@ianoc @andyscott do you have any views on this?

Sounds good. One more point I'd love for us to be able to agree on is that if we're going to change a style we should probably say up front in case another contributor feels strongly about it (and so avoid rework). WDYT?

To be concrete:

def _add_label_of_direct_jar_to(jars2labels, dependency, jar): jars2labels[jar.path] = dependency.label

this is not a helpful method in my view. If a reader cannot understand the 1 line you have replaced with a string description, I don't think the strings are going to help. A comment is possibly useful if there is something tricky going on, but using a dict to point to what a jar came from couldn't be a ton clearer.

If we take this to the extreme, every line of code is replaced with a function that names what it is doing:

def increment_int(x, y): return x + y def lookup_from_dict(d, k): return d[k]

etc...

ittaiz · 2018-07-10T17:49:29Z

scala/private/common.bzl

-  if _provider_of_dependency_contains_label_of(dependency, jar):
-    jars2labels[jar.path] = dependency.jars_to_labels[jar.path]
+    path = jar.path
+    if path not in jars2labels:


not for direct but if you find an unknown and then another sub graph finds it as direct and afterwards a third target depends on both of these then indeed it can miss the known label.

We can fix it by also checking if the path starts with Unknown label (I'd rather we not do it on this PR)

This should not happen now that we're moving everything to scala_import/scala_import_external

We're considering dropping this mechanism of passing the jars2labels in favor of what java_rules are doing (reading the labels of "offending" jars from the manifest and then using (offline) bazel query to understand which target exactly to add [for example support exports])

ittaiz · 2018-07-10T17:54:37Z

To recap:

I'm really sorry it took so long to review such a small PR. This whole area of labels is really important to us and brittle and so I got somewhat of "review-analysis" so sorry.
I saw that you inlined almost all of the private methods you came by. Is that un-pythonic of me to do private methods?

ittaiz

Please rebase (conflicts) and merge

ittaiz · 2018-07-10T18:48:21Z

scala/private/common.bzl

@@ -91,33 +96,23 @@ def filter_not_sources(deps):

 def add_labels_of_jars_to(jars2labels, dependency, all_jars, direct_jars):
  for jar in direct_jars:
-    _add_label_of_direct_jar_to(jars2labels, dependency, jar)
+    jars2labels[jar.path] = dependency.label


Sounds good. One more point I'd love for us to be able to agree on is that if we're going to change a style we should probably say up front in case another contributor feels strongly about it (and so avoid rework). WDYT?

johnynek · 2018-07-10T18:51:28Z

Thanks for the flexibility @ittaiz.

I think it may help to grow the maintainers here. It would be great to have another company actively using rules_scala have an engineer contribute to the rules and help us develop a broader view to make sure we have good buy in, and good continuity as people and companies come and go from the project.

Use a provider for jars2labels

18ec6d1

googlebot added the cla: yes label Jun 23, 2018

johnynek added 2 commits June 23, 2018 13:19

use http to download scala, is safe with a hash

861b472

minor clean ups

5945f59

johnynek commented Jun 24, 2018

View reviewed changes

andyscott reviewed Jun 25, 2018

View reviewed changes

johnynek mentioned this pull request Jun 27, 2018

Aspect-based scrooge implementation #524

Merged

merge master

c0beffd

fix tests

68f58a6

ittaiz reviewed Jul 10, 2018

View reviewed changes

ittaiz approved these changes Jul 10, 2018

View reviewed changes

johnynek merged commit ea4b294 into master Jul 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a provider for jars2labels #534

Use a provider for jars2labels #534

johnynek commented Jun 23, 2018

johnynek commented Jun 23, 2018

johnynek commented Jun 24, 2018

johnynek Jun 24, 2018

ittaiz Jul 10, 2018

ittaiz commented Jun 25, 2018

andyscott Jun 25, 2018 •

edited

Loading

johnynek Jul 10, 2018

johnynek Jul 10, 2018

johnynek commented Jun 29, 2018

ittaiz commented Jul 1, 2018

ittaiz commented Jul 1, 2018

johnynek commented Jul 9, 2018

johnynek commented Jul 10, 2018

johnynek commented Jul 10, 2018

ittaiz Jul 10, 2018

johnynek Jul 10, 2018

ittaiz Jul 10, 2018

johnynek Jul 10, 2018

ittaiz Jul 10, 2018

johnynek Jul 10, 2018

ittaiz Jul 10, 2018

ittaiz commented Jul 10, 2018

ittaiz left a comment

ittaiz Jul 10, 2018

johnynek commented Jul 10, 2018

Use a provider for jars2labels #534

Use a provider for jars2labels #534

Conversation

johnynek commented Jun 23, 2018

johnynek commented Jun 23, 2018

johnynek commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz commented Jun 25, 2018

andyscott Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Jun 29, 2018

ittaiz commented Jul 1, 2018

ittaiz commented Jul 1, 2018

johnynek commented Jul 9, 2018

johnynek commented Jul 10, 2018

johnynek commented Jul 10, 2018

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 10, 2018

ittaiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Jul 10, 2018

andyscott Jun 25, 2018 •

edited

Loading