-
Notifications
You must be signed in to change notification settings - Fork 650
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
fix(autoware_perception_rviz_plugin, autoware_test_utils): add missing packages and library linking #9178
base: main
Are you sure you want to change the base?
fix(autoware_perception_rviz_plugin, autoware_test_utils): add missing packages and library linking #9178
Conversation
Signed-off-by: Mukunda Bharatheesha <mukunda.bharatheesha@nobleo.nl>
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
@kyoichi-sugahara @soblin friendly ping regarding this PR. I can confirm that these changes do not cause any functionality breakage when I test these packages in this fork. |
@mbharatheesha This change is needed for jazzy compatibility, correct? |
@kyoichi-sugahara No worries. I can imagine it is a lot of effort for all of you guys too as there are so many PRs coming in. Many thanks for this amazing open source effort in the autoware project!
In principle, these changes aren't distro specific. Because the changes I have introduced are basically best practice recommendations regarding the way dependencies are specified in For example, Furthermore, in the source files, some objects from
I don't expect this change to cause any issues in |
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.
@mbharatheesha
Thank you so much for datailed explanation and I understood it. LGTM for me and approve it!
I run CI tests to confirm that build error doesn't happen in humble environment just in case.
Once necessary approves from reviewers are received please merge this PR 👍
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9178 +/- ##
==========================================
- Coverage 29.04% 29.03% -0.02%
==========================================
Files 1325 1329 +4
Lines 102577 102608 +31
Branches 39790 39784 -6
==========================================
- Hits 29797 29794 -3
- Misses 69930 69947 +17
- Partials 2850 2867 +17
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mukunda Bharatheesha <mukunda.bharatheesha@nobleo.nl>
@1222-takeshi @YoshiRi A friendly ping on this PR after resolving merge conflicts with the latest master. |
Description
In
autoware_perception_rviz_plugin
, the source code uses objects from packages that are:package.xml
<depend>
tag inpackage.xml
but do not have a correspondingfind_package()
inCMakeLists.txt
.And in
autoware_test_utils
, there was a linking error as some of the functions fromyaml-cpp
were not available to link againstautoware_test_utils
target.This was causing compilation errors when building from source. This PR adds the necessary fixes.
How was this PR tested?
Without the fixes in the PR, compilation would fail with:
And other errors related to objects from packages like
visualization_msgs
etc.After the fixes in this PR, the packages
autoware_perception_rviz_plugin
andautoware_test_utils
compile successfully.Effects on system behavior
None.