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

Bash cleanup #6007

Merged
merged 9 commits into from Dec 28, 2019
Merged

Bash cleanup #6007

merged 9 commits into from Dec 28, 2019

Conversation

mrnoname1000
Copy link
Contributor

@mrnoname1000 mrnoname1000 commented Dec 21, 2019

Here are a bunch of changes cleaning up the Bash launcher. Most of it is assisted by shellcheck.
Changes include syntax improvements, removing unused variables, and some minimal syntax changes to make it easier to port to POSIX shell in the future.
See commit messages for more details.

Expr isn't needed since bash and sh have case statement with globbing,
and expr is only checking for a dot. The else statement is unnecessary
now because the case statement will fall back to ruby_args anyway. This
also removes the need for the "expr --" check at the top.
Most of these changes are thanks to [shellcheck](https://www.shellcheck.net),
the rest are things I saw along the way.
------------------------------------------------------------------------
* All variables are double-quoted except in the following scenarios:
  * It will always be either true or false, i.e. cygwin and use_exec.
  * Variables aren't expanded in the beginning of a case statement.
  * We want the variable to be split on whitespace, i.e. JRUBY_OPTS, so
    we can loop through it.
  * The command will throw an syntax error regardless of quotes on
    incorrect input (i.e. exit [number])

* All tests are converted from bash-specific double brackets to
  single brackets for ease of porting to POSIX shell.
  * Using -a and -o for 'and' and 'or' is defined by POSIX, but it looks
    ugly and shellcheck says it's not well defined, so we can chain
    multiple self-contained tests with the shell's builtin && and ||
    operations.
  * Several tests compare a string to "" with = or !=, which can be
    simplified to testing the string, -s on the string, or -z on the
    string.

* Function declaration is changed to function_name() { ... } to be more
  in line with POSIX

* Command substitution using backticks is replaced with "$(command)".

* "which [command]" is non-standard and replaced with shell builtin
  "command -v [command]".

* Other innocuous changes:
  * A couple lines were indented weirdly, so I fixed them to be more
    consistent.
  * A few variables were only used once, so I replaced them with the
    equivalent value where they were used. Some were unused, e.g. darwin
    and mode.
JAVA_VM wasn't used anywhere and JRUBY_OPTS_TEMP was only used to set
JAVA_VM, so I removed them both.
kares
kares approved these changes Dec 28, 2019
Copy link
Member

@kares kares left a comment

nice cleanup, I am learning a lot!

@enebo enebo merged commit a2ceaf3 into jruby:master Dec 28, 2019
5 checks passed
@enebo
Copy link
Member

@enebo enebo commented Dec 28, 2019

If there ends up being some esoteric platform which somehow disagrees with any of this we should land it sooner than later.

@headius
Copy link
Member

@headius headius commented Dec 29, 2019

I discussed with @mrnoname1000 the remaining steps for POSIX shell support and most of them boil down to leveraging sh argument arrays to replace the += bash arrays we use heavily. Historically the main trouble we had with sh was losing proper quoting when forced to reinterpolate arguments during arg processing. The other challenge is that there's no readlink in posix so we'd either have to require that (probably not a big deal) or import a non-trivial sh-based implementation of it. Thanks for the deep investigation @mrnoname1000!

@enebo
Copy link
Member

@enebo enebo commented Dec 29, 2019

@headius @mrnoname1000 sounds great. Using simplest sh that is portable sounds like a dream to me :)

fi
fi

# Detect Java 9+ by the presence of a jmods directory in JAVA_HOME
if [ -d $JAVA_HOME/jmods ]; then
if [ -d "$JAVA_HOME"/jmods ]; then
Copy link
Member

@eregon eregon Jan 19, 2020

Choose a reason for hiding this comment

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

Note: this actually fixes a warning running on Windows in a Bash shell:

/c/Users/runneradmin/.rubies/jruby-9.2.9.0/bin/jruby: line 155: [: C:\Program: binary operator expected

This happens in GitHub Actions.

Copy link
Member

@headius headius Jan 20, 2020

Choose a reason for hiding this comment

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

Makes sense with spaces in path.

Copy link
Member

@eregon eregon Jan 20, 2020

Choose a reason for hiding this comment

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

Yeah, exactly. Using the Bash launcher Windows is likely unusual though (Just jruby would give precedence to the .exe, only /full/path/to/jruby or jruby.bash in a Bash shell would use the Bash launcher).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants