For Windows: Fix to enable scala_test and scala_binary outputs to be executed on Windows (#1502)

* Fixed path forms passed into the Windows Launcher.
Passing the correct path forms (i.e. rpathlocation vs rootpath) to Windows Launcher.
This enables running scala_test and scala_binary outputs through bazel on windows.

* small lint fixes

* minor changes to resolve review comments
- Added comment on use of env in _write_executable_windows()
- Removed unecessary var in rpathlocation_from_rootpath()

Author: crt-31

scala/private/common.bzl

@@ -1,5 +1,6 @@
 load("@io_bazel_rules_scala//scala:jars_to_labels.bzl", "JarsToLabelsInfo")
 load("@io_bazel_rules_scala//scala:plusone.bzl", "PlusOneDeps")
+load("@bazel_skylib//lib:paths.bzl", "paths")
 
 def write_manifest_file(actions, output_file, main_class):
     # TODO(bazel-team): I don't think this classpath is what you want
@@ -134,3 +135,8 @@ def sanitize_string_for_usage(s):
         else:
             res_array.append("_")
     return "".join(res_array)
+
+#generates rpathlocation that should be used with the rlocation() at runtime. (rpathlocations start with repo name)
+#rootpath arg expects "rootpath" format (i.e. relative to runfilesDir/workspacename). Rootpath can be obtained by $rootpath macro or File.short_path
+def rpathlocation_from_rootpath(ctx, rootpath):
+    return paths.normalize(ctx.workspace_name + "/" + rootpath)

scala/private/phases/phase_write_executable.bzl

@@ -9,6 +9,7 @@ load(
     "first_non_empty",
     "is_windows",
     "java_bin",
+    "java_bin_windows",
     "runfiles_root",
 )
 
@@ -76,21 +77,24 @@ def _phase_write_executable(
 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()],
-    )
+    #launcher expects classpaths to be in rootpath form (ie. relative to runfiles_dir, short_path does the trick)
+    classpath = ";".join([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)
+    java_for_exe = java_bin_windows(ctx)
 
     cpfile = ctx.actions.declare_file("%s.classpath" % ctx.label.name)
     ctx.actions.write(cpfile, classpath)
 
+    #Handle fact that some scala rules add env as an attr and some don't.
+    specifiedEnv = getattr(ctx.attr, "env", {})
+
     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],
-        env = ctx.attr.env,
+        env = specifiedEnv,
         mnemonic = "ExeLauncher",
         progress_message = "Creating exe launcher",
     )

scala/private/rule_impls.bzl

@@ -15,7 +15,7 @@
 
 load("@bazel_skylib//lib:paths.bzl", "paths")
 load("@bazel_tools//tools/jdk:toolchain_utils.bzl", "find_java_toolchain")
-load(":common.bzl", _collect_plugin_paths = "collect_plugin_paths")
+load(":common.bzl", "rpathlocation_from_rootpath", _collect_plugin_paths = "collect_plugin_paths")
 load(":resources.bzl", _resource_paths = "paths")
 
 def expand_location(ctx, flags):
@@ -203,5 +203,18 @@ def java_bin(ctx):
         javabin = "%s/%s" % (runfiles_root_var, java_path)
     return javabin
 
+def java_bin_windows(ctx):
+    java_runtime = specified_java_runtime(
+        ctx,
+        default_runtime = ctx.attr._java_runtime[java_common.JavaRuntimeInfo],
+    )
+
+    if paths.is_absolute(java_runtime.java_executable_runfiles_path):
+        java_bin = java_runtime.java_executable_runfiles_path
+    else:
+        java_bin = rpathlocation_from_rootpath(ctx, java_runtime.java_executable_runfiles_path)
+
+    return java_bin
+
 def is_windows(ctx):
     return ctx.configuration.host_path_separator == ";"

src/java/io/bazel/rulesscala/exe/LauncherFileWriter.java

@@ -11,6 +11,7 @@
 import java.nio.file.*;
 import java.util.Arrays;
 import java.util.List;
+import java.nio.file.Paths;
 
 public class LauncherFileWriter {
   public static void main(String[] args) throws IOException {
@@ -30,10 +31,10 @@ public static void main(String[] args) throws IOException {
             .addKeyValuePair("binary_type", "Java")
             .addKeyValuePair("workspace_name", workspaceName)
             .addKeyValuePair("symlink_runfiles_enabled", "0")
-            .addKeyValuePair("java_bin_path", fullJavaBinPath(workspaceName, Paths.get(javaBinPath)).toString())
-            .addKeyValuePair("jar_bin_path", jarBinPath)
+            .addKeyValuePair("java_bin_path", javaBinPath.replace("\\", "/")) //Expects rpathlocation (i.e. with prepended repo name)
+            .addKeyValuePair("jar_bin_path", rpathlocation_to_rootpath( workspaceName, jarBinPath)) //Expects rootpath location 
             .addKeyValuePair("java_start_class", javaStartClass)
-            .addKeyValuePair("classpath", classpath)
+            .addKeyValuePair("classpath", classpath) //Expects rootpath location
             .addJoinedValues("jvm_flags", "\t", jvmFlags)
             .build();
 
@@ -55,14 +56,17 @@ public static void main(String[] args) throws IOException {
     }
   }
 
-  private static Path fullJavaBinPath(String workspaceName, Path javaBinPath) {
-    if (javaBinPath.isAbsolute()) {
-      return javaBinPath;
-    } else if (javaBinPath.startsWith(Paths.get("external"))) {
-      // Paths under `external/` already have a workspace name.
-      return javaBinPath;
-    } else {
-      return Paths.get(workspaceName).resolve(javaBinPath);
+  //Bazel's java_launcher expects some fields(i.e. jar_bin_path and classpaths) to be rootpath. rpathlocation is relative to runfiledir (always prefix with repo). Rootpath is relative to runfiledir/workspacename. 
+  static String rpathlocation_to_rootpath(String workspaceName, String rpathlocation){
+    Path path = Paths.get(rpathlocation);
+
+    Path result;
+    if(!path.startsWith(workspaceName)){
+      //Assume that if its not in the main workspace, that it is an external repo (and should be one level down)
+      result = Paths.get("../").resolve(path);
+    }else{
+      result = Paths.get(workspaceName).resolve(path);
     }
+    return result.toString().replace("\\", "/");
   }
 }