Skip to content

Use a provider for jars2labels #534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 10, 2018
Merged

Use a provider for jars2labels #534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
18ec6d1
Use a provider for jars2labels
johnynek Jun 23, 2018
861b472
use http to download scala, is safe with a hash
johnynek Jun 23, 2018
5945f59
minor clean ups
johnynek Jun 23, 2018
c0beffd
merge master
johnynek Jul 9, 2018
68f58a6
fix tests
johnynek Jul 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions scala/jars_to_labels.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
JarsToLabelsInfo = provider(fields = [
"jars_to_labels", # dict of path of a jar to a label
])
49 changes: 22 additions & 27 deletions scala/private/common.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
load("@io_bazel_rules_scala//scala:jars_to_labels.bzl", "JarsToLabelsInfo")

_empty_jars2labels = JarsToLabelsInfo(jars_to_labels = {})

def write_manifest(ctx):
main_class = getattr(ctx.attr, "main_class", None)
write_manifest_file(ctx.actions, ctx.outputs.manifest, main_class)
Expand Down Expand Up @@ -42,7 +46,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,
Copy link
Contributor

@andyscott andyscott Jun 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

transitive_compile_jars = depset())

def _collect_jars_when_dependency_analyzer_is_on(dep_targets):
Expand All @@ -62,6 +66,7 @@ def _collect_jars_when_dependency_analyzer_is_on(dep_targets):

compile_jars.append(current_dep_compile_jars)
transitive_compile_jars.append(current_dep_transitive_compile_jars)

add_labels_of_jars_to(jars2labels, dep_target,
current_dep_transitive_compile_jars.to_list(),
current_dep_compile_jars.to_list())
Expand All @@ -71,7 +76,7 @@ def _collect_jars_when_dependency_analyzer_is_on(dep_targets):
return struct(
compile_jars = depset(transitive = compile_jars),
transitive_runtime_jars = depset(transitive = runtime_jars),
jars2labels = jars2labels,
jars2labels = JarsToLabelsInfo(jars_to_labels = jars2labels),
transitive_compile_jars = depset(transitive = transitive_compile_jars))

# When import mavan_jar's for scala macros we have to use the jar:file requirement
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

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

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

def _add_label_of_indirect_jar_to(jars2labels, dependency, jar):
if _label_already_exists(jars2labels, jar):
return

# skylark exposes only labels of direct dependencies.
# to get labels of indirect dependencies we collect them from the providers transitively
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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. We can fix it by also checking if the path starts with Unknown label (I'd rather we not do it on this PR)
  2. This should not happen now that we're moving everything to scala_import/scala_import_external
  3. 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])

# skylark exposes only labels of direct dependencies.
# to get labels of indirect dependencies we collect them from the providers transitively
label = _provider_of_dependency_label_of(dependency, path)
if label == None:
label = "Unknown label of file {jar_path} which came from {dependency_label}".format(
jar_path = path, dependency_label = dependency.label)
jars2labels[path] = label

def _provider_of_dependency_label_of(dependency, path):
if JarsToLabelsInfo in dependency:
return dependency[JarsToLabelsInfo].jars_to_labels.get(path)
else:
jars2labels[
jar.
path] = "Unknown label of file {jar_path} which came from {dependency_label}".format(
jar_path = jar.path, dependency_label = dependency.label)

def _label_already_exists(jars2labels, jar):
return jar.path in jars2labels

def _provider_of_dependency_contains_label_of(dependency, jar):
return hasattr(dependency,
"jars_to_labels") and jar.path in dependency.jars_to_labels
return None

# TODO this seems to have limited value now that JavaInfo has everything
def create_java_provider(scalaattr, transitive_compile_time_jars):
Expand Down
17 changes: 10 additions & 7 deletions scala/private/rule_impls.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ load(
"not_sources_jar",
"write_manifest",
)
load("@io_bazel_rules_scala//scala:jars_to_labels.bzl", "JarsToLabelsInfo")

_java_extension = ".java"
_scala_extension = ".scala"
Expand Down Expand Up @@ -509,7 +510,7 @@ def _lib(ctx, non_macro_lib):
write_manifest(ctx)
outputs = _compile_or_empty(ctx, ctx.outputs.manifest, cjars, srcjars,
non_macro_lib, jars.transitive_compile_jars,
jars.jars2labels, [])
jars.jars2labels.jars_to_labels, [])

transitive_rjars = depset(outputs.full_jars, transitive = [transitive_rjars])

Expand Down Expand Up @@ -543,7 +544,7 @@ def _lib(ctx, non_macro_lib):
return struct(
files = depset([ctx.outputs.jar]), # Here is the default output
scala = scalaattr,
providers = [java_provider],
providers = [java_provider, jars.jars2labels],
runfiles = runfiles,
jars_to_labels = jars.jars2labels,
)
Expand All @@ -564,7 +565,8 @@ def _scala_binary_common(ctx,
implicit_junit_deps_needed_for_java_compilation = []):
write_manifest(ctx)
outputs = _compile_or_empty(ctx, ctx.outputs.manifest, cjars, depset(), False,
transitive_compile_time_jars, jars2labels,
transitive_compile_time_jars,
jars2labels.jars_to_labels,
implicit_junit_deps_needed_for_java_compilation
) # no need to build an ijar for an executable
rjars = depset(outputs.full_jars, transitive = [rjars])
Expand All @@ -590,11 +592,10 @@ def _scala_binary_common(ctx,

return struct(
files = depset([ctx.outputs.executable, ctx.outputs.jar]),
providers = [java_provider],
providers = [java_provider, jars2labels],
scala = scalaattr,
transitive_rjars =
rjars, #calling rules need this for the classpath in the launcher
jars_to_labels = jars2labels,
runfiles = runfiles)

def scala_binary_impl(ctx):
Expand Down Expand Up @@ -683,8 +684,10 @@ def scala_test_impl(ctx):
transitive_compile_jars = depset(
transitive = [scalatest_jars, transitive_compile_jars])
scalatest_jars_list = scalatest_jars.to_list()
add_labels_of_jars_to(jars_to_labels, ctx.attr._scalatest,
scalatest_jars_list, scalatest_jars_list)
j2l = jars_to_labels.jars_to_labels
add_labels_of_jars_to(j2l, ctx.attr._scalatest, scalatest_jars_list,
scalatest_jars_list)
jars_to_labels = JarsToLabelsInfo(jars_to_labels = j2l)

args = " ".join([
"-R \"{path}\"".format(path = ctx.outputs.jar.short_path),
Expand Down
17 changes: 8 additions & 9 deletions scala/scala_import.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
load("@io_bazel_rules_scala//scala:jars_to_labels.bzl", "JarsToLabelsInfo")

#intellij part is tested manually, tread lightly when changing there
#if you change make sure to manually re-import an intellij project and see imports
#are resolved (not red) and clickable
Expand All @@ -20,12 +22,12 @@ def _scala_import_impl(ctx):
return struct(
scala = struct(outputs = struct(jars = intellij_metadata),
),
jars_to_labels = jars2labels,
providers = [
_create_provider(current_jars, transitive_runtime_jars, jars, exports,
ctx.attr.neverlink),
DefaultInfo(files = current_jars,
),
JarsToLabelsInfo(jars_to_labels = jars2labels),
],
)

Expand Down Expand Up @@ -96,15 +98,12 @@ def _collect(deps):

def _collect_labels(deps, jars2labels):
for dep_target in deps:
if JarsToLabelsInfo in dep_target:
jars2labels.update(dep_target[JarsToLabelsInfo].jars_to_labels)
#scala_library doesn't add labels to the direct dependency itself
java_provider = dep_target[JavaInfo]
_transitively_accumulate_labels(dep_target, java_provider, jars2labels)

def _transitively_accumulate_labels(dep_target, java_provider, jars2labels):
if hasattr(dep_target, "jars_to_labels"):
jars2labels.update(dep_target.jars_to_labels)
#scala_library doesn't add labels to the direct dependency itself
for jar in java_provider.compile_jars.to_list():
jars2labels[jar.path] = dep_target.label
for jar in java_provider.compile_jars.to_list():
jars2labels[jar.path] = dep_target.label

def _collect_runtime(runtime_deps):
jar_deps = []
Expand Down