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.

Sign up for GitHub

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

Jump to bottom

Fix gazebo catkin warning #537

Merged
merged 1 commit into from
Feb 20, 2017
Merged

Fix gazebo catkin warning #537

merged 1 commit into from
Feb 20, 2017

Conversation

davetcoleman
Copy link
Collaborator

@davetcoleman davetcoleman commented Feb 10, 2017 •
edited
Loading

Same fix as #521 except for gazebo_ros_control pkg

This PR is sponsored by Vicarious

@davetcoleman
Copy link
Collaborator Author

I'm not 100% this PR is good, I'm getting errors in downstream packages (baxter_simluation):

tried to find library '-lpthread'.  The
  library is neither a target nor built/installed properly.

@j-rivero
Copy link
Contributor

I'm not 100% this PR is good, I'm getting errors in downstream packages (baxter_simluation):

sounds to me like one of the dependencies is exporting that pthread in an invalid way. Maybe looking into the CMakeCache.txt file can give you hint or try to disable the sdformat/boost find_package lines alternatively to see which one is the culprit.

@davetcoleman davetcoleman force-pushed the fix_ros_control_warn branch 3 times, most recently from 686e31f to 99c08dd Compare February 11, 2017 18:21
davetcoleman
davetcoleman commented Feb 11, 2017
View reviewed changes
gazebo_plugins/CMakeLists.txt
@@ -75,8 +75,6 @@ link_directories(
${catkin_LIBRARY_DIRS}
)

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${GAZEBO_CXX_FLAGS}")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this line is a duplicate from previous in this file

gazebo_ros/CMakeLists.txt
@@ -36,21 +36,20 @@ generate_dynamic_reconfigure_options(cfg/Physics.cfg)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this file I only do OCD house keeping :)

gazebo_ros_control/CMakeLists.txt
@@ -31,9 +33,11 @@ catkin_package(
joint_limits_interface
urdf
angles
INCLUDE_DIRS include
LIBRARIES ${PROJECT_NAME} default_robot_hw_sim
DEPENDS gazebo
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From my testing, DEPENDS gazebo caused the issues I was having

Copy link
Contributor

@j-rivero j-rivero Feb 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

According to catkin documentation that DEPENDS clause is fine (at least to my eyes). My vote is for keeping it and debug where is exactly the error.

Copy link
Collaborator Author

@davetcoleman davetcoleman Feb 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I read that documentation and it does not clarify anything for me. It just says DEPENDS - Non-catkin CMake projects that this project depends on but then why does it also not include things like Boost?

Another example - why does gazebo_ros not DEPEND on GAZEBO? My change in this PR was inspired by that CMakeLists file.

Copy link
Contributor

Choose a reason for hiding this comment

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

It just says DEPENDS - Non-catkin CMake projects that this project depends on but then why does it also not include things like Boost?

Another example - why does gazebo_ros not DEPEND on GAZEBO?

I would say that both are bugs or errors in the configuration of our CMakeLists.txt. I found this answer specially useful to understand what are they used for. Now it makes sense that changes in DEPEND will affect third party software.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a really good answer, would be great if it was added to the catkin documentation. I added a link from the wiki to that answer, for now.

Seems I'll have the same pthread linking issues now, I'll work on it soon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I've spent the last hour on the issue and I think our problem is with Gazebo, see https://bitbucket.org/osrf/gazebo/issues/2198/gazebo7-linking-error-with-cmake-catkin

Copy link
Contributor

Choose a reason for hiding this comment

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

given the current state of the issue, I'm ok for not declaring gazebo as DEPENDS here. Dave, if you could please leave the line commented with a comment pointing to the gazebo issue that should be enough to restore it as soon as we find the way of solving the issue. After the comments, good to merge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i've added the comment

@davetcoleman
Copy link
Collaborator Author

This PR is ready for review/merging

This was referenced Feb 14, 2017
Merged
Merged
@davetcoleman davetcoleman mentioned this pull request Feb 16, 2017
Closed
@davetcoleman
Copy link
Collaborator Author

ping @j-rivero, ready for merge

@j-rivero j-rivero merged commit 5a0305f into ros-simulation:kinetic-devel Feb 20, 2017
@j-rivero
Copy link
Contributor

ping @j-rivero, ready for merge

Thanks Dave!

@davetcoleman davetcoleman deleted the fix_ros_control_warn branch February 21, 2017 01:01
j-rivero added a commit that referenced this pull request Mar 3, 2017
@j-rivero
Revert "Fix gazebo catkin warning, cleanup CMakeLists (#537)"
853eb99 
This reverts commit 5a0305f.

For reference and reasons, please check:
https://discourse.ros.org/t/need-to-sync-new-release-of-rqt-topic-indigo-jade-kinetic/1410/4
@j-rivero j-rivero mentioned this pull request Mar 3, 2017
Merged
j-rivero added a commit that referenced this pull request Mar 3, 2017
@j-rivero
Revert catkin warnings to fix regressions (problems with catkin -lpth…
6c9786d 
…reads errors)

For reference and reasons, please check:
https://discourse.ros.org/t/need-to-sync-new-release-of-rqt-topic-indigo-jade-kinetic/1410/4

* Revert "Fix gazebo catkin warning, cleanup CMakeLists (#537)"
This reverts commit 5a0305f.

* Revert "Fix gazebo and sdformat catkin warnings"
This reverts commit 11f95d2.
meyerj pushed a commit to tu-darmstadt-ros-pkg/hector_gazebo that referenced this pull request Mar 5, 2017
@meyerj
hector_gazebo_plugins/hector_gazebo_thermal_camera: removed catkin_pa…
271695d 
…ckage(DEPENDS gazebo) declaration which was a no-op anyway

See ros-simulation/gazebo_ros_pkgs#537.
@osrf-migration osrf-migration mentioned this pull request Apr 19, 2020
Closed
cohen39 pushed a commit to cohen39/gazebo_ros_pkgs that referenced this pull request Nov 15, 2021
@davetcoleman @j-rivero
Fix gazebo catkin warning, cleanup CMakeLists (ros-simulation#537)
ef22084
cohen39 pushed a commit to cohen39/gazebo_ros_pkgs that referenced this pull request Nov 15, 2021
@j-rivero
Revert catkin warnings to fix regressions (problems with catkin -lpth…
effbd2e 
…reads errors)

For reference and reasons, please check:
https://discourse.ros.org/t/need-to-sync-new-release-of-rqt-topic-indigo-jade-kinetic/1410/4

* Revert "Fix gazebo catkin warning, cleanup CMakeLists (ros-simulation#537)"
This reverts commit ef22084.

* Revert "Fix gazebo and sdformat catkin warnings"
This reverts commit e8d66bc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-rivero j-rivero j-rivero left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@davetcoleman @j-rivero