-
Notifications
You must be signed in to change notification settings - Fork 683
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: apply autoware_
prefix for system_monitor
#10017
feat: apply autoware_
prefix for system_monitor
#10017
Conversation
* The `system_monitor` operates independently from other modules in `autoware.universe`, so the `autoware_` prefix is added only to the package name. * The `autoware::` namespace is not used because C language does not support namespaces. * Headers are not moved under `include/autoware` to maintain compatibility for use outside the `autoware` context. * For users utilizing this package within `autoware.universe`, only the package name includes the `autoware_` prefix. This approach explains the unique namespacing and naming conventions for `system_monitor` compared to other packages. Signed-off-by: Junya Sasaki <junya.sasaki@tier4.jp>
Signed-off-by: Junya Sasaki <junya.sasaki@tier4.jp>
Signed-off-by: Junya Sasaki <junya.sasaki@tier4.jp>
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
@sasakisasaki |
Signed-off-by: Junya Sasaki <junya.sasaki@tier4.jp>
…ntext-awared' of github.com:sasakisasaki/autoware.universe into feat-apply-autoware-prefix-for-system-system-monitor-context-awared Signed-off-by: Junya Sasaki <junya.sasaki@tier4.jp>
@TetsuKawa Thank you! Now I fixed |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10017 +/- ##
==========================================
- Coverage 29.78% 29.10% -0.68%
==========================================
Files 1436 1459 +23
Lines 108308 110821 +2513
Branches 42956 43344 +388
==========================================
Hits 32256 32256
- Misses 72920 75433 +2513
Partials 3132 3132
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@sasakisasaki
|
Signed-off-by: Junya Sasaki <junya.sasaki@tier4.jp>
OMG, sorry for leaving the missing fix! @TetsuKawa Thank you so much for your careful check! Honestly, I was forgetting to apply the fix when I switch to the new branch from the old one 😅 . |
@TetsuKawa Also, I'll fix the |
Signed-off-by: Junya Sasaki <junya.sasaki@tier4.jp>
Now ready to review 👍 |
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.
LGTM
Currently, we only verified that the build succeeds. Further checks will be performed by using the updated module. This PR will be ready for review after finishing all the necessary checks/tests.
Executed Commands
As shown in the observed log below, I saw an error as follows. Please check if this is needed to be solved.
Different Points from Other Packages in Prefix-ing
As the commit message says, this PR follows the following points.
Directory Name
autoware_
as a prefix to the directory name.packages.xml
autoware_
to thename
element.CMakeLists.txt
project
name toautoware_***
.Addautoware::
toPLUGIN
.Header Files (.hpp)
Start#ifndef
guards withAUTOWARE__
.Addautoware::
tonamespace
.Source Files (.cpp)
Addautoware::
insideRCLCPP_COMPONENTS_REGISTER_NODE
.Launch Files
autoware_
beforefind-pkg-share
.node pkg="<pkgname>"
toautoware_<pkgname>
.Include
Addautoware
folder as necessary and update include statements accordingly.Verification Points
find-pkg-share
in autowaresensor_launch
as seen ingnss_poser
Observed Logs after ROS2 launch
Notes for Reviewers
As above.
Interface Changes
None.
Effects on System Behavior
All the developers who were using
system_monitor
needs to modify their code. This is maybe kind of non quick-win task.