Skip to content

Fix Intellij Bazel Plugin support for scrooge_scala_library rule #562

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 4 commits into from
Jul 24, 2018
Merged

Fix Intellij Bazel Plugin support for scrooge_scala_library rule #562

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aeb359c
Thread output jars thru so IntelliJ can pick them up.
beala-stripe Jul 16, 2018
57661fa
Fix linting errors.
beala-stripe Jul 17, 2018
58e4bc0
Add todo to remove scala field/provider
beala-stripe Jul 18, 2018
4e71905
Do not collect exported jars.
beala-stripe Jul 23, 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
45 changes: 32 additions & 13 deletions twitter_scrooge/twitter_scrooge.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,15 +185,13 @@ def _compile_scala(ctx, label, output, scrooge_jar, deps_java_info,
expect_java_output = False,
scalac_jvm_flags = [])

return java_common.create_provider(
use_ijar = False,
source_jars = [scrooge_jar],
compile_time_jars = depset(
[output], transitive = [merged_deps.compile_jars]),
transitive_compile_time_jars = depset(
[output], transitive = [merged_deps.transitive_compile_time_jars]),
transitive_runtime_jars = depset(
[output], transitive = [merged_deps.transitive_runtime_jars]))
return JavaInfo(
source_jar = scrooge_jar,
deps = deps_java_info + implicit_deps,
runtime_deps = deps_java_info + implicit_deps,
exports = deps_java_info + implicit_deps,
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask why do you export everything? Doesn’t sound like the right behavior (though I agree that it looks like the existing code does the same)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does seem odd. @johnynek: do you know if this is necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

@johnynek wdyt? As far as I'm concerned we can merge as is but just want to make sure this is indeed what you need

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a comment to the ticket (schemas don't have implementations that users control, so the data types are generally needed by downstream, more-over if any transitive schema changes you will need a recomputation in most cases).

We can revisit if this intuition is wrong.

output_jar = output,
compile_jar = output)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just making sure you intentionally don’t want ijar here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this comment on line 164:

   # this only compiles scala, not the ijar, but we don't
   # want the ijar for generated code anyway: any change	   
   # in the thrift generally will change the interface and	   
   # method bodies


def _empty_java_info(deps_java_info, implicit_deps):
merged_deps = java_common.merge(deps_java_info + implicit_deps)
Expand Down Expand Up @@ -300,10 +298,16 @@ def _scrooge_scala_library_impl(ctx):
all_java = java_common.merge(exports + [aspect_info.java_info])
else:
all_java = aspect_info.java_info
return [
DefaultInfo(files = aspect_info.output_files),
ScroogeInfo(aspect_info = aspect_info), all_java
]

# TODO: Remove `scala` field once JavaInfo supports multiple
# output jars. https://github.com/bazelbuild/rules_scala/issues/564
return struct(
scala = _create_scala_struct(ctx), # For IntelliJ support
providers = [
all_java,
ScroogeInfo(aspect_info = aspect_info),
DefaultInfo(files = aspect_info.output_files)
])

scrooge_scala_library = rule(
implementation = _scrooge_scala_library_impl,
Expand All @@ -314,6 +318,21 @@ scrooge_scala_library = rule(
provides = [DefaultInfo, ScroogeInfo, JavaInfo],
)

def _create_scala_struct(ctx):
"""Create a scala provider in the shape expected by the IntelliJ bazel plugin."""
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we are transitively putting things here?

This part is confusing to me because it seems some jars will end up in many targets.

Why doesn't this work to just put class_jar from the current target? Why do we need the transitive part?

Copy link
Contributor Author

@beala-stripe beala-stripe Jul 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It's transitive in the sense that it pulls jars from the scrooge_scala_library's deps. Is that what you mean? I'm not sure I understand the question. Let me explain my understanding and see if that clears it up or if I'm mistaken:

My understanding is, suppose we have some scrooge target:

scrooge_scala_library(
    name = "my_scrooge",
    exports = [":some_export"],
    deps = [":thrift1", ":thrift2"]
)

The aspect will traverse the deps, call scrooge against each target's sources, produce a jar for each, and store a reference to those jars in each dep's ScroogeAspectInfo.java_info. So the result is :thrift1 and :thrift2 have jars that I need to return to IJ stored in their ScroogeAspectInfo.java_info.outputs.

This function gathers up all those jars by iterating through :thrift1 and :thrift2, pulls the jars out of ScroogeAspectInfo.java_info.outputs and puts them in a place IJ can find them.

It also gets the jars for :some_export since those symbols might be used in the editor.

output_jars = []

for dep in ctx.attr.deps:
for j in dep[ScroogeAspectInfo].java_info.outputs.jars:
output_jars.append(
struct(
class_jar = j.class_jar,
ijar = None,
source_jar = None,
source_jars = []))

return struct(outputs = struct(jars = output_jars))

def _scrooge_scala_import_impl(ctx):
scala_jars = depset(ctx.files.scala_jars)
jars_ji = java_common.create_provider(
Expand Down