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.

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mbharatheesha
Copy link
Contributor

@mbharatheesha mbharatheesha commented Oct 29, 2024

Description

In autoware_perception_rviz_plugin, the source code uses objects from packages that are:

  • either missing from package.xml
  • or they are listed under <depend> tag in package.xml but do not have a corresponding find_package() in CMakeLists.txt.

And in autoware_test_utils, there was a linking error as some of the functions from yaml-cpp were not available to link against autoware_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:

/usr/bin/ld: /nstall/autoware_test_utils/lib/libautoware_test_utils.so: undefined reference to `vtable for YAML::BadConversion'
/usr/bin/ld: /install/autoware_test_utils/lib/libautoware_test_utils.so: undefined reference to `YAML::detail::node_data::insert_map_pair(YAML::detail::node&, YAML::detail::node&)'
/usr/bin/ld: /install/autoware_test_utils/lib/libautoware_test_utils.so: undefined reference to `YAML::detail::memory::create_node()'
....
collect2: error: ld returned 1 exit status

And other errors related to objects from packages like visualization_msgs etc.

After the fixes in this PR, the packages autoware_perception_rviz_plugin and autoware_test_utils compile successfully.

Effects on system behavior

None.

Signed-off-by: Mukunda Bharatheesha <[email protected]>
@github-actions github-actions bot added the component:common Common packages from the autoware-common repository. (auto-assigned) label Oct 29, 2024
Copy link

github-actions bot commented Oct 29, 2024

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@mbharatheesha
Copy link
Contributor Author

@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.

@kyoichi-sugahara
Copy link
Contributor

@mbharatheesha
Thank you for your work!!! And I apologize for my delayed response. I really appreciate the ping via mention.

This change is needed for jazzy compatibility, correct?
The changes look fine, and while CI tests will catch any issues, I just want to confirm - is my understanding correct that these changes won't cause any problems in the humble environment?

@kyoichi-sugahara kyoichi-sugahara added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Nov 5, 2024
@mbharatheesha
Copy link
Contributor Author

mbharatheesha commented Nov 5, 2024

@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!

This change is needed for jazzy compatibility, correct?
The changes look fine, and while CI tests will catch any issues,

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 package.xml and CMakeLists.txt.

For example, rviz_common, is listed as <depend> in package.xml of autoware_perception_rviz_plugin. Meaning, it is a build, exec and test depend for this package. Since, it is also a build depend, it is just good practice to also have a find_package() entry for rviz_common. Similar logic applies to other changes too.

Furthermore, in the source files, some objects from rviz_common::xxx are instantiated. So, in the build order, if rviz_common hasn't compiled first or if it is not available in the install space, before autoware_perception_rviz_plugin is built, then, compilation might fail. (at least, this was the reason, I created this PR, because source build was occasionally failing as objects from rviz_common package weren't available when autoware_perception_rviz_plugin package was being built in my test workspace.)

I just want to confirm - is my understanding correct that these changes won't cause any problems in the humble environment?

I don't expect this change to cause any issues in humble.

kyoichi-sugahara

This comment was marked as duplicate.

Copy link
Contributor

@kyoichi-sugahara kyoichi-sugahara left a 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 👍

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.03%. Comparing base (f0f5736) to head (9fcc1f4).
Report is 24 commits behind head on main.

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     
Flag Coverage Δ *Carryforward flag
differential 28.74% <ø> (?)
total 29.04% <ø> (-0.01%) ⬇️ Carriedforward from 151a5c5

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Mukunda Bharatheesha <[email protected]>
@mbharatheesha
Copy link
Contributor Author

@1222-takeshi @YoshiRi A friendly ping on this PR after resolving merge conflicts with the latest master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:common Common packages from the autoware-common repository. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants