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

Support ROS2 build #5

Merged
merged 11 commits into from
Jan 13, 2022
Merged

Support ROS2 build #5

merged 11 commits into from
Jan 13, 2022

Conversation

kenji-miyake
Copy link

No description provided.

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
@kenji-miyake kenji-miyake force-pushed the support-ros2-build branch 2 times, most recently from a54f0c5 to ce56fef Compare August 10, 2021 15:10
@kenji-miyake
Copy link
Author

It seems action-ros-ci doesn't install dependencies that have condition of ROS_VERSION ? 🤔

@kenji-miyake
Copy link
Author

Seeing the code of action-ros-ci, it's a bit annoying to fix this problem... 🥺 (because of ROS1 and Windows support)

@kenji-miyake
Copy link
Author

Posted a PR. ros-tooling/action-ros-ci#697

@KeisukeShima KeisukeShima changed the base branch from master to tier4/main December 27, 2021 04:43
Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
@@ -11,8 +10,8 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
RUN mkdir -p /root/catkin_ws/src
WORKDIR /root/catkin_ws/src
RUN /bin/bash -c '. /opt/ros/melodic/setup.bash; catkin_init_workspace'
RUN git clone https://github.com/SMRT-AIST/fast_gicp.git --recursive
RUN git clone https://github.com/koide3/hdl_graph_slam.git
RUN git clone https://github.com/SMRT-AIST/fast_gicp.git --recursive --depth=1

Choose a reason for hiding this comment

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

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
@KeisukeShima KeisukeShima changed the title [Test PR] Support ROS2 build Support ROS2 build Dec 27, 2021
@KeisukeShima KeisukeShima marked this pull request as ready for review December 27, 2021 07:29
Copy link

@KeisukeShima KeisukeShima left a comment

Choose a reason for hiding this comment

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

LGTM

@KeisukeShima
Copy link

@kenji-miyake It looks good to me. Please check my changes before you merge this PR.


if(OpenMP_CXX_FOUND)
target_link_libraries(ndt_omp PUBLIC OpenMP::OpenMP_CXX)
target_link_libraries(ndt_omp OpenMP::OpenMP_CXX)
Copy link
Author

Choose a reason for hiding this comment

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

Why did you remove PUBLIC?

Choose a reason for hiding this comment

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

This is because the docker CI for ROS2 failed with the following error.
And the solution to this error is to remove PUBLIC.
https://stackoverflow.com/questions/59522267/cmake-rejects-a-second-target-link-libraries-talking-about-keyword-vs-plain

Starting >>> ndt_omp
--- stderr: ndt_omp
CMake Warning (dev) at CMakeLists.txt:87 (find_package):
  Policy CMP0074 is not set: find_package uses <PackageName>_ROOT variables.
  Run "cmake --help-policy CMP0074" for policy details.  Use the cmake_policy
  command to set the policy and suppress this warning.

  CMake variable PCL_ROOT is set to:

    /usr

  For compatibility, CMake is ignoring the variable.
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Error at CMakeLists.txt:107 (target_link_libraries):
  The plain signature for target_link_libraries has already been used with
  the target "ndt_omp".  All uses of target_link_libraries with a target must
  be either all-keyword or all-plain.

  The uses of the plain signature are here:

   * /opt/ros/foxy/share/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake:145 (target_link_libraries)



---
Failed   <<< ndt_omp [9.05s, exited with code 1]

Copy link
Author

Choose a reason for hiding this comment

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

Is this because of ament_auto_add_library?
I feel all-keyword is better if possible.

Copy link
Author

Choose a reason for hiding this comment

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

Will this work?
https://github.com/ament/ament_cmake/blob/4ac1ed1153ac774116c273021944eb3ea836495c/ament_cmake_auto/cmake/ament_auto_add_library.cmake#L66

ament_auto_add_library(ndt_omp SHARED PUBLIC
  src/pclomp/voxel_grid_covariance_omp.cpp
  src/pclomp/ndt_omp.cpp
  src/pclomp/gicp_omp.cpp
)

target_link_libraries(ndt_omp PUBLIC ${PCL_LIBRARIES})

if(OpenMP_CXX_FOUND)
  target_link_libraries(ndt_omp PUBLIC OpenMP::OpenMP_CXX)

Copy link
Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Will this work? https://github.com/ament/ament_cmake/blob/4ac1ed1153ac774116c273021944eb3ea836495c/ament_cmake_auto/cmake/ament_auto_add_library.cmake#L66

ament_auto_add_library(ndt_omp SHARED PUBLIC
  src/pclomp/voxel_grid_covariance_omp.cpp
  src/pclomp/ndt_omp.cpp
  src/pclomp/gicp_omp.cpp
)

target_link_libraries(ndt_omp PUBLIC ${PCL_LIBRARIES})

if(OpenMP_CXX_FOUND)
  target_link_libraries(ndt_omp PUBLIC OpenMP::OpenMP_CXX)

It doesn't work. 😢

Ah, but since the upstream's style is plain, I think it's okay.

Thanks for letting me know. I'll keep the current code.

Copy link
Author

Choose a reason for hiding this comment

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

🥺
Okay, thank you!

CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
@kenji-miyake kenji-miyake requested a review from wep21 December 27, 2021 08:03
@kenji-miyake
Copy link
Author

@wep21 Could you please double-check? 🙏

Copy link

@wep21 wep21 left a comment

Choose a reason for hiding this comment

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

looks good to me

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
@kenji-miyake kenji-miyake merged commit 53ffe67 into tier4/main Jan 13, 2022
@wep21 wep21 deleted the support-ros2-build branch January 13, 2022 08:15
KeisukeShima added a commit to KeisukeShima/ndt_omp that referenced this pull request Jan 17, 2022
* Support ROS2 build

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* fix merge error

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>

* fix git clone error

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>

* remove duplicate command

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>

* add Dockerfile for foxy and galactic

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>

* fix build error

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>

* fix git clone error

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>

* add new line

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>

* remove ROS2 build

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>

* change to be able to run ci in pull request

Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>

Co-authored-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
Signed-off-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
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.

3 participants