Fix #6476 Use dry run in jetty.sh #6598
Conversation
Use a --dry-run in jetty.sh to pre-expand the java arguments and thus avoid having two JVMs running in the case of exec. Also made a small change to allow script to check the current directory for JETTY_BASE, as that allows testing and runs in the same style as direct calls to start.jar Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw
changed the title
Co-authored-by: Simone Bordet <simone.bordet@gmail.com>
update from review Signed-off-by: Greg Wilkins <gregw@webtide.com>
| @@ -247,21 +247,24 @@ if [ -z "$JETTY_HOME" ]; then | |||
| exit 1 | |||
| fi | |||
|
|
|||
| RUN_DIR=$PWD | |||
There was a problem hiding this comment.
Note that $PWD is a bash feature.
OSX is deprecating bash and moving to zsh.
https://support.apple.com/en-us/HT208050
Basically, Apple is stuck with Bash 3.2 as it's GPLv2, but newer versions of bash are GPLv3.
Apple will not upgrade/ship and/or use anything with GPLv3 license.
They want to get rid of bash entirely simply as their version is very out of date, and have made it clear that it will go away in the future.
Perhaps we should move to RUN_DIR=$(pwd) instead? (it's more compatible)
There was a problem hiding this comment.
Just to be clear, there is zero urgency on this change.
Feel free to leave it as it is for now.
update from review Signed-off-by: Greg Wilkins <gregw@webtide.com>
| @@ -430,7 +433,7 @@ case "`uname`" in | |||
| CYGWIN*) JETTY_START="`cygpath -w $JETTY_START`";; | |||
| esac | |||
|
|
|||
| RUN_ARGS=(${JAVA_OPTIONS[@]} -jar "$JETTY_START" ${JETTY_ARGS[*]}) | |||
| RUN_ARGS=$(echo $JAVA_OPTIONS ; "$JAVA" -jar "$JETTY_START" --dry-run=opts,path,main,args ${JETTY_ARGS[*]} | tail -1 ) | |||
There was a problem hiding this comment.
I don't like the tail -1 as in "why do we take only the last line?" and "surely if there are multiple lines we don't want to take only the last one and discard the previous ones".
I'd rather fail to start if people puts newlines into JAVA_OPTIONS.
There was a problem hiding this comment.
We have no control over what JAVA_OPTIONS users set. We have no control over what output those options generate or indeed if the JVM generates any other output.
If fact I think we should do more, something like:
java --show-version -jar ../jetty-home/start.jar --dry-run=opts,path,main,args __jetty__=__args__ 2>/dev/null | grep __jetty__=__args__ | sed 's/ __jetty__=__args__ / /'
There was a problem hiding this comment.
I think this is simpler!
There was a problem hiding this comment.
We have no control over what JAVA_OPTIONS users set.
That's exactly why we should do exactly nothing to try to format it. We know our --dry-run generates a single line, and if they set JAVA_OPTIONS right, then there is nothing else to do, no tail, no sed, etc.
We don't do tail -1 on the other variables in this file that can be set by users, so why do it here?
Am I missing something?
update from review Signed-off-by: Greg Wilkins <gregw@webtide.com>
update from review Signed-off-by: Greg Wilkins <gregw@webtide.com>
#6476 Use a --dry-run in jetty.sh to pre-expand the java arguments and thus avoid having two JVMs running in the case of exec.
Also made a small change to allow script to check the current directory for JETTY_BASE, as that allows testing and runs in the same style as direct calls to start.jar
Signed-off-by: Greg Wilkins gregw@webtide.com