-
Notifications
You must be signed in to change notification settings - Fork 94
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
Removed poco dependency #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty straightforward, and the OS specifics look ok to me.
Most of my comments boil down to, we should factor out the specifics into functions to abstract OS details, and then that leads me to conclude that maybe we should just put this logic in rcutils
and have class_loader use it from there. Because we will need the same logic for rmw_implementation
and the equivalent rosidl shared library loading logic.
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
2c0c1ae
to
cd6bcdd
Compare
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Can we have this for ros melodic ? |
@ahoarau That seems unlikely. It could be considered for Noetic, but it would break ABI and maybe API in melodic, which would be a deal breaker. |
I'm guessing using a fork in my own workspace would require to build ros from source ? |
Yes. |
rcpputils PR is merged, can you review this ? @wjwwood |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm with some comments. Also looks like Travis ci is still broken which given this pr touches those files should probably be fixed before merging.
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
fc215ec
to
6ebd81b
Compare
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
This reverts commit fe6fa9f.
As introduced in this PR ros2/ros2#867 we use the poco library for loading shared libraries dynamically at runtime. It will be nice to remove this dependency which is relatively large.
This PR removed Poco dependency in Linux, Mac and Windows.
@wjwwood what do you think?
Signed-off-by: Alejandro Hernández ahcorde@gmail.com