Windows support - scala_test

[majcherm-da]

This is based on (includes it) the work done as part of: #717 (Windows support) - it fixes scala_test rule on Windows. Before this change scala_test was using a bit hacky way to pass scala test's Runner arguments by providing a bash script with arguments instead of a java executable (https://github.com/bazelbuild/rules_scala/blob/master/scala/private/rule_impls.bzl#L560). After changes done in #717 bash script is not used on Windows at all, so the workaround for passing args in an embedded bash script is not taken into account at all. With this change args are written into a file and read by the scala-test java wrapper (io.bazel.rulesscala.scala_test.Runner), which was already in place for other reasons. Java wrapper also uses the com.google.devtools.build.runfiles.Runfiles now to obtain absolute paths instead of relying on relative paths, which didn't work on Windows.

Sample test run on Windows:

A distilled changed between initial windows support (#717) and this one can be viewed here: https://github.com/majcherm-da/rules_scala/pull/1/files

[johnynek]

Is there any way we can get the CI to run for Windows? I’d love to merge this and not break people again.

Does Travis support windows?

[majcherm-da]

@johnynek Travis has (limited) Windows support. I setup a Windows pipeline, but as language: java is not supported in Travis for Windows I switched to sh and provided jdk manually.

Test suite on Windows is pretty limited because several rules still do not work properly, but at least basic ones like, scala_binary, scala_test, scala_test_suite and few others now work properly.

[johnynek]

This looks really great! Thanks for working so hard for this.

I did see this:

Exception in thread "main" java.lang.OutOfMemoryError: Metaspace

Do you think we can increase memory settings to address that?

[majcherm-da]

@johnynek I tweaked a bit memory settings and it looks pretty stable now on Windows:
https://travis-ci.org/majcherm-da/rules_scala/builds/517227222
https://travis-ci.org/majcherm-da/rules_scala/builds/517243102
https://travis-ci.org/majcherm-da/rules_scala/builds/517243236
https://travis-ci.org/majcherm-da/rules_scala/builds/517269738

although it happens to fail on test_reproducibility.sh on Linux, but I've seen the same behavior on other builds (other PRs), i.a.:
https://travis-ci.org/bazelbuild/rules_scala/builds/516034647
https://travis-ci.org/bazelbuild/rules_scala/builds/514671717
https://travis-ci.org/bazelbuild/rules_scala/builds/514656205
so it might by flaky (?)

[majcherm-da]

@johnynek, @ittaiz could you have a look if there's anything else missing or it could be merged now?

[johnynek]

Sorry for the latency. The reproducibility test is flakey with jmh. We haven’t run it down yet.

I’ll review tomorrow. I’d love to have windows support. Thanks for all your work on this!

[majcherm-da]

@johnynek, @ittaiz Is there any chance to get this reviewed maybe this week? :)

[ittaiz]

Hi,
First let me apologize for the very long delay.
My mail is a mess ever since google killed inbox and I had to move to gmail :(
Second I want to thank you for all your hard work on getting windows support in. Well done.

I wrote several comments inside but the biggest one is:
Will we have a cat-mouse game of catchup where some PRs break windows support (Because coverage is limited) and then we try to fix it? Mainly asking because the support feels a bit provisory and in a few different places.
It's much better than no support but this is something I'm left with after reviewing the PR

[majcherm-da]

@ittaiz Thanks for the review! I already addressed / responded to all your comments. PTAL

[johnynek]

Thanks for all this work! I'm happy to merge when @ittaiz is. I added some small notes. I hope we can add a link to an issue tracking the lack of ctx API for getting the OS.

[johnynek]

I am going to merge since I think we've given @ittaiz time enough. We can follow up as needed.

Thanks for the help on this!