Windows support - code review fixes

Author: majcherm-da

scala/private/rule_impls.bzl

@@ -1166,7 +1166,7 @@ def scala_test_impl(ctx):
         ctx = ctx,
         executable = executable,
         jvm_flags = [
-            "-DRULES_SCALA_WS=%s" % ctx.workspace_name,
+            "-DRULES_SCALA_MAIN_WS_NAME=%s" % ctx.workspace_name,
             "-DRULES_SCALA_ARGS_FILE=%s" % argsFile.short_path
         ] + ctx.attr.jvm_flags,
         main_class = ctx.attr.main_class,

src/java/io/bazel/rulesscala/exe/BUILD

@@ -5,20 +5,20 @@ java_library(
         "LaunchInfo.java",
     ],
     deps = [
-        "@bazel_tools//tools/java/runfiles:runfiles",
+        "@bazel_tools//tools/java/runfiles",
         "//external:io_bazel_rules_scala/dependency/scala/guava",
     ],
-    visibility = ["//visibility:public"],
+    visibility = ["//visibility:private"],
 )
 
 java_binary(
     name = "exe",
     main_class = "io.bazel.rulesscala.exe.LauncherFileWriter",
-    visibility = ["//visibility:public"],
     runtime_deps = [
         ":exe-lib",
     ],
     data = [
-        "@bazel_tools//tools/launcher:launcher",
+        "@bazel_tools//tools/launcher",
     ],
+    visibility = ["//visibility:public"],
 )

src/java/io/bazel/rulesscala/scala_test/BUILD

@@ -4,6 +4,6 @@ java_library(
     visibility = ["//visibility:public"],
     deps = [
         "//external:io_bazel_rules_scala/dependency/scalatest/scalatest",
-        "@bazel_tools//tools/java/runfiles:runfiles",
+        "@bazel_tools//tools/java/runfiles",
     ],
 )

src/java/io/bazel/rulesscala/scala_test/Runner.java

@@ -2,23 +2,27 @@
 
 import com.google.devtools.build.runfiles.Runfiles;
 import java.io.IOException;
+import java.io.File;
 import java.util.List;
 import java.util.Map;
 import java.nio.charset.Charset;
 import java.nio.file.Files;
 import java.nio.file.Paths;
 
-/** This exists only as a proxy for scala tests's runner to provide access to env variables */
+/** This exists only as a proxy for scala tests's runner to:
+ * - provide access to env variables
+ * - unwrap runner's arguments from a file (passed via file to overcome command-line string limitation on Windows)
+ **/
 public class Runner {
   /**
    * This is the name of the env var set by bazel when a user provides a `--test_filter` test option
    */
   private static final String TESTBRIDGE_TEST_ONLY = "TESTBRIDGE_TEST_ONLY";
 
   /**
-   * This is the name of the system property used to pass Bazel's workspace name
+   * This is the name of the system property used to pass the main workspace name
    */
-  private static final String RULES_SCALA_WS = "RULES_SCALA_WS";
+  private static final String RULES_SCALA_MAIN_WS_NAME = "RULES_SCALA_MAIN_WS_NAME";
 
   /**
    * This is the name of the system property used to pass a short path of the file, which includes
@@ -31,32 +35,26 @@ public static void main(String[] args) throws IOException {
   }
 
   private static String[] extendArgs(String[] args, Map<String, String> env) throws IOException {
-    args = extendFromSystemPropArgs(args);
+    args = extendFromFileArgs(args);
     args = extendFromEnvVar(args, env, TESTBRIDGE_TEST_ONLY, "-s");
     return args;
   }
 
-  private static String[] extendFromSystemPropArgs(String[] args) throws IOException {
-    String rulesWorkspace = System.getProperty(RULES_SCALA_WS);
-    if (rulesWorkspace == null || rulesWorkspace.trim().isEmpty())
-      throw new IllegalArgumentException(RULES_SCALA_WS + " is null or empty.");
-
-    String rulesArgsKey = System.getProperty(RULES_SCALA_ARGS_FILE);
-    if (rulesArgsKey == null || rulesArgsKey.trim().isEmpty())
+  private static String[] extendFromFileArgs(String[] args) throws IOException {
+    String runnerArgsFileKey = System.getProperty(RULES_SCALA_ARGS_FILE);
+    if (runnerArgsFileKey == null || runnerArgsFileKey.trim().isEmpty())
       throw new IllegalArgumentException(RULES_SCALA_ARGS_FILE + " is null or empty.");
 
-    String rulesArgsPath = Runfiles.create().rlocation(rulesWorkspace + "/" + rulesArgsKey);
-    if (rulesArgsPath == null)
-      throw new IllegalArgumentException("rlocation value is null for key: " + rulesArgsKey);
+    String workspace = System.getProperty(RULES_SCALA_MAIN_WS_NAME);
+    if (workspace == null || workspace.trim().isEmpty())
+      throw new IllegalArgumentException(RULES_SCALA_MAIN_WS_NAME + " is null or empty.");
 
-    List<String> runnerArgs = Files.readAllLines(Paths.get(rulesArgsPath), Charset.forName("UTF-8"));
+    String runnerArgsFilePath = Runfiles.create().rlocation(workspace + "/" + runnerArgsFileKey);
+    if (runnerArgsFilePath == null)
+      throw new IllegalArgumentException("rlocation value is null for key: " + runnerArgsFileKey);
 
-    int runpathFlag = runnerArgs.indexOf("-R");
-    if (runpathFlag >= 0) {
-      String runpathKey = runnerArgs.get(runpathFlag + 1);
-      String runpath = Runfiles.create().rlocation(rulesWorkspace + "/" + runpathKey);
-      runnerArgs.set(runpathFlag + 1, runpath);
-    }
+    List<String> runnerArgs = Files.readAllLines(Paths.get(runnerArgsFilePath), Charset.forName("UTF-8"));
+    rlocateRunpathValue(workspace, runnerArgs);
 
     String[] runnerArgsArray = runnerArgs.toArray(new String[runnerArgs.size()]);
 
@@ -73,12 +71,28 @@ private static String[] extendFromEnvVar(
     if (value == null) {
       return args;
     }
-    ;
     String[] flag = new String[] {flagName, value};
     String[] result = new String[args.length + flag.length];
     System.arraycopy(args, 0, result, 0, args.length);
     System.arraycopy(flag, 0, result, args.length, flag.length);
 
     return result;
   }
+
+  /**
+   * Replaces ScalaTest Runner's runpath elements paths (see http://www.scalatest.org/user_guide/using_the_runner)
+   * with values from Bazel's runfiles
+   */
+  private static void rlocateRunpathValue(String rulesWorkspace, List<String> runnerArgs) throws IOException {
+    int runpathFlag = runnerArgs.indexOf("-R");
+    if (runpathFlag >= 0) {
+      String[] runpathElements = runnerArgs.get(runpathFlag + 1).split(File.pathSeparator);
+      Runfiles runfiles = Runfiles.create();
+      for (int i = 0; i < runpathElements.length; i++) {
+        runpathElements[i] = runfiles.rlocation(rulesWorkspace + "/" + runpathElements[i]);
+      }
+      String runpath = String.join(File.separator, runpathElements);
+      runnerArgs.set(runpathFlag + 1, runpath);
+    }
+  }
 }

src/java/io/bazel/rulesscala/scalac/ScalacProcessor.java

@@ -22,6 +22,8 @@
 import scala.tools.nsc.reporters.ConsoleReporter;
 
 class ScalacProcessor implements Processor {
+  private static boolean isWindows = System.getProperty("os.name").toLowerCase().contains("windows");
+
   /** This is the reporter field for scalac, which we want to access */
   private static Field reporterField;
 
@@ -251,7 +253,7 @@ private static void removeTmp(Path tmp) throws IOException {
             @Override
             public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
                 throws IOException {
-              file.toFile().setWritable(true);
+              if (isWindows) file.toFile().setWritable(true);
               Files.delete(file);
               return FileVisitResult.CONTINUE;
             }
@@ -288,11 +290,12 @@ private static void copyResources(
       } else {
         dstr = resource.destination;
       }
-      if (dstr.charAt(0) == '/' || dstr.charAt(0) == '\\') {
+
+      if (dstr.charAt(0) == '/') {
         // we don't want to copy to an absolute destination
         dstr = dstr.substring(1);
       }
-      if (dstr.startsWith("../") || dstr.startsWith("..\\")) {
+      if (dstr.startsWith("../")) {
         // paths to external repositories, for some reason, start with a leading ../
         // we don't want to copy the resource out of our temporary directory, so
         // instead we replace ../ with external/