-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rtt_roscomm: generate dependency typekits #174
base: toolchain-2.9
Are you sure you want to change the base?
Conversation
* When generating typekits, also generate them for all packages that the current package depends on for msg generation. * Pass dependencies onto targets correctly so they are generated in the right order. * Don't generate typekits or service proxies if they have already been generated (if there's a corresponding target or if the corresponding rtt_<pkg> already exists).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution and for suggesting a patch already to resolve #173!
The way rtt_roscomm
was intended to be used by the original authors was with the create_rtt_msgs
script as documented here, and one rtt_foo_msgs
package per foo_msgs
package, where the rtt_bar_msgs
created by that script also declares a dependency on rtt_foo_msgs
if bar_msgs
depends on foo_msgs
. So the dependency handling is in fact available, but not in CMake code. The typekit packages must be created manually once, one per ROS package with message and/or service definitions. The build tool (e.g. catkin) is then able to build the typekit packages in the correct order, following their build dependencies topology. Updated dependencies of the underlying message packages would then require a recreation of the respective typekit package, too, or manual updates.
I am not sure anymore, but I don't think any build time dependencies are required at all, also not for generated headers? So the generated Boost header for bar_msgs
does not include the generated header for foo_msgs
, nor need the resulting libraries to be linked to each other. As a consequence, the rtt_foo_msgs
and rtt_bag_msgs
packages can be built in any order and do not depend on the artifacts of the other. Or not at all. Only at run-time, when loaded into an Orocos process, certain features may not function properly if no typekit is available for nested types.
In my opinion we should not allow a call to macro ros_generate_rtt_typekit(<package>)
that may implicitly includes typekit or service proxy plugins for other packages, only because they cannot be found at the time CMake is invoked. A catkin workspace can be built multiple times and should support selective rebuilding if only one file, for example a message definition, changes. With your suggested change, the outcome of a successful build would be that afterwards all rtt_${package}
exist, and hence after another CMake invocation without a full clean the targets will effectively be disabled and never updated anymore?
As an alternative to the "one package - one typekit package" paradigm implied by create_rtt_msgs
, what should work is to manually create a typekit package for a collection of maybe related ROS message packages, by calling the ros_generate_rtt_typekit(<package>)
macro multiple times in the same package-level CMakeLists.txt
file. At least that did work once with catkin_make
and a single build directory for all packages in a workspace, which effectively results in a single top-level CMakeLists.txt
at workspace level that descends into packages like subdirectories in their dependency order. Maybe some add_dependencies()
calls are missing at the moment indeed to support those macro calls in any order within a single package, such that each typekit library targets has a dependency on the *-generate_boost_headers
targets for each of its dependencies, even if empty in this package.
What could be done and accepted for being merged is an additional macro with the behavior intended by this pull request, like ros_generate_rtt_typekits_recursively(<package>)
, but without changing ros_generate_rtt_typekit(<package>)
. Still, the criteria when to abort the recursion should be more explicit in my opinion, and cannot depend on whether the workspace was already built once before or not.
Another flaw with this approach is that if a typekit library for foo_msgs
is provided by a package rtt_bar_msgs
, because bar_msgs
dependes on foo_msgs
and package rtt_foo_msgs
is not found, then another package's typekit library that also depends on foo_msgs
would again include the rtt_foo_msgs
library, because find_package(rtt_foo_msgs)
would still fail and there is no such package.
set(_package ${package}) | ||
add_subdirectory(${rtt_roscomm_TEMPLATES_DIR}/typekit ${package}_typekit) | ||
|
||
find_package(rtt_${package} QUIET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach would prevent that the typekit is rebuilt in an existing workspace after it has been built once, for example after one message definition was updated?
Closes #173