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

Splitted rosidl_generator_c and rosidl_generator_cpp in two: rosidl_generator_x and rosidl_runtime_x #442

Merged
merged 26 commits into from
Apr 10, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Feb 26, 2020

As discussed in the issue ros2/ros2#442 I have spplited in to rosidl_generator_cand rosidl_generator_cpp.

I don't really know if rosidl_runtime_cpp is needed. because this package is not creating any runtime dependency such as a .so/dylib/dll. But I have seen this comment https://github.com/ros2/rosidl/blob/master/rosidl_generator_cpp/include/rosidl_generator_cpp/message_initialization.hpp#L18 from @clalancette and I dediced to splitt it too, also because I think it makes sense for consistency.

…enerator_x and rosidl_runtime_x

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Feb 26, 2020
@ahcorde ahcorde self-assigned this Feb 26, 2020
rosidl_generator_c/package.xml Outdated Show resolved Hide resolved
rosidl_generator_c/CMakeLists.txt Show resolved Hide resolved
rosidl_generator_c/resource/action__type_support.h.em Outdated Show resolved Hide resolved
rosidl_generator_c/test/test_interfaces.c Outdated Show resolved Hide resolved
rosidl_generator_cpp/CMakeLists.txt Outdated Show resolved Hide resolved
rosidl_runtime_cpp/package.xml Outdated Show resolved Hide resolved
rosidl_runtime_cpp/package.xml Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/feature/splitted_rosidl_generator branch from 6810e75 to e533e72 Compare February 26, 2020 17:59
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/feature/splitted_rosidl_generator branch from e533e72 to 6433657 Compare February 26, 2020 18:02
@ahcorde ahcorde requested a review from dirk-thomas March 3, 2020 12:43
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/feature/splitted_rosidl_generator branch from 643135b to c76cd28 Compare March 3, 2020 19:02
@dirk-thomas
Copy link
Member

Commonly a package only provides headers with a namespace / directory matching the package name. Should we consider updating the namespace / directory of the moved headers? If yes, how much code would be affected by this breaking change?

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 3, 2020

I was trying to do so, but this will require a lot of changes in many packages. I was trying to reduce the amount of noise regarding this change.

Also the resources are in the generator but the headers are in the runtime. This will require also some changes in the resources.

but if this is the way I can put some time on it

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can land the PR as is. I was mostly wondering if it makes sense / if we want to follow up on the naming afterwards.

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 3, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows-container Build Status

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 30, 2020

Removed rosidl_generator_c and rosidl_generator_cpp from rosidl_typesupport_introspection_c and rosidl_typesupport_introspection_cpp because these packages are not generators and they are not making use of the CMakeLists macros

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@rotu
Copy link

rotu commented Apr 8, 2020

This breaks binary compatibility in its public API. Shouldn't it bump the major version number?

@wjwwood
Copy link
Member

wjwwood commented Apr 8, 2020

This breaks binary compatibility in its public API. Shouldn't it bump the major version number?

We will, I assume, when we next release from master.

@dirk-thomas
Copy link
Member

This breaks binary compatibility in its public API. Shouldn't it bump the major version number?

Since the packages in this repo have 0.x versions there is no need to bump the major version. Anything can change anytime in pre 1.x versions.

Independent of this change the goal is to bring several repos / packages to at least version 1.0 for the Foxy release.

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 10, 2020

  • Updated all repos involved in this change.
  • Skipping ros1_bridge, qt_*, rqt_*, and rviz_* packages.
  • Testing against test_rclcpp and rosbag2_tests.
  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • One warning made the builds unstable but there is a (PR pending)
CMake Deprecation Warning at /home/jenkins-agent/workspace/ci_linux-aarch64/ws/install/ament_cmake_export_interfaces/share/ament_cmake_export_interfaces/cmake/ament_export_interfaces.cmake:37 (message):
ament_export_interfaces() is deprecated, use ament_export_targets() instead
Call Stack (most recent call first):
CMakeLists.txt:85 (ament_export_interfaces)

In MacOS

  • projectroot.cppcheck

Windows

  • some failures in test_rclcpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress Actively being worked on (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants