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

rename rosidl_generator_cpp namespace to rosidl_runtime_cpp #456

Merged
merged 2 commits into from
Apr 10, 2020

Conversation

dirk-thomas
Copy link
Member

Follow up of #442 as part of #447 to match namespace with the package the name the symbols are defined in.

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
Copy link
Member Author

Only Linux / FastRTPS with linter tests since the change shouldn't have any runtime impact (if it compiles successfully): Build Status

Since I screwed up the arguments the linter tests didn't run. Since I need to make several more rounds of PRs for all the rosidl_generator_c/cpp API review follow ups I will go ahead and merge this (and fix linter warnings if they appear).

@dirk-thomas dirk-thomas merged commit 4c76601 into master Apr 10, 2020
@delete-merged-branch delete-merged-branch bot deleted the dirk-thomas/rename-namespaces branch April 10, 2020 22:55
dirk-thomas added a commit that referenced this pull request Apr 10, 2020
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
dirk-thomas added a commit that referenced this pull request Apr 10, 2020
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
Copy link
Member Author

I accidentally squash merged the PR. Since the intention was to keep the two commits separate (to maintain history) I force pushed to master to keep them separate.

@Karsten1987
Copy link
Contributor

I just updated my workspace with vcs pull and run into the following errors:

--- stderr: rosidl_runtime_c
/Users/karsten/workspace/ros2/ros2_master/src/ros2/rosidl/rosidl_runtime_c/src/service_type_support.c:15:10: fatal error: 'rosidl_generator_c/service_type_support_struct.h' file not found
#include "rosidl_generator_c/service_type_support_struct.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/karsten/workspace/ros2/ros2_master/src/ros2/rosidl/rosidl_runtime_c/src/message_bounds.c:15:10: fatal error: 'rosidl_generator_c/message_bounds_struct.h' file not found
#include "rosidl_generator_c/message_bounds_struct.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/karsten/workspace/ros2/ros2_master/src/ros2/rosidl/rosidl_runtime_c/src/message_type_support.c:15:10: fatal error: 'rosidl_generator_c/message_type_support_struct.h' file not found
/Users/karsten/workspace/ros2/ros2_master/src/ros2/rosidl/rosidl_runtime_c/src/u16string_functions.c:15:10: fatal error: 'rosidl_generator_c/u16string_functions.h' file not found
#include "rosidl_generator_c/message_type_support_struct.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#include "rosidl_generator_c/u16string_functions.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/karsten/workspace/ros2/ros2_master/src/ros2/rosidl/rosidl_runtime_c/src/string_functions.c:15:10: fatal error: 'rosidl_generator_c/string_functions.h' file not found
#include "rosidl_generator_c/string_functions.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

am I missing something here? I tried compiling with --packages-up-to rosbag2 --cmake-clean-cache

@dirk-thomas
Copy link
Member Author

You might want to check that you have the latest master of this repo as well as all the referenced PRs. If you use a branch on any of them you will likely need to rebase it.

@emersonknapp
Copy link
Collaborator

emersonknapp commented Apr 10, 2020

The build is currently failing on a fresh checkout of master from this PR - https://ci.ros2.org/view/All/job/ci_linux/10100/console

Edit 1 for context: Built up to rosidl_generator_c - it seems to require some headers from rosidl_runtime_c but the dependency direction is in reverse

Edit 2: but i don't fully understand why this change would cause the breakage, it doesn't seem to have touched rosidl_generator_c / rosidl_runtime_c

@Karsten1987
Copy link
Contributor

I don't fully grasp the dependencies here:

rosidl_generator_c is depending on rosidl_runtime_c: https://github.com/ros2/rosidl/blob/master/rosidl_generator_c/CMakeLists.txt#L16
But then, rosidl_runtime_c is using headers from rosidl_generator_c: https://github.com/ros2/rosidl/blob/master/rosidl_runtime_c/src/service_type_support.c#L15

dirk-thomas added a commit that referenced this pull request Apr 11, 2020
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
dirk-thomas added a commit that referenced this pull request Apr 11, 2020
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
Copy link
Member Author

dirk-thomas commented Apr 11, 2020

I accidentally squash merged the PR. Since the intention was to keep the two commits separate (to maintain history) I force pushed to master to keep them separate.

This was the actual problem - I was already working on the follow up patch to move the rosidl_generator_c headers and some changes from that slipped in the force push.

I have fixed the problem with the two recently referenced commits which only touch rosidl_generator_cpp as intended. See Build Status (which is a rebuild of the minimal build @emersonknapp started above).

Sorry for the hassle (it didn't help that I went for a bike ride after force push...).

@dirk-thomas
Copy link
Member Author

I don't fully grasp the dependencies here:
rosidl_generator_c is depending on rosidl_runtime_c: https://github.com/ros2/rosidl/blob/master/rosidl_generator_c/CMakeLists.txt#L16
But then, rosidl_runtime_c is using headers from rosidl_generator_c: https://github.com/ros2/rosidl/blob/master/rosidl_runtime_c/src/service_type_support.c#L15

The headers have been moved in #442 but the directory / namespace has not been updated. This PR does exactly that - update the namespace to match the package name - for the C++ generator / runtime. A similar PR for C will need to follow. All part of the API review in #446 / #447.

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.

None yet

4 participants