Simplify scalac worker args format (#1230)

* Get rid of join in starlark

* Remove defaults handling for values which are always passed

* Move plugins formatting to ScalacWorker next to plugin params

* Move validation from CompileOptions next to others in ScalacWorker

* Formatting

* Add test for CompilerOptions args file parsing

* Remove not required interfaces from CompileOptions.Args

* Java naming

Author: simuons

scala/private/rule_impls.bzl

@@ -15,10 +15,7 @@
 
 load("@bazel_skylib//lib:paths.bzl", "paths")
 load("@bazel_tools//tools/jdk:toolchain_utils.bzl", "find_java_runtime_toolchain", "find_java_toolchain")
-load(
-    ":common.bzl",
-    _collect_plugin_paths = "collect_plugin_paths",
-)
+load(":common.bzl", _collect_plugin_paths = "collect_plugin_paths")
 load(":resources.bzl", _resource_paths = "paths")
 
 def expand_location(ctx, flags):
@@ -85,28 +82,28 @@ def compile_scala(
     args.add("--StatsfileOutput", statsfile)
     args.add("--EnableDiagnosticsReport", enable_diagnostics_report)
     args.add("--DiagnosticsFile", diagnosticsfile)
-    args.add_joined("--Classpath", compiler_classpath_jars, join_with = ctx.configuration.host_path_separator)
-    args.add_joined("--ClasspathResourceSrcs", classpath_resources, join_with = ",")
-    args.add_joined("--Files", sources, join_with = ",")
-    args.add_joined("--Plugins", plugins, join_with = ",")
-    args.add_joined("--ResourceTargets", [p[0] for p in resource_paths], join_with = ",")
-    args.add_joined("--ResourceSources", [p[1] for p in resource_paths], join_with = ",")
-    args.add_joined("--ResourceJars", resource_jars, join_with = ",")
-    args.add_joined("--ScalacOpts", scalacopts, join_with = ":::")
-    args.add_joined("--SourceJars", all_srcjars, join_with = ",")
+    args.add_all("--Classpath", compiler_classpath_jars)
+    args.add_all("--ClasspathResourceSrcs", classpath_resources)
+    args.add_all("--Files", sources)
+    args.add_all("--Plugins", plugins)
+    args.add_all("--ResourceTargets", [p[0] for p in resource_paths])
+    args.add_all("--ResourceSources", [p[1] for p in resource_paths])
+    args.add_all("--ResourceJars", resource_jars)
+    args.add_all("--ScalacOpts", scalacopts)
+    args.add_all("--SourceJars", all_srcjars)
 
     if dependency_info.need_direct_info:
         if dependency_info.need_direct_jars:
-            args.add_joined("--DirectJars", cjars, join_with = ",")
+            args.add_all("--DirectJars", cjars)
         if dependency_info.need_direct_targets:
-            args.add_joined("--DirectTargets", [labels[j.path] for j in cjars.to_list()], join_with = ",")
+            args.add_all("--DirectTargets", [labels[j.path] for j in cjars.to_list()])
 
     if dependency_info.need_indirect_info:
-        args.add_joined("--IndirectJars", transitive_compile_jars, join_with = ",")
-        args.add_joined("--IndirectTargets", [labels[j.path] for j in transitive_compile_jars.to_list()], join_with = ",")
+        args.add_all("--IndirectJars", transitive_compile_jars)
+        args.add_all("--IndirectTargets", [labels[j.path] for j in transitive_compile_jars.to_list()])
 
     if dependency_info.unused_deps_mode != "off":
-        args.add_joined("--UnusedDepsIgnoredTargets", unused_dependency_checker_ignored_targets, join_with = ",")
+        args.add_all("--UnusedDepsIgnoredTargets", unused_dependency_checker_ignored_targets)
 
     outs = [output, statsfile, diagnosticsfile]
 

src/java/io/bazel/rulesscala/scalac/BUILD

@@ -1,4 +1,4 @@
-load("@rules_java//java:defs.bzl", "java_binary")
+load("@rules_java//java:defs.bzl", "java_binary", "java_test")
 
 java_binary(
     name = "scalac",
@@ -27,8 +27,14 @@ filegroup(
         "CompileOptions.java",
         "ProtoReporter.java",
         "ReportableMainClass.java",
-        "Resource.java",
         "ScalacWorker.java",
     ],
     visibility = ["//visibility:public"],
 )
+
+java_test(
+    name = "CompileOptionsTest",
+    srcs = ["CompileOptionsTest.java"],
+    test_class = "io.bazel.rulesscala.scalac.CompileOptionsTest",
+    deps = [":scalac"],
+)

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

@@ -1,185 +1,106 @@
 package io.bazel.rulesscala.scalac;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
+import java.util.Arrays;
+import java.util.LinkedHashMap;
 import java.util.Map;
 
 public class CompileOptions {
-  public final String outputName;
-  public final String manifestPath;
-  public final String[] scalaOpts;
-  public final boolean printCompileTime;
-  public final boolean expectJavaOutput;
-  public final String[] pluginArgs;
-  public final String classpath;
-  public final String[] files;
-  public final String[] sourceJars;
-  public final String[] javaFiles;
-  public final List<Resource> resourceFiles;
-  public final String[] resourceJars;
-  public final String[] classpathResourceFiles;
-  public final String[] directJars;
-  public final String[] directTargets;
-  public final String[] unusedDepsIgnoredTargets;
-  public final String[] indirectJars;
-  public final String[] indirectTargets;
-  public final String strictDepsMode;
-  public final String unusedDependencyCheckerMode;
-  public final String currentTarget;
-  public final String statsfile;
-  public final String dependencyTrackingMethod;
-  public final String diagnosticsFile;
-  public final boolean enableDiagnosticsReport;
-
-  public CompileOptions(List<String> args) {
-    Map<String, String> argMap = buildArgMap(args);
-
-    outputName = getOrError(argMap, "JarOutput", "Missing required arg JarOutput");
-    manifestPath = getOrError(argMap, "Manifest", "Missing required arg Manifest");
-
-    scalaOpts = getTripleColonList(argMap, "ScalacOpts");
-    printCompileTime = booleanGetOrFalse(argMap, "PrintCompileTime");
-    expectJavaOutput = booleanGetOrTrue(argMap, "ExpectJavaOutput");
-    pluginArgs = buildPluginArgs(getOrEmpty(argMap, "Plugins"));
-    classpath = getOrError(argMap, "Classpath", "Must supply the classpath arg");
-    files = getCommaList(argMap, "Files");
-
-    javaFiles = getCommaList(argMap, "JavaFiles");
-
-    if (!expectJavaOutput && javaFiles.length != 0) {
-      throw new RuntimeException("Cannot hava java source files when no expected java output");
-    }
-
-    sourceJars = getCommaList(argMap, "SourceJars");
-    resourceFiles = getResources(argMap);
-    resourceJars = getCommaList(argMap, "ResourceJars");
-    classpathResourceFiles = getCommaList(argMap, "ClasspathResourceSrcs");
-
-    directJars = getCommaList(argMap, "DirectJars");
-    directTargets = getCommaList(argMap, "DirectTargets");
-    unusedDepsIgnoredTargets = getCommaList(argMap, "UnusedDepsIgnoredTargets");
-    indirectJars = getCommaList(argMap, "IndirectJars");
-    indirectTargets = getCommaList(argMap, "IndirectTargets");
-
-    strictDepsMode = getOrElse(argMap, "StrictDepsMode", "off");
-    unusedDependencyCheckerMode = getOrElse(argMap, "UnusedDependencyCheckerMode", "off");
-    currentTarget = getOrElse(argMap, "CurrentTarget", "NA");
-    dependencyTrackingMethod = getOrElse(argMap, "DependencyTrackingMethod", "high-level");
-
-    statsfile = getOrError(argMap, "StatsfileOutput", "Missing required arg StatsfileOutput");
-    enableDiagnosticsReport = booleanGetOrFalse(argMap, "EnableDiagnosticsReport");
-    diagnosticsFile = getOrError(argMap, "DiagnosticsFile", "Missing required arg DiagnosticsFile");
-  }
-
-  private static List<Resource> getResources(Map<String, String> args) {
-    String[] targets = getCommaList(args, "ResourceTargets");
-    String[] sources = getCommaList(args, "ResourceSources");
-
-    if (targets.length != sources.length)
-      throw new RuntimeException(
-          String.format(
-              "mismatch in resources: targets: %s sources: %s",
-              getOrEmpty(args, "ResourceTargets"), getOrEmpty(args, "ResourceSources")));
-
-    List<Resource> resources = new ArrayList<Resource>();
-    for (int idx = 0; idx < targets.length; idx++) {
-      resources.add(new Resource(targets[idx], sources[idx]));
-    }
-    return resources;
-  }
-
-  private static Map<String, String> buildArgMap(List<String> lines) {
-    Map<String, String> args = new HashMap<>();
-    for (int i = 0; i < lines.size(); i += 2) {
-      args.put(lines.get(i).substring(2), lines.get(i + 1));
-    }
-    return args;
-  }
-
-  protected static String[] getTripleColonList(Map<String, String> m, String k) {
-    if (m.containsKey(k)) {
-      String v = m.get(k);
-      if ("".equals(v)) {
-        return new String[] {};
-      } else {
-        return v.split(":::");
-      }
-    } else {
-      return new String[] {};
-    }
-  }
-
-  private static String[] getCommaList(Map<String, String> m, String k) {
-    if (m.containsKey(k)) {
-      String v = m.get(k);
-      if ("".equals(v)) {
-        return new String[] {};
-      } else {
-        return v.split(",");
-      }
-    } else {
-      return new String[] {};
-    }
-  }
-
-  private static String getOrEmpty(Map<String, String> m, String k) {
-    return getOrElse(m, k, "");
-  }
-
-  private static String getOrElse(Map<String, String> attrs, String key, String defaultValue) {
-    if (attrs.containsKey(key)) {
-      return attrs.get(key);
-    } else {
-      return defaultValue;
-    }
-  }
-
-  private static String getOrError(Map<String, String> m, String k, String errorMessage) {
-    if (m.containsKey(k)) {
-      return m.get(k);
-    } else {
-      throw new RuntimeException(errorMessage);
-    }
-  }
-
-  private static boolean booleanGetOrFalse(Map<String, String> m, String k) {
-    if (m.containsKey(k)) {
-      String v = m.get(k);
-      if (v.trim().equals("True") || v.trim().equals("true")) {
-        return true;
-      }
-    }
-    return false;
-  }
-
-  private static boolean booleanGetOrTrue(Map<String, String> m, String k) {
-    if (m.containsKey(k)) {
-      String v = m.get(k);
-      if (v.trim().equals("False") || v.trim().equals("false")) {
-        return false;
-      }
-    }
-    return true;
-  }
-
-  public static String[] buildPluginArgs(String packedPlugins) {
-    String[] pluginElements = packedPlugins.split(",");
-    int numPlugins = 0;
-    for (int i = 0; i < pluginElements.length; i++) {
-      if (pluginElements[i].length() > 0) {
-        numPlugins += 1;
-      }
+    public final String outputName;
+    public final String manifestPath;
+    public final String[] scalaOpts;
+    public final boolean printCompileTime;
+    public final boolean expectJavaOutput;
+    public final String[] plugins;
+    public final String[] classpath;
+    public final String[] files;
+    public final String[] sourceJars;
+    public final String[] javaFiles;
+    public final String[] resourceSources;
+    public final String[] resourceTargets;
+    public final String[] resourceJars;
+    public final String[] classpathResourceFiles;
+    public final String[] directJars;
+    public final String[] directTargets;
+    public final String[] unusedDepsIgnoredTargets;
+    public final String[] indirectJars;
+    public final String[] indirectTargets;
+    public final String strictDepsMode;
+    public final String unusedDependencyCheckerMode;
+    public final String currentTarget;
+    public final String statsfile;
+    public final String dependencyTrackingMethod;
+    public final String diagnosticsFile;
+    public final boolean enableDiagnosticsReport;
+
+    public CompileOptions(String[] lines) {
+        Args args = new Args(lines);
+
+        outputName = args.getSingleOrError("JarOutput");
+        manifestPath = args.getSingleOrError("Manifest");
+
+        scalaOpts = args.getOrEmpty("ScalacOpts");
+        printCompileTime = Boolean.parseBoolean(args.getSingleOrError("PrintCompileTime"));
+        expectJavaOutput = Boolean.parseBoolean(args.getSingleOrError("ExpectJavaOutput"));
+        plugins = args.getOrEmpty("Plugins");
+        classpath = args.getOrEmpty("Classpath");
+        files = args.getOrEmpty("Files");
+        sourceJars = args.getOrEmpty("SourceJars");
+        javaFiles = args.getOrEmpty("JavaFiles");
+
+        resourceSources = args.getOrEmpty("ResourceSources");
+        resourceTargets = args.getOrEmpty("ResourceTargets");
+        resourceJars = args.getOrEmpty("ResourceJars");
+        classpathResourceFiles = args.getOrEmpty("ClasspathResourceSrcs");
+
+        directJars = args.getOrEmpty("DirectJars");
+        directTargets = args.getOrEmpty("DirectTargets");
+        unusedDepsIgnoredTargets = args.getOrEmpty("UnusedDepsIgnoredTargets");
+        indirectJars = args.getOrEmpty("IndirectJars");
+        indirectTargets = args.getOrEmpty("IndirectTargets");
+
+        strictDepsMode = args.getSingleOrError("StrictDepsMode");
+        unusedDependencyCheckerMode = args.getSingleOrError("UnusedDependencyCheckerMode");
+        currentTarget = args.getSingleOrError("CurrentTarget");
+        dependencyTrackingMethod = args.getSingleOrError("DependencyTrackingMethod");
+
+        statsfile = args.getSingleOrError("StatsfileOutput");
+        enableDiagnosticsReport = Boolean.parseBoolean(args.getSingleOrError("EnableDiagnosticsReport"));
+        diagnosticsFile = args.getSingleOrError("DiagnosticsFile");
     }
 
-    String[] result = new String[numPlugins];
-    int idx = 0;
-    for (int i = 0; i < pluginElements.length; i++) {
-      if (pluginElements[i].length() > 0) {
-        result[idx] = "-Xplugin:" + pluginElements[i];
-        idx += 1;
-      }
+    static final class Args {
+
+        private static final String[] EMPTY = new String[]{};
+        private final Map<String, String[]> index = new LinkedHashMap<>();
+
+        Args(String[] lines) {
+            int opt = 0;
+            for (int i = 1, n = lines.length; i <= n; i++) {
+                if (i == n || lines[i].startsWith("--")) {
+                    index.put(
+                            lines[opt].substring(2),
+                            Arrays.copyOfRange(lines, opt + 1, i)
+                    );
+                    opt = i;
+                }
+            }
+        }
+
+        String[] getOrEmpty(String k) {
+            return index.getOrDefault(k, EMPTY);
+        }
+
+        String getSingleOrError(String k) {
+            if (index.containsKey(k)) {
+                String[] v = index.get(k);
+                if (v.length == 1) {
+                    return v[0];
+                } else {
+                    throw new RuntimeException(k + " expected to contain single value but got " + Arrays.toString(v));
+                }
+            } else {
+                throw new RuntimeException("Missing required arg " + k);
+            }
+        }
     }
-    return result;
-  }
 }