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

Move typekits back to https://github.com/orocos/rtt_common_msgs #96

Open
wants to merge 4 commits into
base: toolchain-2.9
Choose a base branch
from

Conversation

meyerj
Copy link
Member

@meyerj meyerj commented Dec 12, 2017

The rtt_common_msgs repository has been merged into rtt_ros_integration in #6 and is unused since toolchain-2.7 and ROS hydro.

While it is still true that some packages and tests in this repo depend on typekit packages, they are very rarely updated and can be skipped for most releases of rtt_ros_integration. This would save some time for doing the ROS releases with bloom and resources on the ROS build farm.

For source builds both repositories have to be added to the workspace because of circular dependencies.

…msgs

This repository has been merged into rtt_ros_integration in #6.
While it is still true that some packages and tests in this repo depend on
typekit packages, they are very rarely updated and can be skipped for most
releases of rtt_ros_integration. This saves some time for doing the releases
and resources on the ROS build farm.

For source builds both repositories have to be added to the workspace because
of circular dependencies.
@meyerj meyerj added this to the 2.9 milestone Dec 12, 2017
@meyerj meyerj mentioned this pull request Mar 1, 2018
@spd-intermodalics
Copy link
Contributor

spd-intermodalics commented Oct 5, 2020

Is the package rtt_common_msgs distributed also through ROS realeases? I haven't seen it in: https://github.com/orocos-gbp
Otherwise it could be that removing these typekits may break someone's project.

Edit: of course, when added as a submodule, they will anyway be distributed under rtt_ros_integration.
But then, this PR can be merged.

…s-back-to-rtt_common_msgs

typekits/rtt_std_msgs/include/orocos/std_msgs/vector_multi_array_adapter.h
@spd-intermodalics spd-intermodalics force-pushed the move-typekits-back-to-rtt_common_msgs branch from d93a84d to a4fbe1f Compare October 5, 2020 16:37
@meyerj
Copy link
Member Author

meyerj commented Oct 6, 2020

In any case, before merging this PR and switching back to rtt_common_msgs, some newer changes from the typekits/ subdirectory in this repository would have to be applied to rtt_common_msgs.

Is the package rtt_common_msgs distributed also through ROS realeases? I haven't seen it in: https://github.com/orocos-gbp
Otherwise it could be that removing these typekits may break someone's project.

With bloom releases are at repository level, not for individual packages. So the package rtt_common_msgs is released as part of https://github.com/orocos-gbp/rtt_ros_integration-release at the moment. That is the whole reason why I suggested to move the typekit packages back to their own repository.

Do not confuse the metapackage rtt_common_msgs with the repository rtt_common_msgs. It is just an old convention in ROS that repositories have a metapackage, formerly called a "stack", of the same name, that depends on all the packages in the repository. But it is not strictly needed nowadays, and people typically install dependencies at the package level by either relying on the system package manager (e.g. apt) or using tools like rosdep for source builds. In my opinion there is no need anymore for a rtt_common_msgs metapackage, because most people never need rtt_stereo_msgs, for example, and typekit packages are not light-weight as it is the case for the stereo_msgs package itself. Removing a metapackage can also not break someone's project because it is forbidden to depend on it anyway.

Edit: of course, when added as a submodule, they will anyway be distributed under rtt_ros_integration.
But then, this PR can be merged.

Bloom normally also clones submodules (ros-infrastructure/bloom#217) and would include rtt_common_msgs again when releasing rtt_ros_integration if it was a submodule, so that defeats the purpose of this suggestion. Maybe there is a way to exclude submodules during the import step, or we could add a CATKIN_IGNORE marker in the same way as we already do for the tests/ subdirectory, to ignore them for the release. But then there is still a chicken-and-egg problem for the first release into a new ROS distro, because both repositories have missing dependencies that cannot be satisfied without the other.

For source builds both repositories have to be added to the workspace because of circular dependencies.

That is still the biggest unresolved problem. Some packages in this repository depend on typekit packages which would be moved to rtt_common_msgs, and of course the typekit packages all depend on rtt_roscomm, causing a circular dependency at the repo level. The CI job on Travis therefore installs rtt_roscomm from binaries to satisfy the dependencies of the upstream workspace rtt_common_msgs which is not what we want to have tested and can have side effects. The cleanest solution would be to rewrite unit tests and use a custom test message and typekit package instead of rtt_std_msgs and rtt_geometry_msgs, and to also move support packages like rtt_tf, rtt_dynamic_reconfigure and rtt_actionlib to their own repositories that can be tested and released independently.

But as all this would be a quite disruptive change for everyone who builds rtt_ros_integration from source at the moment, with only little advantages for the release process. So I would rather recommend to leave it as-is, and to archive the rtt_common_msgs repository, unless someone has a better suggestion.

Future, non-essential packages should be added in their own repository instead of here, ideally one repository per package.

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.

2 participants