Windows batch script improvements (#1042)

* bat: refactor argument loop into functions

* bat: remove unused goto labels

* bat: refactor out config file parse function

* bat: use APP_HOME instead of dynamic environment variable

* bat: add application.ini support

* bat: escape arguments with special characters

* bat: add test for javaOptions

* bat: add test for extra jvm arg with APP_HOME expansion

* bat: allow <APP_NAME>_HOME to be used in batScriptExtraDefines

fixes backwards compat for this snippet from the docs:

  // add jvm parameter for typesafe config
  batScriptExtraDefines += """set _JAVA_OPTS=%_JAVA_OPTS% -Dconfig.file=%EXAMPLE_CLI_HOME%\\conf\\app.config"""

* bat: document add_java/add_app methods

* bat: remove note about <APP_NAME>_config.txt file

* add javadoc comment to ApplicationIniGenerator

Author: dwickern

src/main/resources/com/typesafe/sbt/packager/archetypes/scripts/bat-template

@@ -9,25 +9,26 @@
 
 @echo off
 
-if "%@@APP_ENV_NAME@@_HOME%"=="" set "@@APP_ENV_NAME@@_HOME=%~dp0\\.."
 
-set "APP_LIB_DIR=%@@APP_ENV_NAME@@_HOME%\lib\"
+if "%@@APP_ENV_NAME@@_HOME%"=="" (
+  set "APP_HOME=%~dp0\\.."
+
+  rem Also set the old env name for backwards compatibility
+  set "@@APP_ENV_NAME@@_HOME=%~dp0\\.."
+) else (
+  set "APP_HOME=%@@APP_ENV_NAME@@_HOME%"
+)
+
+set "APP_LIB_DIR=%APP_HOME%\lib\"
 
 rem Detect if we were double clicked, although theoretically A user could
 rem manually run cmd /c
 for %%x in (!cmdcmdline!) do if %%~x==/c set DOUBLECLICKED=1
 
 rem FIRST we load the config file of extra options.
-set "CFG_FILE=%@@APP_ENV_NAME@@_HOME%\@@APP_ENV_NAME@@_config.txt"
+set "CFG_FILE=%APP_HOME%\@@APP_ENV_NAME@@_config.txt"
 set CFG_OPTS=
-if exist "%CFG_FILE%" (
-  FOR /F "tokens=* eol=# usebackq delims=" %%i IN ("%CFG_FILE%") DO (
-    set DO_NOT_REUSE_ME=%%i
-    rem ZOMG (Part #2) WE use !! here to delay the expansion of
-    rem CFG_OPTS, otherwise it remains "" for this loop.
-    set CFG_OPTS=!CFG_OPTS! !DO_NOT_REUSE_ME!
-  )
-)
+call :parse_config "%CFG_FILE%" CFG_OPTS
 
 rem We use the value of the JAVACMD environment variable if defined
 set _JAVACMD=%JAVACMD%
@@ -79,55 +80,14 @@ rem "-J" is stripped, "-D" is left as is, and everything is appended to JAVA_OPT
 set _JAVA_PARAMS=
 set _APP_ARGS=
 
-:param_loop
-call set _PARAM1=%%1
-set "_TEST_PARAM=%~1"
-
-if ["!_PARAM1!"]==[""] goto param_afterloop
-
+@@APP_DEFINES@@
 
-rem ignore arguments that do not start with '-'
-if "%_TEST_PARAM:~0,1%"=="-" goto param_java_check
-set _APP_ARGS=!_APP_ARGS! !_PARAM1!
-shift
-goto param_loop
+rem if configuration files exist, prepend their contents to the script arguments so it can be processed by this runner
+call :parse_config "%SCRIPT_CONF_FILE%" SCRIPT_CONF_ARGS
 
-:param_java_check
-if "!_TEST_PARAM:~0,2!"=="-J" (
-  rem strip -J prefix
-  set _JAVA_PARAMS=!_JAVA_PARAMS! !_TEST_PARAM:~2!
-  shift
-  goto param_loop
-)
-
-if "!_TEST_PARAM:~0,2!"=="-D" (
-  rem test if this was double-quoted property "-Dprop=42"
-  for /F "delims== tokens=1,*" %%G in ("!_TEST_PARAM!") DO (
-    if not ["%%H"] == [""] (
-      set _JAVA_PARAMS=!_JAVA_PARAMS! !_PARAM1!
-    ) else if [%2] neq [] (
-      rem it was a normal property: -Dprop=42 or -Drop="42"
-      call set _PARAM1=%%1=%%2
-      set _JAVA_PARAMS=!_JAVA_PARAMS! !_PARAM1!
-      shift
-    )
-  )
-) else (
-  if "!_TEST_PARAM!"=="-main" (
-    call set CUSTOM_MAIN_CLASS=%%2
-    shift
-  ) else (
-    set _APP_ARGS=!_APP_ARGS! !_PARAM1!
-  )
-)
-shift
-goto param_loop
-:param_afterloop
+call :process_args %SCRIPT_CONF_ARGS% %%*
 
 set _JAVA_OPTS=!_JAVA_OPTS! !_JAVA_PARAMS!
-:run
-
-@@APP_DEFINES@@
 
 if defined CUSTOM_MAIN_CLASS (
     set MAIN_CLASS=!CUSTOM_MAIN_CLASS!
@@ -140,7 +100,79 @@ rem Call the application and pass all arguments unchanged.
 
 @endlocal
 
+exit /B %ERRORLEVEL%
 
-:end
 
-exit /B %ERRORLEVEL%
+rem Loads a configuration file full of default command line options for this script.
+rem First argument is the path to the config file.
+rem Second argument is the name of the environment variable to write to.
+:parse_config
+  set _PARSE_FILE=%~1
+  set _PARSE_OUT=
+  if exist "%_PARSE_FILE%" (
+    FOR /F "tokens=* eol=# usebackq delims=" %%i IN ("%_PARSE_FILE%") DO (
+      set _PARSE_OUT=!_PARSE_OUT! %%i
+    )
+  )
+  set %2=!_PARSE_OUT!
+exit /B 0
+
+
+:add_java
+  set _JAVA_PARAMS=!_JAVA_PARAMS! %*
+exit /B 0
+
+
+:add_app
+  set _APP_ARGS=!_APP_ARGS! %*
+exit /B 0
+
+
+rem Processes incoming arguments and places them in appropriate global variables
+:process_args
+  :param_loop
+  call set _PARAM1=%%1
+  set "_TEST_PARAM=%~1"
+
+  if ["!_PARAM1!"]==[""] goto param_afterloop
+
+
+  rem ignore arguments that do not start with '-'
+  if "%_TEST_PARAM:~0,1%"=="-" goto param_java_check
+  set _APP_ARGS=!_APP_ARGS! !_PARAM1!
+  shift
+  goto param_loop
+
+  :param_java_check
+  if "!_TEST_PARAM:~0,2!"=="-J" (
+    rem strip -J prefix
+    set _JAVA_PARAMS=!_JAVA_PARAMS! !_TEST_PARAM:~2!
+    shift
+    goto param_loop
+  )
+
+  if "!_TEST_PARAM:~0,2!"=="-D" (
+    rem test if this was double-quoted property "-Dprop=42"
+    for /F "delims== tokens=1,*" %%G in ("!_TEST_PARAM!") DO (
+      if not ["%%H"] == [""] (
+        set _JAVA_PARAMS=!_JAVA_PARAMS! !_PARAM1!
+      ) else if [%2] neq [] (
+        rem it was a normal property: -Dprop=42 or -Drop="42"
+        call set _PARAM1=%%1=%%2
+        set _JAVA_PARAMS=!_JAVA_PARAMS! !_PARAM1!
+        shift
+      )
+    )
+  ) else (
+    if "!_TEST_PARAM!"=="-main" (
+      call set CUSTOM_MAIN_CLASS=%%2
+      shift
+    ) else (
+      set _APP_ARGS=!_APP_ARGS! !_PARAM1!
+    )
+  )
+  shift
+  goto param_loop
+  :param_afterloop
+
+exit /B 0

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/ApplicationIniGenerator.scala

@@ -0,0 +1,44 @@
+package com.typesafe.sbt.packager.archetypes.scripts
+
+import java.io.File
+
+import sbt._
+
+trait ApplicationIniGenerator {
+  /**
+    * @return the existing mappings plus a generated application.ini
+    *         if custom javaOptions are specified
+    */
+  def generateApplicationIni(universalMappings: Seq[(File, String)],
+                             javaOptions: Seq[String],
+                             bashScriptConfigLocation: Option[String],
+                             tmpDir: File,
+                             log: Logger): Seq[(File, String)] =
+    bashScriptConfigLocation
+      .collect {
+        case location if javaOptions.nonEmpty =>
+          val configFile = tmpDir / "tmp" / "conf" / "application.ini"
+          val pathMapping = cleanApplicationIniPath(location)
+          //Do not use writeLines here because of issue #637
+          IO.write(configFile, ("# options from build" +: javaOptions).mkString("\n"))
+          val hasConflict = universalMappings.exists {
+            case (file, path) => file != configFile && path == pathMapping
+          }
+          // Warn the user if he tries to specify options
+          if (hasConflict) {
+            log.warn("--------!!! JVM Options are defined twice !!!-----------")
+            log.warn(
+              "application.ini is already present in output package. Will be overridden by 'javaOptions in Universal'"
+            )
+          }
+          (configFile -> pathMapping) +: universalMappings
+
+      }
+      .getOrElse(universalMappings)
+
+  /**
+    * @param path that could be relative to app_home
+    * @return path relative to app_home
+    */
+  protected def cleanApplicationIniPath(path: String): String
+}

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/BashStartScriptPlugin.scala

@@ -16,7 +16,7 @@ import sbt._
   * [[com.typesafe.sbt.packager.archetypes.JavaAppPackaging]].
   *
   */
-object BashStartScriptPlugin extends AutoPlugin {
+object BashStartScriptPlugin extends AutoPlugin with ApplicationIniGenerator {
 
   /**
     * Name of the bash template if user wants to provide custom one
@@ -85,32 +85,6 @@ object BashStartScriptPlugin extends AutoPlugin {
     Seq("template_declares" -> defineString)
   }
 
-  private[this] def generateApplicationIni(universalMappings: Seq[(File, String)],
-                                           javaOptions: Seq[String],
-                                           bashScriptConfigLocation: Option[String],
-                                           tmpDir: File,
-                                           log: Logger): Seq[(File, String)] =
-    bashScriptConfigLocation
-      .collect {
-        case location if javaOptions.nonEmpty =>
-          val configFile = tmpDir / "tmp" / "conf" / "application.ini"
-          //Do not use writeLines here because of issue #637
-          IO.write(configFile, ("# options from build" +: javaOptions).mkString("\n"))
-          val filteredMappings = universalMappings.filter {
-            case (file, path) => path != appIniLocation
-          }
-          // Warn the user if he tries to specify options
-          if (filteredMappings.size < universalMappings.size) {
-            log.warn("--------!!! JVM Options are defined twice !!!-----------")
-            log.warn(
-              "application.ini is already present in output package. Will be overridden by 'javaOptions in Universal'"
-            )
-          }
-          (configFile -> cleanApplicationIniPath(location)) +: filteredMappings
-
-      }
-      .getOrElse(universalMappings)
-
   private[this] def generateStartScripts(config: BashScriptConfig,
                                          mainClass: Option[String],
                                          discoveredMainClasses: Seq[String],
@@ -153,8 +127,7 @@ object BashStartScriptPlugin extends AutoPlugin {
     * @param path that could be relative to app_home
     * @return path relative to app_home
     */
-  private[this] def cleanApplicationIniPath(path: String): String =
-    path.replaceFirst("\\$\\{app_home\\}/../", "")
+  protected def cleanApplicationIniPath(path: String): String = path.stripPrefix("${app_home}/../")
 
   /**
     * Bash defines

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/BatStartScriptKeys.scala

@@ -25,4 +25,8 @@ trait BatStartScriptKeys {
     "A list of extra definitions that should be written to the bat file template."
   )
 
+  val batScriptConfigLocation = TaskKey[Option[String]](
+    "batScriptConfigLocation",
+    "The location where the bat script will load default argument configuration from."
+  )
 }

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/BatStartScriptPlugin.scala

@@ -17,7 +17,7 @@ import sbt._
   * [[com.typesafe.sbt.packager.archetypes.JavaAppPackaging]].
   *
   */
-object BatStartScriptPlugin extends AutoPlugin {
+object BatStartScriptPlugin extends AutoPlugin with ApplicationIniGenerator {
 
   /**
     * Name of the bat template if user wants to provide custom one
@@ -34,6 +34,11 @@ object BatStartScriptPlugin extends AutoPlugin {
     */
   val scriptTargetFolder = "bin"
 
+  /**
+    * Location for the application.ini file used by the bat script to load initialization parameters for jvm and app
+    */
+  val appIniLocation = "%APP_HOME%\\conf\\application.ini"
+
   override val requires = JavaAppPackaging
   override val trigger = AllRequirements
 
@@ -42,19 +47,29 @@ object BatStartScriptPlugin extends AutoPlugin {
 
   private[this] case class BatScriptConfig(executableScriptName: String,
                                            scriptClasspath: Seq[String],
+                                           configLocation: Option[String],
                                            extraDefines: Seq[String],
                                            replacements: Seq[(String, String)],
                                            batScriptTemplateLocation: File)
 
   override def projectSettings: Seq[Setting[_]] = Seq(
     batScriptTemplateLocation := (sourceDirectory.value / "templates" / batTemplate),
+    batScriptConfigLocation := (batScriptConfigLocation ?? Some(appIniLocation)).value,
     batScriptExtraDefines := Nil,
     batScriptReplacements := Replacements(executableScriptName.value),
     // Generating the application configuration
+    mappings in Universal := generateApplicationIni(
+      (mappings in Universal).value,
+      (javaOptions in Universal).value,
+      batScriptConfigLocation.value,
+      (target in Universal).value,
+      streams.value.log
+    ),
     makeBatScripts := generateStartScripts(
       BatScriptConfig(
         executableScriptName = s"${executableScriptName.value}.bat",
         scriptClasspath = (scriptClasspath in batScriptReplacements).value,
+        configLocation = batScriptConfigLocation.value,
         extraDefines = batScriptExtraDefines.value,
         replacements = batScriptReplacements.value,
         batScriptTemplateLocation = batScriptTemplateLocation.value
@@ -104,6 +119,13 @@ object BatStartScriptPlugin extends AutoPlugin {
     clazz(0).toLower + lowerCased + ".bat"
   }
 
+  /**
+    * @param path that could be relative to APP_HOME
+    * @return path relative to APP_HOME
+    */
+  protected def cleanApplicationIniPath(path: String): String =
+    path.stripPrefix("%APP_HOME%\\").stripPrefix("/").replace('\\', '/')
+
   /**
     * Bat script replacements
     */
@@ -113,7 +135,9 @@ object BatStartScriptPlugin extends AutoPlugin {
       Seq("APP_NAME" -> name, "APP_ENV_NAME" -> NameHelper.makeEnvFriendlyName(name))
 
     def appDefines(mainClass: String, config: BatScriptConfig, replacements: Seq[(String, String)]): (String, String) = {
-      val defines = Seq(makeWindowsRelativeClasspathDefine(config.scriptClasspath), Defines.mainClass(mainClass)) ++ config.extraDefines
+      val defines = Seq(makeWindowsRelativeClasspathDefine(config.scriptClasspath), Defines.mainClass(mainClass)) ++
+        config.configLocation.map(Defines.configFileDefine) ++
+        config.extraDefines
       "APP_DEFINES" -> Defines(defines, replacements)
     }
 
@@ -140,6 +164,7 @@ object BatStartScriptPlugin extends AutoPlugin {
       defines.map(replace(_, replacements)).mkString("\r\n")
 
     def mainClass(mainClass: String): String = s"""set "APP_MAIN_CLASS=$mainClass""""
+    def configFileDefine(configFile: String): String = s"""set "SCRIPT_CONF_FILE=$configFile""""
 
     // TODO - use more of the template writer for this...
     private[this] def replace(line: String, replacements: Seq[(String, String)]): String =

src/sbt-test/windows/app-home-var-expansion/build.sbt

@@ -0,0 +1,16 @@
+import com.typesafe.sbt.packager.Compat._
+
+enablePlugins(JavaAppPackaging)
+
+name := "simple-app"
+
+version := "0.1.0"
+
+batScriptExtraDefines += """call :add_java "-Dconfig.file=%APP_HOME%\conf\production.conf""""
+
+TaskKey[Unit]("runCheck") := {
+  val cwd = (stagingDirectory in Universal).value
+  val cmd = Seq((cwd / "bin" / s"${packageName.value}.bat").getAbsolutePath)
+  val configFile = (sys.process.Process(cmd, cwd).!!).replaceAll("\r\n", "")
+  assert(configFile.contains("""stage\bin\\\..\conf\production.conf"""), "Output didn't contain config file path: " + configFile)
+}

src/sbt-test/windows/app-home-var-expansion/project/plugins.sbt

@@ -0,0 +1 @@
+addSbtPlugin("com.typesafe.sbt" % "sbt-native-packager" % sys.props("project.version"))

src/sbt-test/windows/app-home-var-expansion/src/main/scala/MainApp.scala

@@ -0,0 +1,4 @@
+object MainApp extends App {
+  val config = sys.props("config.file") ensuring (_ ne null, "didn't pick up -Dconfig.file argument")
+  print(config)
+}

src/sbt-test/windows/app-home-var-expansion/test

@@ -0,0 +1,3 @@
+# Run the staging and check the script.
+> stage
+> runCheck