orocos-toolchain / orogen

Code generator for components and type handling in Rock - the Robot Construction Kit - and the Orocos Toolchain
http://rock-robotics.org
Other
4 stars 35 forks source link

Fix using_task_library dependency resolution #145

Closed hemker closed 2 years ago

hemker commented 3 years ago

When using a "separate prefixes" build configuration, included headers from a "using_task_library" dependency were not found. I am not 100% sure whether my fix is totally correct to fix the problem, but as far is I understood the used_task_libraries member of the project was never used to generate the task's dependencies in that specific case, so I added them.

doudou commented 3 years ago

Hi @hemker

Could you maybe give us a specific error you are getting ? I am not sure what`s the actual issue.

Just to frame the discussion: what the current code is doing is only include task libraries that are actually used (the two lines before your change) - whether this is a good thing is arguable.

hemker commented 3 years ago

Hey @doudou I just provided a test case to reproduce the problem. In the test module dependent_includes you can find the include #include <cross_consumer/TaskStates.hpp>. The provided test case fails (because the header is not found) without having the extra line in project.rb.

doudou commented 3 years ago

So, I really can`t find a good reason why your change would be bad. And there's definitely a good reason for it...

Just implement the one small change in my review, and create the test package(s) separately. Name them over what they are testing, for instance build_tests/orogen/using_task_library-not_picking_up_not_directly_used_dependencies-publisher and build_tests/orogen/using_task_library-not_picking_up_not_directly_used_dependencies-consumer. And refer to the bug in the buildconf commit.

hemker commented 3 years ago

@doudou Did all required steps.

hemker commented 3 years ago

@doudou Still anything to do here?

doudou commented 2 years ago

My apologies @hemker ... I'm on it now