-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Use JAVA_HOME or java.exe in PATH like the Linux scripts do #18685
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
Use JAVA_HOME or java.exe in PATH like the Linux scripts do #18685
Conversation
@gmarz could you take a look at this please? |
Sorry, I don't think this change should be made. |
The Having one way to do things is ok, but in my opinion using So that's why I try to give you an updated code. At our work we have put ELK into MSI packages and our testers and later users struggle about their forgotten JAVA_HOME variable. If Elasticsearch finds the current Java installed would be much more convenient as no manual set environment is involved. |
Sorry, I realize my comment was a little confusing, but here's what I mean. Currently we have exactly one way to specify the location of Java:
That's exactly why changes like this should be automated. It shouldn't the case that one admin updates Java and another must remember to update In particular, can't you just point |
AFAIK there is only Oracle Java for Windows and not openjdk. So this PR exists to simplify that only Java installation way for Windows: Downloading and installing the EXE (sorry, no MSI, but still the only standard package from Oracle). This PR doesn't change the behaviour if someone still insists to set There is no second way to do it, just do the right way simpler. Why do I want to set JAVA_HOME when there is a standard way from the makers of Java that add a symlink for me? I just don't understand why I should do this extra step, it's just not necessary. Sorry, I'm only a parttime user of Java, and never understood that. I think that if Oracle expected that JAVA_HOME must be set, then I would expect that their installer would do that for me. This PR just reads the target of the symlink (which directs to the java.exe and not the value needed for JAVA_HOME) and calculates the right value for JAVA_HOME automatically. |
Their docs even explain that it needs to be set and how to set it for some of their own applications. A change that I would very much welcome is setting I think that would satisfy your need, do you agree? |
Windows is a different beast than Linux, and while I agree that it can be painful/confusing for Windows users to have to set JAVA_HOME manually, I think @jasontedor is right in that we should stick to the hard rule that JAVA_HOME should be set outside of the scripts. There are too many things that can go wrong by assuming the correct location of Java. What if there are multiple versions installed? JRE vs JDK? 32 vs 64-bit? Does the Oracle MSI update the symlink when these things change? Maybe it does the right thing, but I think it's best not to assume and leave it to the user. IMO, it's a task better suited for the Elasticsearch MSI (which we're currently working on) where we can bake in more complex logic (checking the registry) and invoke user interaction if needed. |
+1 |
This is how it looks like after Java 8 installation.
So |
@gmarz Cool that your are working on a MSI, so I can drop ours soon :-) For Kibana and Logstash as well? That you can install the wrong CPU type and different versions of Java on one machine is the heritage of the lack of a proper package management in the first place. But yes this it is how we have to work with it now. That way may go back to Sun... Just found out that the Server JRE is only deployed as a tar.gz and Oracle does not give any hint how to extract this on a Windows machine: http://docs.oracle.com/javase/8/docs/technotes/guides/install/windows_server_jre.html#CFHGHHFJ My focus for this PR was for the EXE installer that I normally use and that would make it easy to make Elasticsearch run out-of-the-box. |
One other thing. As you can see I'm testing this in Vagrant boxes. I just found the |
We are working on that, it is badly needed exactly so we can feel confident when making packaging changes. See #18475.
We basically run a full suite of packaging tests. Does Elasticsearch start? Can it install a plugin? Does it work if there is a path with a space in the name? Does it work with a custom config? All sorts of fun things like that. |
No, because we can just do something very simple like this: if defined JAVA_HOME goto cont
set JAVA=java.exe
%JAVA% -version >NUL 2>&1
if "%ERRORLEVEL%" == "0" goto start
@rem print some error message here
goto fail
:cont
set JAVA_HOME=%JAVA_HOME:"=%
set JAVA=%JAVA_HOME%/bin/java.exe
if exist %JAVA% goto start
@rem print some error message here
goto fail
:start and then replace the launch command from This is completely off the cuff, completely untested, but gives the idea. |
Ah, so the JAVA_HOME environment is only needed for the path to |
Exactly. And that's why I'm comfortable saying, hey, if |
OK, I'll update the PR. But I prefer to use if [ -x "$JAVA_HOME/bin/java" ]; then
JAVA="$JAVA_HOME/bin/java"
else
JAVA=`which java`
fi
if [ ! -x "$JAVA" ]; then
echo "Could not find any executable java binary. Please install java in your PATH or set JAVA_HOME"
exit 1
fi I will keep the Windows version in about that order. |
Can you double-check if |
I've worked with The old variant
works still with Windows 10. I probably use this. |
I prefer this option too. |
I've come up with this batch solution IF DEFINED JAVA_HOME (
set JAVA=%JAVA_HOME%\bin\java.exe
) ELSE (
FOR %%I IN (java.exe) DO set JAVA=%%~$PATH:I
)
IF EXIST "%JAVA%" GOTO cont
:err
ECHO Could not find any executable java binary. Please install java in your PATH or set JAVA_HOME 1>&2
EXIT /B 1
:cont which looks pretty similar to the bash solution. I've commited that for the elasticsearch*.bat scripts. Still working on But you can have a look at the |
I've updated
PTAL |
Thanks @StefanScherer, it looks great. Can you update the title of the PR? |
@jasontedor better? |
@StefanScherer Thanks! I will merge soon. |
Thanks for working through this one @StefanScherer, your contribution is greatly appreciated. |
Thanks for merging @jasontedor. That's what open source and PR's are for. To review and to understand the intention behind the code both of the maintainer's view and the contributor's view. I've learned a lot :-) |
I've enhanced the Windows batch scripts to retrieve the JAVA_HOME environment from the symlink installed by Oracle's Java. See also the PR elastic/logstash#4913 in logstash repo.