bugGNU Octave - Bugs: bug #54219, Dynamic detection of JVM on macOS

 
 

bug #54219: Dynamic detection of JVM on macOS

Submitter:  Andrew Janke <apjanke>
Submitted:  Sun 01 Jul 2018 01:22:38 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Mac OS
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 29 Mar 2019 09:48:35 PM UTC, comment #23: 

Yep, close as fixed!

Andrew Janke <apjanke>
Fri 29 Mar 2019 08:51:40 PM UTC, comment #22: 

Andrew: I pushed your changeset from comment #21.  OK to close as fixed?

John W. Eaton <jwe>
Group administrator
Fri 29 Mar 2019 07:58:38 PM UTC, comment #21: 

Build fails; looks like a missing "octave::" namespace qualification.


../../octave/libinterp/octave-value/ov-java.cc:679:15: error: unknown type name 'process_execution_result'; did you mean 'octave::process_execution_result'?
              process_execution_result rslt
              ^~~~~~~~~~~~~~~~~~~~~~~~
              octave::process_execution_result
../../octave/libinterp/corefcn/oct-process.h:34:3: note: 'octave::process_execution_result' declared here
  process_execution_result
  ^
  CXX      libinterp/octave-value/liboctave_value_la-ov-re-diag.lo
1 error generated.
make[2]: *** [libinterp/octave-value/liboctave_value_la-ov-java.lo] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2


Here's a patch: file #46668

With the patch, build succeeds, and the dynamic JVM detection feature works.

Thanks!

Andrew Janke <apjanke>
Fri 29 Mar 2019 07:08:07 PM UTC, comment #20: 

I pushed the changes in two parts:

http://hg.savannah.gnu.org/hgweb/octave/rev/c04329ead503
http://hg.savannah.gnu.org/hgweb/octave/rev/2a57165f892c

Andrew: I modified some code inside an #ifdef block for _APPLE_ that I can't test so please try it and let me know if I introduced any dumb errors.

John W. Eaton <jwe>
Group administrator
Fri 29 Mar 2019 04:57:35 PM UTC, comment #19: 

@jwe: Now that 5.1.0 has been released, any reason not to apply the patch here to the development branch?

Rik <rik5>
Group administrator
Sat 12 Jan 2019 02:26:28 AM UTC, comment #18: 

Here's a new revision that I think addresses your concerns.

https://savannah.gnu.org/bugs/download.php?file_id=45940

- Files renamed to oct-process.{h,cc}
- Copyright statement in new files includes just me, since it's all new code
- Changed to use OCTINTERP_API and moved it out of the octave::sys namespace
- Re-using the existing get_jvm_lib_path_in_subdir() instead of adding a new function

The oct-process stuff needs to live in libinterp instead of liboctave because it depends on procstream.{h,cc}, which are in libinterp.


Andrew Janke <apjanke>
Fri 11 Jan 2019 04:35:41 AM UTC, comment #17: 


> it seems you are moving the run_command_and_return_output function that is currently static in toplev.cc to a separate file and putting it in the octave::sys namespace.
>
> I don't see where you are removing the old function.


No, run_command_and_return_output is a new function added in this patch; it's not moved from anywhere.

Would it be easier to read if I condensed this into a single changeset?

Andrew Janke <apjanke>
Wed 09 Jan 2019 12:53:01 PM UTC, comment #16: 

I haven't been following this development, but it seems you are moving the run_command_and_return_output function that is currently static in toplev.cc to a separate file and putting it in the octave::sys namespace.

I don't see where you are removing the old function.

I think this new code could also just go in the octave namespace.

oct-process.{h,cc} is probably OK.  I agree it should go in liboctave unless it uses something from libinterp.

For now, my advice on copyright is:

Since some of this code is coming from an existing file, it should probably have the same copyright as that file.  You are adding code, so you can also add a line for your name and use the current year.

Longer term, I may have different advice about how we should manage copyright notices in Octave, but that deserves a discussion among the developers somewhere else, not just in the comments of a bug report.

John W. Eaton <jwe>
Group administrator
Wed 09 Jan 2019 04:01:25 AM UTC, comment #15: 

Okay, I'll make revisions.

How about `oct-process.{cc,h}` for file names?

Should I put myself in the copyright notice in the new file? Or "John W. Eaton and others"?

Andrew Janke <apjanke>
Tue 08 Jan 2019 07:26:40 PM UTC, comment #14: 

This patch still has some significant problems and needs some more work before it can be pushed.

The first patch introduces a new class in the octave::sys namespace and OCTAVE_API, but installs it in libinterp. However, the entire octave::sys namespace currently lives under liboctave and we should keep it that way. Can the class live in liboctave? If it depends on other components in liboctinterp, then it has to stay in liboctinterp, and so it shouldn't be declared in the octave::sys namespace, and it should use OCTINTERP_API instead.

Further, the new files oct-proc.{cc,h} names are confusing, similar to oct-procbuf.{cc,h}. And the copyright statement lists jwe as the author, which isn't true as far as I can see.

For the second patch, have you looked at merging your new functionality with the "get_jvm_lib_path_in_subdir" function that has already been added and tested on other operating systems?

The file name "/usr/libexec/java_home" should be pulled out as a #define or a string constant instead of written twice.

Mike Miller <mtmiller>
Group Member
Tue 08 Jan 2019 04:35:41 AM UTC, comment #13: 

I've updated the patch to apply cleanly to trunk. Tested this new version locally with various JDK versions and it works for me.

Andrew Janke <apjanke>
Thu 06 Dec 2018 07:34:54 PM UTC, comment #12: 

I can't test it, but if you freshen the patch I can commit it.

Rik <rik5>
Group administrator
Thu 06 Dec 2018 10:51:06 AM UTC, comment #11: 

Hi folks. Any interest in pursuing this? If so, I'll redo the changeset so it merges cleanly against the current trunk, now that JAVA_HOME is supported.

Andrew Janke <apjanke>
Thu 12 Jul 2018 09:13:20 PM UTC, comment #10: 

Find a new file "dyn-jvm-detection-v02.tgz" attached which contains my changes exported as Mercurial changesets. I've confirmed the upload went through this time.

Andrew Janke <apjanke>
Thu 12 Jul 2018 07:54:15 PM UTC, comment #9: 

Yep, looks like I forgot to hit "Submit Changes" on the upload. I'll re-upload as soon as I'm back at my desktop that has my Octave working copy on it. Sorry.

Andrew Janke <apjanke>
Thu 12 Jul 2018 07:51:07 PM UTC, comment #8: 

@Andrew: In comment #7 you said that versions were attached as Mercurial changesets, but I still see only the .patch versions attached to this bug report.  Could you verify and re-upload if necessary?

Rik <rik5>
Group administrator
Tue 03 Jul 2018 09:37:39 AM UTC, comment #7: 

I've attached a new version of the patches, done as Mercurial changesets, to make them easier to use.

https://savannah.gnu.org/bugs/download.php?file_id=44482

Andrew Janke <apjanke>
Mon 02 Jul 2018 12:19:27 AM UTC, comment #6: 

Okay, I've made a new attempt, broken into three patches. This one keeps everything in libinterp to avoid major reorganization of the I/O classes.

  • dyn-jvm-v02-step-01-oct-proc.patch - adds the C++-level run_command_and_return_output function
  • dyn-jvm-v02-step-02-respect-JAVA_HOME.patch - adds JAVA_HOME support on macOS and Linux, taking priority over the compile-time detected path
  • dyn-jvm-v02-step-03-usr_libexec_java_home-on-macOS.patch - adds /usr/libexec/java_home support on macOS


The new logic handles various 'jre/lib', 'jre/lib/server', 'jre/lib/amd64', 'lib/client', etc variations. Same detection logic is used for paths under JAVA_HOME and /usr/libexec/java_home's output, so they're directly substitutable.

Andrew Janke <apjanke>
Sun 01 Jul 2018 11:26:09 PM UTC, comment #5: 


> I dunno. It's like five lines of C++ code; doesn't seem worth splitting it up for just that. I'll defer to your wishes, thought.


But it's not, if you allow libjvm.so or libjvm.dylib to exist under several different directory structures over different versions of Java. Here are all the possibilities on a current Debian system:

  • $JAVA_HOME/jre/lib/amd64/server/libjvm.so
  • $JAVA_HOME/jre/lib/i386/client/libjvm.so
  • $JAVA_HOME/jre/lib/i386/server/libjvm.so
  • $JAVA_HOME/lib/client/libjvm.so
  • $JAVA_HOME/lib/server/libjvm.so


Mike Miller <mtmiller>
Group Member
Sun 01 Jul 2018 11:21:27 PM UTC, comment #4: 

Maybe I'm just envious that no one has yet come up with a cross-distro standard way of declaring where the default Java installation is on GNU/Linux systems :)

If we keep the /usr/libexec/java_home feature, then I see this as (at least) three patches in a series:

  • reorganize the library for whatever changes are needed to get the text output of a subprocess
  • add code to detect JAVA_HOME in the environment and derive the path to libjvm.so or libjvm.dylib from that
  • add code to call /usr/libexec/java_home if it exists, and use the output in the same manner as JAVA_HOME
Mike Miller <mtmiller>
Group Member
Sun 01 Jul 2018 10:26:43 PM UTC, comment #3: 


> Combine this effort with bug #40111, meaning a patch or series of patches that addresses JAVA_HOME detection on all Unix and GNU systems?


That sounds good. I'll pull its considerations into my next patch.

> Use an m-file function callback to do the filesystem logic part of deriving the full file name of the libjvm.so/libjvm.dylib from JAVA_HOME? See for example the m-file code I posted at bug #40111.


I dunno. It's like five lines of C++ code; doesn't seem worth splitting it up for just that. I'll defer to your wishes, thought.

> Separate the library reorganization aspects / new functions necessary, if they are necessary, from the Java changes as distinct changes.


Sure. I can do this by just putting all the new run_command stuff into libinterp instead of liboctave. Will work up a revised patch today.

> What version of Java are you testing with? Does Java on macOS still have a "$JAVA_HOME/jre/lib" directory? On GNU/Linux that is now just "$JAVA_HOME/lib".



  • 1.8.0_171, x86_64: "Java SE 8" /Library/Java/JavaVirtualMachines/jdk1.8.0_171.jdk/Contents/Home
  • 1.8.0_141, x86_64: "Java SE 8" /Library/Java/JavaVirtualMachines/jdk1.8.0_141.jdk/Contents/Home
  • 1.7.0_79, x86_64: "Java SE 7" /Library/Java/JavaVirtualMachines/jdk1.7.0_79.jdk/Contents/Home


You're right; it changed in JDK 9 on macOS, too. Will add logic to cover that.

> Is /usr/libexec/java_home a hard-and-fast well-known name that will never change? Is there some other way to encode or configure the location of that helper command?


It's well-known: it's been around in that location since 2010, and it has a man page. As far as I know, it's never been in another location. I don't know of another way of locating that helper command; I've always just seen people use hardcoded paths to it.

> Should macOS users just have a system initialization octaverc file that calls that command and sets the JAVA_HOME environment variable?


Seems a little unfriendly to put the onus of configuring Java detection on the user, when Octave is capable of doing it itself. Especially because the versions of Java supported by Octave may change over time with different versions, so the user would have to do the work of managing the octave-home/share/octave/version/m/startup/octaverc file for each version.

Andrew Janke <apjanke>
Sun 01 Jul 2018 10:14:27 PM UTC, comment #2: 

I guess my preference would be to keep the call to /usr/libexec/java_home out of Octave, that seems far too specific.

But as part of the Homebrew packaging of Octave, you can install a site startup file, e.g. /usr/share/octave/site/m/startup/octaverc, that runs at startup for all users and uses /usr/libexec/java_home to populate JAVA_HOME, if the user hasn't already set it in their login environment.

Mike Miller <mtmiller>
Group Member
Sun 01 Jul 2018 09:47:19 PM UTC, comment #1: 

What do you think about the following ideas?

  • Combine this effort with bug #40111, meaning a patch or series of patches that addresses JAVA_HOME detection on all Unix and GNU systems?
  • Use an m-file function callback to do the filesystem logic part of deriving the full file name of the libjvm.so/libjvm.dylib from JAVA_HOME? See for example the m-file code I posted at bug #40111.
  • Separate the library reorganization aspects / new functions necessary, if they are necessary, from the Java changes as distinct changes.


What version of Java are you testing with? Does Java on macOS still have a "$JAVA_HOME/jre/lib" directory? On GNU/Linux that is now just "$JAVA_HOME/lib".

Is /usr/libexec/java_home a hard-and-fast well-known name that will never change? Is there some other way to encode or configure the location of that helper command? Should macOS users just have a system initialization octaverc file that calls that command and sets the JAVA_HOME environment variable?

Mike Miller <mtmiller>
Group Member
Sun 01 Jul 2018 01:22:38 PM UTC, original submission:  

It would be nice if Octave could dynamically detect the location of the JVM on macOS, instead of being stuck with the exact path detected at build time. This would let users upgrade their JDK without having to recompile Octave, and would support redistributable Octave builds that go to systems with various versions of the JDK installed.

Here is a patch to do so, using the now-standard '/usr/libexec/java_home' command, and respecting $JAVA_HOME.

Patch details:

Sorry about moving the I/O stuff around. However, it seems like the Right Way to do things if I'm going to introduce a C++-level run_command_and_return_output function that returns C++ objects instead of octave-values.

This patch:

  • Has ov-java.cc's initialize_jvm() detect the JVM location dynamically, using:

  * $JAVA_HOME if it is set
  *  '/usr/libexec/java_home' when no $JAVA_HOME is set and '/usr/libexec/java_home' is available
  * Falling back to the build-time detected path if neither of those works

  • Adds a C++ level run_command_and_return_output that works like the existing one in libinterp, but returns a C++ object instead of octave-values, and does not interact with the Octave frame
  • Moves that run_command_and_return_output's dependencies from libinterp to liboctave (since it seems that's where lower-level non-interpreter-interacting things like I/O and syscalls go)
  • Adds a conditional zlib dependency to liboctave, since some of the I/O functions require it


Sorry for any code style issues in here: I'm no C++ expert, and I'm relatively new to the Octave code base.

The crux of the code is this new section in ov-java.cc:


#  if defined (__APPLE__)
      // Use standard /usr/libexec/java_home if available
      if (jvm_lib_path.empty ())
        {
          octave::sys::file_stat libexec_java_home_exists ("/usr/libexec/java_home");
          if (libexec_java_home_exists)
            {
              octave::sys::process_execution_result rslt = octave::sys::run_command_and_return_output (
                  "/usr/libexec/java_home --failfast --version 1.6+ 2>/dev/null");
              if (rslt.exit_status () == 0)
                {
                  std::string output = rslt.stdout_output ();
                  std::string found_path = output.substr (0, output.length() - 1);
                  jvm_lib_path = found_path + "/jre/lib/server/libjvm.dylib";
                }
            }
        }
#  endif


This patch is against stable, because dev is not building for me right now.

I have some failing unit tests under this patch, but they are also failing for me when I build stable without this patch, so I don't know that they're a regression.

References:

Andrew Janke <apjanke>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #45940:  dyn-jvm-detection-v04.patch added by apjanke (10KiB - application/octet-stream - New revision of dynamic JVM detection)
file #45902:  dyn-jvm-detection-v03.tgz added by apjanke (4KiB - application/gzip - updated patch version)
file #44544:  dyn-jvm-detection-v02.tgz added by apjanke (4KiB - application/gzip)
file #44482:  dyn-jvm-v02-step-01-oct-proc.patch added by apjanke (6KiB - application/octet-stream - new attempt at dynamic JVM detection, broken into steps)
file #44483:  dyn-jvm-v02-step-03-usr_libexec_java_home-on-macOS.patch added by apjanke (2KiB - application/octet-stream - new attempt at dynamic JVM detection, broken into steps)
file #44484:  dyn-jvm-v02-step-02-respect-JAVA_HOME.patch added by apjanke (4KiB - application/octet-stream - new attempt at dynamic JVM detection, broken into steps)
file #44475:  dynamic-jvm-detection-01.patch added by apjanke (70KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by apjanke (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-05-30 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2019-03-29 apjanke Attached File- Added fix-build-failure-on-mac-after-dynamic-jvm-detection-change.patch, #46668
    2019-03-29 jwe StatusPatch Submitted Ready For Test
    2019-03-29 rik5 Release4.4.0 dev
    2019-01-12 apjanke Attached File- Added dyn-jvm-detection-v04.patch, #45940
    2019-01-08 mtmiller Carbon-CopyRemoved 80942 -
    2019-01-08 apjanke Attached File- Added dyn-jvm-detection-v03.tgz, #45902
    2018-07-12 apjanke Attached File- Added dyn-jvm-detection-v02.tgz, #44544
    2018-07-12 rik5 StatusNone Patch Submitted
    2018-07-02 apjanke Attached File- Added dyn-jvm-v02-step-01-oct-proc.patch, #44482
        Attached File- Added dyn-jvm-v02-step-03-usr_libexec_java_home-on-macOS.patch, #44483
        Attached File- Added dyn-jvm-v02-step-02-respect-JAVA_HOME.patch, #44484
    2018-07-01 apjanke Attached File- Added dynamic-jvm-detection-01.patch, #44475

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code