Skip to content
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

Open
wants to merge 1 commit into
base: toolchain-2.9
Choose a base branch
from

Conversation

danieto98
Copy link

@danieto98 danieto98 commented Jun 12, 2024

  • 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).

Closes #173

* 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).
Copy link
Member

@meyerj meyerj left a 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)
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rtt_roscomm: There is no dependency handling when generating typekits
2 participants