refactor CliCommand.expandArg method; fix script.path regression, Windows classpath regression

philwalk · philwalk · commit f1f8b7b3eea4 · 2021-09-20T16:35:40.000-06:00
diff --git a/compiler/src/dotty/tools/MainGenericRunner.scala b/compiler/src/dotty/tools/MainGenericRunner.scala
@@ -126,7 +126,7 @@ object MainGenericRunner {
     case (o @ javaOption(striped)) :: tail =>
       process(tail, settings.withJavaArgs(striped).withScalaArgs(o))
     case (o @ scalaOption(_*)) :: tail =>
-      val remainingArgs = (expandArg(o) ++ tail).toList
+      val remainingArgs = (CommandLineParser.expandArg(o) ++ tail).toList
       process(remainingArgs, settings)
     case (o @ colorOption(_*)) :: tail =>
       process(tail, settings.withScalaArgs(o))
@@ -141,19 +141,6 @@ object MainGenericRunner {
         val newSettings = if arg.startsWith("-") then settings else settings.withPossibleEntryPaths(arg).withModeShouldBePossibleRun
         process(tail, newSettings.withResidualArgs(arg))
 
-  // copy of method private to dotty.tools.dotc.config.CliCommand.distill()
-  // TODO: make it available as a public method and remove this copy?
-  def expandArg(arg: String): List[String] =
-    def stripComment(s: String) = s takeWhile (_ != '#')
-    val path = Paths.get(arg stripPrefix "@")
-    if (!Files.exists(path))
-      System.err.println(s"Argument file ${path.getFileName} could not be found")
-      Nil
-    else
-      val lines = Files.readAllLines(path) // default to UTF-8 encoding
-      val params = lines.asScala map stripComment mkString " "
-      CommandLineParser.tokenize(params)
-
   def main(args: Array[String]): Unit =
     val scalaOpts = envOrNone("SCALA_OPTS").toArray.flatMap(_.split(" "))
     val allArgs = scalaOpts ++ args
@@ -204,6 +191,8 @@ object MainGenericRunner {
         }
         errorFn("", res)
       case ExecuteMode.Script =>
+        val targetScriptPath: String = settings.targetScript.toString.replace('\\', '/')
+        System.setProperty("script.path", targetScriptPath)
         val properArgs =
           List("-classpath", settings.classPath.mkString(classpathSeparator)).filter(Function.const(settings.classPath.nonEmpty))
             ++ settings.residualArgs
diff --git a/compiler/src/dotty/tools/dotc/config/CliCommand.scala b/compiler/src/dotty/tools/dotc/config/CliCommand.scala
@@ -7,8 +7,7 @@ import Settings._
 import core.Contexts._
 import Properties._
 
- import scala.PartialFunction.cond
- import scala.collection.JavaConverters._
+import scala.PartialFunction.cond
 
 trait CliCommand:
 
@@ -42,24 +41,10 @@ trait CliCommand:
 
   /** Distill arguments into summary detailing settings, errors and files to main */
   def distill(args: Array[String], sg: Settings.SettingGroup)(ss: SettingsState = sg.defaultState)(using Context): ArgsSummary =
-    /**
-     * Expands all arguments starting with @ to the contents of the
-     * file named like each argument.
-     */
-    def expandArg(arg: String): List[String] =
-      def stripComment(s: String) = s takeWhile (_ != '#')
-      val path = Paths.get(arg stripPrefix "@")
-      if (!Files.exists(path))
-        report.error(s"Argument file ${path.getFileName} could not be found")
-        Nil
-      else
-        val lines = Files.readAllLines(path) // default to UTF-8 encoding
-        val params = lines.asScala map stripComment mkString " "
-        CommandLineParser.tokenize(params)
 
     // expand out @filename to the contents of that filename
     def expandedArguments = args.toList flatMap {
-      case x if x startsWith "@"  => expandArg(x)
+      case x if x startsWith "@"  => CommandLineParser.expandArg(x)
       case x                      => List(x)
     }
 
diff --git a/compiler/src/dotty/tools/dotc/config/CommandLineParser.scala b/compiler/src/dotty/tools/dotc/config/CommandLineParser.scala
@@ -3,6 +3,8 @@ package dotty.tools.dotc.config
 import scala.annotation.tailrec
 import scala.collection.mutable.ArrayBuffer
 import java.lang.Character.isWhitespace
+import java.nio.file.{Files, Paths}
+import scala.collection.JavaConverters._
 
 /** A simple enough command line parser.
  */
@@ -93,4 +95,19 @@ object CommandLineParser:
 
   def tokenize(line: String): List[String] = tokenize(line, x => throw new ParseException(x))
 
+  /**
+   * Expands all arguments starting with @ to the contents of the
+   * file named like each argument.
+   */
+  def expandArg(arg: String): List[String] =
+    def stripComment(s: String) = s takeWhile (_ != '#')
+    val path = Paths.get(arg stripPrefix "@")
+    if (!Files.exists(path))
+      System.err.println(s"Argument file ${path.getFileName} could not be found")
+      Nil
+    else
+      val lines = Files.readAllLines(path) // default to UTF-8 encoding
+      val params = lines.asScala map stripComment mkString " "
+      tokenize(params)
+
   class ParseException(msg: String) extends RuntimeException(msg)
diff --git a/compiler/test-coursier/dotty/tools/coursier/CoursierScalaTests.scala b/compiler/test-coursier/dotty/tools/coursier/CoursierScalaTests.scala
@@ -122,6 +122,15 @@ class CoursierScalaTests:
       assertTrue(output.mkString("\n").contains("Unable to create a system terminal")) // Scala attempted to create REPL so we can assume it is working
     replWithArgs()
 
+    def argumentFile() =
+      // verify that an arguments file is accepted
+      // verify that setting a user classpath does not remove compiler libraries from the classpath.
+      // arguments file contains "-classpath .", adding current directory to classpath.
+      val source = new File(getClass.getResource("/run/myfile.scala").getPath)
+      val argsFile = new File(getClass.getResource("/run/myargs.txt").getPath)
+      val output = CoursierScalaTests.csScalaCmd(s"@$argsFile", source.absPath)
+      assertEquals(output.mkString("\n"), "Hello")
+
 object CoursierScalaTests:
 
   def execCmd(command: String, options: String*): List[String] =
diff --git a/compiler/test-coursier/run/myargs.txt b/compiler/test-coursier/run/myargs.txt
@@ -0,0 +1 @@
+-classpath .
diff --git a/compiler/test-resources/scripting/argfileClasspath.sc b/compiler/test-resources/scripting/argfileClasspath.sc
@@ -0,0 +1,9 @@
+#!dist/target/pack/bin/scala @compiler/test-resources/scripting/cpArgumentsFile.txt
+
+import java.nio.file.Paths
+
+def main(args: Array[String]): Unit =
+  val cwd = Paths.get(".").toAbsolutePath.toString.replace('\\', '/').replaceAll("/$", "")
+  printf("cwd: %s\n", cwd)
+  printf("classpath: %s\n", sys.props("java.class.path"))
+
diff --git a/compiler/test-resources/scripting/classpathReport.sc b/compiler/test-resources/scripting/classpathReport.sc
@@ -1,9 +1,12 @@
-#!dist/target/pack/bin/scala -classpath 'dist/target/pack/lib/*'
+#!dist/target/pack/bin/scala -classpath dist/target/pack/lib/*
 
 import java.nio.file.Paths
 
 def main(args: Array[String]): Unit =
-  val cwd = Paths.get(".").toAbsolutePath.toString.replace('\\', '/').replaceAll("/$", "")
+  val cwd = Paths.get(".").toAbsolutePath.normalize.toString.norm
   printf("cwd: %s\n", cwd)
-  printf("classpath: %s\n", sys.props("java.class.path"))
+  printf("classpath: %s\n", sys.props("java.class.path").norm)
+
+extension(s: String)
+  def norm: String = s.replace('\\', '/')
 
diff --git a/compiler/test-resources/scripting/cpArgumentsFile.txt b/compiler/test-resources/scripting/cpArgumentsFile.txt
@@ -0,0 +1 @@
+-classpath dist/target/pack/lib/*
diff --git a/compiler/test-resources/scripting/scriptPath.sc b/compiler/test-resources/scripting/scriptPath.sc
@@ -1,10 +1,13 @@
-#!/usr/bin/env scala
+#!dist/target/pack/bin/scala
 
   def main(args: Array[String]): Unit =
     args.zipWithIndex.foreach { case (arg,i) => printf("arg %d: [%s]\n",i,arg) }
     val path = Option(sys.props("script.path")) match {
     case None => printf("no script.path property is defined\n")
     case Some(path) =>
-      printf("script.path: %s\n",path)
+      printf("script.path: %s\n",path.norm)
       assert(path.endsWith("scriptPath.sc"),s"actual path [$path]")
     }
+
+  extension(s: String)
+    def norm: String = s.replace('\\', '/')
diff --git a/compiler/test/dotty/tools/scripting/BashScriptsTests.scala b/compiler/test/dotty/tools/scripting/BashScriptsTests.scala
@@ -84,12 +84,12 @@ class BashScriptsTests:
   @Test def verifyScriptPathProperty =
     val scriptFile = testFiles.find(_.getName == "scriptPath.sc").get
     val expected = s"/${scriptFile.getName}"
-    printf("===> verify valid system property script.path is reported by script [%s]\n", scriptFile.getName)
+    System.err.printf("===> verify valid system property script.path is reported by script [%s]\n", scriptFile.getName)
     val (exitCode, stdout, stderr) = bashCommand(scriptFile.absPath)
     if exitCode == 0 && ! stderr.exists(_.contains("Permission denied")) then
       // var cmd = Array(bashExe, "-c", scriptFile.absPath)
       // val stdout = Process(cmd).lazyLines_!
-      stdout.foreach { printf("######### [%s]\n", _) }
+      stdout.foreach { System.err.printf("######### [%s]\n", _) }
       val valid = stdout.exists { _.endsWith(expected) }
       if valid then printf("# valid script.path reported by [%s]\n", scriptFile.getName)
       assert(valid, s"script ${scriptFile.absPath} did not report valid script.path value")
@@ -99,35 +99,39 @@ class BashScriptsTests:
    */
   @Test def verifyScalaOpts =
     val scriptFile = testFiles.find(_.getName == "classpathReport.sc").get
-    printf("===> verify valid system property script.path is reported by script [%s]\n", scriptFile.getName)
+    printf("===> verify SCALA_OPTS -classpath setting in argument file seen by script [%s]\n", scriptFile.getName)
     val argsfile = createArgsFile() // avoid problems caused by drive letter
     val envPairs = List(("SCALA_OPTS", s"@$argsfile"))
     val (exitCode, stdout, stderr) = bashCommand(scriptFile.absPath, envPairs:_*)
+    printf("\n")
     if exitCode != 0 || stderr.exists(_.contains("Permission denied")) then
       stderr.foreach { System.err.printf("stderr [%s]\n", _) }
       printf("unable to execute script, return value is %d\n", exitCode)
     else
-      // val stdout: Seq[String] = Process(cmd, cwd, envPairs:_*).lazyLines_!.toList
-      val expected = s"${cwd.toString}"
+      val expected = cwd
       val List(line1: String, line2: String) = stdout.take(2)
+      printf("line1 [%s]\n", line1)
       val valid = line2.dropWhile( _ != ' ').trim.startsWith(expected)
+      val psep = if osname.startsWith("Windows") then ';' else ':'
+      printf("line2 start [%s]\n", line2.take(100))
       if valid then printf(s"\n===> success: classpath begins with %s, as reported by [%s]\n", cwd, scriptFile.getName)
-      assert(valid, s"script ${scriptFile.absPath} did not report valid java.class.path first entry")
+      assert(valid, s"script ${scriptFile.getName} did not report valid java.class.path first entry")
 
-  lazy val cwd = Paths.get(dotty.tools.dotc.config.Properties.userDir).toFile
+  lazy val cwd: String = Paths.get(".").toAbsolutePath.normalize.toString.norm
 
   def createArgsFile(): String =
     val utfCharset = java.nio.charset.StandardCharsets.UTF_8.name
-    val text = s"-classpath ${cwd.absPath}"
+    val text = s"-classpath $cwd"
     val path = Files.createTempFile("scriptingTest", ".args")
     Files.write(path, text.getBytes(utfCharset))
-    path.toFile.getAbsolutePath.replace('\\', '/')
+    path.toFile.getAbsolutePath.norm
 
-  extension (str: String) def dropExtension: String =
-    str.reverse.dropWhile(_ != '.').drop(1).reverse
+  extension(str: String)
+    def norm: String = str.replace('\\', '/')
+    def dropExtension: String = str.reverse.dropWhile(_ != '.').drop(1).reverse
 
   extension(f: File) def absPath: String =
-    f.getAbsolutePath.replace('\\', '/')
+    f.getAbsolutePath.norm
 
   lazy val osname = Option(sys.props("os.name")).getOrElse("").toLowerCase
 
diff --git a/compiler/test/dotty/tools/scripting/ClasspathTests.scala b/compiler/test/dotty/tools/scripting/ClasspathTests.scala
@@ -13,104 +13,29 @@ import scala.sys.process._
 import scala.jdk.CollectionConverters._
 import dotty.tools.dotc.config.Properties._
 
-/** Runs all tests contained in `compiler/test-resources/scripting/` */
+/** Test java command line generated by bin/scala and bin/scalac */
 class ClasspathTests:
   val packBinDir = "dist/target/pack/bin"
-  val scalaCopy = makeTestableScriptCopy("scala")
-  val scalacCopy = makeTestableScriptCopy("scalac")
-  val commonCopy = makeTestableScriptCopy("common")
+  val packLibDir = "dist/target/pack/lib"
 
   // only interested in classpath test scripts
-  def testFiles = scripts("/scripting").filter { _.getName.matches("classpath.*[.]sc") }
   val testScriptName = "classpathReport.sc"
-  def testScript = testFiles.find { _.getName == testScriptName } match
+  val testScript = scripts("/scripting").find { _.getName.matches(testScriptName) } match
     case None => sys.error(s"test script not found: ${testScriptName}")
     case Some(file) => file
 
-  def getScriptPath(scriptName: String): Path = Paths.get(s"$packBinDir/$scriptName")
-
   def exists(scriptPath: Path): Boolean = Files.exists(scriptPath)
   def packBinScalaExists:Boolean = exists(Paths.get(s"$packBinDir/scala"))
 
-  // create edited copy of [dist/bin/scala] and [dist/bin/scalac] for scalacEchoTest
-  def makeTestableScriptCopy(scriptName: String): Path =
-    val scriptPath: Path = getScriptPath(scriptName)
-    val scriptCopy: Path = getScriptPath(s"$scriptName-copy")
-    if Files.exists(scriptPath) then
-      val lines = Files.readAllLines(scriptPath).asScala.map {
-        _.replaceAll("/scalac", "/scalac-copy").
-        replaceAll("/common", "/common-copy").
-        replaceFirst("^ *eval(.*JAVACMD.*)", "echo $1")
-      }
-      val bytes = (lines.mkString("\n")+"\n").getBytes
-      Files.write(scriptCopy, bytes)
-
-    scriptCopy
-
-  /*
-   * verify java command line generated by scalac.
-   */
-  @Test def scalacEchoTest =
-    val relpath = testScript.toPath.relpath.norm
-    printf("===> scalacEchoTest for script [%s]\n", relpath)
-    printf("bash is [%s]\n", bashExe)
-
-    if packBinScalaExists then
-      val bashCmdline = s"SCALA_OPTS= ${scalaCopy.norm} -classpath '$wildcardEntry' $relpath"
-     
-      // ask [dist/bin/scalac] to echo generated command line so we can verify some things
-      val cmd = Array(bashExe, "-c", bashCmdline)
-
-      //cmd.foreach { printf("[%s]\n", _) }
-
-      val javaCommandLine = exec(cmd:_*).mkString(" ").split(" ").filter { _.trim.nonEmpty }
-      printf("\n==================== isWin[%s], cygwin[%s], mingw[%s], msys[%s]\n", isWin, cygwin, mingw, msys)
-      javaCommandLine.foreach { printf("java-command[%s]\n", _) }
-
-      val output = scala.collection.mutable.Queue(javaCommandLine:_*)
-      output.dequeueWhile( _ != "dotty.tools.scripting.Main")
-
-      def consumeNext = if output.isEmpty then "" else output.dequeue()
-
-      // assert that we found "dotty.tools.scripting.Main"
-      val str = consumeNext
-      if str != "dotty.tools.scripting.Main" then
-        
-      assert(str == "dotty.tools.scripting.Main", s"found [$str]")
-      val mainArgs = output.copyToArray(Array.ofDim[String](output.length))
-
-      // display command line starting with "dotty.tools.scripting.Main"
-      output.foreach { line =>
-        printf("%s\n", line)
-      }
-
-      // expecting -classpath next
-      assert(consumeNext.replaceAll("'", "") == "-classpath")
-      
-      // 2nd arg to scripting.Main is 'lib/*', with semicolon added if Windows jdk
-    
-      // PR #10761: verify that [dist/bin/scala] -classpath processing adds $psep to wildcard if Windows
-      val classpathValue = consumeNext
-      printf("classpath value [%s]\n", classpathValue)
-      assert( !winshell || classpathValue.contains(psep) )
-    
-      // expecting -script next
-      assert(consumeNext.replaceAll("'", "") == "-script")
-
-      // PR #10761: verify that Windows jdk did not expand single wildcard classpath to multiple file paths
-      if javaCommandLine.last != relpath then
-        printf("last: %s\nrelp: %s\n", javaCommandLine.last, relpath)
-        assert(javaCommandLine.last == relpath, s"unexpected output passed to scripting.Main")
-
   /*
    * verify classpath reported by called script.
    */
-  @Test def hashbangClasspathVerifyTest =
+  @Test def hashbangClasspathVerifyTest = {
     val relpath = testScript.toPath.relpath.norm
     printf("===> hashbangClasspathVerifyTest for script [%s]\n", relpath)
     printf("bash is [%s]\n", bashExe)
 
-    if false && packBinScalaExists then
+    if packBinScalaExists then
       val bashCmdline = s"SCALA_OPTS= $relpath"
       val cmd = Array(bashExe, "-c", bashCmdline)
 
@@ -123,14 +48,15 @@ class ClasspathTests:
       val scriptCp = findTaggedLine("classpath", scriptOutput)
 
       val hashbangClasspathJars = scriptCp.split(psep).map { _.getName }.sorted.distinct
-      val packlibJars = listJars(s"$scriptCwd/dist/target/pack/lib").sorted.distinct
+      val packlibJars = listJars(s"$scriptCwd/$packLibDir").sorted.distinct
      
       // verify that the classpath set in the hashbang line is effective
       if hashbangClasspathJars.size != packlibJars.size then
         printf("%d test script jars in classpath\n", hashbangClasspathJars.size)
         printf("%d jar files in dist/target/pack/lib\n", packlibJars.size)
 
       assert(hashbangClasspathJars.size == packlibJars.size)
+  }
 
 
 //////////////// end of tests ////////////////
diff --git a/dist/bin/scala b/dist/bin/scala
@@ -28,10 +28,39 @@ fi
 
 source "$PROG_HOME/bin/common"
 
+case `uname` in
+CYG*|MINGW*|MSYS*) PSEP=';' ;;
+*) PSEP=':' ;;
+esac
+
+ARGS=()
+while [ $# -gt 0 ]; do
+  case "$1" in
+  -cp | -classpath) # partial fix for #10761
+    # passed as two arguments, e.g. '-classpath' 'lib/*'
+    ARGS+=($1)
+    ARGS+=("$2${PSEP}")
+    shift
+    shift
+    ;;
+  -cp*|-classpath*) # partial fix for #10761
+    # passed as a single argument, e.g. '-classpath lib/*'
+    # (hashbang line can glom args together)
+    ARGS+=('-classpath')
+    ARGS+=("\"${1#* *}${PSEP}\"")
+    shift
+    ;;
+  *)
+    ARGS+=($1)
+    shift
+    ;;
+  esac
+done
+
 # exec here would prevent onExit from being called, leaving terminal in unusable state
 compilerJavaClasspathArgs
 [ -z "${ConEmuPID-}" -o -n "${cygwin-}" ] && export MSYSTEM= PWD= # workaround for #12405
-eval "\"$JAVACMD\"" "-classpath \"$jvm_cp_args\"" "dotty.tools.MainGenericRunner" "-classpath \"$jvm_cp_args\"" "$@"
+eval "\"$JAVACMD\"" "-classpath \"$jvm_cp_args\"" "dotty.tools.MainGenericRunner" "-classpath \"$jvm_cp_args\"" "${ARGS[@]}"
 scala_exit_status=$?
 
 
