Fix -pthread handling in Debian buster #1021
Conversation
FindBoost.cmake blindly adds `${CMAKE_THREAD_LIBS_INIT}` to
`${Boost_LIBRARIES}` when the component `thread` is found.
On Debian buster the `FindThreads.cmake` sets that to `-pthread`.
This breaks a bunch of stuff becakse `-pthread` is a linker flag, not a
library.
There were earlier fixes for `-lpthread`.
This PR expands upon them.
First this PR modifies the fix from ros#998 to not add `-l` to any linker flag.
Second it adds to the fix in ros#975 to make sure `-pthread` is passed to
downstream users.
There's no standard cmake variable for linker flags, so this PR opts to
create an interface target with just the flag, and add that to
`${PROJECT_NAME}_LIBRARIES` instead.
Both this PR and ros-visualization/python_qt_binding#68 are required to strip or `qt_gui_cpp` will fail at link time.
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
cmake/templates/pkgConfig.cmake.in
Outdated
| if(NOT @PROJECT_NAME@_NUM_DUMMY_TARGETS) | ||
| set(@PROJECT_NAME@_NUM_DUMMY_TARGETS 0) | ||
| endif() | ||
| MATH(EXPR VAR "${@PROJECT_NAME@_NUM_DUMMY_TARGETS}+1") |
There was a problem hiding this comment.
This line is a no op since the result is assigned to the variable VAR which isn't used anywhere. I assume the intention was to increment @PROJECT_NAME@_NUM_DUMMY_TARGETS?
Nitpick: the CMake function name should be lower case to match the style guide.
There was a problem hiding this comment.
lowercase and actually used output in 64295ca
cmake/templates/pkgConfig.cmake.in
Outdated
| set(@PROJECT_NAME@_NUM_DUMMY_TARGETS 0) | ||
| endif() | ||
| MATH(EXPR VAR "${@PROJECT_NAME@_NUM_DUMMY_TARGETS}+1") | ||
| add_library("catkin::@PROJECT_NAME@::dummy${@PROJECT_NAME@_NUM_DUMMY_TARGETS}" INTERFACE IMPORTED) |
There was a problem hiding this comment.
Since the target name is visible in downstream packages I would rather prefer a different name, maybe "wrapped-linker-optionN"?
There was a problem hiding this comment.
Just wrapped-linker-optionN or catkin::my-project::wrapped-linker-optionN?
There was a problem hiding this comment.
I don't mind. While the first is unlikely to collide the second one should really not 😉
There was a problem hiding this comment.
Changed target name in 8d6694a
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
|
With this Dockerfile ARG base_image
FROM $base_image
RUN apt update && apt install -y python-pip git cmake
RUN pip install argparse catkin-pkg empy mock nose
RUN git clone -b pthread-linker-fix https://github.com/sloretz/catkin.git \
&& mkdir catkin_build \
&& cd catkin_build \
&& cmake ../catkin \
&& make \
&& CTEST_OUTPUT_ON_FAILURE=1 make testI checked this PR using All builds succeeded, so the tests pass on those 4 platforms. |
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
|
@dirk-thomas I think this is ready for another review. In addition to the 4 Docker images in #1021 (comment) building successfully, I'm able to build all of Melodic upstream development up to |
On Debian buster
FindBoost.cmakeblindly adds${CMAKE_THREAD_LIBS_INIT}to${Boost_LIBRARIES}when the componentthreadis found.FindThreads.cmakesets that variable to-pthread. This breaks a bunch of stuff because-pthreadis a linker flag, not a library.There were earlier fixes for
-lpthread, and this PR expands upon them. First this modifies the fix from #998 to not add-lto any linker flag. Second it adds to the fix in #975 to make sure-pthreadis passed to downstream users. There's no standard cmake variable for linker flags, so this PR opts to create an interface target with just the flag, and add that to${PROJECT_NAME}_LIBRARIESinstead.Both this PR and ros-visualization/python_qt_binding#68 are required to strip or
qt_gui_cppwill fail at link time.