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

Linking with Gazebo using Catkin fails #856

Closed
davetcoleman opened this issue Feb 16, 2017 · 19 comments
Closed

Linking with Gazebo using Catkin fails #856

davetcoleman opened this issue Feb 16, 2017 · 19 comments

Comments

@davetcoleman
Copy link

davetcoleman commented Feb 16, 2017

Some of the exported flags (?) are being done incorrectly with Gazebo when using Catkin. @j-rivero indicates the problem is not within Gazebo because "We have built a lot of third party software on top of Gazebo so it should be related to catkin packages somehow." Though, we've also built even more software with Catkin...

The original issue from bitbucket:

Based on discussions with @j-rivero on this gazebo_ros_pkgs pull request, I've isolated some sort of flag export build problem that results in the error:

CMake Error at /home/dave/ros/current/ws_gazebo_test/devel_release/share/base_pkg/cmake/base_pkgConfig.cmake:141 (message):
  Project 'dummy_pkg' tried to find library '-lpthread'.  The library is
  neither a target nor built/installed properly.  Did you compile project
  'base_pkg'? Did you find_package() it before the subdirectory containing
  its code is included?
Call Stack (most recent call first):
  /opt/ros/kinetic/share/catkin/cmake/catkinConfig.cmake:76 (find_package)
  CMakeLists.txt:6 (find_package)

I think the problem is Gazebo7 is exporting a dependency on pthread which is suggested is bad on StackOverflow.

This occurs when one catkin package depends on Gazebo, and another depends on that package:

base_pkg CMakeLists

cmake_minimum_required(VERSION 2.8.3)
project(base_pkg)

find_package(catkin REQUIRED COMPONENTS
  roscpp
)

find_package(GAZEBO REQUIRED)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${GAZEBO_CXX_FLAGS}")

find_package(Boost REQUIRED COMPONENTS thread)

include_directories(
  ${catkin_INCLUDE_DIRS}
)

catkin_package(
  DEPENDS
    Boost
    GAZEBO
)

dependent pkg CMakeLists

cmake_minimum_required(VERSION 2.8.3)
project(dummy_pkg)

find_package(catkin REQUIRED COMPONENTS
  base_pkg
)

include_directories(
  ${catkin_INCLUDE_DIRS}
)

catkin_package(
  CATKIN_DEPENDS
    base_pkg
)

The full workspace can be tested from this barebones test repo I've just created.

@dirk-thomas
Copy link
Member

The reason is pretty straight forward. Currently catkin_package(DEPENDS foo) requires foo_LIBRARIES to contain items of the following types (see https://github.com/ros/catkin/blob/kinetic-devel/cmake/templates/pkgConfig.cmake.in#L112-L146):

  • build configuration keywords (debug, optimized, general)
  • CMake target names
  • absolute path of a library
  • or a library name (which is being located by calling find_library())

It does not support linker arguments like -lpthread.

The logic could be extended to also accept those and pass them along as-is by adding another elseif block checking if the item starts with -l (or whatever the equivalent on each platform is). I am not sure if that would effect any other code using these variables though.

@wjwwood
Copy link
Member

wjwwood commented Feb 16, 2017

@dirk-thomas do these assumptions about what is in the libraries list apply to just the DEPENDS option in catkin_package() or also to the LIBRARIES option?

@dirk-thomas
Copy link
Member

The restriction actually applies to LIBRARIES. DEPENDS on one level become LIBRARIES on the next level though.

@wjwwood
Copy link
Member

wjwwood commented Feb 16, 2017

In a very pedantic, idealistic sense, passing -lpthread in a _LIBRARIES cmake variable, doesn't make sense to me. As your SO article points out @davetcoleman, the more correct compiler flag is -pthread which is what I'd classify as a compiler flag, i.e. CXX_FLAGS, and not a "library to be linked".

From another angle, if you wanted to communicate that someone should link against a library foo in a directory /path/to/libs you wouldn't put -lfoo in a _LIBRARIES variable nor would you put -L/path/to/libs in a _LIBRARY_DIRS variable. So by analogy, you'd want to put pthread into the _LIBRARIES variable, not -lpthread.

From a practical standpoint, however, maybe we should just have a special case for -pthread and -lpthread? There are other similar flags to gcc which toe the line between compiler flag and link argument that we would probably want to consider as well.

In the meantime, you can create an extras file (I know it's a pain) and manually add the GAZEBO_LIBRARIES to the appropriate cmake variables manually.

@davetcoleman
Copy link
Author

Thank you for the explanations, though I am still confused as to how to fix this issue. Is there a simple one-line export flag I can use to fix this issue? Does every package that depends on a package that Gazebo uses need to have a cmake extra file? I'm not familiar with how to make CMake extra files. I tried a few fixes just now to no avail, such as:

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -lpthread")

@wjwwood
Copy link
Member

wjwwood commented Feb 17, 2017

You would:

  • not put GAZEBO or GAZEBO_LIBRARIES into catkin_package()
  • create an extras file that finds gazebo and adds it to the "exported" libraries of your package manually

There's no "one-line" fix for this, but the second step I mentioned is described in this ros answer:

http://answers.ros.org/question/201036/how-can-catkin-find-ros-libraries-in-non-standard-locations/?answer=209923#post-id-209923


If an exception were added for -lpthread in the catkin code for handling libraries then perhaps nothing would need to be done.

@j-rivero
Copy link

Thanks for comments.

In a very pedantic, idealistic sense, passing -lpthread in a _LIBRARIES cmake variable, doesn't make sense to me.

For completeness: my research found that the -lpthread value in _LIBRARIES is not coming from Gazebo itself, it comes from the Kitware Protobuf module.

@davetcoleman
Copy link
Author

So in my example at the top of this issue, would the CMakeExtras file be added to the base_pkg or the dummy_pkg (that depends on base_pkg)?

@wjwwood
Copy link
Member

wjwwood commented Feb 23, 2017

base_pkg. It would be executed by the dummy_pkg when it find_package(... base_pkg ...).

@davetcoleman
Copy link
Author

Okay, I've created something that compiles with a cmake extras file, but I'm not sure what the point of this whole exercise is anymore.

Should base_pkg still include DEPENDS GAZEBO? This is what causes the pthread issue, so I'm assuming the answer is no.

My example workspace now has base_pkg-extras.cmake.in that allows downstream dummy_pkg to compile an example file that depends on Gazebo without having to specify so in its CMakeLists.txt. Instead, it gets its Gazebo dependency from base_pkg.

For the original issue, this means I should modify gazebo_ros_pkgs to not DEPEND GAZEBO but rather use this cmake-extras file...

@wjwwood
Copy link
Member

wjwwood commented Feb 24, 2017

That is all correct, as far as I can see. This will be necessary unless Gazebo (or the upstream culprit) stops sending -lpthread along or until catkin is patched to make an allowance for -lpthread and/or -pthread.

@wjwwood
Copy link
Member

wjwwood commented Feb 24, 2017

Should base_pkg still include DEPENDS GAZEBO? This is what causes the pthread issue, so I'm assuming the answer is no.

Sorry, forgot to specifically answer this one, you're correct it should either do it in the extras file or have it in the DEPENDS, but not both. And since the DEPENDS does not work right now you'll want to do just the extras file as a workaround.

@davetcoleman
Copy link
Author

catkin is patched to make an allowance for -lpthread and/or -pthread.

Digging into catkin would be a very big project for me that I cannot take on, do you think anyone else would be able to do it?

@j-rivero
Copy link

This will be necessary unless Gazebo (or the upstream culprit) stops sending -lpthread along or until catkin is patched to make an allowance for -lpthread and/or -pthread.

I can easily patch the gazebo cmake module to adapt it to this scenario, not a big problem. I'm more afraid about the real root of the problem which is a not-so-weird library like Protobuf (the module comes directly from Kitware). Potentially we could face the same problem with other software relying on Protobuf, right?

@wjwwood
Copy link
Member

wjwwood commented Feb 24, 2017

I'm not opposed to patching catkin to support this, but I don't have time to do so right now either. The change is trivial, but getting it reviewed and making sure it doesn't break other stuff (and dealing with breaks when they inevitably happen) takes a lot more time.

In my experience, just because a cmake module has been shipped with CMake (from kitware) doesn't really ensure any level of quality. Also, since you're probably using Trusty or Xenial, the CMake version is at least several versions out of date, and so it's not uncommon for cmake modules shipped with cmake to also be out of date.

Here are some potentially related links:

j-rivero pushed a commit to ros-simulation/gazebo_ros_pkgs that referenced this issue Apr 10, 2017
…ing)

Added missing DEPEND clauses to catkin_package to fix gazebo catkin warning. Note that after the change problems could appear related to -lpthreads errors. This is an known issue related to catkin: ros/catkin#856.
@jlack1987
Copy link

Would just like to toss in here that I ran into the same problem independently when trying to link protobuf w/o gazebo. We already forked our own FindProtobuf.cmake file, so I just commented out the chunk that added -lpthread to the libraries and that took care of it. I really appreciate the detailed research done here, saved me a ton of time.

@dirk-thomas
Copy link
Member

I will close this for now since it is not planned to change the behavior of the catkin API atm.

@ubuntuslave
Copy link

In my case, this also happens with PCL 1.9. The infamous -lpthread gets added in the generated CMakeCache.txt file.

@rakeshshrestha31
Copy link

Boost with newer CMake (~3.13) seems to add -lpthread too as referenced in #975

MatthijsBurgh added a commit to tue-robotics/code_profiler that referenced this issue Aug 5, 2020
cohen39 pushed a commit to cohen39/gazebo_ros_pkgs that referenced this issue Nov 15, 2021
…ing)

Added missing DEPEND clauses to catkin_package to fix gazebo catkin warning. Note that after the change problems could appear related to -lpthreads errors. This is an known issue related to catkin: ros/catkin#856.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants