config/opal_setup_java.m4: Fix #5015. #5160
Conversation
|
Can one of the admins verify this patch? |
|
Just glancing at this, and I'm puzzled - as the person who originally added Java support to OMPI, when did somebody change the default to enabling Java bindings?? The requirement for adding this was that it never be enabled by default. |
|
I was wondering that, too, but thought I might best leave figuring that out to somebody else for whom it wasn't 'above their pay grade,' so to speak (this is only my second Open MPI contribution after all.) I'm leave it be for now just to keep the scope of this PR narrow and preserve its initial intent. By all means, though, that's a fair question to ask and answer. |
|
ok to test |
config/opal_setup_java.m4
Outdated
| AC_MSG_ERROR([Cannot continue]) | ||
| AC_MSG_WARN([No recognized OS X/macOS JDK directory structure found under $opal_java_dir]) | ||
| opal_java_found=0 | ||
| AC_MSG_RESULT([Falling back to other platforms' JDK checks]) |
There was a problem hiding this comment.
Good catch -- this does seem like it was the error (from my original pseudocode in a prior github comment).
I think you can get rid of the AC_MSG_RESULT -- it'll be obvious in the configure output that we're trying other places.
There was a problem hiding this comment.
@jsquyres: Just thought I'd be consistent with there being a message there before (albeit the faulty one,) but I'll change it.
That PR accidentally changed Open MPI's build configuration infrastruc- ture's Java toolchain detection logic so that it would, as reported by @bosilca in #5001 (comment) and tracked down by me in #5001 (comment), abort your entire in-progress Open MPI build when it failed to find an OS X/macOS JDK instead of simply falling back to checking for a JDK in locations where it would be found on other platforms. _Oops…!_ Signed-off-by: Bryce Glover <[email protected]>
|
@rhc54 The default is still not to enable Java; we check for Java compilers and whatnot, and then later determine whether to enable the support or not. We could restructure that a bit to not even bother checking for Java if it wasn't requested, if that is desired (I don't think that particular logic has changed in quite a while -- it's in a different area than this particular PR addresses: see 31beff6, back from 2012). |
|
@jsquyres: I've addressed your review comment and force-pushed with lease. |
|
@RandomDSdevel Thank you! Will merge after CI completes. @bosilca This fixes your Java issue. |
|
Magnificent ! |
|
Not that I can see - there is no reason to be checking for Java support if we aren't building the Java bindings. There was a time when we needed them separate because we were supporting non-MPI Java programs (e.g., map-reduce) and ORTE needed to know it could support Java, but that time has passed. |
|
@ggouaillardet Yeah, I noticed that yesterday afternoon, too: the variable we're checking is wrong. But even fixing that, and fixing the condition to be which is not quite accurate, because it's not that the Java compiler isn't found -- it's that we didn't even look if So just to be clear, currently:
We could change the first one to be a little more intelligent to not even search for the Java compiler unless |
Resolves #5001 (comment).