[WIP] Scala import java info #517

anchlovi · 2018-06-12T14:18:34Z

This changes scala_import to use JavaInfo

anchlovi · 2018-06-12T14:20:11Z

Currently this break strict deps:

'bazel build test_expect_failure/scala_import:scala_import_propagates_compile_deps' should have logged "buildozer 'add deps //test_expect_failure/scala_import:cats' //test_expect_failure/scala_import:scala_import_propagates_compile_deps".

ittaiz

Haven't finished but thought you'd like the WIP thoughts as well

ittaiz · 2018-06-12T20:41:06Z

scala/scala_import.bzl

+    exports_provider = _labels_to_provider(ctx.attr.exports)
+
+    jars2labels = _collect_jar_labels(ctx)
+    _add_labels_of_current_code_jars(depset(transitive=[jars_provider.full_compile_jars, exports_provider.full_compile_jars]), ctx.label, jars2labels) #last to override the label of the export compile jars to the current target


I think you want "regular" compile jars here and not "full"

ittaiz · 2018-06-12T20:41:58Z

scala/scala_import.bzl

-  transitive_compile_jars = []
-  runtime_jars = []
-  compile_jars = []
+def _labels_to_provider(labels):


labels here is overloaded. I thought it's the actual labels. Any other ideas?

This the purpose of this method - take a list of labels and merge them to a single JavaInfo

ittaiz · 2018-06-12T20:42:39Z

scala/scala_import.bzl

+    if JavaInfo in jar:
+      fail("jars must contain only jar files")
+
+    providers.append(_jar_to_provider(jar.files.to_list()[0]))


why only take the first file?

WIP - the plan is to use only the code files

ittaiz · 2018-06-13T05:28:37Z

scala/scala_import.bzl

+  return java_common.merge(providers)
+
+def _jar_to_provider(jar):
+  return JavaInfo(


Why are you building the JavaInfo like this (without the deps, exports,runtime_deps)?
I think that's not the correct semantics

At this point there is only a jar file - not a JavaInfo

ittaiz

I think I'm not done yet but I have a hunch that the changes that might come from the comments I've added so far will change the code greatly so I'm not sure I should continue before we discuss some more

ittaiz · 2018-06-13T05:30:14Z

scala/scala_import.bzl

+  providers = []
+  for jar in jars:
+    if JavaInfo in jar:
+      fail("jars must contain only jar files")


should we have a test for this negative case?

and also the error message is about what it should contain but the check is about whether or not this has JavaInfo.
Isn't there a mix? Maybe have a failure here to say targets of jvm_rules aren't allowed here and add another check below to see that the files are only jar files

johnynek

Thanks for this cleanup. Is it still a WIP?

Can you merge with master? There have been a number of merged PRs.

ittaiz · 2018-06-30T04:24:07Z

This is blocked since the JavaInfo skylark API doesn’t support creating a JavaInfo of multiple output jars. There’s a thread about it in bazel jvm sig.

…

On Sat, 30 Jun 2018 at 1:34 P. Oscar Boykin ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks for this cleanup. Is it still a WIP? Can you merge with master? There have been a number of merged PRs. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#517 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFwv7PzrjuKWM7fGrrX2RE8-90iYTks5uBquFgaJpZM4Ukheu> .

johnynek · 2018-06-30T04:53:43Z

Can’t we make two JavaInfo instances and merge them? What does that result in? On Fri, Jun 29, 2018 at 21:24 Ittai Zeidman <notifications@github.com> wrote:

This is blocked since the JavaInfo skylark API doesn’t support creating a JavaInfo of multiple output jars. There’s a thread about it in bazel jvm sig. On Sat, 30 Jun 2018 at 1:34 P. Oscar Boykin ***@***.***> wrote: > ***@***.**** commented on this pull request. > > Thanks for this cleanup. Is it still a WIP? > > Can you merge with master? There have been a number of merged PRs. > > — > You are receiving this because you commented. > > > Reply to this email directly, view it on GitHub > < #517 (review) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/ABUIFwv7PzrjuKWM7fGrrX2RE8-90iYTks5uBquFgaJpZM4Ukheu > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#517 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdm3vtmrkA6DbG2EbZiNKWPdG2x7Tks5uBv1ogaJpZM4Ukheu> .

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

ittaiz · 2018-06-30T05:01:11Z

No, merge drops the output jars. See here for more info https://groups.google.com/forum/#!topic/bazel-sig-jvm/THl2DRqyYW0 On Sat, Jun 30, 2018 at 7:53 AM P. Oscar Boykin <notifications@github.com> wrote:

…

Can’t we make two JavaInfo instances and merge them? What does that result in? On Fri, Jun 29, 2018 at 21:24 Ittai Zeidman ***@***.***> wrote: > This is blocked since the JavaInfo skylark API doesn’t support creating a > JavaInfo of multiple output jars. There’s a thread about it in bazel jvm > sig. > On Sat, 30 Jun 2018 at 1:34 P. Oscar Boykin ***@***.***> > wrote: > > > ***@***.**** commented on this pull request. > > > > Thanks for this cleanup. Is it still a WIP? > > > > Can you merge with master? There have been a number of merged PRs. > > > > — > > You are receiving this because you commented. > > > > > > Reply to this email directly, view it on GitHub > > < > #517 (review) > >, > > or mute the thread > > < > https://github.com/notifications/unsubscribe-auth/ABUIFwv7PzrjuKWM7fGrrX2RE8-90iYTks5uBquFgaJpZM4Ukheu > > > > . > > > > — > You are receiving this because you commented. > > > Reply to this email directly, view it on GitHub > < #517 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAEJdm3vtmrkA6DbG2EbZiNKWPdG2x7Tks5uBv1ogaJpZM4Ukheu > > . > -- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#517 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF5PghNxkB6Afw_afSY71KwMb648Iks5uBwRZgaJpZM4Ukheu> .

johnynek · 2019-01-30T18:12:13Z

@anchlovi what is the status here?

I think JavaInfo issues have been fixed, if I'm not mistaken. Would you have time to merge master and see what the state is?

I appreciate your help.

johnynek · 2019-07-13T18:54:59Z

should we close this one or will you have time to merge master and take another stab at it.

@ittaiz is this one of your team?

ittaiz · 2019-07-13T19:42:43Z

Shachar is indeed on the build at Wix. I'll close this as #781 handled scala_import as well

Shachar Anchelovich added 5 commits June 11, 2018 11:17

initial

51e7b05

Initial

39ab66c

Merge providers

1d98f63

Extract jar2labels

5bee6c0

Move to JavaInfo

e64b994

googlebot added the cla: yes label Jun 12, 2018

ittaiz reviewed Jun 12, 2018

View reviewed changes

ittaiz reviewed Jun 13, 2018

View reviewed changes

Shachar Anchelovich added 2 commits June 13, 2018 10:11

Create JavaInfo for code jars only

1ee6536

User regular compile jars

584b7d2

johnynek reviewed Jun 29, 2018

View reviewed changes

ittaiz closed this Jul 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Scala import java info #517

[WIP] Scala import java info #517

anchlovi commented Jun 12, 2018

anchlovi commented Jun 12, 2018

ittaiz left a comment

ittaiz Jun 12, 2018

ittaiz Jun 12, 2018

anchlovi Jun 13, 2018

ittaiz Jun 12, 2018

anchlovi Jun 13, 2018

ittaiz Jun 13, 2018

anchlovi Jun 13, 2018

ittaiz left a comment

ittaiz Jun 13, 2018

ittaiz Jun 13, 2018

johnynek left a comment

ittaiz commented Jun 30, 2018 via email

johnynek commented Jun 30, 2018 via email

ittaiz commented Jun 30, 2018 via email

johnynek commented Jan 30, 2019

johnynek commented Jul 13, 2019

ittaiz commented Jul 13, 2019

[WIP] Scala import java info #517

[WIP] Scala import java info #517

Conversation

anchlovi commented Jun 12, 2018

anchlovi commented Jun 12, 2018

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

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

johnynek left a comment

Choose a reason for hiding this comment

ittaiz commented Jun 30, 2018 via email

johnynek commented Jun 30, 2018 via email

ittaiz commented Jun 30, 2018 via email

johnynek commented Jan 30, 2019

johnynek commented Jul 13, 2019

ittaiz commented Jul 13, 2019