Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows batch script improvements #1042

Merged
merged 12 commits into from
Oct 23, 2017
Merged

Windows batch script improvements #1042

merged 12 commits into from
Oct 23, 2017

Conversation

dwickern
Copy link
Collaborator

@dwickern dwickern commented Oct 5, 2017

I added some functions to the batch script to mirror the bash script. This means you can set extra java args using similar syntax:

batScriptExtraDefines += """call :add_java "-Dconfig.file=%APP_HOME%\conf\production.conf""""
bashScriptExtraDefines += """addJava "-Dconfig.file=${app_home}/../conf/production.conf""""

Previously the batch script used a dynamic environment variable name <APP_NAME>_HOME, which is difficult to use from the sbt build. Now it's called APP_HOME always.

The bat script will still read <APP_NAME>_HOME from the environment for backwards compatibility.

application.ini

The bat script now supports javaOptions in Universal via a generated application.ini file. Fixes #688

The ini file uses the -J syntax, so you can use the same ini file for both bat and bash, or you can have different files by specifying batScriptConfigLocation and/or bashScriptConfigLocation.

The bat script will still read <APP_NAME>_config.txt for backwards compatibility.

@muuki88
Copy link
Contributor

muuki88 commented Oct 8, 2017

Thanks a lot for your work 😍

I'll review this in depth, when I'm back from my vacation ( ~22.10).

Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a chance to swiftly look through your additions. Could you add a new test that verifies that the configuration works as expected? Also the documentation should be changed.

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"""
Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this amazing additions to the windows support. Only a small hint in the code ( adding some docs ) otherwise this is good to merge.

Would you be interested in becoming a contributor/maintainer for sbt-native-packager? Especially for the windows part? If you have any interested or a curious what this implies, I'm happy to give you a small introduction. Even if you have only little time this would be a great deal of help :)

import sbt._

trait ApplicationIniGenerator {
def generateApplicationIni(universalMappings: Seq[(File, String)],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add some scaladoc here:

Returns the existing mappings with a generated application.ini if
not already provided by the user.

@dwickern
Copy link
Collaborator Author

Sure I'll help maintain this, although I can't make a big time commitment

@muuki88 muuki88 merged commit cee8a07 into sbt:master Oct 23, 2017
@dwickern dwickern deleted the bat branch October 23, 2017 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants