Skip to content

Commit

Permalink
bugfix: Fix running when spaces are involved on windows
Browse files Browse the repository at this point in the history
Previously, we would not escape Java path, classpath or java properties, which meant that whenever spaces were involved things would not work on Windows (possibly on some other systems too). Now, we properly escape all those things.

I tested both by hand and added a test case, which proved to be a pain to write :/

Fixes #4801
  • Loading branch information
tgodzik committed Jan 5, 2023
1 parent 1ea955e commit 468bb97
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 49 deletions.
Expand Up @@ -2,6 +2,8 @@ package scala.meta.internal.metals.debug

import java.io.File

import scala.util.Properties

import scala.meta.internal.metals.MetalsEnrichments._
import scala.meta.io.AbsolutePath

Expand Down Expand Up @@ -42,10 +44,21 @@ object ExtendedScalaMainClass {
arguments: List[String],
mainClass: String,
): String = {
val jvmOptsString = jvmOptions.mkString(" ")
val jvmOptsString =
if (jvmOptions.nonEmpty) jvmOptions.mkString("\"", "\" \"", "\"") else ""
val classpathString = classpath.mkString(File.pathSeparator)
val argumentsString = arguments.mkString(" ")
s"$javaHome $jvmOptsString -classpath $classpathString $mainClass $argumentsString"
// We need to add "" to account for whitespace and also escaped \ before "
val escapedJavaHome = javaHome.toNIO.getRoot().toString +
javaHome.toNIO
.iterator()
.asScala
.map(p => "\"" + p + "\"")
.mkString(File.separator)
val safeJavaHome =
if (Properties.isWin) escapedJavaHome.replace("""\"""", """\\"""")
else escapedJavaHome
s"$safeJavaHome $jvmOptsString -classpath \"$classpathString\" $mainClass $argumentsString"
}

def apply(
Expand Down
118 changes: 71 additions & 47 deletions tests/unit/src/test/scala/tests/CodeLensLspSuite.scala
Expand Up @@ -9,7 +9,7 @@ import com.google.gson.JsonObject
import org.eclipse.lsp4j.Command

class CodeLensLspSuite extends BaseCodeLensLspSuite("codeLenses") {

override protected val changeSpacesToDash = false
override def userConfig: UserConfiguration =
super.userConfig.copy(superMethodLensesEnabled = true)

Expand Down Expand Up @@ -357,56 +357,80 @@ class CodeLensLspSuite extends BaseCodeLensLspSuite("codeLenses") {
|""".stripMargin
)

test("run-shell-command") {

def runFromCommand(cmd: Command) = {
private def runFromCommand(cmd: Command) = {
cmd.getArguments().asScala.toList match {
case (params: DebugSessionParams) :: _ =>
params.getData() match {
case obj: JsonObject =>
val cmd = obj
.get("shellCommand")
.getAsString()
// need to remove all escapes since ShellRunner does escaping itself
// for windows
.replace("\\\"", "")
.replace("\"\\", "\\")
// for linux
.replace("/\"", "/")
.replace("\"/", "/")
// when testing on Windows Program Files is problematic when splitting into list
.replace("Program Files", "ProgramFiles")
.replace("run shell command", "runshellcommand")
.split("\\s+")
.map(
// remove from escaped classpath
_.stripPrefix("\"")
.stripSuffix("\"")
// Add back spaces
.replace("ProgramFiles", "Program Files")
.replace("runshellcommand", "run shell command")
)
ShellRunner
.runSync(cmd.toList, workspace, redirectErrorOutput = false)
.map(_.trim())
.orElse {
scribe.error(
"Couldn't run command specified in shellCommand."
)
scribe.error("The command run was:\n" + cmd.mkString(" "))
None
}
case _ => None
}

cmd.getArguments().asScala.toList match {
case (params: DebugSessionParams) :: _ =>
params.getData() match {
case obj: JsonObject =>
val cmd = obj.get("shellCommand").getAsString().split("\\s+")
ShellRunner
.runSync(cmd.toList, workspace, redirectErrorOutput = false)
.map(_.trim())
.orElse {
scribe.error(
"Couldn't run command specified in shellCommand."
)
scribe.error("The command run was:\n" + cmd.mkString(" "))
None
}
case _ => None
}
case _ => None
}
}

case _ => None
}
def testRunShellCommand(name: String): Unit =
test(name) {
cleanWorkspace()
for {
_ <- initialize(
s"""|/metals.json
|{
| "a": {}
|}
|/a/src/main/scala/a/Main.scala
|package foo
|
|object Main {
| def main(args: Array[String]): Unit = {
| println("Hello java!")
| }
|}
|""".stripMargin
)
_ <- server.didOpen("a/src/main/scala/a/Main.scala")
lenses <- server.codeLenses("a/src/main/scala/a/Main.scala")
_ = assert(lenses.size > 0, "No lenses were generated!")
command = lenses.head.getCommand()
_ = assertEquals(runFromCommand(command), Some("Hello java!"))
} yield ()
}
cleanWorkspace()
for {
_ <- initialize(
s"""|/metals.json
|{
| "a": {}
|}
|/a/src/main/scala/a/Main.scala
|package foo
|
|object Main {
| def main(args: Array[String]): Unit = {
| println("Hello java!")
| }
|}
|""".stripMargin
)
_ <- server.didOpen("a/src/main/scala/a/Main.scala")
lenses <- server.codeLenses("a/src/main/scala/a/Main.scala")
_ = assert(lenses.size > 0, "No lenses were generated!")
command = lenses.head.getCommand()
_ = assertEquals(runFromCommand(command), Some("Hello java!"))
} yield ()

}
testRunShellCommand("run-shell-command")
testRunShellCommand("run shell command")

test("no-stale-supermethod-lenses") {
cleanWorkspace()
for {
Expand Down

0 comments on commit 468bb97

Please sign in to comment.