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

feat(autoware_vehicle_info_utils): porting from universe to core #183

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

Conversation

NorahXiong
Copy link
Contributor

@NorahXiong NorahXiong commented Feb 7, 2025

Description

We are porting autoware_vehicle_info_utils to autoware.core, and this PR adds the package to core.

Following modification were added:

  • reset version in package.xml and remove CHANGELOG.rst
  • modify include file directory structure to match with Autoware Coding Guideline

This must be merged with this PR which removes the package in Autoware Universe.

Related links

Parent Issue:

How was this PR tested?

It is tested using TIER IV evaluator: https://evaluation.ci.tier4.jp/evaluation/reports/ad7243c5-c593-5ed0-8075-c3bc57f60d22?project_id=prd_jt

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

Copy link

github-actions bot commented Feb 7, 2025

Thank you for contributing to the Autoware project!

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

Please ensure:

@liuXinGangChina
Copy link
Contributor

mits san and kondo san @mitsudome-r @youtalk

Since Kang is still having his holiday, We close the original pr and create this new pr which include the modification that mits san required today.

Best regards

心刚

mitsudome-r
mitsudome-r previously approved these changes Feb 7, 2025
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 55.44554% with 45 lines in your changes missing coverage. Please review.

Project coverage is 71.68%. Comparing base (4cb18f5) to head (4b1536a).
Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
...n/autoware_vehicle_info_utils/src/vehicle_info.cpp 54.34% 18 Missing and 3 partials ⚠️
...ehicle_info_utils/test/test_vehicle_info_utils.cpp 40.62% 0 Missing and 19 partials ⚠️
...ware_vehicle_info_utils/src/vehicle_info_utils.cpp 77.27% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
- Coverage   78.75%   71.68%   -7.08%     
==========================================
  Files          11       14       +3     
  Lines         193      279      +86     
  Branches       73      118      +45     
==========================================
+ Hits          152      200      +48     
- Misses         11       31      +20     
- Partials       30       48      +18     
Flag Coverage Δ *Carryforward flag
differential 55.44% <55.44%> (?)
total 80.89% <ø> (+2.14%) ⬆️ Carriedforward from a11a066

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

@mitsudome-r mitsudome-r dismissed their stale review February 12, 2025 07:44

needs another review

@mitsudome-r mitsudome-r self-requested a review February 12, 2025 07:44
@mitsudome-r
Copy link
Member

I get errors when I try to build this with Autoware.Universe.
It seems like the modifications for changing autoware_universe_utils to autoware_utils functions are causing errors in dependent packages.

@liuXinGangChina
Copy link
Contributor

I get errors when I try to build this with Autoware.Universe. It seems like the modifications for changing autoware_universe_utils to autoware_utils functions are causing errors in dependent packages.

Got it, mits san @mitsudome-r

I will look into this one with Norah

Have a nice weekend!

心刚

Copy link
Contributor

@sasakisasaki sasakisasaki left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! It seems there are some duplicated headers as follows. Could you fix please?

vehicle_info.hpp

The same name header exists as follows:

  • common/autoware_vehicle_info_utils/include/autoware/vehicle_info_utils/vehicle_info.hpp
  • common/autoware_vehicle_info_utils/include/autoware_vehicle_info_utils/vehicle_info.hpp

vehicle_info_utils.hpp

The same name header exists as follows:

  • common/autoware_vehicle_info_utils/include/autoware/vehicle_info_utils/vehicle_info_utils.hpp
  • common/autoware_vehicle_info_utils/include/autoware_vehicle_info_utils/vehicle_info_utils.hpp

@sasakisasaki
Copy link
Contributor

sasakisasaki commented Feb 17, 2025

The reason why we observe the following error (EDITED: described in bottom of this my comment) is:

  • The original autoware_vehicle_info_utils has the dependency for autoware_universe_utils (see the attached screenshot below)
  • The autoware_collision_detector has the dependency for autoware_vehicle_info_utils, but does not have the dependency for the autoware_universe_utils while autoware_collision_detector is using autoware_universe_utils
  • Thus, the autoware_collision_detector was using the autoware_universe_utils via autoware_vehicle_info_utils

@mitsudome-r So I guess something like as follows looks needed on the universe side. Do you have any nice idea for resolving this issue? Thanks!

  • On the universe side, fix so the autoware_collision_detector uses autoware_utils
  • Add the dependency autoware_universe_utils for autoware_collision_detector

Screenshot that of this PR

Screenshot from 2025-02-17 21-09-15

Error

***/src/autoware/universe/control/autoware_collision_detector/include/autoware/collision_detector/node.hpp:18:10: fatal error: autoware/universe_utils/ros/polling_subscriber.hpp: No such file or directory
   18 | #include <autoware/universe_utils/ros/polling_subscriber.hpp>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
gmake[2]: *** [CMakeFiles/autoware_collision_detector.dir/build.make:76: CMakeFiles/autoware_collision_detector.dir/src/node.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:139: CMakeFiles/autoware_collision_detector.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2
---
Failed   <<< autoware_collision_detector [3.54s, exited with code 2]

Summary: 18 packages finished [12.3s]
  1 package failed: autoware_collision_detector
  1 package had stderr output: autoware_collision_detector

@sasakisasaki
Copy link
Contributor

sasakisasaki commented Feb 18, 2025

Including me, our reviewer will also check if there is no any missing stuff. On your side, please fill "How was this PR tested?" in the PR description to prevent from something unexpected regression. Thank you!

@sasakisasaki
Copy link
Contributor

I'm now preparing a PR for resolving the autoware_collision_detector's dependency.

@mitsudome-r
Copy link
Member

This must be merged after: autowarefoundation/autoware.universe#10167

@sasakisasaki
Copy link
Contributor

sasakisasaki commented Feb 20, 2025

@NorahXiong As this PR is kind of a breaking change, please enhance the PR description as mentioned here. Careful checks can be done with the information described in the PR description. If no assumed information is provided, it'll be difficult to handle an urgent situation (e.g. fixing build error by manual operation, revert back some changes, ... etc).

@NorahXiong
Copy link
Contributor Author

@NorahXiong As this PR is kind of a breaking change, please enhance the PR description as mentioned here. Careful checks can be done with the information described in the PR description. If no assumed information is provided, it'll be difficult to handle an urgent situation (e.g. fixing build error by manual operation, revert back some changes, ... etc).

@sasakisasaki I thought it was a refactor guideline(mentioned here).

@mitsudome-r Sorry for not building the whole project after the intermediate submission because I didn't notice the autoware.universe CICD was not actually triggerred. I will strictly follow the test procedures.

@mitsudome-r
Copy link
Member

@NorahXiong Thanks. Current Autoware.Universe CI isn't capable of detecting removed packages as modified package. We would like to fix in the future, but it would be nice if you can test it locally for the meantime.

Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

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

The duplicated headers are for the backward compatibility. LGTM

@youtalk
Copy link
Member

youtalk commented Feb 21, 2025

I've committed 4b1536a to resolve the pre-commit problem.

@sasakisasaki sasakisasaki dismissed their stale review February 21, 2025 06:56

As the review for the target files are performed

@sasakisasaki
Copy link
Contributor

sasakisasaki commented Feb 21, 2025

The remaining points we need to ensure are:

  • Improvement of the PR description (merge strategy, how this is tested, ... etc)
    • This becomes important when the urgent situation on where we need to trace the change history and its reasons
  • Tests by evaluator on the universe side succeeds

After solving all these stuffs, I think we can merge all the dependency-involved PR"s" at the same time.

@mitsudome-r
Copy link
Member

@mitsudome-r
Copy link
Member

@sasakisasaki I have updated the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants