Moved to java_common.compile #269

ittaiz · 2017-08-28T09:43:08Z

Was supposed to be a refactoring but found a few missing tests.
The good news are that we indeed have good coverage since this hits many code paths and the tests gave me good (though long) feedback about what I'm breaking.

Possible open issues:

I haven't changed the "files" being returned- No tests demanded it and I'm trying to understand what needs this. I think our internal aspect actually will be broken by this so I'll try to add a test like it today and see if it breaks.
Not sure Intellij integration continues to work now- will see how I triage this. @chaoren can the intellij aspect try to use the java_provider before falling back to scala attr?

ittaiz · 2017-08-28T09:43:34Z

fixes #164

ittaiz · 2017-08-28T09:44:03Z

@or-shachar @natansil I'd appreciate your review as well please

ittaiz · 2017-08-28T11:01:58Z

Travis build is green for 0.5.3
HEAD builds are broken again because of travis CI

ittaiz · 2017-08-28T20:05:02Z

scala/scala.bzl

  scalaattr = struct(
      outputs = rule_outputs,
-      compile_jars = depset([outputs.class_jar]),


@johnynek is there any reason you use here outputs.class_jar and in _lib you use ctx.outputs.jar?
I did a few prints and they seem identical.

@johnynek ping

I don't think there was a plan. I think it probably was just ill-unified.

yeah, it looks like outputs.class_jar is always ctx.outputs.jar in all cases. We could probably remove it and only use ctx.outputs.jar to be more clear.

Decided to leave it for now. Since the outputs.class_jar (although the unclear name) adds symmetry to the collection of the java jar.
We'll handle it when we'll do a bigger refactoring and re-alignment

ittaiz · 2017-08-28T20:43:31Z

scala/scala.bzl

@@ -614,31 +670,33 @@ def _scala_binary_common(ctx, cjars, rjars, transitive_compile_time_jars, jars2l
      files=depset([ctx.outputs.executable]),


@johnynek can we add here the actual jars? is there a reason to only return the executable?

@johnynek ping

I'm not really sure what needs to be there actually. Since scala_binary creates a default executable, maybe it should only be the executable? Maybe it doesn't matter. Honestly, some of the bazel stuff is not super well documented and I wrote some of this about 2 years ago now and it might have both changed in bazel and also I may have misunderstood it at the time.

I'm afraid I don't have a great answer for you as to what should be in there.

This is what the java binary does so I'm guessing it's modeled after that.
We found a workaround so the current status quo is ok

ittaiz · 2017-08-28T21:23:54Z

@chaoren if java_provider isn't an option (though I'd really love to understand why) then I think I can just add an attribute scala.outputs.jars which will have an array of both the scala code and the java code and also add an empty scala.transitive_exports since I don't know what it means and I don't think we need it.
From poking around the aspect those seem to be the only mandatory parts, right?
WDYT in general?

chaoren · 2017-08-29T15:42:38Z

@ittaiz

if java_provider isn't an option (though I'd really love to understand why)

We can't easily identify the rule as scala if we're not using scalaattr. It doesn't look like we can refer to the java provider by name (it's just a list of providers), so if another (unsupported) rule provides similar looking providers we can't tell them apart.

I think I can just add an attribute scala.outputs.jars which will have an array of both the scala code and the java code and also add an empty scala.transitive_exports since I don't know what it means and I don't think we need it.

Yes that seems fine.

johnynek

I'm excited about this. Some clarifying questions.

Thanks for pushing on this!

johnynek · 2017-08-29T17:17:02Z

src/java/io/bazel/rulesscala/test_discovery/DiscoveredTestSuite.scala

+    classes
+  }
+
+  private def discoverClassesIn(archivePath: String) = {


can we add a return type here. I think it is Array[Class[_]]

don't see this yet.

johnynek · 2017-08-29T17:18:21Z

src/java/io/bazel/rulesscala/test_discovery/DiscoveredTestSuite.scala

    new JarInputStream(new FileInputStream(archivePath))

-  private def archivePath: String =
-    System.getProperty("bazel.discover.classes.archive.file.path")


is this going to break anyone, or is this only expected to be set by a rule? If a rule, is it our rule? should we use io.bazel.rules_scala or something to distinguish? Can we add a comment in any case to ease reading the code?

Won't break anyone
our JUnit rule
I'm actually thinking of factoring out the JUnit "discover test classes" mechanism (which this is a part of) since it's not scala related but JUnit related.
When we added this we couldn't do this refactoring due to how scala and java interoped. Due to this I'd rather not change the name.
Re the comment- If one will search the repo for this text he will find it in the bzl so I think it's ok for now but if you feel strongly about it I'll add the comment

I would like to add the comment to explain the magic string because honestly, I was foggy when I looked back at it. This well help someone understand how it was set.

johnynek · 2017-08-29T17:23:18Z

scala/scala.bzl

+    scala_sources_java_provider = java_common.create_provider(
+        #the line below means we can't use the provider from java_common.compile
+        #since it will include the full jar when I want to expose only the ijar
+        #on the other hand we can't supply the java_common.compile the scala ijar since it needs the internals


I don't know what internals mean here. I'm not sure why the ijar of the scalac compilation along with all the ijars we need for compilation is not sufficient. Can you add to the comment to possibly explain this?

Sure but maybe let's first see I'm not just mistaken :) : Won't ijar strip parts from the scalac jar the java sources need? They're the same compilation "group"

Maybe a test can be devised for this...

ijar strips the code and leaves the classes and the signatures. Imagine all methods were replaced with throw new Exception(""). I don't see how javac can use that code at compile time except as annotation processors (which are the closest things java has to macros as far as I know).

So, I think using the ijar is exactly what we want since bazel is now driving this compilation. There could be times where I think bazel could recompile scala, but see that the java didn't change and the compiled ijar didn't change.

Sounds good. I'll use the ijar.
I wonder if I should add a no recompilation for same target when scala
changes and java doesn't recompile.
I'll probably add it

👍

Sounds good. Would be nice to see a test that we can change some private method contents and not recompile the java if we can test that.

johnynek · 2017-08-29T17:25:17Z

scala/scala.bzl

+                java_toolchain = ctx.attr._java_toolchain,
+                host_javabase = ctx.attr._host_javabase,
+    )
+    return struct(jar = full_java_jar, ijar = provider.compile_jars.to_list().pop())


is there really a contract that the first item is the ijar for this item? Can we add a comment to that effect?

This jar only includes the java output, not the scala, right?

See the comment 5 lines above. Is it clear enough?
Yes.

yes that's fine.

johnynek · 2017-08-29T17:29:53Z

scala/scala.bzl

    # We assume that if a srcjar is present, it is not empty
    if len(ctx.files.srcs) + len(srcjars) == 0:
        _build_nosrc_jar(ctx, buildijar)
        #  no need to build ijar when empty
-        return struct(ijar=ctx.outputs.jar, class_jar=ctx.outputs.jar)
+        return struct(ijar=ctx.outputs.jar, class_jar=ctx.outputs.jar, java_jar = False)


wait, here this is a bool, but below it is a struct, I think. Could we use java_jar = None here?

Will do. In python it's the same though I don't know what's the idiomatic way.

ittaiz · 2017-08-30T07:17:51Z

@chaoren I hope it's ok I'm asking but what does a jvm rule (kotlin for example) need for it to be supported? Other than exposing java provider.

natansil

Looks like this code does the job that it intends to do,
But we need to find a way to have better modularization and abstraction of scala.bzl...

as a first step, maybe split collection, compilation and post-compilation to different files

natansil · 2017-08-30T16:04:10Z

scala/scala.bzl

    prefixesFlag = "-Dbazel.discover.classes.prefixes=%s" % ",".join(ctx.attr.prefixes),
    suffixesFlag = "-Dbazel.discover.classes.suffixes=%s" % ",".join(ctx.attr.suffixes),
    printFlag = "-Dbazel.discover.classes.print.discovered=%s" % ctx.attr.print_discovered_classes)

+def _get_archives_short_path(scala_archive, maybe_java_archive):
+  java_archive_short_path = ""
+  if maybe_java_archive:


Maybe instead of this if, you can pass this method a list of archives, and some how archives that are None will not get into the list...

I have a feeling it's indirection and not abstraction. There are just two instances and if I understand correctly you're suggestion is to still have the if just for a "generic" item, right?

ended up doing it for a different reason, it allowed me to pass a more generic out argument from the _scala_binary_common

natansil · 2017-08-30T16:06:50Z

scala/scala.bzl

+    scala_sources_java_provider = java_common.create_provider(
+        #the line below means we can't use the provider from java_common.compile
+        #since it will include the full jar when I want to expose only the ijar
+        #on the other hand we can't supply the java_common.compile the scala ijar since it needs the internals


Maybe a test can be devised for this...

chaoren · 2017-08-31T00:01:26Z

@chaoren I hope it's ok I'm asking but what does a jvm rule (kotlin for example) need for it to be supported? Other than exposing java provider.

Technically, we just need to be able to recognize and handle it in the aspect. Do you mean if we decide to support the rule? Then it's probably based on popularity and if we decide it's worth the maintenance cost.

ittaiz · 2017-08-31T06:35:13Z

I was more aiming at: "can we have a generic jvm interface for languages to implement and then automatically be supported by the plugin (like expose java provider)" it's far away from the original issue that I'll drop it. I'll change the attribute to return both jars. Thanks!

…

On Thu, 31 Aug 2017 at 3:01 Chaoren Lin ***@***.***> wrote: @chaoren <https://github.com/chaoren> I hope it's ok I'm asking but what does a jvm rule (kotlin for example) need for it to be supported? Other than exposing java provider. Technically, we just need to be able to recognize and handle it in the aspect. Do you mean if we decide to support the rule? Then it's probably based on popularity and if we decide it's worth the maintenance cost. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#269 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF2eIs-9f0jP1XucqPR8AHLqKGycRks5sdffXgaJpZM4PETYz> .

ittaiz · 2017-08-31T13:38:25Z

@johnynek apart from the ijar change and the IntelliJ change, are we good?
I'd like to merge it tomorrow.
Next change is to add support for strict deps (we first need @iirina to allow passing warn/default from skylark)

johnynek

this is looking great. I'm really excited. I'm +1 on this, just would like a couple of comments added.

Sorry I have lost some context on your questions.

johnynek · 2017-09-02T01:58:23Z

src/java/io/bazel/rulesscala/test_discovery/DiscoveredTestSuite.scala

+    classes
+  }
+
+  private def discoverClassesIn(archivePath: String) = {


don't see this yet.

johnynek · 2017-09-02T01:59:24Z

src/java/io/bazel/rulesscala/test_discovery/DiscoveredTestSuite.scala

    new JarInputStream(new FileInputStream(archivePath))

-  private def archivePath: String =
-    System.getProperty("bazel.discover.classes.archive.file.path")


I would like to add the comment to explain the magic string because honestly, I was foggy when I looked back at it. This well help someone understand how it was set.

johnynek · 2017-09-02T02:01:49Z

scala/scala.bzl

+    scala_sources_java_provider = java_common.create_provider(
+        #the line below means we can't use the provider from java_common.compile
+        #since it will include the full jar when I want to expose only the ijar
+        #on the other hand we can't supply the java_common.compile the scala ijar since it needs the internals


👍

Sounds good. Would be nice to see a test that we can change some private method contents and not recompile the java if we can test that.

johnynek · 2017-09-02T02:03:08Z

scala/scala.bzl

+                java_toolchain = ctx.attr._java_toolchain,
+                host_javabase = ctx.attr._host_javabase,
+    )
+    return struct(jar = full_java_jar, ijar = provider.compile_jars.to_list().pop())


yes that's fine.

johnynek · 2017-09-02T02:06:31Z

scala/scala.bzl

  scalaattr = struct(
      outputs = rule_outputs,
-      compile_jars = depset([outputs.class_jar]),


I don't think there was a plan. I think it probably was just ill-unified.

johnynek · 2017-09-02T02:10:07Z

scala/scala.bzl

@@ -614,31 +670,33 @@ def _scala_binary_common(ctx, cjars, rjars, transitive_compile_time_jars, jars2l
      files=depset([ctx.outputs.executable]),


I'm not really sure what needs to be there actually. Since scala_binary creates a default executable, maybe it should only be the executable? Maybe it doesn't matter. Honestly, some of the bazel stuff is not super well documented and I wrote some of this about 2 years ago now and it might have both changed in bazel and also I may have misunderstood it at the time.

I'm afraid I don't have a great answer for you as to what should be in there.

johnynek · 2017-09-02T02:10:51Z

scala/scala.bzl

  scalaattr = struct(
      outputs = rule_outputs,
-      compile_jars = depset([outputs.class_jar]),


yeah, it looks like outputs.class_jar is always ctx.outputs.jar in all cases. We could probably remove it and only use ctx.outputs.jar to be more clear.

johnynek · 2017-09-02T02:12:36Z

scala/scala.bzl


 def _scala_repl_impl(ctx):
  # need scala-compiler for MainGenericRunner below
  jars = _collect_jars_from_common_ctx(ctx, extra_runtime_deps = [ctx.attr._scalacompiler])
  (cjars, transitive_rjars) = (jars.compile_jars, jars.transitive_runtime_jars)
-  transitive_rjars += [ctx.outputs.jar]


this is removed because _scala_binary_common is putting it in also, is that right?

johnynek · 2017-09-02T02:13:50Z

scala/scala.bzl

  "manifest": "%{name}_MANIFEST.MF",
 }

+library_outputs = common_outputs + {


nice cleanup.

johnynek · 2017-09-02T04:46:33Z

Yeah, other than the ijar, comments and type annotation, yes.

…

On Thu, Aug 31, 2017 at 03:38 Ittai Zeidman ***@***.***> wrote: @johnynek <https://github.com/johnynek> apart from the ijar change and the IntelliJ change, are we good? I'd like to merge it tomorrow. Next change is to add support for strict deps (we first need @iirina <https://github.com/iirina> to allow passing warn/default from skylark) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#269 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdu0X-O54s74YMsuWuqz-g-74Qti5ks5sdrdRgaJpZM4PETYz> .

…utputs as different files

…sources

added regression test for passing scala ijars

unified some full-jars/ijars collection junit archives list generation now uses the full jars list

ittaiz · 2017-09-02T16:52:05Z

@johnynek I've handled the return type, the comment, the use of the scala ijar to java siblings and exposing java jars to intellij

ittaiz · 2017-09-02T17:35:34Z

Build is green on 0.5.3

johnynek · 2017-09-02T18:48:09Z

pro-style work sir!

Thanks for pushing this all the way through.

ittaiz · 2017-09-02T18:58:06Z

:) Sure thing. Thank you for your inputs, I think they improved the end result.
Next up I'll open a PR to support strict java deps, I'm just waiting for bazelbuild/bazel#3626 since currently warn/default aren't supported so it will break the build

ittaiz · 2017-09-02T19:43:24Z

btw, I actually think squashing here wasn't the best approach since I tried keeping the commits pretty separated. The benefit from not squashing is smaller commits to comb when going through the history.

johnynek · 2017-09-02T19:50:05Z

you and I seem to disagree about squashed commits. :)

Ultimately, I like one atomic commit of the reviewed and accepted change, and I like to keep each change very small (ideally much under 400 lines).

In your case, for instance, we could have done all the clean up changes in a separate PR (which may have been 1/3 of the diff at the end of the day).

Maybe we should hash out some standard and document it in the repo.

johnynek · 2017-09-02T19:52:31Z

To be clear: this diff was a good size, but by making each reviewed PR atomic and as small as possible, we never have any confusion that any sha in master was not previous state which was reviewed. I know people disagree about this.

Honestly, merge commits might be fine if the git tooling was clearer and easier to use to make it clear that there is no valid view of master that steps into these commits.

ittaiz · 2017-09-02T20:03:56Z

I guess we'll do it at some point :) To be honest I guess the truth (as always) is somewhere in the middle since I could have squashed it into 3 commits. BTW, I meant to use rebase and not merge (which github ui supports), this sidesteps the merge commit issue.

…

On Sat, Sep 2, 2017 at 10:52 PM P. Oscar Boykin ***@***.***> wrote: To be clear: this diff was a good size, but by making each reviewed PR atomic and as small as possible, we never have any confusion that any sha in master was not previous state which was reviewed. I know people disagree about this. Honestly, merge commits might be fine if the git tooling was clearer and easier to use to make it clear that there is no valid view of master that steps into these commits. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#269 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF_bnly3vD1roIcKNGY1TFZjIpTkyks5sebIAgaJpZM4PETYz> .

googlebot added the cla: yes label Aug 28, 2017

ittaiz requested a review from johnynek August 28, 2017 09:43

ittaiz commented Aug 28, 2017

View reviewed changes

johnynek reviewed Aug 29, 2017

View reviewed changes

natansil reviewed Aug 30, 2017

View reviewed changes

johnynek reviewed Sep 2, 2017

View reviewed changes

ittaiz added 6 commits September 2, 2017 16:59

removed outputs duplicate declaration in preparation of adding java o…

9fb1c2f

…utputs as different files

moved from manual javac to java_common.compile

57e2cac

added support for compiling and discovering java junit tests

425bf53

added support for binary common to depend on binary common with java …

8143bb4

…sources

added regression test for passing jvm flags via javacopts

d57e712

passing java ijars to dependencies

8307307

added regression test for passing scala ijars

ittaiz force-pushed the java_common_compile branch from e4baf85 to 8307307 Compare September 2, 2017 14:01

ittaiz added 4 commits September 2, 2017 17:05

added return type annotation

cdc18ae

added comment to clarify where test suite gets archives from

b168ef2

expose scala.outputs.jars for intellij plugin

49a5999

unified some full-jars/ijars collection junit archives list generation now uses the full jars list

java sources depend on ijar of scala siblings

fbb053f

johnynek merged commit 1da7e63 into bazelbuild:master Sep 2, 2017

johnynek mentioned this pull request Sep 2, 2017

Move from manual javac to java_common.compile #164

Closed

ittaiz deleted the java_common_compile branch September 2, 2017 20:21

		@@ -614,31 +670,33 @@ def _scala_binary_common(ctx, cjars, rjars, transitive_compile_time_jars, jars2l
		files=depset([ctx.outputs.executable]),

Moved to java_common.compile #269

Moved to java_common.compile #269

Conversation

ittaiz commented Aug 28, 2017 • edited Loading

ittaiz commented Aug 28, 2017

ittaiz commented Aug 28, 2017

ittaiz commented Aug 28, 2017

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

ittaiz commented Aug 28, 2017

chaoren commented Aug 29, 2017

johnynek 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

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

ittaiz commented Aug 30, 2017

natansil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaoren commented Aug 31, 2017

ittaiz commented Aug 31, 2017 via email

ittaiz commented Aug 31, 2017

johnynek 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

johnynek commented Sep 2, 2017 via email

ittaiz commented Sep 2, 2017

ittaiz commented Sep 2, 2017

johnynek commented Sep 2, 2017

ittaiz commented Sep 2, 2017

ittaiz commented Sep 2, 2017

johnynek commented Sep 2, 2017

johnynek commented Sep 2, 2017

ittaiz commented Sep 2, 2017 via email

ittaiz commented Aug 28, 2017 •

edited

Loading

ittaiz Sep 2, 2017 •

edited

Loading