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

Included utils to load, unload and get symbols from shared libraries #215

Merged
merged 35 commits into from
Apr 1, 2020
Merged

Included utils to load, unload and get symbols from shared libraries #215

merged 35 commits into from
Apr 1, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Mar 10, 2020

As discussed in this PR ros/class_loader#139. It makes sense to include the logic of loading/unloading shared libraries and symbols in rcutils . These functions will be used in class_loader, rmw_implementation, rosidl_typesupport and rosbag2.

This other PR makes use of this new capability ros/class_loader#139

@ahcorde ahcorde added the enhancement New feature or request label Mar 10, 2020
@ahcorde ahcorde requested a review from wjwwood March 10, 2020 11:10
@ahcorde ahcorde self-assigned this Mar 10, 2020
@ahcorde ahcorde force-pushed the ahcorde/feature/shared_library branch 2 times, most recently from 5bcd3b4 to b29006f Compare March 10, 2020 11:13
@ahcorde
Included utils to load, unload and get symbols from shared libraries
268533b 
Shared library working on Windows

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/feature/shared_library branch from b29006f to 268533b Compare March 10, 2020 11:13
@ahcorde ahcorde mentioned this pull request Mar 10, 2020
Closed
7 tasks
dirk-thomas
dirk-thomas reviewed Mar 10, 2020
View reviewed changes
src/shared_library.c Outdated Show resolved Hide resolved
src/shared_library.c Outdated Show resolved Hide resolved
src/shared_library.c Outdated Show resolved Hide resolved
src/shared_library.c Outdated Show resolved Hide resolved
src/shared_library.c Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@ahcorde
platform independent code moved outside of the conditional blocks
076723e 
Signed-off-by: ahcorde <ahcorde@gmail.com>
dirk-thomas
dirk-thomas reviewed Mar 10, 2020
View reviewed changes
src/shared_library.c Outdated Show resolved Hide resolved
wjwwood
wjwwood requested changes Mar 10, 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.

I left some requested changes, thanks for factoring it out to this package, I think it will be useful here.

It would be nice to have a stanza in each of the functions like this one:

rcutils/include/rcutils/time.h

Lines 59 to 65 in af3e145

* <hr>
* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | Yes
* Uses Atomics | No
* Lock-Free | Yes

We may have to just say "* depends on underlying OS functions like dlopen()", etc... but at least it makes people calling these functions think about it when using it, even if it's not a clear yes/no for each.

include/rcutils/shared_library.h Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
src/shared_library.c Outdated Show resolved Hide resolved
src/shared_library.c Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Show resolved Hide resolved
@ahcorde ahcorde force-pushed the ahcorde/feature/shared_library branch 8 times, most recently from 232a58a to 54a0fa5 Compare March 11, 2020 17:12
@ahcorde
Included feedback
e87960b 
Signed-off-by: ahcorde <ahcorde@gmail.com>

Added shared library test

fix return error on shared library for windows

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/feature/shared_library branch from 54a0fa5 to e87960b Compare March 11, 2020 17:14
wjwwood
wjwwood requested changes Mar 11, 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.

I left some more comments. Also I think at least one of Dirk's comments still need addressing, about the linking in cmake I think.

Thanks for iterating on it!

include/rcutils/shared_library.h Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
src/shared_library.c Outdated Show resolved Hide resolved
src/shared_library.c Outdated Show resolved Hide resolved
src/shared_library.c Show resolved Hide resolved
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
@ahcorde
Exported CMake_DL_LIBS
696e0ea 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Included more feedback
955e9ae 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Improved tests
d1926b7 
Signed-off-by: ahcorde <ahcorde@gmail.com>
This was referenced Mar 13, 2020
Merged
Merged
@ahcorde
Fixed argument checks in rcutils_has_symbol
277b748 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde mentioned this pull request Mar 13, 2020
Merged
@ahcorde
Improved docs
13fe9ff 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
update shared library docs
af0d73e 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Avoided leak when a library is loaded two time without unload
8dc66df 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
changed the order to deallocate memory
e154f9c 
Signed-off-by: ahcorde <ahcorde@gmail.com>
dirk-thomas
dirk-thomas previously approved these changes Mar 31, 2020
View reviewed changes
@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 31, 2020 •
edited
Loading

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

@ahcorde
Added extension for different platforms in test_shared_libraries
124cf99 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
fixed ccplint
c6bbb2c 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Fixed dummy library name
80b7ea1 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Right library name in windows in test_shared_library
2779d6b 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
preprocessor directives not indented
8ab106f 
Signed-off-by: ahcorde <ahcorde@gmail.com>
dirk-thomas
dirk-thomas reviewed Mar 31, 2020
View reviewed changes
include/rcutils/shared_library.h Outdated Show resolved Hide resolved
src/shared_library.c Outdated Show resolved Hide resolved
@dirk-thomas dirk-thomas dismissed their stale review March 31, 2020 18:25

More changes

@ahcorde
fixed rcutils_snprintf buffer size
26b7157 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Added target_compile_definitions for dummy_shared_library
66187a3 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
defined len of str in rcutils_get_platform_library_name
510bf35 
Signed-off-by: ahcorde <ahcorde@gmail.com>
dirk-thomas
dirk-thomas reviewed Apr 1, 2020
View reviewed changes
src/shared_library.c Outdated Show resolved Hide resolved
src/shared_library.c Outdated Show resolved Hide resolved
@ahcorde
Removed limitation on the path size for the library name
e049c20 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
fixed condition len string in rcutils_get_platform_library_name
5cc468e 
Signed-off-by: ahcorde <ahcorde@gmail.com>
dirk-thomas
dirk-thomas approved these changes Apr 1, 2020
View reviewed changes
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

With passing CI.

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 1, 2020

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

@ahcorde ahcorde merged commit 35f2985 into master Apr 1, 2020
@delete-merged-branch delete-merged-branch bot deleted the ahcorde/feature/shared_library branch April 1, 2020 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Assignees

@ahcorde ahcorde

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahcorde @wjwwood @dirk-thomas