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 3 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
55 changes: 42 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
Copy link
Contributor

Choose a reason for hiding this comment

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

We’re trying to kill this integration in favor of JavaInfo. Is there a specific issue this solves and which JavaInfo doesn’t solve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly, yes. In order to use JavaInfo here, I need a way of setting multiple output jars on a JavaInfo, but neither the JavaInfo constructor nor the java_common.create_provider allow me to do this.

Once this fix is released, we can ditch the scala field and go back to just returning a JavaInfo with multiple output jars.

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. WDYT about adding a TODO and an issue to track this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Created #564 and added a todo comment.

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,31 @@ 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 = []

if ctx.attr.exports:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why we want the exports here. The exports should be another target, so can't the intellij plugin see those? I don't even think we are using exports in scrooge anywhere internally.

Can we remove this part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup I think you're right. Removed.

for exp in ctx.attr.exports:
for j in exp[JavaInfo].outputs.jars:
output_jars.append(
struct(
class_jar = j.class_jar,
ijar = None,
source_jar = None,
source_jars = []))

for dep in ctx.attr.deps:
Copy link
Contributor

Choose a reason for hiding this comment

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

okay, I guess this is correct, since the deps are actually the thrifts that we are going to trigger generation of, so pretending that this target created them, I guess is okay (rather than the aspect generating them).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the aspects generate a Java/Scala provider is an interesting idea. I tried implementing this, but it doesn't seem like aspects support returning an old style struct provider. I tried returning a struct with a scala field from the scrooge aspect, but it wasn't visible in the IJ plugin.

Once the java_common.merge fix is released, the scrooge aspect could attach a JavaInfo with multiple outputs to the thrift_library targets.

Copy link
Contributor

Choose a reason for hiding this comment

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

always a yak stack....

next year in Jerusalem.

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