Bat scripts to work with JAVA_HOME with parantheses by pgomulka · Pull Request #39712 · elastic/elasticsearch

pgomulka · 2019-03-05T17:01:50Z

the elasticsearch.bat and elasticsearch-env.bat won't work if JAVA contains parentheses. This seems to be the limitation of FOR /F IN (command) DO syntax.
The JAVA variable present in a command contains a path to a binary to start elasticsearch (with spaces & parans). We can workaround the problem of spaces and parentheses in this path by referring this variable with a CALL command.
Note that executing binaries with CALL is an undocumented behaviour (but works)

closes #38578
closes #38624
closes #33405
closes #30606

elasticmachine · 2019-03-05T17:01:53Z

Pinging @elastic/es-core-infra

the elasticsearch.bat and elasticsearch-env.bat won't work if JAVA contains parentheses. This seems to be the limitation of FOR /F IN (command) DO syntax. The JAVA variable present in a command contains a path to a binary to start elasticsearch (with spaces & parans). We can workaround the problem of spaces and parentheses in this path by referring this variable with a CALL command. Note that executing binaries with CALL is an undocumented behaviour (but works)

…tartup_scripts

pgomulka · 2019-03-12T15:15:14Z

@rjernst would you be able to have a look into this PR?

rjernst

Sorry this review never got published before. Some of these comments might have already been addressed.

rjernst · 2019-03-05T17:25:19Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

+                                             .filter(path -> path.contains("Java") == false)
+                                             .collect(joining(";"));
+
+                sh.runIgnoreExitCode("cmd /c mklink /D 'C:\\Program Files (x86)\\java' $Env:JAVA_HOME");


Can you add a note that this can be done in powershell once we have min PS 5.0?

you mean the symbolic link creation? Is the PS 5 available since certain windows version?

Why do we ingnore the exit code? If this fails, the test cannot proceed right?

good spot, test should not proceed if this fails

rjernst · 2019-03-05T17:26:36Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

        Archives.stopElasticsearch(installation);
    }

+    public void test51JavaHomeContainParansAndSpace() throws IOException {


Typo: Parans -> Parens

You might just rename this to "special characters" instead of only allow parens and spaces, something like:

test51JavaHomeWithSpecialCharacters

rjernst · 2019-03-05T17:27:53Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

+            final String javaHome = sh.run("$Env:JAVA_HOME").stdout.trim();
+
+            try {
+                final String newPath = Arrays.stream(originalPath.split(";"))


This should not be necessary anymore. In 6.7 there shouldn't be any test using PATH anymore, except intentionally.

so the vagrant images are guaranteed not to have java inside PATH? I feel like it wouldn't harm to make sure PATH does not have java in there.

No, java is in the PATH. I don't think we should change that. In 6.7 I made sure all the tests pass in JAVA_HOME, to avoid triggering the deprecation warning output starting in that version when using java on the PATH. In 7.0+ we no longer look at PATH at all to find java.

rjernst · 2019-03-05T17:28:20Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

+                Archives.runElasticsearch(installation, sh);
+
+                Archives.stopElasticsearch(installation);
+            } catch (IOException e) {


No need to catch, just add a checked exception to the test method

it is because this code is inside lambda onWindows( ()-> {here} )

We should change the signature of PlatformAction.run to have a checked Exception, so that we don't need these intermediate try/catches.

rjernst · 2019-03-05T17:29:22Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

+    public void test51JavaHomeContainParansAndSpace() throws IOException {
+        assumeThat(installation, is(notNullValue()));
+
+        Platforms.onWindows(() -> {


I believe we have a similar test in bats, can you add the linux side here as well so we can just remove that test?

rjernst · 2019-03-05T17:33:25Z

distribution/src/bin/elasticsearch-env.bat


 if not defined ES_TMPDIR (
-  for /f "tokens=* usebackq" %%a in (`"%JAVA% -cp "!ES_CLASSPATH!" "org.elasticsearch.tools.launchers.TempDirectory""`) do set ES_TMPDIR=%%a
+  for /f "tokens=* usebackq" %%a in (`CALL %JAVA% -cp "!ES_CLASSPATH!" "org.elasticsearch.tools.launchers.TempDirectory"`) do set  ES_TMPDIR=%%a


Discussed this offline: at first glance I had thought this was modifying the deprecated for loop that searches the path for java. Now I see this is a different loop (although I am confused why we need a loop at all: TempDirectory only prints one line). I believe this entire PR can be moved back to basing on master.

pgomulka · 2019-03-22T15:00:36Z

@elasticmachine run elasticsearch-ci/packaging-sample

pgomulka · 2019-04-03T07:33:05Z

This needs to be backported to 6.7 branch (for 6.7.2 version) and 7.x (for 7.1)

…39712) the elasticsearch.bat and elasticsearch-env.bat won't work if JAVA contains parentheses. This seems to be the limitation of FOR /F IN (command) DO syntax. The JAVA variable present in a command contains a path to a binary to start elasticsearch (with spaces & parens). We can workaround the problem of spaces and parentheses in this path by referring this variable with a CALL command. Note that executing binaries with CALL is an undocumented behaviour (but works) closes elastic#38578 closes elastic#38624 closes elastic#33405 closes elastic#30606

the elasticsearch.bat and elasticsearch-env.bat won't work if JAVA contains parentheses. This seems to be the limitation of FOR /F IN (command) DO syntax. The JAVA variable present in a command contains a path to a binary to start elasticsearch (with spaces & parens). We can workaround the problem of spaces and parentheses in this path by referring this variable with a CALL command. Note that executing binaries with CALL is an undocumented behaviour (but works) closes elastic#38578 closes elastic#38624 closes elastic#33405 closes elastic#30606

backports : Bat scripts to work with JAVA_HOME with parentheses (#39712) Link to SYSTEM_JAVA_HOME on windows (#40806)

backports : Migrate systemd packaging tests from bats to java (#39954) Bat scripts to work with JAVA_HOME with parentheses (#39712) Link to SYSTEM_JAVA_HOME on windows (#40806)

…40768) the elasticsearch.bat and elasticsearch-env.bat won't work if JAVA contains parentheses. This seems to be the limitation of FOR /F IN (command) DO syntax. The JAVA variable present in a command contains a path to a binary to start elasticsearch (with spaces & parens). We can workaround the problem of spaces and parentheses in this path by referring this variable with a CALL command. Note that executing binaries with CALL is an undocumented behaviour (but works) closes #38578 closes #38624 closes #33405 closes #30606 backports: * Bat scripts to work with JAVA_HOME with parentheses(#39712) * Link to SYSTEM_JAVA_HOME on windows (#40806)

the elasticsearch.bat and elasticsearch-env.bat won't work if JAVA contains parentheses. This seems to be the limitation of FOR /F IN (command) DO syntax. The JAVA variable present in a command contains a path to a binary to start elasticsearch (with spaces & parens). We can workaround the problem of spaces and parentheses in this path by referring this variable with a CALL command. Note that executing binaries with CALL is an undocumented behaviour (but works) closes elastic#38578 closes elastic#38624 closes elastic#33405 closes elastic#30606

jpountz · 2019-07-05T10:24:02Z

@pgomulka I'm assuming there is nothing left to backport and removed the backport pending label.

pgomulka added :Core/Infra/Core Core issues without another label v6.7.0 labels Mar 5, 2019

pgomulka self-assigned this Mar 5, 2019

pgomulka requested a review from rjernst March 5, 2019 17:01

pgomulka force-pushed the feature/windows_startup_scripts branch from 14cd39f to fe6435b Compare March 6, 2019 07:24

pgomulka changed the base branch from 6.7 to master March 6, 2019 07:24

pgomulka added v7.0.0 v8.0.0 labels Mar 6, 2019

Merge remote-tracking branch 'upstream/master' into feature/windows_s…

ded1387

…tartup_scripts

danielmitterdorfer added the >bug label Mar 7, 2019

rjernst reviewed Mar 13, 2019

View reviewed changes

pgomulka added 4 commits March 14, 2019 09:52

refactor run method

fe06d04

Linux version of java with spaces

baa4bec

compile fix

b3be477

fix temp path

74816f6

Mpdreamz mentioned this pull request Mar 19, 2019

Windows MSI 6.6.1 installation failure elastic/windows-installers#262

Closed

danielmitterdorfer added v6.7.1 and removed v6.7.0 labels Mar 21, 2019

pgomulka added 3 commits March 21, 2019 18:10

Merge branch 'master' into feature/windows_startup_scripts

5ac3a5e

minor cleanup

152cafa

not looking into PATH at all

bb72cb4

pgomulka requested a review from rjernst March 21, 2019 17:18

pgomulka added 2 commits March 22, 2019 11:40

compile error

a7365f8

compile errors

7ea2662

pgomulka added 2 commits March 25, 2019 17:35

fix the linux version of the test

e64b134

Merge branch 'master' into feature/windows_startup_scripts

8e140b0

colings86 added v6.7.2 and removed v6.7.1 labels Apr 3, 2019

pgomulka added the v7.2.0 label Apr 3, 2019

jakelandis added v7.0.0-rc2 and removed v7.0.0 labels Apr 3, 2019

rjernst mentioned this pull request Apr 3, 2019

Link to SYSTEM_JAVA_HOME on windows #40806

Merged

This was referenced Apr 4, 2019

Bat scripts to work with JAVA_HOME with parentheses Backports #40832

Merged

Bat scripts to work with JAVA_HOME with parentheses Backports #40770

Merged

pgomulka added a commit that referenced this pull request Apr 4, 2019

Bat scripts to work with JAVA_HOME with parentheses Backports (#40832)

65d2518

backports : Bat scripts to work with JAVA_HOME with parentheses (#39712) Link to SYSTEM_JAVA_HOME on windows (#40806)

jakelandis added v7.3.0 and removed v6.7.2 v7.0.0-rc2 v7.2.0 labels Jun 17, 2019

jpountz removed the backport pending label Jul 5, 2019

williamrandolph mentioned this pull request Feb 25, 2020

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

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

ChrisHegarty unassigned pgomulka Oct 29, 2024

Conversation

pgomulka commented Mar 5, 2019

Uh oh!

elasticmachine commented Mar 5, 2019

Uh oh!

pgomulka commented Mar 12, 2019

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Mar 22, 2019

Uh oh!

pgomulka commented Apr 3, 2019

Uh oh!

jpountz commented Jul 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants