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

Attempts to print a stacktrace on segfault in our integration tests #2776

Open
wants to merge 14 commits into
base: gz-sim9
Choose a base branch
from

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Feb 14, 2025

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎉 New feature

Closes #

Depends on: gazebo-tooling/action-gz-ci#81

Summary

Our integration tests often segfault in CI leaving us clueless as to whats happening. This commit is an attempt to vendor backward-cpp and at the same time allows us to get a stacktrace of our segfaulting tests in CI. The current status of this PR is draft.

Other options considered:

We vendor backward-cpp in gz-launch and in gz-utils. Sadly the ubuntu package is not very useful as no cmake targets are provided. I could try to vendor it in gz-cmake or write a bespoke FindBackward package. However, our goal is to use it in tests. This is a very specific use case and I feel maybe the simplest option is to vendor it here and link it against the tests. That way we do not introduce more dependencies via 3rd party package managers and the user has no need to have anything else installed.

Test it

This commit has an example of a test I purposely introduced a segfault in. You can take a look at the github runners and see the stacktrace, Unfortunately, for Jenkins, there may need to be some minor changes on the buildfarm side. In particular we need to install libdwarf-dev, libdw-dev and binutils-dev. We will also need to build with RelWithDebInfo.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Our integration tests often segfault in CI leaving us clueless as to
whats happening. This commit is an attempt to vendor backward-cpp and
at the same time allows us to get a stacktrace of our code. The current
status of this PR is draft.

Other options considered:

We vendor backward-cpp in `gz-launch` and in `gz-utils`. Sadly the
ubuntu package is not very useful as no cmake targets are provided. I
could try to vendor it in gz-cmake or write a besopoke `FindBackward`
package. However, our goal is to use it in tests. This is a very
specific use case and I feel maybe the simplest option is to vendor it
here and link it against the tests. That way we do not introduce more
dependencies via 3rd party package managers and the user has no need to
have anything else installed.

Signed-off-by: Arjo Chakravarty <[email protected]>
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Feb 14, 2025
@arjo129
Copy link
Contributor Author

arjo129 commented Feb 18, 2025

While we can get a stacktrace, we are missing debug symbols. We probably need a PR on the CI side that enables building with debug symbols.

image

image

For reference on my PC this is what I get:
image

Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 linked an issue Feb 18, 2025 that may be closed by this pull request
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129
Copy link
Contributor Author

arjo129 commented Feb 18, 2025

Update: Prints a pretty trace on github ci.
image

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 marked this pull request as ready for review February 19, 2025 07:36
@arjo129 arjo129 requested a review from mjcarroll as a code owner February 19, 2025 07:36
Copy link

codecov bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (gz-sim9@2a9ee98). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff             @@
##             gz-sim9    #2776   +/-   ##
==========================================
  Coverage           ?   68.86%           
==========================================
  Files              ?      343           
  Lines              ?    33194           
  Branches           ?        0           
==========================================
  Hits               ?    22859           
  Misses             ?    10335           
  Partials           ?        0           

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

@j-rivero
Copy link
Contributor

Thanks for the PR Arjo ! Quite cool. Some comments:

We vendor backward-cpp in gz-launch and in gz-utils.

I think that we should try to stop that practice and find a way to generalize it for all the libraries if possible.

Sadly the ubuntu package is not very useful as no cmake targets are provided.

Can you elaborate on this point? I've checked the building rules of the Debian package and seems quite standard as well as the rest of packaging. I see latest 1.6 version available in the LTSs of Ubuntu.

I could try to vendor it in gz-cmake or write a bespoke FindBackward package.

We could add some cmake code to gz-cmake that handles: the optional use of backward-cpp, the look for the system support or internal vendoring (to keep support for gz-utils/gz-launch) and the extra linking needed. Maybe creating a new FEATURES block for the gz_build_tests, something like:

gz_build_tests(TYPE UNIT
  SOURCES
     ${gtest_sources}
  LIB_DEPS
     ....
  FEATURES
     USE_BACKWARDS_CPP=ON

That way we do not introduce more dependencies via 3rd party package managers and the user has no need to have anything else installed.

Those are nice benefits but I'm afraid that are not free. There are cons affecting the building times and specially the maintenance effort that increases a lot when more libraries are embedding new copies of it.

@arjo129
Copy link
Contributor Author

arjo129 commented Feb 20, 2025

First of all backward-cpp is touted as a header-only library. All its logic exists in its header. The author has provided support for installing it via FetchContent however if you look through the CMakeFile for Backward-cpp only the header files are installed along with a cmake config.

Sadly the ubuntu package is not very useful as no cmake targets are provided.

In ubuntu we have libbackward-cpp-dev. This comes with the following:

libbackward-cpp-dev: /usr/include/backward.hpp
libbackward-cpp-dev: /usr/lib/backward/BackwardConfig.cmake
libbackward-cpp-dev: /usr/share/doc/libbackward-cpp-dev/changelog.Debian.gz
libbackward-cpp-dev: /usr/share/doc/libbackward-cpp-dev/copyright

There is no cmake target provided or pkg config provided. Additionally, the binary object is not installed. As far as things are concerned, this is not great. We can't FindPackage(backward-cpp) or link to backward_object. We probably could get away with just including backward via #include <backward.hpp> and have a custom cmake check if the backward header exists (I would still have to include backward.cc in my own project.) but now lets talk of other platforms.

Fedora: No backward-cpp package
Brew: No backward-cpp package exists
Conda: A package exists and cmake targets exist as well. i.e. You can FindPackage(.

At the end of the day we will still have to vendor backward.cc as we have been doing for the other platforms (except conda).

As far as this PR is concerned shoving the library here in the tests directory has no downstream affect on our users. Its just like shoving gtest in our tests directory.

FWIW: There is a backward_ros package and they also vendor it seperately thanks to pecularities of the library.

@j-rivero
Copy link
Contributor

There is no cmake target provided or pkg config provided. Additionally, the binary object is not installed. As far as things are concerned, this is not great. We can't FindPackage(backward-cpp)

The package is providing a BackwardConfig.cmake is it different than the vendoring experience?

/tmp/foo via △ v3.28.3 ❯ cat CMakeLists.txt 
cmake_minimum_required(VERSION 3.10)
project(check_backward)
find_package(Backward)

/tmp/foo via △ v3.28.3 ❯ cmake .
-- The C compiler identification is GNU 13.3.0
-- The CXX compiler identification is GNU 13.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found libdw: /usr/lib/x86_64-linux-gnu/libdw.so  
-- Could NOT find libbfd (missing: LIBBFD_LIBRARY LIBBFD_INCLUDE_DIR) 
-- Could NOT find libdwarf (missing: LIBDWARF_LIBRARY LIBDWARF_INCLUDE_DIR) 
-- Found Backward: /usr/lib/backward  
-- Configuring done (0.3s)
-- Generating done (0.0s)
-- Build files have been written to: /tmp/foo

or link to backward_object

Being a header-only package, upstream probably expect the consumer package to take care of handling the compilations units.

Fedora: No backward-cpp package
Brew: No backward-cpp package exists

Fedora would not a blocker for us in this moment but for Brew we would need to patch the package in the Brew Formula. That is more like a problem.

Conda: A package exists and cmake targets exist as well. i.e. You can FindPackage(

If I'm looking the files correctly, it does install the same files than Ubuntu.

Not going to block the PR that is quite useful. I've created gazebosim/gz-cmake#477 to follow possible future works there.

@iche033
Copy link
Contributor

iche033 commented Feb 21, 2025

just want to double check on what's the best practice for vendoring an external package or using a header only library? Do we need to vendor all the files from package, e.g. doc and test src files? In other gz packages, I see that we're keeping a copy of only the necessary hdr / src files: e.g. gz-launch's backward vendor directory only includes the backward cmake and hpp files, and gz-common/graphics/src/VHACD only has the hpp file.

@@ -27,13 +27,15 @@ jobs:
extra_args: --all-files
- name: Compile and test
id: ci
uses: gazebo-tooling/action-gz-ci@jammy
uses: gazebo-tooling/action-gz-ci@jammy # Temporary for testing
Copy link
Contributor

Choose a reason for hiding this comment

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

is this comment still needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic
Projects
Status: Inbox
Development

Successfully merging this pull request may close these issues.

Consider adding Backward-cpp for gz_build_tests
3 participants