-
Notifications
You must be signed in to change notification settings - Fork 342
backwards-cpp support for gz-sim-main and gz-sim-gui #2919
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
Conversation
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
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.
I tried this branch and introduced a segfault, however the stack traces are not being printed. I'll go through and check more thoroughly.
So after reverting the version of backward to the current one, I managed to link it to I don't think the above branch has the ideal solution however. TBH the CMake-fu going on in the library exceeds my domain of knowledge. One option is to get rid of all the cmake stuff and only include the header. we can manually instantiate I think whats happening is that LMK what you think @j-rivero |
Signed-off-by: Jose Luis Rivero <[email protected]>
Makes sense. The build log does not display any modifcation of code being done by the linker. re-reading the backwards-cpp lib instructions we can use either the header file or the .cpp file. I think that you are using the later when linking against Said that, I've brought the backwards-cpp code into the main/gui in 42383a5 by including the headers and the signal handler. Segfaulting on propose triggers clear stacktraces on my system after using |
This reverts commit 42383a5.
Signed-off-by: Jose Luis Rivero <[email protected]>
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.
This works great for the binaries but introduces a regression on the tests where segfaulting tests no longer print stack traces.
CMakeLists.txt
Outdated
|
||
if (UNIX AND NOT APPLE) | ||
set (EXTRA_TEST_LIB_DEPS stdc++fs backward_object) | ||
set (EXTRA_TEST_LIB_DEPS stdc++fs Backward::Backward) |
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.
Sadly, this won't work for the tests, perhaps Backward::Object
? Or linking tests with whole-archive options enabled.
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
You are right. I've reverted the usage of the header-only files, I think that it is more intrusive for the code 255dd43. For using the linking approach and the "object" CMake target we need to stop using the find_package and use the add_subdirectory as detailed in the backwards-cpp README. Changing that and using For the special case of Seems good enough for me after tesing tests and binaries 0109512 |
Signed-off-by: Arjo Chakravarty <[email protected]>
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.
Thanks! LGTM! There was a minor style issue with regards to spacing, I've pushed a commit that fixes it.
Ubuntu servers are having a bad day. To merge the PR I wanted to give a look into how much overload in time we are adding with this PR. Comparing a couple of build from the same day, 28 May:
Let's see if we can get data from today builds when servers are back. |
@osrf-jenkins run tests |
install( | ||
FILES "BackwardConfig.cmake" | ||
DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}" | ||
) |
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.
this reverted #2838, and @j-rivero predicted this fragility in #2838 (comment)
for now the debbuild is unstable due to these installed files since BackwardConfig.cmake is not included in any package install rules
@j-rivero I think we want to avoid installing $ dpkg -L libgz-sim10-dev | rg backward
/usr/include/backward.hpp |
There are different workarounds for this including the modification of the files imported that we did in #2838 or the customization of our deb metadata. The first is fragile as time has demonstrated, the second is only affecting .deb installations and leaving other packagers/builds with the problem. There is an open PR with changes close to what Steve was proposing bombela/backward-cpp#338. I'm include to resurrect the code changes and document it a bit better for gaining visibility on the changes. I'll open a PR. |
🎉 New feature
Closes #2906
Summary
The PR adds support for backwards-cpp in gz-sim-main and gz-sim-gui. To do so:
Checklist
codecheck
passed (See contributing)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.