rosjava / genjava

RosJava message definition and serialization artifact generators.
5 stars 32 forks source link

Fix compile error: #4

Open k-okada opened 8 years ago

k-okada commented 8 years ago

genjava failed to compile in

1) if there is depends to message_generation but no message ware set, pr2_2dnav and pr2_2dnav_slam 2) if there is implict depends to message_generation

, which are reporeted on https://github.com/jsk-ros-pkg/jsk_visualization/issues/551#issuecomment-165996226

to check case 1) You can download https://github.com/PR2/pr2_navigation_apps package

to check case 2) you can check this with 1.0.27 of https://github.com/jsk-ros-pkg/jsk_visualization, or if you change 'message_generaion' to one of output of rospack depends-on message_generation, say 'nodelet'

wkentaro commented 8 years ago

Strictly speaking, I think case 2 is not correct, because it fails when compiling package which depends on message_generation explicitly but not implicitly. (With 'explicitly', I mean the package calls cmake macros like add_message_files ) http://answers.ros.org/question/186986/cmake-error-unknown-cmake-command-add_message_files/ So even if your change is merged, it fails when there is a package which does not implicitly depend on message_generation but explicitly. (I know this is quit rare, so your change does work in most cases)

In my opinion, genjava should show error message, when it finds a package which is not correctly configured. (like not listed message_generation as build_depend)

k-okada commented 8 years ago

I used explicitly as message_generation is listed in package.xml and implicitly mean some package listed in package.xml depends on message_genetation. I choose this word from https://github.com/ros-infrastructure/rospkg/blob/master/src/rospkg/rospack.py#L245

◉ Kei Okada

2016/01/16 14:35、Kentaro Wada notifications@github.com のメッセージ:

Strictly speaking, I think case 2 is not correct, because it fails when compiling package which depends on message_generation explicitly but not implicitly. (With 'explicitly', I mean the package calls cmake macros like add_message_files ) http://answers.ros.org/question/186986/cmake-error-unknown-cmake-command-add_message_files/ So even if your change is merged, it fails when there is a package which does not implicitly depend on message_generation but explicitly. (I know this is quit rare, so your change does work in most cases)

In my opinion, genjava should show error message, when it finds a package which is not correctly configured. (like not listed message_generation as build_depend)

— Reply to this email directly or view it on GitHub.

wkentaro commented 8 years ago

I see. In that case, the error is because of the not-well-configured package and the change in https://github.com/k-okada/genjava/commit/0634856f4020ee98b022bd2c7b77d04d629e151c is not necessary, no?

I think it should be a warning.

stonier commented 8 years ago

It's been quite some while since I've looked at/run this code, but I'll try to provide some useful input anyway.

For 1. the first commit looks like it will certainly make it more robust.

For 2. I would say that genjava should fail when there is only an implicit dependency (using the conventional meaning @k-okada is referring to, i.e. a build_depends listed not in its own package.xml but one of its dependencies) on message_generation. I agree with @wkentaro - it would be good to have a decent warning. However this should probably be built into the message generation cmake api at a higher level (I think).

I added some notes to the second commit.

k-okada commented 8 years ago

I agree to it is better to output warning message on that case, but other message generators except rosjava may not fall nor warn. So what we can do at message generator layer is just to pass both cases, I think.

◉ Kei Okada

furushchev commented 8 years ago

Any proceedings? There are still a lot of packages that cannot be built. Should we fix all malformed package.xml of packages or genjava?