improvedi `libjvm` discovery

[ExpandingMan]

Hello all. I've cleaned up the JVM discovery a bit. First, the standard JAVA_HOME locations are now stored in the global constant JAVA_HOME_CANDIDATES, making it slightly easier and more transparent for future committers to put new directories there. Additionally, I have added the directory /usr/lib/jvm/default to these, which is the default JAVA_HOME on Arch linux (pretty sure this is the standard for modern Java on modern linux distros).

Lastly, the message that tells the user that libjvm.so has been loaded has been changed to @info so that it can be controlled with logging. Is there any interest in removing this message entirely? It seems a little pedantic to tell the user that you are loading the JVM library when JavaCall is loaded, I mean of course it has to load a library. The only disadvantage I can see of removing that message is that (as I found out the hardway) if you do @everywhere using JavaCall you will of course load many instances of the library. Still, I'd vote to remove the message.

[dfdx]

I think the logic and priority for libjvm discovery should be:

1. ENV["JAVA_HOME"] if user has specified it.
2. System tools for JVM discovery, including WinReg and /usr/libexec/java_home.
3. A list of predefined locations.

[ExpandingMan]

That's what this is doing now? If you look at the order it populates javahomes and then libpaths it puts in the environment first, followed by the windows thing, followed by the default linux directories.

By the way, concerning the appveyor failure, were we ever expecting this to be successful on 32-bit windows?

[dfdx]

That's what this is doing now?

Yes, but as I mention in inline comments (can you see them?) you break some parts of this logic.

By the way, concerning the appveyor failure, were we ever expecting this to be successful on 32-bit windows?

Discussed recently in #80. In short, it shouldn't block the PR.

[ExpandingMan]

Oddly, I can't see your inline comments (not sure why that's happening, normally I can see them).

Could you let me know what you would like to see changed?

[dfdx]

Oh, no, not again! It seems like GitHub secretly and randomly hides my comments from time to time! :smile:

Line 19: const JAVA_HOME_CANDIDATES = ["/usr/libexec/java_home/",

As far as I understand, /usr/libexec/java_home isn't a directory, but instead an utility to detect Java home location - in original code it's executed as binary and the result of execution is added to javahomes.

Line 51: ENV["JAVA_HOME"] = javahome_winreg()

This way you unconditionally overwrite user-provided JAVA_HOME environment variable with the one in WinReg. UPD. Oh, at this point you have already pushed previous ENV["JAVA_HOME"] to javahomes. But then why to overwrite environment variable?

[ExpandingMan]

Sorry, that was silly of me I missed both those things. Should be ok now.

[ExpandingMan]

Any opinion on whether we should still report having loaded the lib?

[dfdx]

isfile(LIBEXEC_JAVA_HOME) && push!(javahomes, chomp(read(LIBEXEC_JAVA_HOME, String)))

First LIbEXEC_JAVA_HOME should indeed be a string, but second one should be a command (note backquotes in the original code). read runs this command and reads its output as a string. E.g. on Linux try:

read(`ls`, String)

Any opinion on whether we should still report having loaded the lib?

As for me, I'd be happy to make it @debug - useful when you need a tip on what's going on, but invisible otherwise.

[ExpandingMan]

Hm, I don't think that method of read is documented anywhere. Do you think I should open a Julia issue?

Ok, I think we are all good now. I think the @debug message is a good idea.

[dfdx]

Hmm, @edit read(ls, String) points me to a function with docstring, but I don't see this docstring in @doc read. Anyway, I inferred this signature from the previous code, I didn't know about this feature either.

The code looks good to me now. Let's wait a day to see if anyone has other comments (e.g. regarding println vs. @debug) and if nothing I'll merge this PR.

[ExpandingMan]

Sounds good.

Confirmed what you're seeing on the read function. I think I'll open an issue as I never, ever would have found that function on my own.

[dfdx]

Merged.