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

Make .bat start script honor -J and -D arguments. #218

Merged
merged 1 commit into from
May 17, 2014

Conversation

gourlaysama
Copy link
Contributor

This makes the .bat start script parse and use -J and -D arguments.

Specifically:

  • Parameters starting with -J are stripped of the prefix, unquoted if
    necessary and appended to %JAVA_OPTS%.
  • Parameters starting with -D are unquoted if necessary and then
    appended to the others. The right-hand side of a property can be
    quoted or not.
  • The exact arguments passed to the script are also given as-is to the
    main class (using %*), so, as before, the main class should ignore
    arguments starting with -J and -D. This could be improved by manually
    parsing all arguments (not just -J and -D), but this is a mine field
    when it comes to quoting and delimiters...

This is adapted from my similar contribution to scala 2.10, see
scala/scala#2767.


I've wanted to do this for some time, so here it is. This is also somewhat related to #155.

@lightbend-cla-validator
Copy link

Hi @gourlaysama,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement:

http://typesafe.com/contribute/cla

@gourlaysama
Copy link
Contributor Author

I signed the CLA :-)

@muuki88 muuki88 added the windows label Apr 6, 2014
@muuki88
Copy link
Contributor

muuki88 commented Apr 6, 2014

Thanks a lot @gourlaysama ! Bat-Gurus are always welcome :) I will try this on windows as I only get the intention of your code (mostly through the comments). Looking at the conversion of scala/scala#2767 everything should be fine.

set _JAVA_PARAMS=%_TEST_PARAM:~2%
)

if "%_TEST_PARAM:~0,2%"=="-D" (
Copy link
Member

Choose a reason for hiding this comment

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

Nice, so this loop could do more sorts of arg parsing than just -D vs. -J.

@jsuereth
Copy link
Member

jsuereth commented Apr 7, 2014

This LGTM. we don't have automated windows tests though, so I may need to check this out manually to try it.

@muuki88
Copy link
Contributor

muuki88 commented Apr 7, 2014

Okay, I tested this (Windows 7). My App looks like this

println(System.getProperty("test", "no value"))
println("Max Memory: " + (Runtime.getRuntime.maxMemory / (1024*1024)) + " Mb")

Results for System Properties

Parameters Expected Output Output Result
-Dtest=value value value OK
-J-Dtest=value value (nothing, not even the fallback value) Should this work?
-Dtest="long value" long value Powershell expects input with >> I think OK
"-Dtest="long value" long value long value OK

Results for jvm properties

everything worked as expected

@gourlaysama
Copy link
Contributor Author

  • Good point about -J-D.... I didn't consider it since that was what -D was all about. I guess it should still work, for the sake of consistency.
  • -Dtest="long value" works for me, both in cmd and powershell.
  • I just found a powershell-only bug: -Dcom.whatever.property=v is split by powershell into two arguments as -Dcom and .whatever.property=v, whereas CMD splits it as -Dcom.whatever.property and v (with the = separator gone). The script currently expects the latter, so the property is not set when run from powershell. The workaround is to quote the whole thing. I am looking into fixing that. The script is going to get uglier...

I'll ping you when I update the PR.

@gourlaysama
Copy link
Contributor Author

Hum, it seems that the problem with -D java properties and powershell is a well known problem I just rediscovered, and that it is up to the caller to properly escape the - with a backtick or quote the whole thing, as in:

./test `-Dcom.whatever.property=v

or

./test "-Dcom.whatever.property=v"

So anyway, that's not really a bug in the bat file. I'll just update the script with proper support for -J-D, and that should be it.

This makes the .bat start script parse and use -J and -D arguments.

Specifically:
 - Parameters starting with -J are stripped of the prefix, unquoted if
   necessary and appended to %JAVA_OPTS%.
 - Parameters starting with -D are unquoted if necessary and then
   appended to the others. The right-hand side of a property can be
   quoted or not.
 - The exact arguments passed to the script are also given as-is to the
   main class (using `%*`), so, as before, the main class should ignore
   arguments starting with -J and -D. This could be improved by manually
   parsing all arguments (not just -J and -D), but this is a mine field
   when it comes to quoting and delimiters...

This is adapted from my similar contribution to scala 2.10, see
scala/scala#2767.
@gourlaysama
Copy link
Contributor Author

@muuki88 I updated the script. Here is a little summary:

  • -Dfoo=42, "Dfoo=42" and -Dfoo="42" work in both CMD and Powershell,
  • -Dfoo.bar=42 and -Dfoo.bar="42" only work in CMD, PS requires to backtick-escape the -,
  • "-Dfoo.bar=42" work both in CMD and Powershell,
  • all the above is the same with -J-D instead of -D.

@muuki88
Copy link
Contributor

muuki88 commented Apr 9, 2014

Awesome! Thanks a lot for your effort. @jsuereth or I will try to reproduce the results an then merge it.

@muuki88 muuki88 added this to the 0.8.0 - windows services milestone Apr 11, 2014
@gourlaysama
Copy link
Contributor Author

ping @muuki88: did you get a chance to test this?

@muuki88
Copy link
Contributor

muuki88 commented May 13, 2014

no! unfortunatly not. Now the 0.7.0 release is out, this is the next thing to test. Sorry for the long delay :/

@muuki88 muuki88 self-assigned this May 17, 2014
muuki88 added a commit that referenced this pull request May 17, 2014
Make .bat start script honor -J and -D arguments.
@muuki88 muuki88 merged commit e7fa376 into sbt:master May 17, 2014
@muuki88
Copy link
Contributor

muuki88 commented May 17, 2014

Sorry for the long delay. Thanks for your bash magic :)

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.

4 participants