Skip to content

Conversation

@seal-mt
Copy link

@seal-mt seal-mt commented Mar 30, 2017

If the java installation path on windows contains spaces (e.g. c:\Program Files) and %JAVA_HOME% isn't set the batch scripts for startup will fail, because file tests and starting java use only the part before the first space instead of the whole path. I added double quotes around all occurrences of %JAVA% where quotes are necessary to handle spaces.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@rjernst
Copy link
Member

rjernst commented Jun 13, 2017

@seal-mt We have tests for spaces in paths. Can you give a reproduction of this (ie show what concrete paths you have for java which cause the current code to break)?

@seal-mt
Copy link
Author

seal-mt commented Jun 14, 2017

@rjernst I installed OpenJDK 1.8.0_131-1 from https://github.com/ojdkbuild/ojdkbuild. The msi installs java by default into C:\Program Files\ojdkbuild\java-1.8.0-openjdk-1.8.0.131-1. After installing PATH is changed, but JAVA_HOME is not set.
After that i downloaded and unzipped elasticsearch-5.4.1.zip. When now calling elasticsearch.bat from command line the java executable is correct found in elasticsearch.in.bat in line 6 (FOR %%I IN ...), but the comparison in line 9 (IF NOT EXIST %JAVA%) fails until i wrap %JAVA% in double quotes.
The next errors occurred when calling java for version checking and running elasticsearch, so in the PR i changed usage of %JAVA% throughout the scripts.
Tested on a Windows Server 2012 R2 System.

I hope this helps.

@clintongormley clintongormley requested a review from Mpdreamz June 20, 2017 12:27
@clintongormley clintongormley added the :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts label Jun 20, 2017
@rjernst
Copy link
Member

rjernst commented Jun 22, 2017

After installing PATH is changed, but JAVA_HOME is not set

I missed this before. I think the underlying problem is with the loop that finds java when java home is not set.

FOR %%I IN (java.exe) DO set JAVA=%%~$PATH:I

That line needs to be quoted correctly to handle spaces. Then everything else should work.

jasontedor added a commit that referenced this pull request Jun 23, 2017
When JAVA_HOME is not set we try to detect the location of Java. If its
location contains a space, due to a lack of quoting we will be
unsuccessful in invoking Java. This commit adds the necessary quoting to
handle this case.

Relates #23822
jasontedor added a commit that referenced this pull request Jun 23, 2017
When JAVA_HOME is not set we try to detect the location of Java. If its
location contains a space, due to a lack of quoting we will be
unsuccessful in invoking Java. This commit adds the necessary quoting to
handle this case.

Relates #23822
jasontedor added a commit that referenced this pull request Jun 23, 2017
When JAVA_HOME is not set we try to detect the location of Java. If its
location contains a space, due to a lack of quoting we will be
unsuccessful in invoking Java. This commit adds the necessary quoting to
handle this case.

Relates #23822
@jasontedor
Copy link
Member

That's correct @rjernst, and all that is needed here is quotes for that case. I want to get this fixed in 5.5.0 which means I elected to push a fix directly:

Thanks so much for the report @seal-mt, we would not have caught this one without your comment above.

@jasontedor jasontedor closed this Jun 23, 2017
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts feedback_needed Team:Delivery Meta label for Delivery team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants