Add jvm_flags attribute test cases to expose issue building javac command line #101

petemounce · 2016-10-25T23:15:55Z

This is mentioned in #99, and this is a reproduction for that (but I don't know where to fix it).

bazel-io · 2016-10-25T23:15:56Z

Can one of the admins verify this patch?

googlebot · 2016-10-25T23:15:57Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

petemounce · 2016-10-25T23:23:53Z

With this diff, I now get what looks like the correct command line for javac, but the following error:

diff --git i/scala/scala.bzl w/scala/scala.bzl
index debac4a..aacef5f 100644
--- i/scala/scala.bzl
+++ w/scala/scala.bzl
@@ -202,6 +202,7 @@ SourceJars: {srcjars}
            [ctx.outputs.manifest,
             ctx.file._ijar,
             ctx.file._java,
+            ctx.file._javac,
             argfile])
     ctx.action(
         inputs=ins,
diff --git i/src/java/io/bazel/rulesscala/scalac/ScalaCInvoker.java w/src/java/io/bazel/rulesscala/scalac/ScalaCInvoker.java
index 628b569..1298fda 100644
--- i/src/java/io/bazel/rulesscala/scalac/ScalaCInvoker.java
+++ w/src/java/io/bazel/rulesscala/scalac/ScalaCInvoker.java
@@ -274,6 +274,7 @@ public class ScalaCInvoker {
   private static void compileJavaSources(CompileOptions ops, Path tmpPath) throws IOException, InterruptedException {
     StringBuilder cmd = new StringBuilder();
     cmd.append(ops.javacPath);
+    cmd.append(" ");
     if (ops.jvmFlags != "") cmd.append(ops.jvmFlags);
     if (ops.javacOpts != "") cmd.append(ops.javacOpts);

$ bazel test --verbose_failures //...

...

INFO: Found 69 targets and 1 test target...
ERROR: /Users/peter/src/oss/rules_scala/test/BUILD:167:1: scala //test:MixJavaScalaLibWithJvmFlags failed: scalac failed: error executing command
  (cd /private/var/tmp/_bazel_peter/cb0f8a6254f19f3f9fe9f1ab970ecbf6/execroot/rules_scala && \
  exec env - \
  bazel-out/host/bin/src/java/io/bazel/rulesscala/scalac/scalac '--jvm_flag=-Xms1G' '--jvm_flag=-Xmx4G' @bazel-out/local-fastbuild/bin/test/MixJavaScalaLibWithJvmFlags_worker_input): com.google.devtools.build.lib.shell.BadExitStatusException: Process exited with status 1.
Exception in thread "main" java.lang.RuntimeException: nope
    at io.bazel.rulesscala.scalac.ScalaCInvoker.main(ScalaCInvoker.java:339)
Caused by: java.io.IOException: Cannot run program "external/local_jdk/bin/javac -J-Xms1G -J-Xmx4G": error=2, No such file or directory
    at java.lang.ProcessBuilder.start(ProcessBuilder.java:1048)
    at io.bazel.rulesscala.scalac.ScalaCInvoker.compileJavaSources(ScalaCInvoker.java:294)
    at io.bazel.rulesscala.scalac.ScalaCInvoker.processRequest(ScalaCInvoker.java:237)
    at io.bazel.rulesscala.scalac.ScalaCInvoker.main(ScalaCInvoker.java:335)
Caused by: java.io.IOException: error=2, No such file or directory
    at java.lang.UNIXProcess.forkAndExec(Native Method)
    at java.lang.UNIXProcess.<init>(UNIXProcess.java:247)
    at java.lang.ProcessImpl.start(ProcessImpl.java:134)
    at java.lang.ProcessBuilder.start(ProcessBuilder.java:1029)
    ... 3 more
INFO: Elapsed time: 10.626s, Critical Path: 7.67s
//test:HelloLibTest                                                   NO STATUS

Executed 0 out of 1 test: 1 was skipped.

johnynek · 2016-10-26T00:23:06Z

I guess the issue is here:

https://github.com/bazelbuild/rules_scala/blob/master/src/java/io/bazel/rulesscala/scalac/ScalaCInvoker.java#L288

this approach of gluing args onto the command is probably broken.

Probably better to use this method instead:
https://docs.oracle.com/javase/7/docs/api/java/lang/ProcessBuilder.html#command(java.util.List)

then we can add the args we need rather than string manipulation.

…mand line

petemounce · 2016-10-30T21:50:00Z

I expect #104 to fix this.

johnynek · 2016-10-30T22:07:42Z

@petemounce can you merge master and see if the test goes green? I hope so, and then I'll merge.

petemounce · 2016-10-30T22:17:08Z

@johnynek I rebased just now.

johnynek · 2016-10-30T22:19:26Z

Huh. A test failed:

Test test_build_is_identical failed

Building twice does not produce the same output, but only with this change, it seems. :/

This reverts commit 3340a06 (because I made it to the wrong branch)

petemounce · 2016-10-30T22:40:07Z

Locally (OSX 10.12), all the tests pass.

johnynek · 2016-10-30T22:42:37Z

just to check, you did: https://github.com/bazelbuild/rules_scala/blob/master/test_run.sh ./test_run.sh to test this?

clearly this is a test-only change, so if it fails, it is not a problem with the PR, but an actual bug.

If the code is non-identical on multiple runs, this usually means the caching is going to be broken and cause unneeded recomputation when there are non-semantic changes to the code (whitespace, for instance).

petemounce · 2016-10-30T22:43:55Z

Yes; here's the output, and a git status.

peter at petermounce-mbp-imp in ~/src/oss/rules_scala (expose_jvm_flags_issue●)
$ ./test_run.sh
running test bazel build test/...
 Test bazel build test/... successful
running test bazel test test/...
 Test bazel test test/... successful
running test bazel run test/src/main/scala/scala/test/twitter_scrooge:justscrooges
 Test bazel run test/src/main/scala/scala/test/twitter_scrooge:justscrooges successful
running test bazel run test:JavaBinary
 Test bazel run test:JavaBinary successful
running test bazel run test:JavaBinary2
 Test bazel run test:JavaBinary2 successful
running test bazel run test:MixJavaScalaLibBinary
 Test bazel run test:MixJavaScalaLibBinary successful
running test bazel run test:ScalaBinary
 Test bazel run test:ScalaBinary successful
running test bazel run test:ScalaLibBinary
 Test bazel run test:ScalaLibBinary successful
running test test_disappearing_class
 Test test_disappearing_class successful
running test find -L ./bazel-testlogs -iname *.xml
 Test find -L ./bazel-testlogs -iname *.xml successful
running test xmllint_test
 Test xmllint_test successful
running test test_disappearing_class
 Test test_disappearing_class successful
running test test_build_is_identical
 Test test_build_is_identical successful
running test test_transitive_deps
 Test test_transitive_deps successful
running test test_repl
 Test test_repl successful

peter at petermounce-mbp-imp in ~/src/oss/rules_scala (expose_jvm_flags_issue●)
$ gs
On branch expose_jvm_flags_issue
Your branch is up-to-date with 'origin/expose_jvm_flags_issue'.
Untracked files:
    diff.patch
    hash1
    hash2

nothing added to commit but untracked files present

ittaiz · 2016-11-06T05:47:31Z

test/BUILD

@@ -157,6 +164,13 @@ scala_library(
           glob(["src/main/scala/scala/test/mix_java_scala/*.java"]),
    jvm_flags = ["-Xms1G", "-Xmx4G"],
 )
+#Mix java scala
+scala_library(
+    name = "MixJavaScalaLibWithJvmFlags",


@petemounce why is this target needed?
Asking for two reasons:

Nothing depends on it or runs it in the test_run.sh

The "existing" MixJavaScalaLib target also uses the jvm_flags attribute so other than the name I don't see any difference.

Did this push anything or conversely trying to protect from a regression? If so how is it different from the existing target?

oh sorry, I think the "existing" one got it from #104 which you rebased on top off.
Any objection to removing this target?

No, no objection, I'll do that now.

Ah - this one has a slightly more explicit name than the other scala+java mix target, which would make a test failure more explicit. In #104, the existing scala+java mix target had the jvm_flags attribute added by itself, so now could fail if there's an issue with mixed compilation or jvm flags.

What do you think?

@petemounce sorry for the delay. fair enough. but wdyt about removing the jvm_flags from this target? It's purpose is to check JavaScala mix, the jvm_flags can be tested solely in the new target

I think it was the addition of jvm_flags on a mixed scala+java library that caused the error.

ok, why do we need OtherLibButWithJvmFlags?
Can we also remove the jvm_flags from MixJavaScalaLib and only retain it in the new target?
Last question what is protected by MixJavaScalaLibWithJvmFlags (assuming MixJavaScalaLib doesn't have jvm_flags)? How is it protected? What test will break?

johnynek · 2017-04-19T18:03:16Z

could you possibly rebase on master and see if the tests are green now? I think they should be.

ittaiz · 2017-04-30T09:11:20Z

I think this might have been handled by #104
@petemounce WDYT?

johnynek · 2017-06-29T05:35:32Z

I think #234 also should put the nail in this. Reopen if there is still an issue.

googlebot added the cla: no label Oct 25, 2016

petemounce mentioned this pull request Oct 27, 2016

Projects broken by testonly enforcement on ci.bazel.io: re2, rules_scala, rules_web, TensorFlow, TensorFlow_Serving bazelbuild/bazel#1967

Closed

Add jvm_flags attribute test cases to expose issue building javac com…

ec1440f

…mand line

petemounce force-pushed the expose_jvm_flags_issue branch from cfa1cbd to ec1440f Compare October 30, 2016 21:42

Peter Mounce added 2 commits October 30, 2016 22:32

Correct hash (fetched from gh releases)

3340a06

Revert "Correct hash (fetched from gh releases)"

2c9034f

This reverts commit 3340a06 (because I made it to the wrong branch)

johnynek mentioned this pull request Nov 5, 2016

Build is not identical when scala rules compile java sources #108

Closed

ittaiz reviewed Nov 6, 2016

View reviewed changes

johnynek closed this Jun 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jvm_flags attribute test cases to expose issue building javac command line #101

Add jvm_flags attribute test cases to expose issue building javac command line #101

petemounce commented Oct 25, 2016

bazel-io commented Oct 25, 2016

googlebot commented Oct 25, 2016

petemounce commented Oct 25, 2016

johnynek commented Oct 26, 2016

petemounce commented Oct 30, 2016

johnynek commented Oct 30, 2016

petemounce commented Oct 30, 2016

johnynek commented Oct 30, 2016

petemounce commented Oct 30, 2016

johnynek commented Oct 30, 2016

petemounce commented Oct 30, 2016

ittaiz Nov 6, 2016

ittaiz Nov 6, 2016

petemounce Nov 23, 2016

petemounce Nov 23, 2016

ittaiz Nov 27, 2016

petemounce Nov 30, 2016

ittaiz Nov 30, 2016

johnynek commented Apr 19, 2017

ittaiz commented Apr 30, 2017

johnynek commented Jun 29, 2017

Add jvm_flags attribute test cases to expose issue building javac command line #101

Add jvm_flags attribute test cases to expose issue building javac command line #101

Conversation

petemounce commented Oct 25, 2016

bazel-io commented Oct 25, 2016

googlebot commented Oct 25, 2016

petemounce commented Oct 25, 2016

johnynek commented Oct 26, 2016

petemounce commented Oct 30, 2016

johnynek commented Oct 30, 2016

petemounce commented Oct 30, 2016

johnynek commented Oct 30, 2016

petemounce commented Oct 30, 2016

johnynek commented Oct 30, 2016

petemounce commented Oct 30, 2016

ittaiz Nov 6, 2016

Choose a reason for hiding this comment

ittaiz Nov 6, 2016

Choose a reason for hiding this comment

petemounce Nov 23, 2016

Choose a reason for hiding this comment

petemounce Nov 23, 2016

Choose a reason for hiding this comment

ittaiz Nov 27, 2016

Choose a reason for hiding this comment

petemounce Nov 30, 2016

Choose a reason for hiding this comment

ittaiz Nov 30, 2016

Choose a reason for hiding this comment

johnynek commented Apr 19, 2017

ittaiz commented Apr 30, 2017

johnynek commented Jun 29, 2017