Bug#915148: [Pkg-cmake-team] Bug#915148: cmake: regression in ros-ros-comm build
On 12/7/18 7:44 AM, Jochen Sprickerhof wrote: >> Both are valid ways to link to the pthread library, which is all >> the `-pthread` flag does when used to drive linking. > > I don't agree. Quoting from my mail: > > "Define additional macros required" Macros are for compiling. It is not used during linking. Either way, `-lfoo` can appear for other libraries and ros-catkin needs the fix. -Brad
Bug#915148: [Pkg-cmake-team] Bug#915148: cmake: regression in ros-ros-comm build
* Brad King [2018-12-07 07:29]: On 12/6/18 4:16 PM, Jochen Sprickerhof wrote: after reading up on this, I think this needs fixing in cmake. So neither adding -lpthread, nor adding /usr/lib/x86_64-linux-gnu/libpthread.so seems correct to me. Both are valid ways to link to the pthread library, which is all the `-pthread` flag does when used to drive linking. I don't agree. Quoting from my mail: * Jochen Sprickerhof [2018-12-06 22:16]: | -pthread | | Define additional macros required for using the POSIX threads | library. You should use this option consistently for both | compilation and linking. This option is supported on GNU/Linux | targets, most other Unix derivatives, and also on x86 Cygwin and | MinGW targets. "Define additional macros required" see: diff <(g++ -dM -E -x c++ - < /dev/null) <(g++ -pthread -dM -E -x c++ - < /dev/null) 106a107 #define _REENTRANT 1 (But I'm not a compiler expert.) The patch here: http://launchpadlibrarian.net/399812910/ros-catkin_0.7.14-7_0.7.14-7ubuntu1.diff.gz is not a workaround. The input to that logic could contain -lfoo generated by other means for other libraries. This case just happened to expose the existing bug in that logic. I agree. Actually looking at the applied fix https://sources.debian.org/src/ros-catkin/0.7.14-8/cmake/templates/pkgConfig.cmake.in/#L118 that logic may need additional work to handle plain `-pthread` or other link flags. I don't agree, -pthread should be in CXXFLAGS, whereas the pkgConfig.cmake process @PKG_CONFIG_LIBRARIES@. CMake's FindBoost and FindThreads could use some work to use `-pthread` more completely (perhaps with a policy to use it by default on appropriate compilers), and there is an issue related to this here: +1. Cheers Jochen signature.asc Description: PGP signature
Bug#915148: [Pkg-cmake-team] Bug#915148: cmake: regression in ros-ros-comm build
On 12/6/18 4:16 PM, Jochen Sprickerhof wrote: > after reading up on this, I think this needs fixing in cmake. > > So neither adding -lpthread, > nor adding /usr/lib/x86_64-linux-gnu/libpthread.so > seems correct to me. Both are valid ways to link to the pthread library, which is all the `-pthread` flag does when used to drive linking. > I will revert the workaround in ros-catkin if you agree. The patch here: http://launchpadlibrarian.net/399812910/ros-catkin_0.7.14-7_0.7.14-7ubuntu1.diff.gz is not a workaround. The input to that logic could contain -lfoo generated by other means for other libraries. This case just happened to expose the existing bug in that logic. Actually looking at the applied fix https://sources.debian.org/src/ros-catkin/0.7.14-8/cmake/templates/pkgConfig.cmake.in/#L118 that logic may need additional work to handle plain `-pthread` or other link flags. > [FindThreads] promotes CMAKE_THREAD_PREFER_PTHREAD to get -pthread It's THREADS_PREFER_PTHREAD_FLAG for that. CMake's FindBoost and FindThreads could use some work to use `-pthread` more completely (perhaps with a policy to use it by default on appropriate compilers), and there is an issue related to this here: https://gitlab.kitware.com/cmake/cmake/issues/18638 However, that does not mean the above change should be reverted. -Brad
Bug#915148: [Pkg-cmake-team] Bug#915148: cmake: regression in ros-ros-comm build
control: reopen -1 control: reassign -1 src:cmake control: affects -1 ros-ros-comm control: tags -1 - patch Hi, after reading up on this, I think this needs fixing in cmake. man gcc: | -pthread | | Define additional macros required for using the POSIX threads | library. You should use this option consistently for both | compilation and linking. This option is supported on GNU/Linux | targets, most other Unix derivatives, and also on x86 Cygwin and | MinGW targets. So neither adding -lpthread, as done by https://gitlab.kitware.com/cmake/cmake/commit/bd831ed0948a1e99f573f0056f2bee5d3b21009e nor adding /usr/lib/x86_64-linux-gnu/libpthread.so as done by https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=563479 seems correct to me. Note that https://cmake.org/cmake/help/v3.1/module/FindThreads.html promotes CMAKE_THREAD_PREFER_PTHREAD to get -pthread, but it would still need to go into CXXFLAGS (through a to be defined BOOST_FLAGS, maybe), to my understanding. Also having CMAKE_THREAD_PREFER_PTHREAD as default for gcc and clang may make sense? I will revert the workaround in ros-catkin if you agree. Cheers Jochen signature.asc Description: PGP signature
Bug#915148: [Pkg-cmake-team] Bug#915148: cmake: regression in ros-ros-comm build
Hi Gianfranco,
* Gianfranco Costamagna [2018-12-05 10:36]:
catkin upstream would prefer to add -pthread instead of -lpthread to the
compiler flags. See the discussion here:
https://github.com/ros/catkin/issues/856
Would this be possible?
what about:
| elseif(${library} MATCHES "^-l") |
|
list(APPEND @PROJECT_NAME@_LIBRARIES ${library})
| elseif(${library} MATCHES "^-p") |
|
list(APPEND @PROJECT_NAME@_LIBRARIES ${library})
I think you got me wrong, the current workaround works already.
BTW, -lpthread is generally wrong, and useful only to fix underlinking.The right thing to
do is to add "-pthread" to CFLAGS, not to LDFLAGS
This is what I was asking for and should be fixed in cmake. So I would
propose to reopen this and assign to cmake, do you agree?
Cheers Jochen
signature.asc
Description: PGP signature
Bug#915148: [Pkg-cmake-team] Bug#915148: cmake: regression in ros-ros-comm build
>catkin upstream would prefer to add -pthread instead of -lpthread to the
>compiler flags. See the discussion here:
>
>https://github.com/ros/catkin/issues/856
>
>Would this be possible?
what about:
| elseif(${library} MATCHES "^-l") |
|
list(APPEND @PROJECT_NAME@_LIBRARIES ${library})
| elseif(${library} MATCHES "^-p") |
|
list(APPEND @PROJECT_NAME@_LIBRARIES ${library})
this way you can strip them both.BTW, -lpthread is generally wrong, and useful
only to fix underlinking.The right thing to do is to add "-pthread" to CFLAGS,
not to LDFLAGS
G.
Bug#915148: [Pkg-cmake-team] Bug#915148: cmake: regression in ros-ros-comm build
Hi Felix, catkin upstream would prefer to add -pthread instead of -lpthread to the compiler flags. See the discussion here: https://github.com/ros/catkin/issues/856 Would this be possible? Cheers Jochen * Jochen Sprickerhof [2018-12-02 14:59]: Hi, makes sense to me, I uploaded -8 with the patch and send it upstream: https://github.com/ros/catkin/pull/975 Thank for it Felix. Cheers Jochen -- debian-science-maintainers mailing list debian-science-maintain...@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/debian-science-maintainers signature.asc Description: PGP signature
Bug#915148: [Pkg-cmake-team] Bug#915148: cmake: regression in ros-ros-comm build
Hi, makes sense to me, I uploaded -8 with the patch and send it upstream: https://github.com/ros/catkin/pull/975 Thank for it Felix. Cheers Jochen signature.asc Description: PGP signature
Bug#915148: [Pkg-cmake-team] Bug#915148: cmake: regression in ros-ros-comm build
control: reassign -1 src:ros-catkincontrol: found -1 0.7.14-7control: tags -1
patch
Hello,
>Adding something like this would probably fix it:
>> elseif(${library} MATCHES "^-l")
>> list(APPEND @PROJECT_NAME@_LIBRARIES ${library})
>
>CCing Brad, maybe you could comment if ros-catkin should expect -l... entries
>there or FindBoost
>needs to be changed?
I like the rationale behind your initial change! I committed the change in
Ubuntuhttp://launchpadlibrarian.net/399812910/ros-catkin_0.7.14-7_0.7.14-7ubuntu1.diff.gz
and I'm ccing ros-catkin maintainers, I hope they will accept and forward
upstream the change too.
What do you think Jochen, Leopold?
thanks!
Gianfranco
Bug#915148: [Pkg-cmake-team] Bug#915148: cmake: regression in ros-ros-comm build
Hi,
On 01.12.18 06:31, Gianfranco Costamagna wrote:
Package: cmake
Version: 3.13.1-1
Severity: serious
Affects: ros-ros-comm
A simple ros-ros-comm rebuild now fails with the new cmake in unstable,
due to lpthread not being found as target.
I don't know if catkin is to blame, or something else, but clearly the
old cmake in testing is not having issues.
I'm opening this bug to prevent testing migration, until the problem is
sorted out (in one way or the other)
snip of the failure:
-- Using CATKIN_TEST_RESULTS_DIR:
/<>/ros-ros-comm-1.14.3+ds1/obj-x86_64-linux-gnu/test_results
-- Found gtest: gtests will be built
-- nosetests not found, Python tests can not be run (try installing package
'python-nose')
-- catkin 0.7.14
-- Boost version: 1.67.0
-- Found the following Boost libraries:
-- system
-- thread
-- chrono
-- date_time
-- atomic
CMake Error at
obj-x86_64-linux-gnu/devel/share/rostest/cmake/rostestConfig.cmake:146
(message):
Project 'rostest' tried to find library '-lpthread'. The library is
neither a target nor built/installed properly. Did you compile project
'rostest'? Did you find_package() it before the subdirectory containing its
code is included?
Call Stack (most recent call first):
/usr/share/catkin/cmake/catkinConfig.cmake:87 (find_package)
tools/rostest/CMakeLists.txt:23 (find_package)
-- Configuring incomplete, errors occurred!
See also
"/<>/ros-ros-comm-1.14.3+ds1/obj-x86_64-linux-gnu/CMakeFiles/CMakeOutput.log".
See also
"/<>/ros-ros-comm-1.14.3+ds1/obj-x86_64-linux-gnu/CMakeFiles/CMakeError.log".
cd obj-x86_64-linux-gnu && tail -v -n \+0 CMakeCache.txt
==> CMakeCache.txt <==
This problem occurs since my commit adds ${CMAKE_THREAD_LIBS_INIT}
("-lpthread") to
Boost_LIBRARIES when needed:
https://gitlab.kitware.com/cmake/cmake/commit/bd831ed0948a1e99f573f0056f2bee5d3b21009e
ros-catkin iterates over dependency libraries (Boost_LIBRARIES among other
things) and does this:
https://sources.debian.org/src/ros-catkin/0.7.14-7/cmake/templates/pkgConfig.cmake.in/#L118
Until my commit Boost_LIBRARIES only contained absolute paths to boost
libraries so that code
just passes them on. For the "-lpthread" case it calls find_library(...
"-lpthread") which fails.
Adding something like this would probably fix it:
> elseif(${library} MATCHES "^-l")
> list(APPEND @PROJECT_NAME@_LIBRARIES ${library})
CCing Brad, maybe you could comment if ros-catkin should expect -l... entries
there or FindBoost
needs to be changed?
Cheers,
Felix
