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

Removed poco dependency #139

Merged
merged 26 commits into from
Apr 8, 2020
Merged

Removed poco dependency #139

merged 26 commits into from
Apr 8, 2020

Conversation

ahcorde
Copy link

@ahcorde ahcorde commented Mar 9, 2020

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

wjwwood
wjwwood reviewed Mar 9, 2020
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.

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.

include/class_loader/class_loader_core.hpp Outdated Show resolved Hide resolved
src/class_loader.cpp Outdated Show resolved Hide resolved
src/class_loader_core.cpp Outdated Show resolved Hide resolved
src/class_loader_core.cpp Outdated Show resolved Hide resolved
@ahcorde
removed poco
cd6bcdd 
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/feature/removed_poco branch from 2c0c1ae to cd6bcdd Compare March 10, 2020 11:09
This was referenced Mar 10, 2020
Merged
Closed
@ahcorde
ament export dependencies rcutils
8fad77f 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
update rcutils shared library
cd4cf07 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
removed some traces and fixed string copy
3a4b7bb 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Adapted to rcutils
992b830 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Removed comment
436c75e 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
fixed allocator
ee10faa 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
error when dynamic library suffix is unknown
2f4b886 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
fixed rcutils_unload_shared_library function name
6f5b98f 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Used new and delete
d907d82 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Updated to use rcpputils
3694ee9 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Alphabetize includes
6b921ab 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
catch bad alloc exception
cb88610 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahoarau
Copy link

ahoarau commented Mar 24, 2020

Can we have this for ros melodic ?

@wjwwood
Copy link
Member

wjwwood commented Mar 24, 2020

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.

@ahoarau
Copy link

ahoarau commented Mar 25, 2020

I'm guessing using a fork in my own workspace would require to build ros from source ?

@wjwwood
Copy link
Member

wjwwood commented Mar 25, 2020

Yes.

@ahcorde
Copy link
Author

ahcorde commented Apr 2, 2020

rcpputils PR is merged, can you review this ? @wjwwood

@ahcorde ahcorde requested a review from wjwwood April 2, 2020 07:26
wjwwood
wjwwood approved these changes Apr 2, 2020
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 with some comments. Also looks like Travis ci is still broken which given this pr touches those files should probably be fixed before merging.

CMakeLists.txt Outdated Show resolved Hide resolved
include/class_loader/meta_object.hpp Outdated Show resolved Hide resolved
dirk-thomas
dirk-thomas reviewed Apr 2, 2020
View reviewed changes
package.xml Outdated Show resolved Hide resolved
src/class_loader.cpp Outdated Show resolved Hide resolved
@ahcorde
Fixed doc
021d150 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
package.xml following the schema order
139b38a 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Using rcpputil function to get library name
f8d2d73 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Removed redundant target_link_libraries
bb0a187 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Fixed failure in macos
e369b9a 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Author

ahcorde commented Apr 3, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ahcorde
Fixed appveyor
347a9f6 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Fixed Travis
6ebd81b 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/feature/removed_poco branch from fc215ec to 6ebd81b Compare April 3, 2020 16:47
@ahcorde
Fixed travis and appveyor
d8dfa7f 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Fixed travis and appveyor
1b6fa0f 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde mentioned this pull request Apr 3, 2020
Merged
@ahcorde
Another try to fix travis
3d51027 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Restore try catch when unload_library
2fd6c8e 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
updated travis to bionic
5db8ca8 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
wakey wakey GitHub Actions
2fbe8df 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Author

ahcorde commented Apr 8, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ahcorde ahcorde merged commit fe6fa9f into ros:ros2 Apr 8, 2020
@emersonknapp emersonknapp mentioned this pull request Apr 8, 2020
Merged
jacobperron added a commit that referenced this pull request Apr 9, 2020
@jacobperron
Revert "Removed poco dependency (#139)"
c0b38bf 
This reverts commit fe6fa9f.
@jacobperron jacobperron mentioned this pull request Apr 9, 2020
Closed
@cottsay cottsay mentioned this pull request Apr 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas left review comments

@wjwwood wjwwood wjwwood approved these changes

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
4 participants
@ahcorde @ahoarau @wjwwood @dirk-thomas