-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Bash launcher refactor #1860
Bash launcher refactor #1860
Conversation
The script could still use further refactoring, but this could go in as-is if it doesn't break anything (the command-line tests pass.) I could make a separate patch for this bug for the 1.7 version of jruby.bash if desired, it was just:
before |
…unwanted expansions
I feel better about incorporating this into master, as 1.7 is now well-established and jruby.bash is most people's entry-point into JRuby. |
@enebo Any objections to this? We have not made an effort to maintain the bash script in some time, and this looks like a nice refactor. |
@mpapis @wayneeseguin We'd really appreciate having a few bash experts help review these changes to our launcher! I'll owe you one! |
I will do review tomorrow |
@headius I don't doubt it is a nice refactor and I agree we should fix this on master and possibly backport after a few 9000 releases to 1.7.x if there are no issues. |
first problem I see is missing support for spaces in paths, any: |
# Record the exit status immediately, or it will be overridden. | ||
JRUBY_STATUS=$? | ||
# ----- Verify and Set Required Environment Variables ------------------------- | ||
set_self_path "$0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in rare cases when bash
is linked to zsh
with some extra config to symulate bash
the $0
would be main
instead of the script path, it might be safer to do main "$0" "$@"
and then on the beginnning of main()
do something like typeset started_as="$1" ; shift
and use $started_as
instead of $0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does happen :)
not sure if you care for performance, but it can be improved - all |
this code mixes the new style [ -x file ]
`code` the new "safer" style is: [[ -x "file" ]]
"$(code)" the differences is: |
https://github.com/mjc/jruby/blob/bash-launcher-refactor/bin/jruby.bash#L195-L227 - mostly style but could changed to an case "$val" in
(-Xmx*) JAVA_MEM="$val" ;;
(-Xms*) JAVA_MEM_MIN="$val" ;;
...
esac |
again style https://github.com/mjc/jruby/blob/bash-launcher-refactor/bin/jruby.bash#L229-L236 could be: set_defines_from_x_options()
{
val=${1:2}
case "$val" in
(*.*) java_args+=("-Djruby.${val}")
(*) ruby_args+=("-X${val}")
esac
} |
in fact all |
I would also split some longer functions into smaller ones, but it's not required |
and hopefully last one use |
SELF_PATH=$(cd -P -- "$(dirname -- "$1")" >/dev/null && pwd -P) && SELF_PATH=$SELF_PATH/$(basename -- "$1") | ||
|
||
# resolve symlinks | ||
while [ -h "$SELF_PATH" ]; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are working within a BASH context, as @mpapis mentioned, be sure to use [[ ]] vs [ ], it is builtin and handles edge case 'gotchas' nicely so they don't 'gotcha'.
This comment applies to all occurrences in this document, I'll refrain from commenting on each line.
Both I’m just writing JRuby package for Alpine Linux. After I had read the scripts I decided that instead of patching The new script is more clean and simple, robust, POSIX compatible (i.e. can be run with Busybox’s ash or even dash), without Windows/cygwin garbage (there’s already a native launcher for Windows) and Truffle stuff (since it’s still experimental, it’s IMO not needed for regular users yet). I wrote it specifically for Alpine Linux, so it has different defaults and omits some features (like Truffle). I had thought that I’ll modify it for general purpose and send PR, but then I found this two years old PR, yet still unresolved, so I’m not sure if there’s actually any interest in it. There’s the new script, maybe it’ll be useful: #!/bin/sh
# vim: set ts=4:
#
# This jruby start script is rewrite of (poor) upstream's jruby.bash.
# It's POSIX compatible, more clean, with sensible defaults for Alpine Linux
# and without Windows, OS X and Truffle stuff. Beside that it should support
# all the options of jruby.bash (hopefully) with the same behaviour.
#
# -----BEGIN HELP-----
#
# Environment variables:
# CLASSPATH Additional paths to be added to the end of the Java classpath.
# This may be overwritten by CLI argument -J-classpath or -J-cp.
# JAVA_HOME Path to directory where is JRE/JVM installed to use specific java/jdb binary,
# not the one on the PATH.
# JAVA_OPTS Default options to be passed to JVM.
# JRUBY_HOME Path to directory where JRuby is installed (default is /usr/share/java/jruby).
# JRUBY_OPTS Default JRuby command line arguments.
# JRUBY_SHELL Path to the system shell executable (default is /bin/sh).
# PROFILE_ARGS Arguments for the instrumented profiler.
#
# Deprecated environment variables (for compatibility):
# JAVA_MEM The -Xmx (maximum memory allocation) parameter for JVM (e.g. -Xmx128M).
# This may be overwritten by CLI argument -J-Xmx.
# JAVA_STACK The -Xss (stack size) parameter for JVM (default is -Xss2048k).
# This may be overwritten by CLI argument -J-Xss.
# JAVA_VM What JIT mode to use; -client, or -server (default is -server).
# This may be overwritten by CLI arguments --server, --client, and --dev.
# JRUBY_PARENT_CLASSPATH Classpath propagated from the parent JRuby process.
#
# All the environment variables are optional.
# -----END HELP-----
#
set -eu
readonly LIBJANSI_PATH='/usr/lib/java/jansi-native'
readonly LIBJFFI_PATH='/usr/lib/java/jffi'
readonly MAIN_CLASS_JRUBY='org.jruby.Main'
readonly MAIN_CLASS_NGCLIENT='org.jruby.util.NailMain'
readonly MAIN_CLASS_NGSERVER='org.jruby.main.NailServerMain'
readonly JRUBY_HOME="${JRUBY_HOME:-"/usr/share/java/jruby"}"
readonly JRUBY_CP="$JRUBY_HOME/lib/jruby.jar"
readonly JRUBY_SHELL="${JRUBY_SHELL:-"/bin/sh"}"
readonly NAILGUN_CMD="$JRUBY_HOME/tool/nailgun/ng"
readonly PROFILE_ARGS="${PROFILE_ARGS:-}"
java_stack="${JAVA_STACK:-"-Xss2048k"}"
java_cmd="${JAVA_HOME:+"$JAVA_HOME/bin/"}java"
java_opts="${JAVA_OPTS:-} ${JAVA_MEM:-} $java_stack \
-Djffi.boot.library.path=$LIBJFFI_PATH \
-Dlibrary.jansi.path=$LIBJANSI_PATH"
# Use the same classpath propagated from parent jruby, if provided.
classpath="${JRUBY_PARENT_CLASSPATH:-}"
if [ -z "$classpath" ]; then
_excluded='jruby.jar jruby-truffle.jar jruby-complete.jar'
# Add all JARs from $JRUBY_HOME/lib to the classpath, except $_excluded.
classpath=$(find "$JRUBY_HOME/lib" -maxdepth 1 \
-name '*.jar' $(printf '! -name %s ' $_excluded) -print0 \
| xargs -0 printf '%s:')
classpath="${classpath%?}" # %? removes leading ":"
fi
extra_cp="${CLASSPATH:-}"
java_vm="${JAVA_VM:-"-server"}"
main_class="$MAIN_CLASS_JRUBY"
nailgun_client='no'
ruby_args=''
verify_jruby='no'
# Split out any -J argument for passing to the JVM.
# Scanning for args is aborted by '--'.
set -- ${JRUBY_OPTS:-} $@
while [ $# -gt 0 ]; do
case "$1" in
# Stuff after "-J" goes to JVM.
-J | -J-X)
"$java_cmd" -help
printf '\n(Prepend -J in front of these options when using "jruby" command)\n'
exit 1
;;
-J-classpath | -J-cp)
extra_cp="$2"
shift
;;
-J-ea)
verify_jruby='yes'
java_opts="$java_opts ${1:2}"
;;
-J*)
java_opts="$java_opts ${1:2}"
;;
# Pass -X... and -X? search options through.
-X*\.\.\.|-X*\?)
ruby_args="$ruby_args $1"
;;
# Match -Xa.b.c=d to translate to -Da.b.c=d as a Java option.
-X*)
val=${1:2}
if expr "$val" : '.*[.]' > /dev/null; then
java_opts="$java_opts -Djruby.$val"
else
ruby_args="$ruby_args -X$val"
fi
;;
# Match switches that take an argument.
-C | -e | -I | -S)
ruby_args="$ruby_args $1 $2"
shift
;;
# Match same switches with argument stuck together.
-e* | -I* | -S*)
ruby_args="$ruby_args $1"
;;
# Run with JMX management enabled.
--manage)
java_opts="$java_opts -Dcom.sun.management.jmxremote \
-Djruby.management.enabled=true"
;;
# Don't launch a GUI window, no matter what.
--headless)
java_opts="$java_opts -Djava.awt.headless=true"
;;
# Run under JDB (debug mode).
--jdb)
java_cmd="${JAVA_HOME:+"$JAVA_HOME/bin/"}jdb"
java_opts="$java_opts -sourcepath $JRUBY_HOME/lib/ruby/1.9:."
ruby_args="$ruby_args -X+C"
;;
--client | --server)
java_vm="${1#?}" # #? removes leading "-"
;;
--dev)
java_vm='-client'
java_opts="$java_opts -XX:+TieredCompilation -XX:TieredStopAtLevel=1 \
-Djruby.compile.mode=OFF -Djruby.compile.invokedynamic=false"
;;
--sample)
java_opts="$java_opts -Xprof"
;;
# Start up as Nailgun server.
--ng-server)
main_class="$MAIN_CLASS_NGSERVER"
verify_jruby='yes'
;;
# Use native Nailgun client to toss commands to server.
--ng-client | --ng)
nailgun_client='yes'
;;
# Warn but ignore.
--1.8 | --1.9 | --2.0)
echo "WARNING: $1 is ignored"
;;
-h | -help | --help)
"$java_cmd" -help
# Print help from the top of this script.
sed -En '/-{5}BEGIN HELP-{5}/,/-{5}END HELP-{5}/p' "$0" \
| sed -E "s/^# ?//; 1d;\$d;"
exit 0
;;
# Abort processing on the double dash.
--)
break
;;
# Other opts go to ruby.
-*)
ruby_args="$ruby_args $1"
;;
# Abort processing on first non-opt arg.
*)
break
;;
esac
shift
done
# Remove possible duplications of some common JVM options.
for opt in -Xmx -Xms -Xss; do
val=$(expr "$java_opts" : ".*$opt\([^ \t]*\).*" ||:) # gets the later one
if [ -n "$val" ]; then
# Remove all occurrences of $opt and append the last one to the end.
java_opts=$(echo "$java_opts" | sed "s/$opt[^ \t]*//g")
java_opts="$java_opts ${opt}${val}"
fi
done
classpath="$classpath${extra_cp:+":$extra_cp"}"
java_opts="$java_opts $java_vm
-Djruby.home=$JRUBY_HOME
-Djruby.lib=$JRUBY_HOME/lib
-Djruby.script=jruby
-Djruby.shell=$JRUBY_SHELL"
# Append the rest of the arguments.
ruby_args="$ruby_args $@"
# Put $ruby_args back into the position arguments $1, $2, ...
set -- $ruby_args
if [ "$nailgun_client" = 'yes' ]; then
if [ ! -f "$NAILGUN_CMD" ]; then
echo 'ERROR: ng executable not found' 1>&2
exit 1
fi
exec "$NAILGUN_CMD" $MAIN_CLASS_NGCLIENT $@
elif [ "$verify_jruby" = 'yes' ]; then
[ -n "$PROFILE_ARGS" ] && echo 'Running with instrumented profiler'
set +e
"$java_cmd" $PROFILE_ARGS $java_opts \
-classpath "$JRUBY_CP:$classpath" $main_class $@
exit_code=$?
set -e
if [ -n "$PROFILE_ARGS" ]; then
echo 'Profiling results:'
cat profile.txt
rm profile.txt
fi
exit $exit_code
else
exec "$java_cmd" $java_opts -Xbootclasspath/a:"$JRUBY_CP" \
${classpath:+"-classpath $classpath"} \
$main_class $@
fi |
@jirutka I believe you are missing some quoting for things like JRUBY_HOME containing a space? This is a real issue since people run this script using bash/mingw on windows even if nearly unheard of on unixy systems. In general quoting with paths are for this case. Also does your script still work for the case mentioned in https://www.mail-archive.com/dev@jruby.codehaus.org/msg03551.html (what was once JRUBY-2699)? I am happy you are making this cleaner and run on more environments but I think stuff removed will need some individual justifications and/or someone else with more experience with bash to verify things are cool. @wayneeseguin @mpapis I don't know if you guys have any time but it would really help. My bash knowledge is from decades of limping my .profile/.bashrcs along so I do not feel up to the task :) |
I might try to review today evening, in worst case next week, but in anyway it would be best seen as a PR where I can comment of fork from to provide improvements |
As I’ve explicitly stated, I dropped Windows support. Windows users usually doesn’t have mingw (do they?) and there’s already a native launcher, so I don’t think it’s worth to increase complexity of the script for this. Trying to support everything in single script is a road to hell.
Well, then such users must use some other script or native launcher… EDIT: I’ve misunderstood the point, spaces in
Kind of… It requires
Well, I dropped support for Windows and OS X (although it’s not problem to add these few OS X specific stuff, and I use OS X too), so it’s actually opposite.
I should remind that the script above is written for Alpine Linux, it was not intended to be adopted by upstream as-is. However, if you’re interested and you’ll not insist on supporting Windows, then I can modify it (e.g. bring Truffle support etc.) and send PR. |
@jirutka heh. You know I missed a couple of sentences in your original comment. I misread that this was just what you intended to use on alpine linux. So in that case, we may or may not want to consider merging your code and cleaning up the things we will still need in the original script. So thanks for sharing and if we can get someone interested in merging/cleaning up the original script then that will be great. At this point, I think the project should consider if we should consider dropping some level of support in this script like yours does. @mpapis I rescind my request unless you want to help @jirutka find any potential issues with his launcher script. |
@enebo …if we can get someone interested in merging/cleaning up the original script… I’ll be happy to do it, if there’s interest from JRuby devs, but as I stated, I don’t want to struggle with support for Windows/mingw. |
spaces in paths - OSX with a new disk - complete nightmare |
I don’t understand…? What a new disk has common with this? |
@jirutka spaces in paths happen when you add a new disk to OSX, this PR is to improve the scripts used in jruby - this might include OSX, so we need to account for spaces in paths. |
New disk is mounted under |
usually the home gets moved to the second drive, it contains spaces often enough to be a problem, and I guess users might want to install jruby in home directory? |
Okay… you’re concerned about spaces just in |
the spaces are common concern, when writing shell scripts it's best to use arrays / param shifting to avoid space problems, from what I have seen so far in your script you just flatten down all params to single strings like with |
In Unix world, it’s not, because you just avoid spaces in paths and CLI arguments.
Arrays are not supported by ash, dash, … it’s a bash extension, not POSIX.
And also in current jruby.sh:262. The problem is that arguments handling is very complicated in these jruby start scripts; it doesn’t handle just CLI arguments, but it merges them with |
@jirutka Thanks for looking at this! Here's my thoughts on the whole thing. It would be really nice to have a better .sh script, so this is a good start. For platforms where bash is not available, we'll have a better option. If this can support all behaviors of the bash script, it's worth making the move. However... Past attempts to support all behavior of the bash script almost always ended up breaking because arguments could not be processed as an array. The complexity you found in our bash script has come about largely due to the complexity of argument processing, and arrays actually made that a lot cleaner and simpler. The spaces-in-paths issue is probably the real killer here, since a sh script can only deal with strings and has to re-parse arguments. This is really the biggest reason we have to use bash arrays. And it's not sufficient to say "don't use spaces". People do, OSes do...it has to work, and this is not negotiable. This is the primary reason why we have never been able to replace the bash script. There may be a way to do this, though: make the shell script call bash when available. Here's the launchers we currently have:
The native launchers were added to avoid problems with scripts, both the bash/sh scripts on *nix and the bat scripts on Windows (which were horrendous). We recommend people install them, but most do not (though I believe rvm does try to install jruby-launcher for all JRuby installs). mjruby is expected to replace jruby-launcher for 9.1. Because it requires the mjruby build toolchain, however, it does not support all platforms that jruby-launcher does. So... We have jruby.bash and jruby-launcher, which are largely feature-complete but very cumbersom to maintain. We have jruby.sh which works in more places than jruby.bash, but which has feature gaps (e.g. dealing with spaces in arguments). And we have mjruby, which is as complete as jruby-launcher but only installs on a few systems. Copying @jkutner. I'm thinking this plan might work, and I'd like input:
There's another possibility too. We could make jruby.sh the sole launcher in our .nix distribution, but have it recommend installing our launcher gem to get a better command line experience. That would install mjruby if supported, jruby-launcher if it can be built, or jruby.bash if bash is available. If none of those options work, |
@headius I believe mjruby is being merged into jruby-launcher and not the other way around. |
I'm removing the milestone from this. We may need improvements for the bash script, this is true. And there are outstanding bugs. Some of those are fixed by going to the native launcher, but we will always need a working non-native launcher at least for bootstrapping. All that said, this is a low priority for anyone working on JRuby proper right now. I think the whole PR needs a reboot, because it has become a hodge-podge of old patches and new discussions that are often working at cross purposes. If folks out there want to see a better bash/sh script, open a new issue or PR and do the work. It's unlikely to come out of jruby core, since we all hate bash/sh and wouldn't know where to start. |
This is the beginnings of a refactoring for the bash launcher. I intend to modernize it and fix the following bug.
Currently the JRUBY_OPTS and option parsing code requires options that affect some kinds of code to be earlier in the option list than other kinds. For example:
this returns: jruby: unknown option --sample
as does this.
This means that some options cannot currently be specified in JRUBY_OPTS if it is at all possible that anything launched with JRUBY_OPTS set may use some other options.
This rewrite is still in progress, I'm sending the pull request in case I end up not having time to finish it.