-
Notifications
You must be signed in to change notification settings - Fork 900
config/opal_setup_java.m4: Improve JDK Tool Path Resolution on OS X/macOS #5015
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
config/opal_setup_java.m4: Improve JDK Tool Path Resolution on OS X/macOS #5015
Conversation
Can one of the admins verify this patch? |
@RandomDSdevel can you please describe what this PR is fixing ? #5001 has been merged into master, and I will PR shortly vs the release branches. |
Whoops, accidentally forgot to make sure |
Sure thing, @ggouaillardet; as I understand things, Java headers and executables can be found in a few different ways:
From what I understand of
With this PR, the same
As such, I believe Open MPI's configuration system should behave along the lines explained here even though #5001 has already been merged, as this would actually be more correct. Please let me know, however, if I've missed anything either in this analysis or, again, in this PR. |
@ggouaillardet and/or @bwbarrett: Does anything else remain to be done on this PR on my side? |
I've been keeping things rebased against the latest commits to |
@kawashima-fj and @ggouaillardet, can one of you comment on @RandomDSdevel's questions and if it looks right to you, merge it into master? |
@RandomDSdevel Could you remove trailing spaces at the end of lines and update the PR ( Though I cannot understand the need of this change fully because of my macOS knowledge shortage, the intent of this change and the change itself seem reasonable. I'll merge it once the PR is updated. |
@RandomDSdevel, Without this change, who suffers the problem? All OS X/macOS users who want Java binding? Or users with a particular JDK installation? I want to know impact of this problem. |
config/opal_setup_java.m4
Outdated
[AC_MSG_RESULT([found ($opal_java_dir)]) | ||
opal_java_found=1 | ||
with_jdk_headers=$opal_java_dir | ||
with_jdk_bindir=/usr/bin], | ||
if test "$opal_java_dir" = /System/Library/Frameworks/JavaVM.framework/Versions/Current; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of testing the value of $opal_java_dir
, could we check the existence of $opal_java_dir/{include,bin}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not unless you want to drop support for Apple's Java shims. (I'm not sure what their status is, but Apple stopped shipping its own derivative Java distribution after Java…6, I believe, so odds are they've actually been deprecated since some time after that…?) I noted in my toolpath analysis above that the header and executable directories under $opal_java_dir
differ in naming scheme depending on what that variable's contents are, did I not? In any case, I felt it safer to leave messing with that to the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RandomDSdevel First of all, many thanks for your patience. This is a complex topic; forgive us for being so pedantic trying to understand it.
- I would actually love to see much of config/opal_setup_java.m4: Improve JDK Tool Path Resolution on OS X/macOS #5015 (comment) turned into a comment in the .m4 file. This is complex stuff; putting all that hard-won knowledge down as a comment in the code could save someone from making a mistake in the future while "fixing" something (but not having the background knowledge of why the code currently is the way it is).
- I think what @ggouaillardet was suggesting was not incongruent with your analysis. I think he was suggesting something like this (typed directly here in github / not tested -- YMMV):
if test -d "$opal_java_dir/Headers" && test -d "$opal_java_dir/Commands"; then
with_jdk_headers="$opal_java_dir/Headers"
with_jdk_bindir="$opal_java_dir/Commands"
elif test -d "$opal_java_dir/include" && test -d "$opal_java_dir/bin"; then
with_jdk_headers="$opal_java_dir/include"
with_jdk_bindir="$opal_java_dir/bin"
else
AC_MSG_WARN([No recognized directory structure found under $opal_java_dir])
AC_MSG_ERROR([Cannot continue])
fi
This simply prevents us from having the hard-coded ".../Current" directory name in 2 places. It also is marginally more defensive in that it also checks for the presence of include
and bin
if the Headers
and Commands
checks fail.
Is there a reason that would drop support for Apple's Java shims? It looks like the same logic to me -- is there a subtle implication that I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RandomDSdevel First of all, many thanks for your patience. This is a complex topic; forgive us for being so pedantic trying to understand it.
⋮
You're welcome, and no problem.
⋮
- I would actually love to see much of #5015 (comment) turned into a comment in the .m4 file. This is complex stuff; putting all that hard-won knowledge down as a comment in the code could save someone from making a mistake in the future while "fixing" something (but not having the background knowledge of why the code currently is the way it is).
⋮
Agreed, but…that would be rather a lot of text, don't you think? I could just put URIs pointing to the relevant discussion into such a comment instead; that would be a lot shorter.
⋮
I think what @ggouaillardet was suggesting was not incongruent with your analysis. I think he was suggesting something like this (typed directly here in github / not tested -- YMMV):
if test -d "$opal_java_dir/Headers" && test -d "$opal_java_dir/Commands"; then with_jdk_headers="$opal_java_dir/Headers" with_jdk_bindir="$opal_java_dir/Commands" elif test -d "$opal_java_dir/include" && test -d "$opal_java_dir/bin"; then with_jdk_headers="$opal_java_dir/include" with_jdk_bindir="$opal_java_dir/bin" else AC_MSG_WARN([No recognized directory structure found under $opal_java_dir]) AC_MSG_ERROR([Cannot continue]) fi
This simply prevents us from having the hard-coded ".../Current" directory name in 2 places. It also is marginally more defensive in that it also checks for the presence of
include
andbin
if theHeaders
andCommands
checks fail.⋮
Ah, OK; that does seem a bit nicer, so I'll put that in place now.
⋮
Is there a reason that would drop support for Apple's Java shims? It looks like the same logic to me -- is there a subtle implication that I'm missing?
Sorry, I should have repeated some additional context from downstream in Homebrew/homebrew-core#26009 (comment) here; here it is:
… (Per an old message which I found on one of the OpenJDK mailing lists via this Ask Different answer when I was looking around to see what information I could jump off from to troubleshoot [a possible lead on this issue]:
⋮
Do NOT remove any content in the JavaVM.framework. Those items are required by Java 7, 8, 9+ as well as Java SE 6. No modern version of OS X has a Java JDK inside there anyway.
⋮
⋮
However, if you run:
…$ sudo -i
⋮ # Enter password…
…# find / -type f -perm +111 -exec file -p {} \; -exec head -c 1 /dev/zero \; | xargs -0 -n 1 sh -c 'echo "$@" | head -n 1' -- | grep -v x86_64 | grep -E Mach-O.+i386
⋮ # Wait for that to finish…
…# logout
(at least on my system, which has OS X v10.11.6 'El Capitan,' build 15G20015, installed,) then you'll notice that the resulting long list includes the following output:
⋮
/System/Library/Frameworks/JavaVM.framework/Versions/A/Resources/.compatibility/libIASupport.jnilib: Mach-O dynamically linked shared library i386
⋮
There might also be some 32-bit components elsewhere on Macs upon which /System/Library/Frameworks/JavaMV.framework
might additionally depend, but I haven't looked into what those might be. (I also haven't asked anywhere to see if this might have changed in versions of OS X/macOS newer than the one I have installed on my system.)
What implications this factoid might have, if any, for the future of Apple's Java shims, I don't know, but all 32-bit OS X/macOS binaries currently have a limited lifespan, as noted in this notice on Apple's Developer portal:
64-bit Requirement for Mac Apps
June 28, 2017
At WWDC 2017, we announced new apps submitted to the Mac App Store must support 64-bit starting January 2018, and Mac app updates and existing apps must support 64-bit starting June 2018. If you distribute your apps outside the Mac App Store, we highly recommend distributing 64-bit binaries to make sure your users can continue to run your apps on future versions of macOS. macOS High Sierra will be the last macOS release to support 32-bit apps without compromise.
Whether Apple will use this aa an excuse to remove their vestigial Java shims entirely or choose to update that lone 32-bit component inside it (and any the entire shim framework might depend on) to be 64-bit executables, I again don't know. Relying solely on users only having instances of Oracle's official JDK installed on their system might, however, turn out to be a safer idea in the long run, but I'm deeming that outside the scope of this PR for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed most of your review comments with a new revision of this PR's sole commit pushed to its staging branch on my Open MPI repository fork. I left out adding an additional comment as you asked for now, though, pending any guidance you might have on what you might want it to look like (just something short pointing through one or more link vs. a copied-and-pasted variant of my giant wall of text from #5015 (comment).) (I should also note that said wall of text was adapted from an e-mail that I posted to [email protected] and mentioned in my commit message.)
Does. Edit: Did a final rebase but nearly forgot to remove trailing space as requested (at least I think I got them all…) |
Without this change, the Open MPI build system mixes up Apple's Java shims with users' official Oracle Java installations as I explained above. With this change, #5000 would have been caught during build configuration instead of later on in the build process much closer to its end when an attempt to invoke |
config/opal_setup_java.m4
Outdated
AS_IF([test -d $opal_java_dir], | ||
[AC_MSG_RESULT([found ($opal_java_dir)]) | ||
opal_java_found=1 | ||
with_jdk_headers=$opal_java_dir | ||
with_jdk_bindir=/usr/bin], | ||
if test "$opal_java_dir/Headers" && test -d "$opal_java_dir/Commands"; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget -d
before "$opal_java_dir/Headers"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, clumsy me; it appears I did. Fix incoming…
That looks good to me (i.e., the As for the comment, I honestly don't mind long comments. I leave it to your discretion: a long comment, or a pointer to #5015 (comment). Either way, this overall issue should be cited in the comment (i.e., #5015), since the overall discussion may be useful in the future. But that one specific comment (or something derived from it) is the crux of the design philosophy, and should specifically be called out, either by reference or by value. Thanks! |
…acOS. Also avoid picking up Apple's Java shims via the sym. links to them in `/usr/bin` on systems where any one of them could possibly exhibit behavior that is erratic and, to some extent, likely to be incorrect nowadays (cf.: - https://www.mail-archive.com/[email protected]/msg20551.html - #5015 (comment) - the last part of #5015 (comment) - #5015 (comment) for more detailed context.) Works alongside #5001 to close #5000. Signed-off-by: Bryce Glover <[email protected]>
How's that for a first pass at adding documentation about this PR's associated changes to the makefile source, @jsquyres? I'm sure the wording could see some improvement, but I think I covered everything sufficiently with it. |
Looks like there might have been a CI infrastructure hiccup…? Can somebody restart the build, please? |
bot:ompi:retest Looks like the same bad merge issue |
@RandomDSdevel Thanks for your update and explanation! I understand that users who are affected by this problem are:
|
Thanks, @bwbarrett! |
Not exactly: those shims stay in place even if you don't have an Apple JDK installed, and they're still around on newer systems. Otherwise that's correct aside from how it interacted with the old path-detection logic, which is now fixed, and how things were before #5001. |
@RandomDSdevel, sorry, I misunderstood. Thanks for your clarification. |
@kawashima-fj, @ggouaillardet, @bwbarrett, and/or @jsquyres: Can we get this backported from |
…acOS. Also avoid picking up Apple's Java shims via the sym. links to them in `/usr/bin` on systems where any one of them could possibly exhibit behavior that is erratic and, to some extent, likely to be incorrect nowadays (cf.: - https://www.mail-archive.com/[email protected]/msg20551.html - open-mpi#5015 (comment) - the last part of open-mpi#5015 (comment) - open-mpi#5015 (comment) for more detailed context.) Works alongside open-mpi#5001 to close open-mpi#5000. Signed-off-by: Bryce Glover <[email protected]> (cherry picked from commit open-mpi/ompi@8c32cd8)
…acOS. Also avoid picking up Apple's Java shims via the sym. links to them in `/usr/bin` on systems where any one of them could possibly exhibit behavior that is erratic and, to some extent, likely to be incorrect nowadays (cf.: - https://www.mail-archive.com/[email protected]/msg20551.html - open-mpi#5015 (comment) - the last part of open-mpi#5015 (comment) - open-mpi#5015 (comment) for more detailed context.) Works alongside open-mpi#5001 to close open-mpi#5000. Signed-off-by: Bryce Glover <[email protected]> (cherry picked from commit open-mpi/ompi@8c32cd8)
@RandomDSdevel note |
Funny how that works sometimes, eh? |
I'm still looking to downstream #5119 to Homebrew as a patch to its distribution of v3.0.1 until v3.0.2 has finished baking, but said PR still hasn't received the review you requested from @kawashima-fj. |
Sorry, I'm late. There was 9-day holiday in Japan. I reviewed and approved v3.0/v3.1 PRs. Merging this commit to v2.x branch fails because it requires the commit ff5af73 beforehand. But it does not have signed-off. Can I cherry-pick it without signed-off? |
@kawashima-fj 8c32cd8 is the commit that should be backported (and it was signed off) |
@ggouaillardet 8c32cd8 is based on ff5af73 (
|
@kawashima-fj oh i see, this is an old commit that was never merged into the master. the last option looks good enough to me (e.g. do nothing but ask users to upgrade) My second option would be to cherry-pick both commits, squash them and keep the last commit message, and append a credit to the original author in the commit message. |
Ok. If no one wants OMPI v2.1 + OS X/macOS + Oracle JDK + Open MPI Java binding, I'll update the |
Golden Week, eh? (I looked it up.) Well, that's as good a reason as any! I hope you enjoyed it! |
FWIW, I would not advise cherry picking 2 commits and squashing them -- that just makes them harder to track. Instead, I would cherry pick the 2 commits in the correct order (to avoid the conflicts), and then manually add a signed-off-by line on the commit that is missing it (e.g., via an interactive rebase or whatever your favorite commit-message-edit method you prefer). |
Per #5001 (comment) addressing #5001 (comment), see also #5160. |
config/opal_setup_java.m4: Fix #5015.
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 open-mpi#5001 (comment) and tracked down by me in open-mpi#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]> (cherry picked from commit 4a05c7e)
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 open-mpi#5001 (comment) and tracked down by me in open-mpi#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]> (cherry picked from commit open-mpi/ompi@4a05c7e)
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 open-mpi#5001 (comment) and tracked down by me in open-mpi#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]> (cherry picked from commit 4a05c7e)
…acOS. Also avoid picking up Apple's Java shims via the sym. links to them in `/usr/bin` on systems where any one of them could possibly exhibit behavior that is erratic and, to some extent, likely to be incorrect nowadays (cf.: - https://www.mail-archive.com/[email protected]/msg20551.html - #5015 (comment) - the last part of #5015 (comment) - #5015 (comment) for more detailed context.) Works alongside #5001 to close #5000. Signed-off-by: Bryce Glover <[email protected]> (cherry picked from commit 8c32cd8) Signed-off-by: KAWASHIMA Takahiro <[email protected]>
This is just a first pass at this, as I expect my changes to be thoroughly reviewed. I will, of course, address feedback as it comes in.