JavaInfo.merge and output_jars

[ittai zeidman]

Hi,

We're trying to move scala_import to use JavaInfo and we've encountered a small bump in the road.

scala_import accepts `jars` as the thing it imports (modeled after java_import) but JavaInfo in its skylark form allows only one output_jar.

One can create a JavaInfo for each jar and then merge them but I think (not 100% sure) this messes up the output_jar.

In addition we also want to move scala_library to use JavaInfo and we'll need to merge there as well since we have both scalac and javac JavaInfos.

What mainly triggered this mail is this comment from JavaInfo.java:

// When a rule merges multiple JavaProviders, its purpose is to pass on information, so

// it doesn't have any output jars.

Maybe the question is just about the role/significance of the output_jar?

Would appreciate your thoughts and comments,

Ittai

[Lukács T. Berki]

You are probably confused by the concept "output jar", which is not a surprise, because it's a confusing concept :)

JavaRuleOutputJarsProvider#getOutputJars() provides the jar produced by that particular rule *only*. It's there so that aspects (e.g. the one that converts .jar files to .dex files) can process each jar exactly once. java_import is a special case in that it produces multiple output jars. 

I think the right answer is to make it possible to create JavaRuleOutputJarsProvider instances from Skylark that have multiple output jars (AFAICS that's not possible right now) The right API, I'll defer to the Java folks I just added to this thread.

--
You received this message because you are subscribed to the Google Groups "Bazel/JVM Special Interest Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-sig-jv...@googlegroups.com.
To post to this group, send email to bazel-...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/CAOfK4wVFqF-KmiAvREKfRfbqNcu5_6Kvbh5jbS4OxABq-%3D%3DroQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--

Lukács T. Berki | Software Engineer | lbe...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

[ittai zeidman]

Thanks!
Would it be incorrect in the above cases (scala_import with jars and scala_library with Java jar and Scala jar) to have multiple JavaInfo instances and then merge them?

Sounds to me like if the output jar is only there for aspects ( and not for example to control compile deps) then it might be redundant to create many JavaInfo instances and then merge them but it should be correct.

Wdyt?

[Tom Lundell]

On Wed, Jun 13, 2018 at 2:00 PM ittai zeidman <itt...@gmail.com> wrote:

Thanks!
Would it be incorrect in the above cases (scala_import with jars and scala_library with Java jar and Scala jar) to have multiple JavaInfo instances and then merge them?

Sounds to me like if the output jar is only there for aspects ( and not for example to control compile deps) then it might be redundant to create many JavaInfo instances and then merge them but it should be correct.

Wdyt?

On Wed, 13 Jun 2018 at 15:39 Lukács T. Berki <lbe...@google.com> wrote:

You are probably confused by the concept "output jar", which is not a surprise, because it's a confusing concept :)

JavaRuleOutputJarsProvider#getOutputJars() provides the jar produced by that particular rule *only*. It's there so that aspects (e.g. the one that converts .jar files to .dex files) can process each jar exactly once. java_import is a special case in that it produces multiple output jars. 

I think the right answer is to make it possible to create JavaRuleOutputJarsProvider instances from Skylark that have multiple output jars (AFAICS that's not possible right now) The right API, I'll defer to the Java folks I just added to this thread.

Yes, that would be the way to do it. java_import does present multiple output jars today and can do so because it has access to this native API.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/CAOfK4wWAGNCjqzhceDK9mFamUM-X0dGCeQsToF94dfHF%2B0Pn5g%40mail.gmail.com.

[ittai zeidman]

“That” being Lukacs’s suggestion?
If so does this mean my above suggestion is incorrect as a stopgap?

[Tom Lundell]

On Wed, Jun 13, 2018 at 3:28 PM ittai zeidman <itt...@gmail.com> wrote:

“That” being Lukacs’s suggestion?

Yes

 

If so does this mean my above suggestion is incorrect as a stopgap?

It wasn't a bad thought for a workaround, but unfortunately when you merge JavaInfo instances it _drops_ the output jars from all of them. So it will have the opposite effect :(

[ittai zeidman]

I see.
So currently it sounds like we’re blocked from moving to JavaInfo.

Anyone know what is the deprecation plan for the two existing constructors (java_common and the intermediate JavaInfo)? 

rules_scala can’t move to support 0.14 since it tries to run with all deprecation flags on but this sounds like it’s blocked.

[Tom Lundell]

On Wed, Jun 13, 2018 at 3:43 PM ittai zeidman <itt...@gmail.com> wrote:

I see.
So currently it sounds like we’re blocked from moving to JavaInfo.

If that's the case we should fix it. Is anything tracking this?

I had a look, and the API will be a little uncomfortable to write since we have to pair up the jar / compile jar. So we can't just change it to accept an output_jars parameter.

 

Anyone know what is the deprecation plan for the two existing constructors (java_common and the intermediate JavaInfo)? 

The flag is there, but we won't delete the old behaviour until users (including yourself) are able to migrate.

[ittai zeidman]

No. I actually thought it’s most likely an inconvenience but not a blocker.
I’ll open one in the morning

[Tom Lundell]

Actually does anyone know *why* we allow java_import to include multiple jars? Ittai, why would your rule need that? I haven't come across the use case myself in the wild.

[Liam Miller-Cushon]

On Wed, Jun 13, 2018 at 5:26 PM 'Tom Lundell' via Bazel/JVM Special Interest Group <bazel-...@googlegroups.com> wrote:

Actually does anyone know *why* we allow java_import to include multiple jars? Ittai, why would your rule need that? I haven't come across the use case myself in the wild.

Theoretically if we start enforcing strict deps for java_import, there could be cyclic references between two jars that would require putting them in the same import.

Practically, I don't know of any reasons to support multiple jars today, but if support was removed we'd need more export-only java_library rules to aggregate related jars.

Making java_import take a single jar would also solve #3651 (java_import has many jars and one srcjar).

[Kevin Bierhoff]

I think it would be nice to give Skylark rules the ability to "easily" produce multiple output Jars.  Besides import-like rules, this would be useful for rules that "naturally" produce two Jars, as for instance the native android_library rule does.  More generally, any rule that has multiple compile-like actions could take advantage.  I'm not sure whether that's best done by supporting multiple output_jars in JavaInfo's constructor or another mechanism, but as I said I think it would be good to allow this especially given there are several native rules (java_import, android_library) that use this feature.  It's also just confusing IMHO that the native providers support multiple Jars but JavaInfo doesn't.

[Tom Lundell]

On Wed, Jun 13, 2018 at 9:24 PM 'Kevin Bierhoff' via Bazel/JVM Special Interest Group <bazel-...@googlegroups.com> wrote:

I think it would be nice to give Skylark rules the ability to "easily" produce multiple output Jars.  Besides import-like rules, this would be useful for rules that "naturally" produce two Jars, as for instance the native android_library rule does.  More generally, any rule that has multiple compile-like actions could take advantage.  I'm not sure whether that's best done by supporting multiple output_jars in JavaInfo's constructor or another mechanism, but as I said I think it would be good to allow this especially given there are several native rules (java_import, android_library) that use this feature.  It's also just confusing IMHO that the native providers support multiple Jars but JavaInfo doesn't.

If there's a good reason for it, then we'll do it. I think it should be in the constructor of JavaInfo.

It's not hard, it's just awkward to achieve with a single constructor and named parameters. People sometimes claim Java builders are a kludge because Java doesn't have named or optional parameters, but this is actually a circumstance that would be perfect for a builder. It's uncomfortable to provide an API where you have to pass matching jars and compile jars in some sort of list or dict.

Do you say "pass a list of tuples of jars and compile jars"? Or "pass one list of jars and one list of compile jars and the indices have to match up and the lists have to be the same length"? Or "pass a list of dicts with these required keys"?

 

On Wednesday, June 13, 2018 at 5:34:24 PM UTC-7, Liam Miller-Cushon wrote:

On Wed, Jun 13, 2018 at 5:26 PM 'Tom Lundell' via Bazel/JVM Special Interest Group <bazel-...@googlegroups.com> wrote:

Actually does anyone know *why* we allow java_import to include multiple jars? Ittai, why would your rule need that? I haven't come across the use case myself in the wild.

Theoretically if we start enforcing strict deps for java_import, there could be cyclic references between two jars that would require putting them in the same import.

Practically, I don't know of any reasons to support multiple jars today, but if support was removed we'd need more export-only java_library rules to aggregate related jars.

Making java_import take a single jar would also solve #3651 (java_import has many jars and one srcjar).

--

You received this message because you are subscribed to the Google Groups "Bazel/JVM Special Interest Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-sig-jv...@googlegroups.com.
To post to this group, send email to bazel-...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/ddb55888-255a-4bde-a2b3-ba3b8a4c9683%40googlegroups.com.

[ittai zeidman]

1. I don’t think we need multiple, we started with one and changed to many to comply with java_import.
2. scala_library needs multiple since it generates a jar from java code and scala code which can be in a cycle.
3. I’d go for passing a dict

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/CAPvN2rWPA_qC0GT9da68WUzt5jG6HfF2EZr99v3BuySrVm1F3A%40mail.gmail.com.

[Lukács T. Berki]

Do we need a dict? JavaCompilationArgsProvider only has separate nested sets for the compile time/runtime jars.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/CAOfK4wWPKbUSGQHLh3j7i9FeYfMvejN-VjS8jrpkpPZZFRFqVw%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

[Irina Iancu]

Hi, sorry for joining the party so late. 

I am reluctant about passing multiple output jars to a JavaInfo constructor, in any form. As Tom mentioned it is difficult to design such a constructor. The current status of JavaInfo reached a much cleaner version. The problem of getting the output jars after merging multiple providers can be solved by java_common.merge adding all the output jars to JavaRuleOutputJarsProvider instead of returning an empty one. This is also what java_import does so it's reasonable.

When java_common.merge was created the java_common API was different and it introduced questions like "what does it really mean to merge these providers"? The question still persists. It's necessary to avoid just throwing jars at JavaInfo and aim for JavaInfo to have a meaning instead. The meaning is that JavaInfo encapsulates everything that is related to a compilation (output jar, ijar, dependencies that were used, etc) that resulted to a certain output jar. In theory having 2 jars (e.g. one as a result of javac and one as a result of scalac) means there should be 2 different providers. Merging them together breaks the meaning of JavaInfo. In otrder to avoid this both JavaInfos need to be returned in a Skylark dictionary and be properly retrieved and from the consumer rule.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/CAOu%2B0LVU-9ywPi9XM_ZU0yGjchADseSheNqsf%2BRZvJtkmEo%3DNg%40mail.gmail.com.

[ittai zeidman]

On Mon, 2 Jul 2018 at 11:59 Irina Iancu <elena...@google.com> wrote:

Hi, sorry for joining the party so late. 

I am reluctant about passing multiple output jars to a JavaInfo constructor, in any form. As Tom mentioned it is difficult to design such a constructor. The current status of JavaInfo reached a much cleaner version. The problem of getting the output jars after merging multiple providers can be solved by java_common.merge adding all the output jars to JavaRuleOutputJarsProvider instead of returning an empty one. This is also what java_import does so it's reasonable.

When java_common.merge was created the java_common API was different and it introduced questions like "what does it really mean to merge these providers"? The question still persists. It's necessary to avoid just throwing jars at JavaInfo and aim for JavaInfo to have a meaning instead. The meaning is that JavaInfo encapsulates everything that is related to a compilation (output jar, ijar, dependencies that were used, etc) that resulted to a certain output jar

I have to say I disagree. This is one possible meaning which honesty I think is wrong. JavaInfo is a provider which aims to convey information about a target, not about specific actions of the rule.

I think JavaInfo should convey jvm specific information between jvm rules and for jvm related tools (IDEs) so that they can interop with it. For this purpose both scalac output and javac output serve the same purpose and are part of the same “object”.

Concretely we are blocked for quite some time and can’t progress in all scala_* rules

[P. Oscar Boykin]

I think one issue that is coming up was something I noticed with providers while recently implementing a few rules:

1. Merge is something we want on any provider where you can accept more than one input (a commutative operation, typically called a Semigroup).

2. If we allow empty inputs, we should have a way to construct and empty provider (the identity element of a commutative monoid). This should be the same as merging an empty list.

3. There is a difference between output position and input position. This is like variance in many programming languages. We typically consume one provider but we can supply many.

Combining these: JavaInfo is about input. It is what rules use to build compile and runtime classpaths. That’s it. That is not the whole story about a rule output. It may produce one or more jars. Those can be described by a different provider because that is irrelevant for consuming as an input dependency: for that we only need to know our compile and runtime classpaths. For the scala, we allow mixed java and scala compilation targets where we have dependencies between scala and java. However, the way this is implemented in scalac, you will produce two jars: the scala jar and the result of us called java_common.compile using the output of the scala as a dep (scalac handles the circular dependency by parsing the java but not emitting java class files). So, we can’t both use JavaInfo as the output of such a rule and merge the java output since we lose the output jars. I guess that is a bad pattern anyway and we need to augment our output to include an output jars provider.

I have not studied JavaRuleOutputJarsProvider but this seems to follow the pattern above: use a different provider for different applications. So, we want to make sure we can construct JavaRuleOutputJarsProvider if we expect to use this to interop. I’m not sure we need to though. We can make ScalaRuleOutput that has our output and then other can transform in their own rule as needed.

Lastly, I have realized writing rules is pretty easy if you only plan macro-level interoperability, but composing Providers requires skylark compositions so you need to keep your skylark rules very clean and composable or otherwise you immediately hit issues when you want to do anything non-trivial. Also, as we have seen, versioning in skylark is harder than the rule syntax exposed to macros. Frequently changing skylark APIs are a major headache for us as it creates a compatibility matrix problem (which Foo rules can work with Bar rules in bazel Quux).

A comment: if we conceive of JavaInfo as the info needed to build classpaths (things that go in deps, runtime_deps and exports), then source_jars is misplaced there as are output jars. I assume the source jars are for IDEs to access code that might have been generated by codegen. We can imagine JvmSourceJarProvider which can give you a depset of generated source jars and maybe a set of languages generated, or perhaps a dict of language to jar that may contain code of that language (really we have a bipartite graph of language and jar, but maybe we want to require that each jar can only contain a single language (java, scala, kotlin, clojure, ...)

Thanks for reading and your comments.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/CAOfK4wWpdZM1tmX%2BZnWQFdUag5noEsvoQ8URRhtYam3M_cjWsg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

[itt...@wix.com]

I think there's a lot of value in what you're saying Oscar.

One small note is that AFAIU the current JavaInfo is actually a composition of providers (at least internally in the java code) and maybe this is the root of the problem.

That, like you say, we need to have one provider for interoperability between JVM rules (compile, runtime, neverlink, exports) and another one for tools (source jars, output jars, etc).

Irina,

WDYT?

[P. Oscar Boykin]

It seems to me that neverlink is about *how* we construct runtime and compile time paths but does not need to be added to JavaInfo fields. Neverlink, as I understand, is when you put something on the compile time path, but not on the runtime path.

Similarly with exports: exports are just JavaInfo objects you merge ino the output JavaInfo, no special handling needed.

As far as I can see, we really only need compile time, runtime and transitive versions of these. I think we also have full_compile_time (and transitive) if we want to supply the full jars, but not sure why that is needed.

I am not sure why we need JavaRuleOutputJars instead of just DefaultInfo. I’d like to know that.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/d9767d06-d088-4ca1-a982-e37d46457f8f%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Tom Lundell]

On Wed, Jul 4, 2018 at 11:01 AM P. Oscar Boykin <oscar....@gmail.com> wrote:

It seems to me that neverlink is about *how* we construct runtime and compile time paths but does not need to be added to JavaInfo fields. Neverlink, as I understand, is when you put something on the compile time path, but not on the runtime path.

Similarly with exports: exports are just JavaInfo objects you merge ino the output JavaInfo, no special handling needed.

As far as I can see, we really only need compile time, runtime and transitive versions of these. I think we also have full_compile_time (and transitive) if we want to supply the full jars, but not sure why that is needed.

I am not sure why we need JavaRuleOutputJars instead of just DefaultInfo. I’d like to know that.

It's for tool support. The bazel IntelliJ plugin introspect rules to collect jars to add to the project when you're importing it.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/CAE6AC97k4V2Mxt%3D2k9DY1eMeP5kABFRumdwgrWpyZdK7o0Brmg%40mail.gmail.com.

[Ittai Zeidman]

Oscar,

It sounds to me a bit like your trying to get back to the original JavaInfo which required you to pass in direct compile jars, direct runtime jars, transitive compile jars, transitive runtime jars and maybe others.
There were a few problems with this API, and I hope +Elena-Irina Iancu can elaborate, but basically it required doing a lot of jar handling in each jvm rule.

If you want a higher level abstraction it needs semantic meaning which very shortly leads to supporting neverlink, exports, etc.

Maybe Irina can also share the original design doc that moved us to more semantic meaning. 

You received this message because you are subscribed to a topic in the Google Groups "Bazel/JVM Special Interest Group" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/bazel-sig-jvm/THl2DRqyYW0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to bazel-sig-jv...@googlegroups.com.

To post to this group, send email to bazel-...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/CAPvN2rV%2B_%3D%2BzJ_1bqbdSHmtwpc1nUb__odKCynUZgNdnwuH72A%40mail.gmail.com.

[Irina Iancu]

Thanks everyone for contributing to this discussion. My answers are below.

 

Maybe Irina can also share the original design doc that moved us to more semantic meaning. 

Here is the design document with the API changes and it explains why the old API was changed. 

All JVM rules need to return the same provider so they can be recognized as JVM rules. The rule implementations would become more complicated if they have to deal with different input and output providers.

I have to say I disagree. This is one possible meaning which honesty I think is wrong. JavaInfo is a provider which aims to convey information about a target, not about specific actions of the rule.

Well yes and no. I agree the provider should not be about a specific action but it is more about the concept. It's not about a specific bazel action, but about all the actions in the given target. However, all other actions have as input the main output jar that was the output of a compilation. Let me rephrase: Creating a JavaInfo will encapsulate everything that is related to a compilation (output jar, ijar, dependencies that were used, etc) that resulted to a certain output jar. This way JavaInfo will internally know which artifacts are related and how. After creation JavaInfo can not have this meaning anymore (e.g. by merging multiple JavaInfos).

A target can have multiple implicit outputs. In the java_library case you know which these are and they are conveniently exposed via JavaInfo. In the case of other JVM language, like scala, the scalac output jar is very similar as concept to the javac output jar, but it is another implicit output. Creating 2 JavaInfos and merging them together is the correct way to do it. 

There is an easy solution here that requires fixing java_common.merge, mostly by merging the JavaRuleOutputJarsProvider (this was not possible with the old API). This is feasible now because JavaInfo knows which output jar corresponds to which source jar/ijar. I am happy to take care of this and unblock you.

[Ittai Zeidman]

Thanks for your reply.
We’ve had a long thread so I want to make sure I understand your suggestion: you’re suggesting to fix java_common.merge so that it will expose the output jars of the underlying JavaInfo instances?

Additionally are there more elements merge “‘messes” up? Are exports preserved?

[Irina Iancu]

you’re suggesting to fix java_common.merge so that it will expose the output jars of the underlying JavaInfo instances? 

Yes. This will affect JavaInfo.outputs. For example, considering 

finalJavaInfo = java_common.merge([javaInfo, scalaInfo]) 

where javaInfo was created using java_common.compile() and scalaInfo using JavaInfo() then finalJavaInfo.outputs.jars contains both the jars from javaInfo and scalaInfo. 

• finalJavaInfo.outputs.jars[0].class_jar will have the jar obtained from the java compilation 
  
• scalaInfo.finalJavaInfo.outputs.jars[1].class_jar is the jar obtained from the scala compilation 

This is correct as finalJavaInfo has all the outputs jars that were created in this target.

Additionally are there more elements merge “‘messes” up? Are exports preserved?

Exports are preserved. I'll do a round of going through all providers and add some if they are missing. I don't see anything obvious that is messed up right now.

 

[P. Oscar Boykin]

Fixing to preserve outputs on merge, I think will work for us in rules_scala to correctly interop with the intellij rules.

What is the timeline on this change?

[Irina Iancu]

The fix is already submitted in this commit.

[be...@stripe.com]

Thanks for your work on this, Irina! Your fix will be very useful to me on something I'm working on (getting IntelliJ to support the new implementation of scrooge_scala_library). Is there an ETA on when your fix will land in a release? I found the ticket for 0.16.0. Would it be best for me to request it in there?

Thanks!

Alex

[Ittai Zeidman]

It will be in 0.17.0
See Bazel’s release policy for more details

You received this message because you are subscribed to a topic in the Google Groups "Bazel/JVM Special Interest Group" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/bazel-sig-jvm/THl2DRqyYW0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to bazel-sig-jv...@googlegroups.com.

To post to this group, send email to bazel-...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/3c821c72-4cbe-4623-b022-8deebfc30627%40googlegroups.com.

[Irina Iancu]

Hi Alex,

Thanks for your interest! As Ittai said this will land in 0.17.0 (see 0.17.0 release issue for tracking progress). We cherry-pick only regressions after a release candidate was already cut and this commit doesn't qualify as one.

Cheers,

Irina

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-sig-jvm/CAFRCsYTp%3D-%2B7RCyE1xbg1W7_Ve6CwPMwa1_rSRuEWLJyx2gQUw%40mail.gmail.com.