Pull Request for adding scope keywords feature in rosidl_target_interfaces (Issue#400) #798
Conversation
FahadRazaKhan
requested review from
adityapande-1995,
methylDragon,
quarkytale and
sloretz
as code owners
|
Do I need to sign-off my commit? |
FahadRazaKhan
changed the title
Yes. Besides that, please remove other changes to this PR that don't directly have to do with the issue you are trying to fix. It makes it difficult to review and see what is actually being changed. |
|
@clalancette Thanks for the feedback. For the sign-off, should I rebase my branch or is it unsafe? |
Yes, rebasing is fine.
Yes. You'll need to make sure your change is "clean" on top of those, so we only see the differences you are trying to make. When you rebase to add the sign-off, you should also make sure this is the case. |
FahadRazaKhan
force-pushed
the
rolling
branch
2 times, most recently
from
d4d5add to
c870a90
Compare
…target_interfaces function. The feature is added for the better compatibility with the target_link_libraries command as requested in issue#400 Signed-off-by: Fahad <fahad.raza@desaysv.com>
|
@clalancette I have removed the other changes. Can you please review. Thanks. |
Can you be more specific, sorry I did not get it. |
|
@clalancette and @fujitatomoya any update? |
im sorry, bad memory 😓 I think what i mean was to add an explanation in function doc header, line 22 - |
Signed-off-by: Fahad <fahad.raza@desaysv.com>
|
@fujitatomoya thank you for your explanation. I have added a few comments in the doc header as you suggested. Please have a review. |
|
@fujitatomoya just a reminder that I have updated the doc header. |
|
@clalancette Hi, it's been a month since I updated the PR. I believe @fujitatomoya is busy so can someone else review and approve it please? |
|
@fujitatomoya any update? are you asking someone else for another review? |
|
@clalancette @sloretz can you have a look when you have time? |
|
@clalancette @sloretz @fujitatomoya guys this is a long pending PR. Would you please give it a final review and accept for merge. Thanks. |
|
Thank you for the PR and patience @FahadRazaKhan . The bad news is I don't think we should take this PR because this method has been deprecated for three years. The good news is there's already a solution to the problem you described that you can use today. Say you want to use C++ messages from the same package, and you have a line like this: rosidl_target_interfaces(target_name ${PROJECT_NAME} "rosidl_typesupport_cpp")Change it to this rosidl_get_typesupport_target(cpp_typesupport_target ${PROJECT_NAME} "rosidl_typesupport_cpp")
target_link_libraries(target_name PRIVATE "${cpp_typesupport_target}")See this PR for a real example: https://github.com/ros2/common_interfaces/pull/156/files |
This PR is for the issue#400 "Using rosidl_target_interfaces incompatible with keyword target_link_libraries command". As rosidl_target_interfaces did not have the scope keyword options it could become incompatible with the target_link_libraries command if used with a scope keyword.