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.

Sign up for GitHub

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

Jump to bottom

export find_ros1_package cmake #164

Merged
merged 7 commits into from
Feb 27, 2019
Merged

export find_ros1_package cmake #164

merged 7 commits into from
Feb 27, 2019

Conversation

Karsten1987
Copy link
Contributor

Signed-off-by: Karsten Knese karsten@openrobotics.org

I believe this is the correct fix for exporting the find_ros1_package.cmake. This allows other packages to use this macro straight after finding the ros1_bridge` package.
see: ros2/rosbag2#90 (comment)

please advise how to test this patch. Also I would appreciate if this could be backported to crystal.

@Karsten1987 Karsten1987 self-assigned this Feb 13, 2019
@Karsten1987 Karsten1987 added the in progress Actively being worked on (Kanban column) label Feb 13, 2019
@Karsten1987
export find_ros1_package cmake
2c5ff62 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@dirk-thomas
Copy link
Member

please advise how to test this patch.

You can test it with the referenced rosbag2 PR which shouldn't need the include calls anymore.

@Karsten1987
Copy link
Contributor Author

in order to compile these packages, the ros1_bridge has to be compiled. Which CI job could do that?

@dirk-thomas
Copy link
Member

https://ci.ros2.org/job/ci_packaging_linux/

@Karsten1987
export pkg-config dependency
81c939d 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Contributor Author

In order to use the cmake macros, the pkg-config dependency has to be exported with it.
I am not sure, but when interpreting rep140 correctly, the pkg-config dependency should be exported via build_export_depend. Please advise if not.

@Karsten1987 Karsten1987 mentioned this pull request Feb 14, 2019
Merged
@dirk-thomas
Copy link
Member

buildtool_export_depend - "tool" because pkg-config needs to be present on the host system rather than on the target system in case of cross compilation.

This was referenced Feb 19, 2019
Merged
Closed
@Karsten1987
use buildtool_export_depend
5e02ce1 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987 Karsten1987 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 22, 2019
@Karsten1987
Copy link
Contributor Author

comments are addressed and CI was setup here: Build Status

putting this in review

dirk-thomas
dirk-thomas reviewed Feb 22, 2019
View reviewed changes
cmake/find_ros1_package.cmake Outdated
# call ament_package() to avoid ament_tools treating this
# as a plain CMake pkg
ament_package()
return()
Copy link
Member

Choose a reason for hiding this comment

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

This CMake extra file can't call ament_package(). Instead the find_package(PkgConfig) call should be REQUIRED.

@Karsten1987
make pkg-config required
d5e2f02 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
also export cmake macros if ros1 is not found
4eb0929 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Contributor Author

new CI: Build Status

dirk-thomas
dirk-thomas reviewed Feb 22, 2019
View reviewed changes
CMakeLists.txt Outdated
@@ -69,7 +71,9 @@ ament_export_libraries(${PROJECT_NAME})

ament_python_install_package(${PROJECT_NAME})

ament_package()
ament_package(
CONFIG_EXTRAS cmake/find_ros1_package.cmake cmake/find_ros1_interface_packages.cmake
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: maybe move the duplicated arguments into a variable?

@Karsten1987 Karsten1987 mentioned this pull request Feb 23, 2019
Merged
@paulbovbel

This comment has been minimized.

Sign in to view
@dirk-thomas

This comment has been minimized.

Sign in to view
@Karsten1987
set cmake files in variable
0769a4f 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
dirk-thomas
dirk-thomas reviewed Feb 26, 2019
View reviewed changes
CMakeLists.txt Outdated
@@ -18,6 +18,7 @@ find_package(rmw_implementation_cmake REQUIRED)
find_package(std_msgs REQUIRED)

# find ROS 1 packages
list(APPEND ros1_cmake_macros cmake/find_ros1_package.cmake cmake/find_ros1_interface_packages.cmake)
Copy link
Member

Choose a reason for hiding this comment

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

Since the variable ros1_cmake_macros doesn't exist yet use set() instead. Based on the usage I would also suggest to name the variable cmake_extra_files.

@Karsten1987
use set instead of list
587ab84 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Contributor Author

latest CI: Build Status

merging :)

@Karsten1987 Karsten1987 merged commit 70c3735 into master Feb 27, 2019
@Karsten1987 Karsten1987 removed the in review Waiting for review (Kanban column) label Feb 27, 2019
@Karsten1987 Karsten1987 deleted the export_cmake_file branch February 27, 2019 00:37
@Karsten1987 Karsten1987 mentioned this pull request Mar 4, 2019
Closed
Karsten1987 added a commit that referenced this pull request Mar 7, 2019
@Karsten1987
export find_ros1_package cmake (#164)
c9fb349 
* export find_ros1_package cmake

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* export pkg-config dependency

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* use buildtool_export_depend

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* make pkg-config required

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* also export cmake macros if ros1 is not found

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* set cmake files in variable

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* use set instead of list

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Karsten1987 added a commit that referenced this pull request Mar 11, 2019
@Karsten1987
export find_ros1_package cmake (#164) (#172)
cc3316d 
* export find_ros1_package cmake

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* export pkg-config dependency

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* use buildtool_export_depend

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* make pkg-config required

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* also export cmake macros if ros1 is not found

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* set cmake files in variable

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* use set instead of list

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
dhananjaysathe pushed a commit to rapyuta-robotics/ros1_bridge that referenced this pull request Aug 22, 2019
@Karsten1987 @dhananjaysathe
export find_ros1_package cmake (ros2#164)
8cc9195 
* export find_ros1_package cmake

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* export pkg-config dependency

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* use buildtool_export_depend

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* make pkg-config required

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* also export cmake macros if ros1 is not found

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* set cmake files in variable

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* use set instead of list

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Signed-off-by: Dhananjay Sathe <dhananjay.sathe@rapyuta-robotics.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Assignees

@Karsten1987 Karsten1987

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Karsten1987 @dirk-thomas @paulbovbel