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

Handle special characters and spaces in JAVA_HOME path in elasticsearch-service.bat #52676

Merged
merged 1 commit into from
Mar 2, 2020

Conversation

shaheerakr
Copy link
Contributor

I was unable to install elasticsearch service, due to the following issue
error
The batch script was not able to parse the JAVA_HOME path correctly if there was a space in it, which is the case shown above as JAVA_HOME is set to C:\Program Files (x86)\jdk-12.0.2

@cla-checker-service
Copy link

cla-checker-service bot commented Feb 22, 2020

💚 CLA has been signed

@gwbrown gwbrown added the :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts label Feb 25, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Packaging)

@jasontedor
Copy link
Member

jasontedor commented Feb 25, 2020

@rjernst Can you find someone to test and review this? I suspect if this change is right, a similar change is needed in the non-service batch script.

@rjernst rjernst requested review from williamrandolph and removed request for jasontedor February 25, 2020 01:37
@williamrandolph
Copy link
Contributor

@jasontedor I'm still reviewing, but it looks like we already made this change to bin/elasticsearch.bat in #39712, and this PR is just bringing the service script in line.

@jasontedor
Copy link
Member

@williamrandolph Awesome, thanks for pointing that out.

@williamrandolph
Copy link
Contributor

@shaheerakr I'd like to be able to merge this PR. Are you able to sign the Contributor License Agreement?

@shaheerakr
Copy link
Contributor Author

@williamrandolph I have signed the Contributor licence agreement but somehow it is not updating here

@shaheerakr
Copy link
Contributor Author

@williamrandolph Contributor Licence Agreement is updated here, you can merge now

@williamrandolph
Copy link
Contributor

@elasticmachine test this please

@williamrandolph
Copy link
Contributor

@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/bwc

@williamrandolph williamrandolph changed the title fix ES_PATH_CONF in jvm_options_parser in elasticsearch-service.bat Handle special characters and spaces in JAVA_HOME path in elasticsearch-service.bat Mar 2, 2020
@shaheerakr
Copy link
Contributor Author

@williamrandolph are these changes ready to be merged or do i need to change anything for this unsuccessful test?

@williamrandolph
Copy link
Contributor

@shaheerakr The failing test looks like the issue from #52364, and I don't believe it has anything to do with your commit. I'll try running the test again to see if it passes.

@elasticmachine please run elasticsearch-ci/bwc

@williamrandolph
Copy link
Contributor

@elasticmachine please run elasticsearch-ci/bwc

@williamrandolph williamrandolph merged commit 3571f1d into elastic:master Mar 2, 2020
@williamrandolph
Copy link
Contributor

I don't believe the failing test could have had anything to do with this code, so I've gone ahead and merged. The packaging tests have passed.

williamrandolph added a commit that referenced this pull request Mar 3, 2020
… (#53057)

* Handle special chars in JAVA_HOME in elasticsearch-service.bat (#52676)

* Test case for windows service where JAVA_HOME path contains spaces (#53028)

Co-authored-by: Muhammad Shaheer Akram <[email protected]>
@williamrandolph
Copy link
Contributor

@shaheerakr Thanks again for finding and fixing this issue. We appreciate the contribution.

@shaheerakr
Copy link
Contributor Author

no problem @williamrandolph these changes will be available from 7.6.2 right?

williamrandolph added a commit to williamrandolph/elasticsearch that referenced this pull request Mar 5, 2020
…ic#52676) (elastic#53057)

* Handle special chars in JAVA_HOME in elasticsearch-service.bat (elastic#52676)

* Test case for windows service where JAVA_HOME path contains spaces (elastic#53028)

Co-authored-by: Muhammad Shaheer Akram <[email protected]>
williamrandolph added a commit that referenced this pull request Mar 5, 2020
… (#53057) (#53141)

* Handle special chars in JAVA_HOME in elasticsearch-service.bat (#52676)

* Test case for windows service where JAVA_HOME path contains spaces (#53028)

Co-authored-by: Muhammad Shaheer Akram <[email protected]>
@williamrandolph
Copy link
Contributor

@shaheerakr Yes, the changes will now be in the next 7.6.x release as well as in 7.7.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team v7.6.2 v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants