From 639b812a0131cd49aa1a7c5bcf9a204aeafb0c2a Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Sat, 11 Jan 2020 11:11:55 -0800 Subject: [PATCH 1/2] Don't strip quotes in _sbt_ scripted command arguments Stripping quotation marks makes it impossible to cleanly test certain sbt features without resorting to weird hacks. For example: > set Compile / scalacOptions += "-Xfatal-warnings" did not work while > set Compile / scalacOptions += '"-Xfatal-warnings"' did. I leave the single quote parser unchanged since single quotes are not really used in sbt and so there is utility in leaving them as a way to group arguments that should not be split apart. This change should only affect the scripted tests in the sbt repo. We can consider making stripQuotes = false the default for the plugin as well. --- .../sbt/internal/scripted/ScriptedTests.scala | 9 +++-- .../internal/scripted/TestScriptParser.scala | 35 ++++++++++++++----- sbt/src/sbt-test/actions/doc/test | 32 ++++++++--------- sbt/src/sbt-test/package/lazy-name/test | 14 ++++---- sbt/src/sbt-test/package/manifest/test | 4 +-- sbt/src/sbt-test/package/resources/test | 4 +-- sbt/src/sbt-test/run/fork/test | 12 +++---- sbt/src/sbt-test/tests/arguments/test | 4 ++- .../sbt/scriptedtest/ScriptedTests.scala | 2 +- 9 files changed, 70 insertions(+), 46 deletions(-) diff --git a/internal/util-scripted/src/main/scala/sbt/internal/scripted/ScriptedTests.scala b/internal/util-scripted/src/main/scala/sbt/internal/scripted/ScriptedTests.scala index 7d196dd70..12dc82c6f 100644 --- a/internal/util-scripted/src/main/scala/sbt/internal/scripted/ScriptedTests.scala +++ b/internal/util-scripted/src/main/scala/sbt/internal/scripted/ScriptedTests.scala @@ -58,8 +58,11 @@ object ScriptedRunnerImpl { final class ScriptedTests( resourceBaseDirectory: File, bufferLog: Boolean, - handlersProvider: HandlersProvider + handlersProvider: HandlersProvider, + stripQuotes: Boolean ) { + def this(resourceBaseDirectory: File, bufferLog: Boolean, handlersProvider: HandlersProvider) = + this(resourceBaseDirectory, bufferLog, handlersProvider, true) private val testResources = new Resources(resourceBaseDirectory) private val consoleAppender: ConsoleAppender = ConsoleAppender() @@ -127,10 +130,10 @@ final class ScriptedTests( } val pendingString = if (pending) " [PENDING]" else "" - def runTest() = { + def runTest(): Unit = { val run = new ScriptRunner val parser = createParser() - run(parser.parse(file)) + run(parser.parse(file, stripQuotes)) } def testFailed(): Unit = { if (pending) buffered.clearBuffer() else buffered.stopBuffer() diff --git a/internal/util-scripted/src/main/scala/sbt/internal/scripted/TestScriptParser.scala b/internal/util-scripted/src/main/scala/sbt/internal/scripted/TestScriptParser.scala index 80cdbc99b..9d2548059 100644 --- a/internal/util-scripted/src/main/scala/sbt/internal/scripted/TestScriptParser.scala +++ b/internal/util-scripted/src/main/scala/sbt/internal/scripted/TestScriptParser.scala @@ -50,11 +50,22 @@ class TestScriptParser(handlers: Map[Char, StatementHandler]) extends RegexParse if (handlers.keys.exists(key => key == '+' || key == '-')) sys.error("Start characters cannot be '+' or '-'") + @deprecated("Use variant that specifies whether to strip quotes or not", "1.4.0") def parse(scriptFile: File): List[(StatementHandler, Statement)] = - parse(read(scriptFile), Some(scriptFile.getAbsolutePath)) - def parse(script: String): List[(StatementHandler, Statement)] = parse(script, None) - private def parse(script: String, label: Option[String]): List[(StatementHandler, Statement)] = { - parseAll(statements, script) match { + parse(scriptFile, stripQuotes = true) + def parse(scriptFile: File, stripQuotes: Boolean): List[(StatementHandler, Statement)] = + parse(read(scriptFile), Some(scriptFile.getAbsolutePath), stripQuotes) + @deprecated("Use variant that specifies whether to strip quotes or not", "1.4.0") + def parse(script: String): List[(StatementHandler, Statement)] = + parse(script, None, stripQuotes = true) + def parse(script: String, stripQuotes: Boolean): List[(StatementHandler, Statement)] = + parse(script, None, stripQuotes) + private def parse( + script: String, + label: Option[String], + stripQuotes: Boolean + ): List[(StatementHandler, Statement)] = { + parseAll(statements(stripQuotes), script) match { case Success(result, next) => result case err: NoSuccess => { val labelString = label.map("'" + _ + "' ").getOrElse("") @@ -63,15 +74,21 @@ class TestScriptParser(handlers: Map[Char, StatementHandler]) extends RegexParse } } + @deprecated("Use variant that specifies whether to strip quotes or not", "1.4.0") lazy val statements = rep1(space ~> statement <~ newline) + def statements(stripQuotes: Boolean): Parser[List[(StatementHandler, Statement)]] = + rep1(space ~> statement(stripQuotes) <~ newline) - def statement: Parser[(StatementHandler, Statement)] = { + @deprecated("Use variant that specifies whether to strip quotes or not", "1.4.0") + def statement: Parser[(StatementHandler, Statement)] = statement(stripQuotes = true) + def statement(stripQuotes: Boolean): Parser[(StatementHandler, Statement)] = { trait PositionalStatement extends Positional { def tuple: (StatementHandler, Statement) } positioned { - val command = (word | err("expected command")) - val arguments = rep(space ~> (word | failure("expected argument"))) + val w = if (stripQuotes) word else rawWord + val command = w | err("expected command") + val arguments = rep(space ~> w | failure("expected argument")) (successParser ~ (space ~> startCharacterParser <~ space) ~! command ~! arguments) ^^ { case successExpected ~ start ~ command ~ arguments => new PositionalStatement { @@ -86,7 +103,9 @@ class TestScriptParser(handlers: Map[Char, StatementHandler]) extends RegexParse def space: Parser[String] = """[ \t]*""".r lazy val word: Parser[String] = - ("\'" ~> "[^'\n\r]*".r <~ "\'") | ("\"" ~> "[^\"\n\r]*".r <~ "\"") | WordRegex + ("\'" ~> "[^'\n\r]*".r <~ "\'") | "\"" ~> "[^\"\n\r]*".r <~ "\'" | WordRegex + private lazy val rawWord: Parser[String] = + ("\'" ~> "[^'\n\r]*".r <~ "\'") | "\"[^\"\n\r]*\"".r | WordRegex def startCharacterParser: Parser[Char] = elem("start character", handlers.contains _) | diff --git a/sbt/src/sbt-test/actions/doc/test b/sbt/src/sbt-test/actions/doc/test index 23839647f..25420a6b2 100644 --- a/sbt/src/sbt-test/actions/doc/test +++ b/sbt/src/sbt-test/actions/doc/test @@ -1,33 +1,33 @@ -> doc -> 'set sources in (Compile, doc) := { val src = (sources in Compile).value; src.filterNot(_.getName contains "B") }' +> set sources in (Compile, doc) := { val src = (sources in Compile).value; src.filterNot(_.getName contains "B") } # hybrid project, only scaladoc run > doc -$ exists "target/api/index.js" -$ absent "target/api/scala" -$ absent "target/api/java" +$ exists target/api/index.js +$ absent target/api/scala +$ absent target/api/java -> 'set sources in (Compile, doc) := { val src = (sources in (Compile, doc)).value; src.filterNot(_.getName endsWith ".java") }' +> set sources in (Compile, doc) := { val src = (sources in (Compile, doc)).value; src.filterNot(_.getName endsWith ".java") } # compile task is superfluous. Since doc task preceded by compile task has been problematic due to scala # compiler's way of handling empty classpath. We have it here to test that our workaround works. -> ; clean ; compile ; doc +> clean ; compile ; doc # pure scala project, only scaladoc at top level -$ exists "target/api/index.js" -$ absent "target/api/package-list" -$ absent "target/api/scala" -$ absent "target/api/java" +$ exists target/api/index.js +$ absent target/api/package-list +$ absent target/api/scala +$ absent target/api/java -> 'set sources in (Compile, doc) := { val src = (sources in Compile).value; src.filter(_.getName endsWith ".java") }' +> set sources in (Compile, doc) := { val src = (sources in Compile).value; src.filter(_.getName endsWith ".java") } -> ; clean ; doc +> clean ; doc # pure java project, only javadoc at top level -$ exists "target/api/index.html" -$ absent "target/api/index.js" +$ exists target/api/index.html +$ absent target/api/index.js # pending -# $ absent "target/api/scala" -# $ absent "target/api/java" +# $ absent target/api/scala +# $ absent target/api/java diff --git a/sbt/src/sbt-test/package/lazy-name/test b/sbt/src/sbt-test/package/lazy-name/test index ce4ba25f0..9afdadb29 100644 --- a/sbt/src/sbt-test/package/lazy-name/test +++ b/sbt/src/sbt-test/package/lazy-name/test @@ -4,19 +4,19 @@ # much purpose other than checking that the 'set' command # re-evaluates the project data. -> 'set name := "lazy-package-name"' +> set name := "lazy-package-name" > set crossPaths := false -> 'set version := "0.1.1"' +> set version := "0.1.1" > package -$ exists "target/lazy-package-name-0.1.1.jar" +$ exists target/lazy-package-name-0.1.1.jar > clean -> 'set version := "0.1.2"' +> set version := "0.1.2" > package -$ exists "target/lazy-package-name-0.1.2.jar" +$ exists target/lazy-package-name-0.1.2.jar > clean -> 'set version := "0.1.3"' +> set version := "0.1.3" > package -$ exists "target/lazy-package-name-0.1.3.jar" +$ exists target/lazy-package-name-0.1.3.jar diff --git a/sbt/src/sbt-test/package/manifest/test b/sbt/src/sbt-test/package/manifest/test index 20c80ee3c..e77e9b799 100644 --- a/sbt/src/sbt-test/package/manifest/test +++ b/sbt/src/sbt-test/package/manifest/test @@ -1,4 +1,4 @@ > package -$ exists "./target/jar-manifest-test-0.2.jar" -$ exec java -jar "./target/jar-manifest-test-0.2.jar" +$ exists ./target/jar-manifest-test-0.2.jar +$ exec java -jar ./target/jar-manifest-test-0.2.jar > run \ No newline at end of file diff --git a/sbt/src/sbt-test/package/resources/test b/sbt/src/sbt-test/package/resources/test index aa29e4d8c..fefcded95 100644 --- a/sbt/src/sbt-test/package/resources/test +++ b/sbt/src/sbt-test/package/resources/test @@ -10,7 +10,7 @@ # This should fail because sbt should include the resource in the jar but it won't have the right # directory structure --$ exec java -jar "./target/main-resources-test-0.1.jar" +-$ exec java -jar ./target/main-resources-test-0.1.jar # Give the resource the right directory structure $ mkdir src/main/resources/jartest @@ -27,4 +27,4 @@ $ delete src/main/resources/main_resource_test > package # This should succeed because sbt should include the resource in the jar with the right directory structure -$ exec java -jar "./target/main-resources-test-0.1.jar" \ No newline at end of file +$ exec java -jar ./target/main-resources-test-0.1.jar \ No newline at end of file diff --git a/sbt/src/sbt-test/run/fork/test b/sbt/src/sbt-test/run/fork/test index 1c5cbb769..8f4fd8c25 100644 --- a/sbt/src/sbt-test/run/fork/test +++ b/sbt/src/sbt-test/run/fork/test @@ -1,18 +1,18 @@ -> run "fork" +> run fork $ exists flag $ delete flag -$ mkdir "forked" +$ mkdir forked > set fork := true -> 'set baseDirectory in run := baseDirectory(_ / "forked").value' +> set baseDirectory in run := baseDirectory(_ / "forked").value -> run "forked" +> run forked $ exists forked/flag $ absent flag $ delete forked/flag -> 'set envVars += ("flag.name" -> "env.flag")' -> run "forked" +> set envVars += ("flag.name" -> "env.flag") +> run forked $ exists forked/env.flag $ absent flag $ absent forked/flag diff --git a/sbt/src/sbt-test/tests/arguments/test b/sbt/src/sbt-test/tests/arguments/test index c410396aa..90bdc402e 100644 --- a/sbt/src/sbt-test/tests/arguments/test +++ b/sbt/src/sbt-test/tests/arguments/test @@ -23,4 +23,6 @@ $ delete success3 $ touch failure3 -> test:test -$ delete failure3 \ No newline at end of file +$ delete failure3 + +> set Compile / scalacOptions += "-Xfatal-warnings" diff --git a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala index f463837f9..419af51f6 100644 --- a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala +++ b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala @@ -443,7 +443,7 @@ final class ScriptedTests( catching(classOf[TestException]).withApply(testFailed).andFinally(log.clear).apply { preScriptedHook(testDirectory) val parser = new TestScriptParser(handlers) - val handlersAndStatements = parser.parse(file) + val handlersAndStatements = parser.parse(file, stripQuotes = false) runner.apply(handlersAndStatements, states) // Handle successful tests From b9231a49cc1713ea751473cc73bea113d342dfcf Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Sat, 11 Jan 2020 11:26:37 -0800 Subject: [PATCH 2/2] Don't add extra quotation marks in scripted commands Scripted automatically adding quotation marks when an argument contains spaces breaks arguments that already were wrapped in quotations. The scripted test should be responsible for its own escaping. Since this is part of scripted-sbt-redux, we don't have to worry about downstream builds relying on the old escaping behavior. --- sbt/src/sbt-test/tests/arguments/build.sbt | 8 ++++++++ sbt/src/sbt-test/tests/arguments/test | 4 ++++ .../src/main/scala/sbt/scriptedtest/SbtHandler.scala | 10 +--------- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/sbt/src/sbt-test/tests/arguments/build.sbt b/sbt/src/sbt-test/tests/arguments/build.sbt index 4c441e7dd..9d9f79162 100644 --- a/sbt/src/sbt-test/tests/arguments/build.sbt +++ b/sbt/src/sbt-test/tests/arguments/build.sbt @@ -2,8 +2,16 @@ val scalatest = "org.scalatest" %% "scalatest" % "3.0.5" ThisBuild / scalaVersion := "2.12.10" +val foo = settingKey[Seq[String]]("foo") +val checkFoo = inputKey[Unit]("check contents of foo") + lazy val root = (project in file(".")) .settings( + foo := Nil, + checkFoo := { + val arguments = Def.spaceDelimited("").parsed.mkString(" ") + assert(foo.value.contains(arguments)) + }, libraryDependencies += scalatest % Test, // testOptions in Test += Tests.Argument(TestFrameworks.ScalaTest, "-f", "result.txt", "-eNDXEHLO") testOptions in Configurations.Test ++= { diff --git a/sbt/src/sbt-test/tests/arguments/test b/sbt/src/sbt-test/tests/arguments/test index 90bdc402e..792eb6bc7 100644 --- a/sbt/src/sbt-test/tests/arguments/test +++ b/sbt/src/sbt-test/tests/arguments/test @@ -26,3 +26,7 @@ $ touch failure3 $ delete failure3 > set Compile / scalacOptions += "-Xfatal-warnings" + +> set foo += "an argument with spaces" + +> checkFoo an argument with spaces diff --git a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/SbtHandler.scala b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/SbtHandler.scala index 842662a95..78ec27c87 100644 --- a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/SbtHandler.scala +++ b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/SbtHandler.scala @@ -27,7 +27,7 @@ final class SbtHandler(remoteSbtCreator: RemoteSbtCreator) extends StatementHand def apply(command: String, arguments: List[String], i: Option[SbtInstance]): Option[SbtInstance] = onSbtInstance(i) { (_, server) => - send((command :: arguments.map(escape)).mkString(" "), server) + send((command :: arguments).mkString(" "), server) receive(s"$command failed", server) } @@ -80,12 +80,4 @@ final class SbtHandler(remoteSbtCreator: RemoteSbtCreator) extends StatementHand catch { case _: SocketException => throw new TestFailed("Remote sbt initialization failed") } p } - - // if the argument contains spaces, enclose it in quotes, quoting backslashes and quotes - def escape(argument: String) = { - import java.util.regex.Pattern.{ quote => q } - if (argument.contains(" ")) - "\"" + argument.replaceAll(q("""\"""), """\\""").replaceAll(q("\""), "\\\"") + "\"" - else argument - } }