Test load and lookup functionality. #135
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
CI up to
|
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
CI again on Windows to make sure warnings are gone:
|
rmw_implementation/CMakeLists.txt
Outdated
| target_include_directories(${PROJECT_NAME} PUBLIC | ||
| "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>" | ||
| "$<INSTALL_INTERFACE:include>") |
There was a problem hiding this comment.
I have to say that I'm a bit concerned about making this header file "public". If we put this in, then it seems like load_library and lookup_symbol are APIs that downstream consumers should be able to call. And as far as I can tell, that is not the case.
One alternative here is to do something similar to what rviz_default_plugins does. That is, put the header file in the src directory, and don't install it. Then have the tests include the header file via relative paths. I don't love that scheme, but it has the property that it keeps this implementation private (as it looks like it should be), but also makes the symbols available to tests. What do you think?
There was a problem hiding this comment.
I don't have a strong opinion in favor or against exposing rmw implementation lookup.
One alternative here is to do something similar to what rviz_default_plugins does.
I'm OK with that scheme too.
There was a problem hiding this comment.
Though that doesn't change the fact that the symbols will be public.
There was a problem hiding this comment.
Though that doesn't change the fact that the symbols will be public.
Just to recap a private conversation: while that is true, it is more around the messaging to external consumers. If a function is in a publicly-accessible, installed header, then that seems like something a user might want to call. If it is hidden away in a src directory, and the user has to go looking through symbols to find it, then it is pretty clear they are doing something unsupported.
There was a problem hiding this comment.
Agreed. See b885711. Went the extra mile to only make symbols public if building for testing.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| #ifndef FUNCTIONS_HPP_ |
There was a problem hiding this comment.
I think this will run afoul of the linters, but maybe they don't care about private headers? (I don't care either way)
There was a problem hiding this comment.
It was actually cpplint that forced me to use this guard.
There was a problem hiding this comment.
It was actually
cpplintthat forced me to use this guard.
Haha, OK.
|
@ros-pull-request-builder retest this please |
|
CI up to
|
|
Even though http://build.ros2.org/job/Rpr__rmw_implementation__ubuntu_focal_amd64/135/ failed on a rosdep timeout, http://build.ros2.org/job/Rpr__rmw_implementation__ubuntu_focal_amd64/136/ did pass. Not sure why it wasn't reflected here. |
|
Oh, there we go. It was my browser cache 😅 |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Precisely what the title says. It exposes a few symbols to enable some testing. It also refactors the implementation a bit to make it allow testing.