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

Protection against double -l #998

Merged
merged 1 commit into from
Mar 12, 2019
Merged

Conversation

lepalom
Copy link
Contributor

@lepalom lepalom commented Mar 12, 2019

I have been working rebuilding ROS Melodic for Stretch using the debian packages
and I have been blocked in an error in qt_gui_cpp:

/usr/bin/ld: cannot find -l-lpthread

that error have been found in several places:

https://aur.archlinux.org/packages/ros-melodic-qt-gui-cpp/

ros/ros-overlay#711

and probably more places.

You have been working in a patch for something similar:

#975

But I think that the problems comes from another place and @ipa-mdl point me to
the error when here said:

ipa-mdl commented on 2 Jan

@dirk-thomas: TL;DR: Upstream CMake just started to inject -lpthread into
Boost_LIBRARIES .

The problem comes from the the .pc files. It's strange to me because from all
the pc files generated by catkin that we have in debian (Buster/Sid), I have
only detected rosconsole.pc that had -l-lpthread.

But, in the backported version that I have, there are more. I'm using
cmake-3.13.2-1 and maybe, it comes because that version. The vast majority of
debian packages were generated before cmake 3.12 or 3.13 arrived to the archive.
In Ubuntu bionic cmake is 3.10.2.

The solution is simple, is this patch.

I have to admit that in gazebo, Jose Rivero incorporated a patch from Bas
Couwenberg solving a very similar issue.

What do you think?

@dirk-thomas
Copy link
Member

Thanks for the patch. I verified that the problem exists in e.g. Ubuntu Disco (which has a new enough CMake version to export -lpthread) and this patch fixes the generated .pc files.

@lepalom
Copy link
Contributor Author

lepalom commented Mar 13, 2019

Not solving yet all the issues :-(
ros-visualization/qt_gui_core#174

@dirk-thomas
Copy link
Member

Not solving yet all the issues :-(
ros-visualization/qt_gui_core#174

This report wasn't used the latest state of all patches.

@lepalom
Copy link
Contributor Author

lepalom commented Mar 14, 2019

well, I was using the 0.3.4 version that seem that is the version in melodic:
https://github.com/ros-gbp/python_qt_binding-release

what about to release a 0.3.5 with all that patches?

@dirk-thomas
Copy link
Member

what about to release a 0.3.5 with all that patches?

Sounds good: ros/rosdistro#20577

sloretz added a commit to sloretz/catkin that referenced this pull request Aug 3, 2019
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>
dirk-thomas pushed a commit that referenced this pull request Oct 7, 2019
* Fix -pthread handling in Debian buster

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 #998 to not add `-l` to any linker flag.
Second it adds to the fix in #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>

* math() output actually used

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* dummyN -> wrapped-linker-optionsN

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Fix pre-3.13.0 target property setting

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Add test for propagation of linker options

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Force add CMakeLists.txt

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Indent using 2 spaces

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Increment until target is unique

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants