Windows support - code review fixes

Author: majcherm-da

scala/private/rule_impls.bzl

@@ -592,94 +592,102 @@ def _jar_path_based_on_java_bin(ctx):
 
 def _write_executable(ctx, executable, rjars, main_class, jvm_flags, wrapper, use_jacoco):
     if (_is_windows(ctx)):
-        classpath = ";".join(
-            [("external/%s" % (j.short_path[3:]) if j.short_path.startswith("../") else j.short_path) for j in rjars.to_list()],
+        return _write_executable_windows(ctx, executable, rjars, main_class, jvm_flags, wrapper, use_jacoco)
+    else:
+        return _write_executable_non_windows(ctx, executable, rjars, main_class, jvm_flags, wrapper, use_jacoco)
+
+def _write_executable_windows(ctx, executable, rjars, main_class, jvm_flags, wrapper, use_jacoco):
+    # NOTE: `use_jacoco` is currently ignored on Windows.
+    # TODO: tests coverage support for Windows
+    classpath = ";".join(
+        [("external/%s" % (j.short_path[3:]) if j.short_path.startswith("../") else j.short_path) for j in rjars.to_list()],
+    )
+    jvm_flags_str = ";".join(jvm_flags)
+    java_for_exe = str(ctx.attr._java_runtime[java_common.JavaRuntimeInfo].java_executable_exec_path)
+
+    cpfile = ctx.actions.declare_file("%s.classpath" % ctx.label.name)
+    ctx.actions.write(cpfile, classpath)
+
+    ctx.actions.run(
+        outputs = [executable],
+        inputs = [cpfile],
+        executable = ctx.attr._exe.files_to_run.executable,
+        arguments = [executable.path, ctx.workspace_name, java_for_exe, main_class, cpfile.path, jvm_flags_str],
+        mnemonic = "ExeLauncher",
+        progress_message = "Creating exe launcher",
+    )
+    return []
+
+def _write_executable_non_windows(ctx, executable, rjars, main_class, jvm_flags, wrapper, use_jacoco):
+    template = ctx.attr._java_stub_template.files.to_list()[0]
+
+    jvm_flags = " ".join(
+        [ctx.expand_location(f, ctx.attr.data) for f in jvm_flags],
+    )
+
+    javabin = "export REAL_EXTERNAL_JAVA_BIN=${JAVABIN};JAVABIN=%s/%s" % (
+        _runfiles_root(ctx),
+        wrapper.short_path,
+    )
+
+    if use_jacoco and _coverage_replacements_provider.is_enabled(ctx):
+        classpath = ":".join(
+            ["${RUNPATH}%s" % (j.short_path) for j in rjars.to_list() + ctx.files._jacocorunner + ctx.files._lcov_merger],
         )
-        jvm_flags_str = ";".join(jvm_flags)
-        java_for_exe = str(ctx.attr._java_runtime[java_common.JavaRuntimeInfo].java_executable_exec_path)
-
-        cpfile = ctx.actions.declare_file("%s.classpath" % ctx.label.name)
-        ctx.actions.write(cpfile, classpath)
-
-        ctx.actions.run(
-            outputs = [executable],
-            inputs = [cpfile],
-            executable = ctx.attr._exe.files_to_run.executable,
-            arguments = [executable.path, ctx.workspace_name, java_for_exe, main_class, cpfile.path, jvm_flags_str],
-            mnemonic = "ExeLauncher",
-            progress_message = "Creating exe launcher",
+        jacoco_metadata_file = ctx.actions.declare_file(
+            "%s.jacoco_metadata.txt" % ctx.attr.name,
+            sibling = executable,
         )
-        return []
+        ctx.actions.write(jacoco_metadata_file, "\n".join([
+            jar.short_path.replace("../", "external/")
+            for jar in rjars
+        ]))
+        ctx.actions.expand_template(
+            template = template,
+            output = executable,
+            substitutions = {
+                "%classpath%": classpath,
+                "%javabin%": javabin,
+                "%jarbin%": _jar_path_based_on_java_bin(ctx),
+                "%jvm_flags%": jvm_flags,
+                "%needs_runfiles%": "",
+                "%runfiles_manifest_only%": "",
+                "%workspace_prefix%": ctx.workspace_name + "/",
+                "%java_start_class%": "com.google.testing.coverage.JacocoCoverageRunner",
+                "%set_jacoco_metadata%": "export JACOCO_METADATA_JAR=\"$JAVA_RUNFILES/{}/{}\"".format(ctx.workspace_name, jacoco_metadata_file.short_path),
+                "%set_jacoco_main_class%": """export JACOCO_MAIN_CLASS={}""".format(main_class),
+                "%set_jacoco_java_runfiles_root%": """export JACOCO_JAVA_RUNFILES_ROOT=$JAVA_RUNFILES/{}/""".format(ctx.workspace_name),
+                "%set_java_coverage_new_implementation%": """export JAVA_COVERAGE_NEW_IMPLEMENTATION=YES""",
+            },
+            is_executable = True,
+        )
+        return [jacoco_metadata_file]
     else:
-        template = ctx.attr._java_stub_template.files.to_list()[0]
-
-        jvm_flags = " ".join(
-            [ctx.expand_location(f, ctx.attr.data) for f in jvm_flags],
+        # RUNPATH is defined here:
+        # https://github.com/bazelbuild/bazel/blob/0.4.5/src/main/java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt#L227
+        classpath = ":".join(
+            ["${RUNPATH}%s" % (j.short_path) for j in rjars.to_list()],
         )
-
-        javabin = "export REAL_EXTERNAL_JAVA_BIN=${JAVABIN};JAVABIN=%s/%s" % (
-            _runfiles_root(ctx),
-            wrapper.short_path,
+        ctx.actions.expand_template(
+            template = template,
+            output = executable,
+            substitutions = {
+                "%classpath%": classpath,
+                "%java_start_class%": main_class,
+                "%javabin%": javabin,
+                "%jarbin%": _jar_path_based_on_java_bin(ctx),
+                "%jvm_flags%": jvm_flags,
+                "%needs_runfiles%": "",
+                "%runfiles_manifest_only%": "",
+                "%set_jacoco_metadata%": "",
+                "%set_jacoco_main_class%": "",
+                "%set_jacoco_java_runfiles_root%": "",
+                "%workspace_prefix%": ctx.workspace_name + "/",
+                "%set_java_coverage_new_implementation%": """export JAVA_COVERAGE_NEW_IMPLEMENTATION=NO""",
+            },
+            is_executable = True,
         )
-
-        if use_jacoco and _coverage_replacements_provider.is_enabled(ctx):
-            classpath = ":".join(
-                ["${RUNPATH}%s" % (j.short_path) for j in rjars.to_list() + ctx.files._jacocorunner + ctx.files._lcov_merger],
-            )
-            jacoco_metadata_file = ctx.actions.declare_file(
-                "%s.jacoco_metadata.txt" % ctx.attr.name,
-                sibling = executable,
-            )
-            ctx.actions.write(jacoco_metadata_file, "\n".join([
-                jar.short_path.replace("../", "external/")
-                for jar in rjars
-            ]))
-            ctx.actions.expand_template(
-                template = template,
-                output = executable,
-                substitutions = {
-                    "%classpath%": classpath,
-                    "%javabin%": javabin,
-                    "%jarbin%": _jar_path_based_on_java_bin(ctx),
-                    "%jvm_flags%": jvm_flags,
-                    "%needs_runfiles%": "",
-                    "%runfiles_manifest_only%": "",
-                    "%workspace_prefix%": ctx.workspace_name + "/",
-                    "%java_start_class%": "com.google.testing.coverage.JacocoCoverageRunner",
-                    "%set_jacoco_metadata%": "export JACOCO_METADATA_JAR=\"$JAVA_RUNFILES/{}/{}\"".format(ctx.workspace_name, jacoco_metadata_file.short_path),
-                    "%set_jacoco_main_class%": """export JACOCO_MAIN_CLASS={}""".format(main_class),
-                    "%set_jacoco_java_runfiles_root%": """export JACOCO_JAVA_RUNFILES_ROOT=$JAVA_RUNFILES/{}/""".format(ctx.workspace_name),
-                    "%set_java_coverage_new_implementation%": """export JAVA_COVERAGE_NEW_IMPLEMENTATION=YES""",
-                },
-                is_executable = True,
-            )
-            return [jacoco_metadata_file]
-        else:
-            # RUNPATH is defined here:
-            # https://github.com/bazelbuild/bazel/blob/0.4.5/src/main/java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt#L227
-            classpath = ":".join(
-                ["${RUNPATH}%s" % (j.short_path) for j in rjars.to_list()],
-            )
-            ctx.actions.expand_template(
-                template = template,
-                output = executable,
-                substitutions = {
-                    "%classpath%": classpath,
-                    "%java_start_class%": main_class,
-                    "%javabin%": javabin,
-                    "%jarbin%": _jar_path_based_on_java_bin(ctx),
-                    "%jvm_flags%": jvm_flags,
-                    "%needs_runfiles%": "",
-                    "%runfiles_manifest_only%": "",
-                    "%set_jacoco_metadata%": "",
-                    "%set_jacoco_main_class%": "",
-                    "%set_jacoco_java_runfiles_root%": "",
-                    "%workspace_prefix%": ctx.workspace_name + "/",
-                    "%set_java_coverage_new_implementation%": """export JAVA_COVERAGE_NEW_IMPLEMENTATION=NO""",
-                },
-                is_executable = True,
-            )
-            return []
+        return []
 
 def _declare_executable(ctx):
     if (_is_windows(ctx)):
@@ -1166,7 +1174,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/exe/LaunchInfo.java

@@ -14,7 +14,6 @@
 
 package io.bazel.rulesscala.exe;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import java.io.IOException;
@@ -41,7 +40,6 @@ public static Builder builder() {
   }
 
   /** Writes this object's entries to {@code out}, returns the total written amount in bytes. */
-  @VisibleForTesting
   long write(OutputStream out) throws IOException {
     long len = 0;
     for (Entry e : entries) {

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/

test/BUILD

@@ -101,6 +101,13 @@ scala_test_suite(
     ],
 )
 
+scala_test_suite(
+    name = "LongTestSuiteNamesSuite",
+    size = "small",
+    srcs = glob(["longnames/**/*.scala"]),
+    use_short_names = True,
+)
+
 scala_test_suite(
     name = "TestSuitePassesKwArgs",
     size = "small",  # Not a macro, can pass test-specific attributes.