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

chore(glog): add initialization check #6792

Merged
merged 1 commit into from
May 9, 2024

Conversation

TakaHoribe
Copy link
Contributor

@TakaHoribe TakaHoribe commented Apr 11, 2024

Description

Add IsGoogleLoggingInitialized() check before glog initialization as the multiple initializations terminate the process.

Related links

Must be merged after

Tests performed

  • Build
  • Run psim
  • Confirm the glog works correctly as before
    • kill -15 <pid> should show the stack trace for the glog-installed process, e.g. map_based_prediction
      image

Notes for reviewers

None

Interface changes

None

Effects on system behavior

None

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added component:perception Advanced sensor data processing and environment understanding. (auto-assigned) component:localization Vehicle's position determination in its environment. (auto-assigned) component:common Common packages from the autoware-common repository. (auto-assigned) component:evaluator Evaluation tools for planning, localization etc. (auto-assigned) labels Apr 11, 2024
@xmfcx xmfcx changed the title chore(glog): add initializetion check chore(glog): add initialization check Apr 11, 2024
@xmfcx xmfcx added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Apr 11, 2024
@xmfcx
Copy link
Contributor

xmfcx commented Apr 11, 2024

@TakaHoribe we used https://github.com/bombela/backward-cpp before for similar purposes and it was really good at printing the causes of segfaults.

Maybe you can try it too.

@xmfcx
Copy link
Contributor

xmfcx commented Apr 11, 2024

Also you need to add

  universe/external/glog:  # TODO(Horibe): to use isGoogleInitialized() API in v0.6.0. Remove when the rosdep glog version is updated to v0.6.0 (already updated in Ubuntu 24.04)
    type: git
    url: https://github.com/tier4/glog.git
    version: v0.6.0_t4-ros

to

@xmfcx
Copy link
Contributor

xmfcx commented Apr 11, 2024

It seems they made a ROS 2 package for it too: https://github.com/pal-robotics/backward_ros

I will test backward-cpp and open up a separate PR.

Never mind, I couldn't get it working with ROS 2 component containers. Let's keep using glog's signal handlers.

Tested again and it works:

@TakaHoribe TakaHoribe requested a review from kosuke55 as a code owner May 8, 2024 06:52
Copy link
Contributor

@KYabuuchi KYabuuchi left a comment

Choose a reason for hiding this comment

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

LGTM for the localization packages 👍

  • pose_estimator_arbiter
  • yabloc_common
  • yabloc_particle_filter
  • ndt_scan_matcher

@TakaHoribe
Copy link
Contributor Author

TakaHoribe commented May 8, 2024

@xmfcx Thank you for the investigation of backward-cpp. Let me merge this PR first. The backward-cpp looks much better than glog, but it still has an issue with ros-component. If the issue is sloved, I want to replace all glog with backward-cpp.

xmfcx
xmfcx previously approved these changes May 8, 2024
Copy link
Contributor

@xmfcx xmfcx left a comment

Choose a reason for hiding this comment

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

Of course, I agree.

@xmfcx xmfcx dismissed their stale review May 8, 2024 08:06

need to rebase

@xmfcx
Copy link
Contributor

xmfcx commented May 8, 2024

@TakaHoribe could you rebase to latest main? The CI is failing.

@TakaHoribe TakaHoribe force-pushed the glog-initialize-check branch from 4999452 to df57e69 Compare May 8, 2024 08:19
@xmfcx
Copy link
Contributor

xmfcx commented May 8, 2024

From above: #6792 (comment)

Also you need to add

  universe/external/glog:  # TODO(Horibe): to use isGoogleInitialized() API in v0.6.0. Remove when the rosdep glog version is updated to v0.6.0 (already updated in Ubuntu 24.04)
    type: git
    url: https://github.com/tier4/glog.git
    version: v0.6.0_t4-ros

to

Or CI won't pass. I thought we added this before but it was here. @TakaHoribe

@xmfcx
Copy link
Contributor

xmfcx commented May 8, 2024

I could add it too but you should Allow edits from maintainers on the PR.

@TakaHoribe
Copy link
Contributor Author

@xmfcx Thank you! I submitted it in a different PR: #6952

@xmfcx
Copy link
Contributor

xmfcx commented May 9, 2024

Now needs to be rebased again 🙏

@TakaHoribe TakaHoribe force-pushed the glog-initialize-check branch from df57e69 to d40be32 Compare May 9, 2024 02:28
@xmfcx xmfcx merged commit 788b2e3 into autowarefoundation:main May 9, 2024
20 of 22 checks passed
vividf pushed a commit to vividf/autoware.universe that referenced this pull request May 16, 2024
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
@TakaHoribe TakaHoribe deleted the glog-initialize-check branch July 31, 2024 08:04
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) component:evaluator Evaluation tools for planning, localization etc. (auto-assigned) component:localization Vehicle's position determination in its environment. (auto-assigned) component:perception Advanced sensor data processing and environment understanding. (auto-assigned) 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