RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.24k stars 1.25k forks source link

Can't construct drake LCM types from Matlab #2473

Closed rdeits closed 8 years ago

rdeits commented 8 years ago

On master, I'm no longer able to construct any of the Drake LCM types from Matlab (which uses the Java types internally):

>> drake.lcmt_foot_flag()
Undefined variable "drake" or class "drake.lcmt_foot_flag".
>> drake.lcmt_viewer_draw()
Undefined variable "drake" or class "drake.lcmt_viewer_draw".

I've verified that lcmtypes_drake.jar exists and is on matlab's javaclasspath. Is it possible that the source-1.6 changes could have broken this somehow?

rdeits commented 8 years ago

I'm on OSX 11.4, matlab 2015b, and javac -version gives 1.8.0_74

rdeits commented 8 years ago

My Ubuntu 14.04 machine appears to be working fine, though.

liangfok commented 8 years ago

Where can I find instructions on how to construct Drake LCM types from Matlab?

rdeits commented 8 years ago

I don't know of any instructions, but they are used in a variety of places, particularly within the BotVisualizer. You can see an example in https://github.com/RobotLocomotion/drake/blob/master/drake/systems/plants/BotVisualizer.m#L71

david-german-tri commented 8 years ago

Taking a look now. We run OS X Matlab tests on postsubmit, and it's green, so either this has no test coverage, CI is falsely reporting success, or the CI builder config doesn't match your machine.

david-german-tri commented 8 years ago

@rdeits, or anyone else in front of a Mac, could you post the results of jar tf install/share/java/lcmtypes_drake.jar?

liangfok commented 8 years ago

Here's what I see on my OS X machine:

$ jar tf build/share/java/lcmtypes_drake.jar
META-INF/
META-INF/MANIFEST.MF
drake/lcmt_body_motion_data.class
drake/lcmt_body_wrench_data.class
drake/lcmt_drake_signal.class
drake/lcmt_driving_command_t.class
drake/lcmt_euler_floating_joint_state_t.class
drake/lcmt_foot_flag.class
drake/lcmt_force_torque.class
drake/lcmt_joint_pd_override.class
drake/lcmt_piecewise_polynomial.class
drake/lcmt_polynomial.class
drake/lcmt_polynomial_matrix.class
drake/lcmt_qp_controller_input.class
drake/lcmt_quadrotor_input_t.class
drake/lcmt_quadrotor_output_t.class
drake/lcmt_robot_state.class
drake/lcmt_scope_data.class
drake/lcmt_simple_car_config_t.class
drake/lcmt_simple_car_state_t.class
drake/lcmt_simulation_command.class
drake/lcmt_support_data.class
drake/lcmt_viewer_command.class
drake/lcmt_viewer_draw.class
drake/lcmt_viewer_geometry_data.class
drake/lcmt_viewer_link_data.class
drake/lcmt_viewer_load_robot.class
drake/lcmt_whole_body_data.class
drake/lcmt_zmp_com_observer_state.class
drake/lcmt_zmp_data.class

Note that this is in my build directory instead of install directory. Not sure if this is significant.

rdeits commented 8 years ago

I also see:

± jar tf build/share/java/lcmtypes_drake.jar 
META-INF/
META-INF/MANIFEST.MF
drake/lcmt_body_motion_data.class
drake/lcmt_body_wrench_data.class
drake/lcmt_drake_signal.class
drake/lcmt_driving_command_t.class
drake/lcmt_euler_floating_joint_state_t.class
drake/lcmt_foot_flag.class
drake/lcmt_force_torque.class
drake/lcmt_joint_pd_override.class
drake/lcmt_piecewise_polynomial.class
drake/lcmt_polynomial.class
drake/lcmt_polynomial_matrix.class
drake/lcmt_qp_controller_input.class
drake/lcmt_quadrotor_input_t.class
drake/lcmt_quadrotor_output_t.class
drake/lcmt_robot_state.class
drake/lcmt_scope_data.class
drake/lcmt_simple_car_config_t.class
drake/lcmt_simple_car_state_t.class
drake/lcmt_simulation_command.class
drake/lcmt_support_data.class
drake/lcmt_viewer_command.class
drake/lcmt_viewer_draw.class
drake/lcmt_viewer_geometry_data.class
drake/lcmt_viewer_link_data.class
drake/lcmt_viewer_load_robot.class
drake/lcmt_whole_body_data.class
drake/lcmt_zmp_com_observer_state.class
drake/lcmt_zmp_data.class
david-german-tri commented 8 years ago

Huh. lcmt_foot_flag is there, which bolsters the source-1.6 hypothesis. I'll try to reproduce when I next have a Mac handy (probably tomorrow).

david-german-tri commented 8 years ago

From @RussTedrake on #2508 (duplicate):

The specific failure for me was that I could not communicate via lcm to the BotVisualizer. The build servers do not have adequate coverage of this case. We should consider removing the lines about visualizers from here: https://github.com/RobotLocomotion/drake/blob/master/drake/CMakeLists.txt#L189

@rdeits points out that adding such coverage would resolve #1256.

jamiesnape commented 8 years ago

The issue is more likely that the version of Java used with MATLAB is different and incompatible with the one that is used to compile the other code. Adding -source 1.6 is not really the solution. For example, -source 1.7 would probably work for MATLAB 2015b. If I recall, there is something like a MATLAB_JAVA variable you can set to a different JDK.

david-german-tri commented 8 years ago

https://groups.google.com/d/topic/lcm-users/bjjie-0rxcc/discussion

@rdeits, @liangfok, and/or @RussTedrake - would you mind running version -java in MATLAB when you get a chance, and posting the results here? On R2016a/Ubuntu, I get:

Java 1.7.0_60-b19 with Oracle Corporation Java HotSpot(TM) 64-Bit Server VM mixed mode

... which probably explains why it's happy.

liangfok commented 8 years ago

Here's what I see using OS X 10.11.5 and Matlab R2016a:

>> version -java

ans =

Java 1.7.0_75-b13 with Oracle Corporation Java HotSpot(TM) 64-Bit Server VM mixed mode
david-german-tri commented 8 years ago

@liangfok - thanks! I guess MathWorks upgraded to Java 7 on OS X in R2016a.

Here are some options, none of which I like. Anyone have better ideas?

  1. Tell users to install a modern JDK, but use the -source flag to forcibly downgrade javac to emit bytecode that our supported version of MATLAB can accept. This is brittle because we're compiling the JDK with a possibly-incompatible compiler. However, we can probably expect Oracle to minimize the breakage in practice. (In effect, this option is "revert #2404".)
  2. Tell users to force MATLAB to use a modern JVM with the MATLAB_JAVA flag. This is brittle because MathWorks expressly does not backport JVM compatibility to older versions of MATLAB, although again, we can probably expect Oracle to minimize the breakage in practice.
  3. Tell users to install an obsolete JDK that matches their version of MATLAB. This is unpleasant for users because they might have other software that wants a modern JDK.
  4. Make our supported version of MATLAB track the modern JDK as closely as possible. This is also unpleasant to users who don't want to buy new MATLAB licenses that frequently.
sherm1 commented 8 years ago

version -java in R2016a on Windows 10: Java 1.7.0_60-b19 with Oracle Corporation Java HotSpot(TM) 64-Bit Server VM mixed mode

jwnimmer-tri commented 8 years ago

Choice 3 seems like an appropriate level of "you break it you bought it". If you prefer to use older-matlab, you have to install the corresponding older-jdk.

Also, now that we know that the matlab version substantially matters (recent releases are not interchangeable), we need to explicitly list which version(s) of matlab we support in the developers.rst matrix, get them under jenkins, and allow ourselves to support as best-effort-only the out-of-matrix varieties.

jamiesnape commented 8 years ago

For completeness, the fifth choice, which is probably the most unpalatable is installing two JDKs and setting both the -source and -bootstrap flags. Basically, no one does it, because you usually may as well just use the same compiler and libraries from a single JDK to compile everything.

Installing 1.7 on all CI configurations is going to be easiest. Installing 1.6 is not going to be possible without nasty side-effects as Jenkins itself uses Java to connect to slaves and 1.6 has security issues. The jump from 1.7 to 1.8 was a bigger change than 1.6 to 1.7 if I recall correctly.

david-german-tri commented 8 years ago

EDIT: This proposal has been overtaken by events. Read on.

OK, here's a proposal. Comments and passive votes (:+1:, :-1:) welcome! If this plan gets buy-in, I'll farm it out into separate issues.

jwnimmer-tri commented 8 years ago

We should provide a CMake option (off by default) to set -source to an arbitrary value ...

Instead of adding more knobs that must be set to one specific thing or else explosions, wouldn't it be more robust to have our CMake script interrogate the matlab version, and then configure the (super)build java options to match that?

jamiesnape commented 8 years ago

This means upgrading to R2016a on CI

We had to request physical DVDs from MathWorks before, so it will take it a while (possibly a very long while) for this to happen.

rdeits commented 8 years ago

With Matlab R2015b on OSX 10.11.4 I see:

Java 1.7.0_75-b13 with Oracle Corporation Java HotSpot(TM) 64-Bit Server VM mixed mode
RussTedrake commented 8 years ago

installing new versions of matlab all the time is a pain. i would like to not force it on myself (let alone the world) too often. matlab ships the version of jdk that they need to run. i don't understand the problem with setting a -source (and possibly -bootstrap) flag to make my newer jdk backwards compatible? agreed that we could potentially check the matlab jdk version and only do that if necessary...

jamiesnape commented 8 years ago

To use -bootstrap you need to install the JDK anyway so you may as well compile with the compiler that ships. The flag -source only adjusts the language settings. If you analyzed the byte code you would see fragments of packages that ship with JDK 1.8 and that is not a good thing at all.

jamiesnape commented 8 years ago

My suggestion would be to stick with MATLAB 2015b and specify that (only) JDK 1.7 should be installed. We could also say that if a developer uses a different version of MATLAB, it is their responsibility to install a matching JDK.

david-german-tri commented 8 years ago

Based on @rdeits version -java output above, I'm no longer confident we've even root-caused the problem. MATLAB 2015b is using Java 1.7 on his machine, so why would building the Drake jarfiles with -source 1.6 be necessary?

Robin, just for completeness, could I trouble you to run javac -version? EDIT: Never mind, you already did.

jamiesnape commented 8 years ago

He is using JDK 1.8, which is incompatible with the JRE 1.7 used by MATLAB. See the second comment.

david-german-tri commented 8 years ago

@jamiesnape Ah, thanks! According to the javac 1.8 docs, -source 1.6 will still imply -target 1.8. However, #2404 also removed -target 1.6, and that's the real root cause. @rdeits and @RussTedrake are getting bytecode that targets JRE 8, and feeding it to MATLAB that is running JRE 7. This problem doesn't crop up on Ubuntu 14.04 because it has javac 1.7.0_101.

Kudos to @jwnimmer-tri for catching this.

david-german-tri commented 8 years ago

So, revised proposal. Let's have the superbuild do something like this:

matlab -noawt -nodesktop -nosplash -r "version -java,quit" | grep Java | awk '{print $2}'

If there's a discrepancy with javac -version in the release digits, we can emit a warning and set -source and -target to match the MATLAB JRE. @jamiesnape, does that sound reasonable to you?

jamiesnape commented 8 years ago

Sure. Sounds good.

billhoffman commented 8 years ago

Should be able to look at the code for the matlab tests for how to run matlab and capture output on all platforms from cmake execute_process.

RussTedrake commented 8 years ago

setting -target seems like the simplest solution by far!

jwnimmer-tri commented 8 years ago

In the interim, putting back -target 1.6 unconditionally (partially reverting 2024) may be worthwhile. (Possibly it should be -target 1.7 -- I really hope nobody is running JRE 6 anymore!) That may even be a fine medium-term solution, without needing to scrape matlab for its version.

jamiesnape commented 8 years ago

I should have the fix in by the end of the day, so you may wish to wait. I do not necessarily disagree with just setting always -target 1.7, but I am mostly done with getting the output from MATLAB, so I may as well test that out.