java rules should be able to depend on Skylark rules

[johnynek]

See:

https://github.com/bazelbuild/rules_scala/blob/master/test/BUILD#L15

The java libraries should probe to see if there are jars, transitive_runtime deps, etc...

[philwo]

Assigning to @laurentlb to get an idea how feasible this is.

[johnynek]

I would say there could be some contract that the java rule expects, and if skylark rules for scala, clojure, etc... follow that, the java rule would accept them.

This kind of jvm interop is pretty important for these other jvm languages.

[johnynek]

here is a possible work around approach:

bazelbuild/rules_scala#18

[davidzchen]

@johnynek - Just out of curiosity, why do the deps labels for some of the scala targets not have the leading : in https://github.com/bazelbuild/rules_scala/blob/master/test/BUILD#L15?

scala_library(
    name = "HelloLib",
    srcs = ["HelloLib.scala"],
    deps = [
        "OtherJavaLib",
        "OtherLib",
        "MacroTest"
    ],
)

[johnynek]

@davidzchen it worked and I started removing them (fewer characters the better). Should targets always have : in the same package? Why does this work if so?

[ulfjack]

We've generally been using ":" to indicate rules, and "" to
indicate files.

Bazel handles both kinds of references as label references relative to the
current package. It performs lookup by first checking for a rule or an
output file, and if neither exists, it assumes that you're referencing a
source file. At the point in time where that happens, it does not check if
the source file exists.

On Thu, Mar 3, 2016 at 10:29 AM, P. Oscar Boykin notifications@github.com
wrote:

@davidzchen https://github.com/davidzchen it worked and I started
removing them (fewer characters the better). Should targets always have :
in the same package? Why does this work if so?

—
Reply to this email directly or view it on GitHub
#970 (comment).

[davidzchen]

Thanks for the explanations!

I think using : to distinguish between rules and files makes it more immediately clear whether a label referenced is a rule or file and would save time when reading BUILD files and understanding the dependencies between targets. There are cases where you could have input files that have no file extensions, in which case it would look like a rule unless you use the leading :. It would be better to continue using the same convention of using : to distinguish between rules and files for consistency.

@ulfjack - Is there a particular reason why Bazel allows referencing rules without the leading :? Would it be a good idea for us to enforce this convention?

[johnynek]

+1 to stronger checks. Would be a nice bazelrc option to require that.

[ulfjack]

Yes, I think it would be possible to have a stricter check.

On Fri, Mar 4, 2016 at 12:21 AM, P. Oscar Boykin notifications@github.com
wrote:

+1 to stronger checks. Would be a nice bazelrc option to require that.

—
Reply to this email directly or view it on GitHub
#970 (comment).

[jcoveney]

This conversation got derailed a bit around the :label, but I am curious how we want to deal with interop between JVM languages

I agree with @johnynek that the best way would be to create a contract that jar producing things can follow, and then some tools to pick them up and build a classpath

[ulfjack]

Carmi started to work on a Skylark API for Java such that custom rules can interop with the built-in ones and that we can eventually rewrite the built-in rules in Skylark. It's not there yet, but it's coming.

[ulfjack]

Maybe we can publish the current proposal and have the discussion publically?

[johnynek]

That sounds great.

On Tuesday, May 24, 2016, Ulf Adams notifications@github.com wrote:

Maybe we can publish the current proposal and have the discussion
publically?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#970 (comment)

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

[ittaiz]

any news about this? I just stumbled on another use-case- I want to use the java_test rule from scala since my testing library (specs2) integrates with JUnit and that's how we work with it today.
If we had this then it's simply a matter of combining the two

[cgrushko]

I'm currently refactoring bazel/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibraryHelper.java to make it easier to expose.
@ulfjack, IIUC, actually allowing java_library --> Skylark java to interop is slightly tangent to this?

[ulfjack]

@dslomov is working on a more complete proposal. Dmitry, would you mind giving more details on that?

[ittaiz]

I know the importance of this is understood but just to give a small reminder- I want to add junit support for scala.
Naturally I tried to use composition and use scala_library and java_test until I remembered this issue...
So instead of something along the lines of:

def scala_junit_test(name, srcs, visibility = None, **kwargs):
    scala_library(
        name = name + "_junit_lib",
        srcs = srcs,
        visibility = ['//visibility:private'],
        **kwargs
    )
    native.java_test(
        name = name,
        srcs = java_srcs,
        deps = [":" + name + "_junit_lib"] + kwargs.get("deps",[]),
        visibility = visibility,
        **kwargs
    )

I'll have to go and reimplement junit running much like java and groovy rules already do

[lberki]

I see. Then, to be consistent, how about using java_common.compile() for compiling the Java code? Then you'd be automatically consistent with what Bazel otherwise does. Confusingly, it does not use @bazel_tools//tools/jdk:javac by default, but a javac bundled in it (called JavaBuilder)

[iirina]

@ittaiz Could you provide a little (non-working of course) demo for the scala_junit_test? Just a few Java and scala files and a BUILD fille to reflect how you would like the API to look like? It would make me more confident that my solution would work for your case and I would realize faster what needs to be done.

[ittaiz]

@iirina I actually have something working already in my fork
It's very basic but it shows the gist. I still need to evolve it to support class patterns.
It also uses JUnitCore and not the bazel runner which is not as feature rich.
I think that in essence this could be minimized to just building the scala library and generating the list of classes to run. I'd like to turn it into a macro but like I commented in #2539 it's unclear to me how to pass some of the info when in macro land.

[ittaiz]

@iirina please see my pull request to see how I worked around it. IIUC the main changes I'll need to make will be that in _scala_junit_test_impl I will replace the creation of the launcher and the scala binary with calling native.java_test and passing to it the generated test suite name and a jvm arg which is needed. Any news btw?

[iirina]

(sorry for the late reply!)

I'd like to turn it into a macro

I'm not sure what you would like to turn into a macro? scala_junit_test? I don't think that's feasible, as you'll have to make use of the java_common module and return providers.

I will replace the creation of the launcher and the scala binary with calling native.java_test and passing to it the generated test suite name and a jvm arg which is needed.

Are you asking about details on your current implementation of scala_junit_test or about when you would use the java_common module? Also, what jvm arg is needed?

Any news byw?

I added the possibility of creating a provider from compile and runtime jars (3695880) but I don't think it's going to be in the next release (scheduled this week). You could still try it out by building bazel at head.

I am trying now to play around with your implementation of scala_junit_test and try to incorporate java_common. Does _compile(ctx, _jars, dep_srcjars, buildijar) compile both Java and scala sources? How does it work?

[ittaiz]

Re macro- In a nutshell I mainly need to change scala_library to return the providers but then I'll have other issues. To keep this issue sane I'll skip the rest of the details but if this of interest to you I'd really appreciate continuing this on #2539

About my question about the launcher creation never mind- It relates to my implementation of scala_junit_test and it might be unclear. Again for sanity of this issue I think we can move on.

About provider creation that's great! I'm actually usually on HEAD so that shouldn't be a problem.

About _compile- yes it does compile both Java and Scala sources. Currently this work is done in the worker. You can see there compileScalaSources and compileJavaSources. This outputs one jar file which contains the java and scala classes. I think this should make the introduction of the java_common provider fairly easy no? Sounds to me like it's collecting the providers and adding the current output. The thing I'm not sure about (and which might complicate things) is the relation between the _collect_jars method and the providers. Maybe if the scala rules start returning a java_common provider with compile-time/runtime jars then _collect_jars becomes obsolete.

[iirina]

To keep this issue sane I'll skip the rest of the details but if this of interest to you I'd really appreciate continuing this on #2539

I would actually like to have #2539 for tracking the progress on fixing that issue from the native java_test. If you would like to discuss this particular issue with scala_library the best way would be to send an email to bazel-discuss@googlegroups.com and I'll reply there. The same goes for other issues that you are trying to solve.

I think this should make the introduction of the java_common provider fairly easy no?

Yes, you should be able to pass the output jar to java_common.create_provider.

The thing I'm not sure about (and which might complicate things) is the relation between the _collect_jars method and the providers. Maybe if the scala rules start returning a java_common provider with compile-time/runtime jars then _collect_jars becomes obsolete.

So you should be able to collect the providers from both scala and java deps (meaning scala_library must also return java_common.provider), collect the one that would be returned by _scala_binary_common, merge all of them and return the result. And then you could have a java_test depending on your rule. In theory it should work. _collect_jars could just be replaced by _collect_providersI guess.

[sdtwigg]

I have been working with this experimentally and have had a few rough parts:
Original stream-of-conscious discussion here bazelbuild/rules_scala#164 (comment) but will try to distill into better action items. The key items are in bold, my analysis will follow after each item. I apologize in advance that they aren't necessarily 'ordered' well....

• 'java_skylark_library' that provides JavaProvider can be in a deps or runtime_deps for java_library but not exports.
  The attempt is blocked by the attribute restrictions in https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/bazel/rules/java/ . Whereas deps and runtime_deps let you bypass the rule type whitelist by providing a JavaProvider, exports simply looks at the rule type whitelist. From my analysis of how exports actually 'flows' is that exports only really needs a JavaProvider as well and so exports should also check for a JavaProvider before erroring due to bad rule type.
  In JavaLibrary, compile jars are collected simply from the JavaCompilationArgsProvider inside each JavaProvider (via JavaLibraryHelper).
  As a separate mechanism, after actions have been generated and are now just setting up its providers for future rules, JavaLibrary (via JavaCommon) creates its JavaCompilationArgsProvider in its JavaProvider from all the jars it created plus all jars in the merger of JavaProvider.JavaCompilationArgsProvider from all rules in exports, runtime_deps, and deps. Note: JavaCompilationArgsProvider itself contains a recursive and non-recursive JavaCompilationArgs, which each contains compile and runtime jars. The non-recursive JavaCompilationArgs usually only adds jars from exports, not deps or runtime_deps. Also, the JavaCompilationArtifacts for the current target are usually split into ijar->compile jars and real jar -> runtime jars. Finally, runtime_deps only add to runtime jars and neverlink stops runtime jars from being added (except runtime_deps will always add jars).

• Cannot get compile_jars from JavaProvider
  In setting up a compilation, it would be highly convenient to just be able to merge all the JavaProviders from dependencies and then call compile_jars on the resulting provider to get all the jars needed to compile (which, from the prior analysis, looks to be the ijar from each dep and then all the compile_jars in your exports). This would then very closely mimic how JavaLibraryHelper.addLibrariesToAttributesInternal works.
  This seems like an easy add: The constructor for JavaProvider already exposes "transitive_runtime_jars" to skylark as getRecursiveJavaCompilationArgs().getRuntimeJars(). Just need another entry in JavaProvider of "compile_jars" as getJavaCompilationArgs().getCompileTimeJars().

• minor issue: create_provider produces a JavaCompilationArgsProvider with recursive==non-recursive
  Since most of the time compile jars cares about the non-recursive one and runtime jars cares about the recursive one, you can work around this by making compile_time_jars = non-recursive set of your ijar + all export compile_jars and runtime_jars = your jar + runtime jars of deps, exports, and runtime_deps.
  However, if you had the ability to rebuild JavaProvider where only the non-recursive set in JavaCompilationArgsProvider was blank and the recursive set stayed the same, would be closer to fine by doing the following set of operations:
  merge all deps JavaProvider together -> merge in all runtime_deps JavaProvider -> clear non-recursive set -> merge in all exports -> merge in create_provider with just local artifacts

• minor issue: Cannot provide a JavaSkylarkApiProvider
  This might not actually be a problem as more fields of JavaProvider become readable by Skylark rules. Also, technically, since JavaSkylarkApiProvider is only used by OTHER Skylark rules, it is possible to completely fake one by using nested structs.
  That said, it seems like one could envision a world (especially with compile_jars above) where either JavaProvider totally subsumes JavaSkylarkApiProvider or any rule with a JavaProvider automatically uses said provider to construct a JavaSkylarkApiProvider.The latter is tempting because JavaSkylarkApiProvider is actually documented so other rules clients might be more likely to attempt dep.java before dep[java_common.provider]. Perhaps ultimately, dep.java just becomes an alias to dep[java_common.provider].

• minor issue: JavaSkylarkApiProvider.transitive_exports just a list of labels
  Again, this may not be a problem if compile_jars in JavaProvider becomes available since that is likely a sufficient replacement for . Also, this can potentially be worked around via [dep for dep in target.java.transitive_deps if dep.owner in target.java.transitive_exports] although that seems inefficient. Note, the docs say dep.owner can be empty but my intuition is that this is only true when said item is a raw file and, fortunately as discovered in first item, all exports must be a rule so all files in deps from exports should have an owner.

I left out addressing/mimicking a bunch of other features in the native java rules (neverlink, non_strict, whatever DependencyArtifacts in JavaCompilationArgs add, etc.) but I think this would go a long way to allowing users to closely mimic what is the 'common path' of a java_library rule. It would actually be an interesting exercise (perhaps for testing purposes) to see in how many places then java_library in https://github.com/bazelbuild/bazel/blob/master/tools/build_rules/java_rules_skylark.bzl could just be used instead of native.java_library without the rest of the build graph noticing at all.

[iirina]

Thanks for all your feedback, Stephen! To address your items:

'java_skylark_library' that provides JavaProvider can be in a deps or runtime_deps for java_library but not exports.
Yes, you are right. exports should also allow rules that return JavaProvider. This can be easily fixed.

Cannot get compile_jars from JavaProvider

JavaProvider was first designed as a black-box until java_common had a pretty stable API. The plan was to open it up steadily, as soon as there was a need for something. Returning a compile_jars is a very reasonable and easy to implement request, so it should be added ASAP.

minor issue: create_provider produces a JavaCompilationArgsProvider with recursive==non-recursive

Yup, this API needs some more work, that's why it's undocumented. Your idea sounds good, I will look into it.

minor issue: Cannot provide a JavaSkylarkApiProvider

JavaSkylarkApiProvider should go away eventually. It was added to facilitate retrieving information from java rules, but it wasn't adequate to be used for the sandwich implementation. That's when JavaProvider came along. It will wrap more native providers than it does now, probably all of those in JavaSkylarkApiProvider, and its API will be more open and will include most of the fields exposed via JavaSkylarkApiProvider.

minor issue: JavaSkylarkApiProvider.transitive_exports just a list of labels

This should be solved by previous comments.

[ittaiz]

thanks @sdtwigg for leading this integration in the rules_scala area! I'm sure we're all waiting eagerly for progress there

[hlopko]

Hello Irina, is this still expected to be fixed in 0.5? Is this a release blocker? We're still at least 2 weeks away from the release but we should be getting ready.

[iirina]

Yup, this is expected to be in the next release. All important changes have been submitted and a few small patches will follow soon.

[hlopko]

After offline discussion not treating as a release blocker. Although it seems this issue will be closed by then anyway :)

[iirina]

@ittaiz Is there anything else you would need for implementing the scala rules using java_common? The issues raised by @sdtwigg are WIP.

[ittaiz]

@sdtwigg took the lead there so if he thinks he has all the tools he needs then we're good. @sdtwigg, Any loose ends towards our ability to use java_common.compile, depend on scala from java and remove the scala provider?

…

On Mon, 10 Apr 2017 at 14:43 Irina Iancu ***@***.***> wrote: @ittaiz <https://github.com/ittaiz> Is there anything else you would need for implementing the scala rules using java_common? The issues raised by @sdtwigg <https://github.com/sdtwigg> are WIP. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#970 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFzWweNkWemiK-n6gyb0WcdvEFMFEks5ruhX1gaJpZM4HhVSL> .

[sdtwigg]

I am reasonably confident everything is in order with those proposals. Using sdtwigg/rules_scala@9c8f720 as a base, it looks like java_common should be a drop in. I haven't experimented with the java_common.compile stuff yet but it seems reasonable to add as a follow-on (modulo a few questions wrt java_toolchain interacting with other toolchain proposals and host_javabase). Once I get convergence with Irina on the proposals (primarily on the Skylark API although Friday PR discussion resolved almost all of this): will make a bazel fork with the changes integrated. This can then be used to test an experimental follow-on rules_scala commit that drops using the scala provider in favor of JavaProvider. If that works then would be enough to say the sandwich is fulfilled. This can also be done in parallel to the various pending bazel PRs (since the bazel fork would just assume they are all cherry-picked in). There are a handful of potential follow-on concerns regarding the sandwich and inter-operating with other features in java rules but I don't think is worth primary consideration. Just something to keep in mind for eventual consideration. On Mon, Apr 10, 2017 at 2:54 PM, Ittai Zeidman <notifications@github.com> wrote:

…

@sdtwigg took the lead there so if he thinks he has all the tools he needs then we're good. @sdtwigg, Any loose ends towards our ability to use java_common.compile, depend on scala from java and remove the scala provider? On Mon, 10 Apr 2017 at 14:43 Irina Iancu ***@***.***> wrote: > @ittaiz <https://github.com/ittaiz> Is there anything else you would need > for implementing the scala rules using java_common? The issues raised by > @sdtwigg <https://github.com/sdtwigg> are WIP. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#970 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABUIFzWweNkWemiK- n6gyb0WcdvEFMFEks5ruhX1gaJpZM4HhVSL> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#970 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA9_-DLduFieSVYhFaMJp76aZuEUqpUoks5ruqUfgaJpZM4HhVSL> .

[damienmg]

Moving to 0.6 for the remaining work to do, the basics for rules_scala should be already there.

[hlopko]

Ok, thanks everybody!

[ittaiz]

Maybe the right thing to do is to open several smaller issues for the remaining work? Dmitry already commented the thread might be too long here

…

On Tue, 11 Apr 2017 at 11:30 Marcel Hlopko ***@***.***> wrote: Ok, thanks everybody! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#970 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF73ThQpdseao394ikRinENrc3J47ks5ruzpBgaJpZM4HhVSL> .

[steren]

+1 to this suggestion, it will be easier to track and to understand what's remaining.

[lberki]

In that case, let me close this bug and continue the discussion on per-issue ones.

[ittaiz]

@lbrelki do we have additional issues for these? I don't think so

[iirina]

@ittaiz There is one that I know of and that is creating a provider with empty non-recursive compilation jars. I'm already working on that and it should be submitted next week.
@sdtwigg Is there anything else?

[ittaiz]

@iirina do you mean 9671: Loosen java_library.exports and java_import.*?
IINM there is no github issue for that but if indeed that's the only issue left then maybe it's ok. Maybe @sdtwigg can add more.