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 cmake target libraries and linking so plugins work #667

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix cmake target libraries and linking so plugins work #667

wants to merge 4 commits into from

Conversation

jlack1987
Copy link

@jlack1987 jlack1987 commented Apr 13, 2021 •
edited

The mav_msgs library created in rotors_gazebo_plugins isn't linked to a few of the libraries that use it and also it is not installed. This MR fixes that stuff. I also changed the name of the library to mav_proto_msgs bc mav_msgs is the name of a package and it is a bit confusing(at least it was to me reading through it) having a library given the same name as a package being depended upon.

This fixes this issue

@ethzasl-jenkins
Copy link

Can one of the admins verify this patch?

Jaeyoung-Lim
Jaeyoung-Lim requested changes Apr 13, 2021
View reviewed changes
Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

Thanks for the fix,

Although renaming the package is problematic, since this package is being deployed with the ROS build farm. (e.g. ros-melodic-mav-msgs)

rotors_gazebo_plugins/CMakeLists.txt
# point forward.
# NOTE: This is deprecated, should be using target_link_libraries instead
link_libraries(mav_msgs)
add_library(mav_proto_msgs SHARED ${PROTO_SRCS})
Copy link
Member

Choose a reason for hiding this comment

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

Could you not rename the package? This would need to propagate further than just this cmake file.

Copy link
Author

Choose a reason for hiding this comment

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

This doesn't rename a package. It renames the cmake library created for the protobuf msgs

@Jaeyoung-Lim
Copy link
Member

@ethzasl-jenkins Test this please

@jlack1987
Copy link
Author

Although renaming the package is problematic, since this package is being deployed with the ROS build farm. (e.g. ros-melodic-mav-msgs)

Just to clarify, the changes here do not rename a package. The CMakeLists.txt was already such that there was a cmake library with the same name as a package being depended upon. This was confusing to me and i'm surprised it didn't cause any errors/warnings. The change here renames the library which is then linked to other targets.

@jlack1987
Merge branch 'master' of https://github.com/ethz-asl/rotors_simulator …
38800dc 
…into feature/fix-gazebo-plugins

# Conflicts:
#	rotors_gazebo_plugins/CMakeLists.txt
@jlack1987
Copy link
Author

I saw another MR was merged so I pulled master. I also changed how I was fixing the linking issue by just appending the missing proto libraries to the target_linking_LIBRARIES list which made fixing these issues much cleaner. Let me know if there's anything else I can do to get this merged. Thanks!

@jlack1987 jlack1987 mentioned this pull request Jan 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jaeyoung-Lim Jaeyoung-Lim Jaeyoung-Lim requested changes

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.

undefined symbol: _ZN14gz_sensor_msgs9ActuatorsC1Ev
3 participants
@jlack1987 @ethzasl-jenkins @Jaeyoung-Lim