Fix Intellij Bazel Plugin support for scrooge_scala_library rule #562

beala-stripe · 2018-07-17T18:21:04Z

Overview

This fixes intelliJ support for scrooge_scala_library targets using the new aspect based implementation (#524). Without this fix, symbol resolution and jump-to-def support does not work.

Previously, the scrooge_scala_library rule would return a scala field on its provider struct, which contained output jars (full path: scala -> outputs -> [].jars -> class_jar). This IJ plugin would inspect this field and collect the jars for IDE support.

The new scrooge_scala_library does not return a scala field, breaking IJ support. It does return a JavaInfo provider, which the IJ plugin does pick up, but it lacks an outputs field. One seemingly straightforward solution would be to simply set the outputs field on the existing JavaInfo provider. There are two reasons this doesn't work:

There is at least one place several JavaInfos need to be merged together, and the current implementation of java_common.merge strips the outputs field. There is a fix forthcoming for this, but it's not landing until 0.16.0.
There is no way AFAIK for setting multiple output_jars on a JavaInfo. The JavaInfo constructor only accepts one output jar and java_common.create_provider doesn't set the outputs field at all.

The solution in this PR threads the output jars through and puts them in the scala field that the IJ plugin expects. Concretely, this means using the JavaInfo constructor to set output_jar explicitly, and going back to old style providers, where the rule implementation returns a struct rather than a list of providers.

I believe this is an acceptable fix because once the java_common.merge fix is released in 0.16.0, the scala field can be removed in favor of putting the jars JavaInfos outputs, and we can go back to new-style providers (returning a list rather than a struct).

Testing

Tests pass (./test_all.sh) and I've verified the fix works with the IJ plugin on our repos at Stripe.

That said, this is my first bazel rules PR and would appreciate extra scrutiny, in particular in making sure I've constructed all the JavaInfos correctly.

johnynek

Can we add anything to the intellij test to make sure we don’t backslide here?

ittaiz

Thanks!

ittaiz · 2018-07-18T03:53:58Z

twitter_scrooge/twitter_scrooge.bzl

+      source_jar = scrooge_jar,
+      deps = deps_java_info + implicit_deps,
+      runtime_deps = deps_java_info + implicit_deps,
+      exports = deps_java_info + implicit_deps,


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)

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

@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

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.

ittaiz · 2018-07-18T03:54:31Z

twitter_scrooge/twitter_scrooge.bzl

+      runtime_deps = deps_java_info + implicit_deps,
+      exports = deps_java_info + implicit_deps,
+      output_jar = output,
+      compile_jar = output)


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

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

ittaiz · 2018-07-18T03:55:48Z

twitter_scrooge/twitter_scrooge.bzl

-  ]
+
+  return struct(
+      scala = _create_scala_struct(ctx),  # For IntelliJ support


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

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.

sounds good. WDYT about adding a TODO and an issue to track this?

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

beala-stripe · 2018-07-18T17:58:18Z

@johnynek: A splendid idea: bazelbuild/intellij#366

beala-stripe

Thanks for the quick feedback @ittaiz! Much appreciated!

beala-stripe · 2018-07-18T18:16:07Z

twitter_scrooge/twitter_scrooge.bzl

-  ]
+
+  return struct(
+      scala = _create_scala_struct(ctx),  # For IntelliJ support


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.

beala-stripe · 2018-07-18T18:16:19Z

twitter_scrooge/twitter_scrooge.bzl

+      runtime_deps = deps_java_info + implicit_deps,
+      exports = deps_java_info + implicit_deps,
+      output_jar = output,
+      compile_jar = output)


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

beala-stripe · 2018-07-18T18:16:51Z

twitter_scrooge/twitter_scrooge.bzl

+      source_jar = scrooge_jar,
+      deps = deps_java_info + implicit_deps,
+      runtime_deps = deps_java_info + implicit_deps,
+      exports = deps_java_info + implicit_deps,


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

johnynek · 2018-07-18T22:46:55Z

Regarding exports, my thinking is: for schemas, all dependencies should are things that show up on the API: if schema foo depends on schema bar, the user of foo is going to need bar to use foo, pretty much 100% of the time. Requiring explicit dependencies, seems like just adding more toil.

Secondly: the motivation for thin dependencies is so you can avoid recomputation, but when a schema changes, almost always the API changes, so you don't get to avoid a recomputation, so the motivation from a caching perspective also disappears.

So, that's why I put the transitive dependencies into exports.

johnynek · 2018-07-18T22:51:12Z

twitter_scrooge/twitter_scrooge.bzl

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


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?

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.

johnynek · 2018-07-20T22:45:01Z

twitter_scrooge/twitter_scrooge.bzl

+  """Create a scala provider in the shape expected by the IntelliJ bazel plugin."""
+  output_jars = []
+
+  if ctx.attr.exports:


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?

Yup I think you're right. Removed.

johnynek · 2018-07-20T22:45:49Z

twitter_scrooge/twitter_scrooge.bzl

+                source_jar = None,
+                source_jars = []))
+
+  for dep in ctx.attr.deps:


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

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.

always a yak stack....

next year in Jerusalem.

Thread output jars thru so IntelliJ can pick them up.

aeb359c

googlebot added the cla: yes label Jul 17, 2018

Fix linting errors.

57661fa

johnynek reviewed Jul 17, 2018

View reviewed changes

ittaiz reviewed Jul 18, 2018

View reviewed changes

beala-stripe mentioned this pull request Jul 18, 2018

Add tests for scrooge_scala_library target type bazelbuild/intellij#366

Closed

beala-stripe commented Jul 18, 2018

View reviewed changes

beala-stripe mentioned this pull request Jul 18, 2018

scrooge_scala_library: Remove scala field on provider and instead return jars in JavaInfo provider #564

Open

Add todo to remove scala field/provider

58e4bc0

johnynek reviewed Jul 18, 2018

View reviewed changes

johnynek reviewed Jul 20, 2018

View reviewed changes

Do not collect exported jars.

4e71905

johnynek approved these changes Jul 24, 2018

View reviewed changes

johnynek merged commit 8359fc6 into bazelbuild:master Jul 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Intellij Bazel Plugin support for scrooge_scala_library rule #562

Fix Intellij Bazel Plugin support for scrooge_scala_library rule #562

beala-stripe commented Jul 17, 2018

johnynek left a comment

ittaiz left a comment

ittaiz Jul 18, 2018

beala-stripe Jul 18, 2018

ittaiz Jul 18, 2018

johnynek Jul 18, 2018

ittaiz Jul 18, 2018

beala-stripe Jul 18, 2018

ittaiz Jul 18, 2018

beala-stripe Jul 18, 2018

ittaiz Jul 18, 2018

beala-stripe Jul 18, 2018

beala-stripe commented Jul 18, 2018

beala-stripe left a comment

beala-stripe Jul 18, 2018

beala-stripe Jul 18, 2018

beala-stripe Jul 18, 2018

johnynek commented Jul 18, 2018

johnynek Jul 18, 2018

beala-stripe Jul 18, 2018 •

edited

Loading

johnynek Jul 20, 2018

beala-stripe Jul 23, 2018

johnynek Jul 20, 2018

beala-stripe Jul 23, 2018

johnynek Jul 24, 2018

Fix Intellij Bazel Plugin support for scrooge_scala_library rule #562

Fix Intellij Bazel Plugin support for scrooge_scala_library rule #562

Conversation

beala-stripe commented Jul 17, 2018

Overview

Testing

johnynek left a comment

Choose a reason for hiding this comment

ittaiz 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

beala-stripe commented Jul 18, 2018

beala-stripe 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

johnynek commented Jul 18, 2018

Choose a reason for hiding this comment

beala-stripe Jul 18, 2018 • edited Loading

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

beala-stripe Jul 18, 2018 •

edited

Loading