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 thread library of the system by Threads::Threads instead of via -pthread flag #3102

Merged

Conversation

SunBlack
Copy link
Contributor

This fixes my current linker error I get since #3094

/usr/bin/ld: CMakeFiles/pcl_example_outofcore_with_lod.dir/example_outofcore_with_lod.cpp.o: undefined reference to symbol 'pthread_rwlock_wrlock@@GLIBC_2.2.5'
//lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line
clang: error: linker command failed with exit code 1 (use -v to see invocation)

This issue was caused by missing linking of thread library in PCL_ADD_LIBRARY.

As we always add Threads::Threads (except for PCL-CUDA-CMake-Scripts), we don't add pthread anymore to CMAKE_CXX_FLAGS.

@SunBlack SunBlack changed the title Add thread library of the system by Threads::Threads instead of -pthread flag Add thread library of the system by Threads::Threads instead of via -pthread flag May 24, 2019
CMakeLists.txt Outdated
@@ -90,6 +90,7 @@ else()
set(CMAKE_CXX_FLAGS_DEFAULT "")
endif()

find_package(Threads REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

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

Our dependency finding is usually performed at the the bottom of this file. Did you had the add it here to be able to reference Threads::Threads inside the functions in pcl_targets.cmake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to call find_package before any call to PCL_ADD_EXAMPLE, PCL_ADD_LIBRARY, ... but yeah, I could move to the other dependencies.

Copy link
Contributor Author

@SunBlack SunBlack May 25, 2019

Choose a reason for hiding this comment

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

Updated code to fix it. There is only one question open: what do you think about THREADS_PREFER_PTHREAD_FLAG option? Should be set it or not?

Btw: find_package is required, because Unix requires threading library and on Windows Threads::Threads will be always an empty target, so MSVC doesn't complain that there is no threading library on Windows.

Copy link
Member

Choose a reason for hiding this comment

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

Updated code to fix it. There is only one question open: what do you think about THREADS_PREFER_PTHREAD_FLAG option? Should be set it or not?

Sorry. Totally overlooked this question. I have no reason to be opinionated here so I would not set anything.

cmake/pcl_targets.cmake Show resolved Hide resolved
@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented May 25, 2019

Come to think about it, I'm not sure it is a good idea to add Threads::Threads to every single executable. Wouldn't it more correct to add them to the actual PCL libs which require it i.e, common, io, outofcore and visualization (At least these are the one which were touched with #3060), and let it libs propagate down the linking stage?

@SunBlack
Copy link
Contributor Author

Come to think about it, I'm not sure it is a good idea to add Threads::Threads to every single executable. Wouldn't it more correct to add them to the actual PCL libs which require it i.e, common, io, outofcore and visualization (At least these are the one which were touched with #3079), and let it libs propagate down the linking stage?

It's a quick and dirty fix, so everything is compiling again. The pthread flag was already added to every executable and I don't believe they always need them. In general we have to check later all dependencies, because currently we always link against Boost - but I don't believe it will be needed everywhere after migration to std and I also don't believe that this will someone check before release of 1.10.0 ;).

…ead flag. This commit also fix

"//lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line". This issue was caused by missing linking of thread library in PCL_ADD_LIBRARY.
@SunBlack SunBlack force-pushed the fix_threading_library_dependency branch from bd8c6d1 to 73bf70d Compare May 25, 2019 11:38
@taketwo taketwo merged commit a38e6e7 into PointCloudLibrary:master May 30, 2019
@SunBlack SunBlack deleted the fix_threading_library_dependency branch June 3, 2019 15:28
@taketwo taketwo removed the c++14 label Jan 14, 2020
@taketwo taketwo changed the title Add thread library of the system by Threads::Threads instead of via -pthread flag Add thread library of the system by Threads::Threads instead of via -pthread flag Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants