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

exclude ros1 nodelets #152

Merged
merged 3 commits into from
Dec 12, 2018
Merged

exclude ros1 nodelets #152

merged 3 commits into from
Dec 12, 2018

Conversation

Karsten1987
Copy link
Contributor

@Karsten1987 Karsten1987 commented Dec 10, 2018 •
edited
Loading

connects to ros2/rosbag2#69
this PR excludes nodelets so that rosbag2 can replay ros1 bag files without linking conflicts.

@Karsten1987 Karsten1987 self-assigned this Dec 10, 2018
@Karsten1987 Karsten1987 added the in progress Actively being worked on (Kanban column) label Dec 10, 2018
@Karsten1987
Copy link
Contributor Author

CI packaging job to test the bridge
Build Status

@dirk-thomas
Copy link
Member

👍 This will fix the imminent problem and not link against libnodelet.

I am wondering though if this works if ROS 1 is built in isolation. Are the services in the nodelet package just not available for bridging or does it fail anywhere due to no finding their headers?

Martin-Idel-SI
Martin-Idel-SI reviewed Dec 11, 2018
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@nuclearsandwich nuclearsandwich added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 11, 2018
@Karsten1987
Copy link
Contributor Author

Build Status

sloretz
sloretz approved these changes Dec 11, 2018
View reviewed changes
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

The change seems safe, though it seems likely to happen again unless there is a way to link against just the generated message library in a package instead of all libraries in a package.

@wjwwood
Copy link
Member

wjwwood commented Dec 12, 2018

The right solution to this problem is to ensure that the class loader used in ROS 1 and ROS 2 can be used at the same time, either by using different namespaces or by hiding conflicting symbols so they don't collide at link time or by making it so that the same exact version of class loader is used in both.

wjwwood
wjwwood approved these changes Dec 12, 2018
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm as a workaround, though there should be at least a TODO about it in the code and probably an issue.

The chances of someone fixing this problem down the road and remembering to change this, without an issue or TODO to cross reference, is pretty small in my opinion.

@Karsten1987 Karsten1987 merged commit 070600e into master Dec 12, 2018
@Karsten1987 Karsten1987 deleted the exclude_nodelet branch December 12, 2018 16:04
@Karsten1987 Karsten1987 removed the in review Waiting for review (Kanban column) label Dec 12, 2018
@Karsten1987 Karsten1987 mentioned this pull request Dec 12, 2018
Open
@Karsten1987
Copy link
Contributor Author

I went ahead and merged this. I've put a TODO in the CMakeLists and opened an issue on ros/class_loader.
I wasn't entirely sure whether class_loader is the right repo over ros/pluginlib.

dhananjaysathe pushed a commit to rapyuta-robotics/ros1_bridge that referenced this pull request Aug 22, 2019
@Karsten1987 @dhananjaysathe
exclude ros1 nodelets (ros2#152)
dd579a4 
* exclude ros1 nodelets

* find_package() only if neccessary

* add todo

Signed-off-by: Dhananjay Sathe <dhananjay.sathe@rapyuta-robotics.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Martin-Idel-SI Martin-Idel-SI Martin-Idel-SI left review comments

@wjwwood wjwwood wjwwood approved these changes

@sloretz sloretz sloretz approved these changes

Assignees

@Karsten1987 Karsten1987

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Karsten1987 @dirk-thomas @wjwwood @sloretz @Martin-Idel-SI @nuclearsandwich