Skip to content

Support ( in path and actually set JAVA_OPTS #5214

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

Closed
wants to merge 1 commit into from

Conversation

ecki
Copy link
Contributor

@ecki ecki commented Apr 27, 2016

The current logic for printing %JAVA_HOME% inside if ( ) block failes if the path contains a ( which is the case for 32bit java (C:\Program Files (x86)\java\jdk)

The current logic for appending to JAVA_OPTS does not work as it will be expanded while interpreting the else block. Use goto instead.

Personally I do not at all like that this glutters all my global environemnt, but hey I guess to each its own taste :)

The current logic for printing %JAVA_HOME% inside if ( ) block failes if the path contains a `(` which is the case for 32bit java (`C:\Program Files (x86)\java\jdk`)

The current logic for appending to JAVA_OPTS does not work as it will be expanded while interpreting the else block. Use goto instead.
@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@suyograo
Copy link
Contributor

This was introduced by @StefanScherer in #4913

@StefanScherer can you check this out and let us know if it works in your testing?

if defined JAVA_EXE (
echo Using JAVA_HOME=%JAVA_HOME% retrieved from %ProgramData%\Oracle\java\javapath\java.exe
)
if defined JAVA_EXE echo Using JAVA_HOME=%JAVA_HOME% retrieved from %ProgramData%\Oracle\java\javapath\java.exe
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely the more elegant way without the braces solving the problem showing and old variable

@StefanScherer
Copy link
Contributor

@suyograo I've got some minutes to test the PR:

PS C:\Users\vagrant\logstash> git fetch origin pull/5214/head:test
From https://github.com/elastic/logstash
 * [new ref]         refs/pull/5214/head -> test
PS C:\Users\vagrant\logstash> git checkout test
Switched to branch 'test'
PS C:\Users\vagrant\logstash> notepad bin\setup.bat
PS C:\Users\vagrant\logstash> .\bin\setup.bat
Using JAVA_HOME=C:\Program Files\Java\jre1.8.0_77 retrieved from C:\ProgramData\Oracle\java\javapath\java.exe
Unable to find JRuby.
If you are a user, this is a bug.
If you are a developer, please run 'rake bootstrap'. Running 'rake' requires the 'ruby' program be available.

It still works finding and setting JAVA_HOME, I only cloned the repo without JRuby, so the other error is normal.

LGTM

@elasticsearch-bot
Copy link

Suyog Rao merged this into the following branches!

Branch Commits
master 65b1f99
5.0 3e4cb0e
2.x 9b712e5
2.3 e277492

elasticsearch-bot pushed a commit that referenced this pull request May 6, 2016
The current logic for printing %JAVA_HOME% inside if ( ) block failes if the path contains a `(` which is the case for 32bit java (`C:\Program Files (x86)\java\jdk`)

The current logic for appending to JAVA_OPTS does not work as it will be expanded while interpreting the else block. Use goto instead.

Fixes #5214
elasticsearch-bot pushed a commit that referenced this pull request May 6, 2016
The current logic for printing %JAVA_HOME% inside if ( ) block failes if the path contains a `(` which is the case for 32bit java (`C:\Program Files (x86)\java\jdk`)

The current logic for appending to JAVA_OPTS does not work as it will be expanded while interpreting the else block. Use goto instead.

Fixes #5214
elasticsearch-bot pushed a commit that referenced this pull request May 6, 2016
The current logic for printing %JAVA_HOME% inside if ( ) block failes if the path contains a `(` which is the case for 32bit java (`C:\Program Files (x86)\java\jdk`)

The current logic for appending to JAVA_OPTS does not work as it will be expanded while interpreting the else block. Use goto instead.

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

Successfully merging this pull request may close these issues.

5 participants