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

Add missing find_package #11

Closed
wants to merge 1 commit into from
Closed

Add missing find_package #11

wants to merge 1 commit into from

Conversation

kledom
Copy link

@kledom kledom commented May 6, 2020

Add missing find_package call

Signed-off-by: Dominik Kleiser KleiserDominik@outlook.com

Signed-off-by: Dominik Kleiser <KleiserDominik@outlook.com>
@ravijo
Copy link
Owner

ravijo commented May 6, 2020

Hi @kledom

Thank you very much for your suggestion. I am a bit concerned about find_package(Threads REQUIRED) right now. I will point my reasons below-

  1. For newer versions, i.e., CMake 3.9+, the find_package(Threads REQUIRED) is not required.

  2. For older versions, i.e., CMake < 3.9, the find_package(Threads REQUIRED) is required.

    The reference for the above two points can be found here.

  3. In our CMakeLists.txt , we are supporting cmake_minimum_required(VERSION 2.8.3).

  4. As per the webpage here, we also need to set target_link_libraries while using find_package(OpenMP).

  5. A Github page here shows that find_package(Threads REQUIRED) is not required and set CMAKE_CXX_FLAGS as set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}") as we are doing.

  6. Additionally, an answer given in SO doesn't mention the need for using find_package(Threads REQUIRED).

Considering all the points mentioned above, what is your opinion? What do you think?

@kledom
Copy link
Author

kledom commented May 6, 2020

Thank you for looking into this.

I'm running on Ubuntu 18.04 with ROS Melodic and CMake 3.17.1.
Without

find_package(Threads REQUIRED)

building fails with the error message

/usr/bin/ld: cannot find -lThreads::Threads

I'm not sure when this line is needed or not.

@ravijo ravijo added the help wanted Extra attention is needed label May 6, 2020
@ravijo
Copy link
Owner

ravijo commented May 6, 2020

I am glad to see that you made a workaround quickly by adding find_package(Threads REQUIRED)

I'm running on Ubuntu 18.04 with ROS Melodic and CMake 3.17.1.

For now,

  • I mark it "help wanted" as we need to reproduce it on multiple machines to confirm.
  • I am keeping this task on my todo list.

As always, feel free to add your comments/suggestions, please.

@kledom
Copy link
Author

kledom commented May 6, 2020

I'll close this request. I think the issue you created is the better place for this discussion.

@kledom kledom closed this May 6, 2020
@ravijo
Copy link
Owner

ravijo commented May 9, 2020

Hi @kledom

I just wanted to give you an update on this issue. Please check here.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants