Skip to content
This repository has been archived by the owner on Jan 14, 2023. It is now read-only.

Fix compile error: #4

Open
wants to merge 2 commits into
base: indigo
Choose a base branch
from
Open

Conversation

k-okada
Copy link

@k-okada k-okada commented Jan 16, 2016

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 jsk-ros-pkg/jsk_visualization#551 (comment)

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'

  • gradle_project.py: run genjava if the package depends on message_generation/genmsg implicitly
  • genjava-extras.camke.em: do not run genjava when three is no message to build

@wkentaro
Copy link

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
Copy link
Author

k-okada commented Jan 16, 2016

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 [email protected] のメッセージ:

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
Copy link

I see.
In that case, the error is because of the not-well-configured package and the change in k-okada@0634856 is not necessary, no?

I think it should be a warning.

@stonier
Copy link
Contributor

stonier commented Jan 23, 2016

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
Copy link
Author

k-okada commented Jan 25, 2016

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
Copy link

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

fmessmer added a commit to fmessmer/genjava that referenced this pull request Nov 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants