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.

Sign up for GitHub

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

Fix #6476 Use dry run in jetty.sh #6598

Merged
merged 6 commits into from
Aug 13, 2021
Merged

Fix #6476 Use dry run in jetty.sh #6598

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
96355fd
Fix #6597 Use dry run in jetty.sh
gregw Aug 11, 2021
5f335ad
Do not add JAVA_OPTIONS to args
gregw Aug 11, 2021
4cced35
Fix #6597 Use dry run in jetty.sh
gregw Aug 11, 2021
b491e65
Fix #6597 Use dry run in jetty.sh
gregw Aug 11, 2021
d0a2d4c
Fix #6597 Use dry run in jetty.sh
gregw Aug 13, 2021
d998bd1
Fix #6597 Use dry run in jetty.sh
gregw Aug 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions jetty-home/src/main/resources/bin/jetty.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ NAME=$(echo $(basename $0) | sed -e 's/^[SK][0-9]*//' -e 's/\.sh$//')
# <Arg><Property name="jetty.home" default="."/>/webapps/jetty.war</Arg>
#
# JETTY_BASE
# Where your Jetty base directory is. If not set, the value from
# Where your Jetty base directory is. If not set, then the currently
# directory is checked, otherwise the value from
# $JETTY_HOME will be used.
#
# JETTY_RUN
Expand Down Expand Up @@ -238,7 +239,6 @@ then
fi
fi


##################################################
# No JETTY_HOME yet? We're out of luck!
##################################################
Expand All @@ -247,20 +247,23 @@ if [ -z "$JETTY_HOME" ]; then
exit 1
fi

RUN_DIR=$(pwd)
cd "$JETTY_HOME"
JETTY_HOME=$PWD

JETTY_HOME=$(pwd)

##################################################
# Set JETTY_BASE
##################################################
export JETTY_BASE
if [ -z "$JETTY_BASE" ]; then
JETTY_BASE=$JETTY_HOME
if [ -d "$RUN_DIR/start.d" -o -f "$RUN_DIR/start.ini" ]; then
JETTY_BASE=$RUN_DIR
else
JETTY_BASE=$JETTY_HOME
fi
fi

cd "$JETTY_BASE"
JETTY_BASE=$PWD

JETTY_BASE=$(pwd)

#####################################################
# Check that jetty is where we think it is
Expand Down Expand Up @@ -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 )
Copy link
Contributor

@sbordet sbordet Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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__ / /'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is simpler!

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

RUN_CMD=("$JAVA" ${RUN_ARGS[@]})

#####################################################
Expand Down