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.

Sign up for GitHub

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

Jump to bottom

Fix rosbag2_py bug when using libc++ #529

Merged
merged 18 commits into from
Oct 5, 2020
Merged

Fix rosbag2_py bug when using libc++ #529

merged 18 commits into from
Oct 5, 2020

Conversation

ivanpauno
Copy link
Member

This fixes a weird rtti bug in rosbag2_py tests when using.
I will re-add an AMENT_IGNORE file before merging, as #504 also needs a fix.

Fix #505.

@ivanpauno ivanpauno added the bug Something isn't working label Oct 2, 2020
@ivanpauno ivanpauno requested a review from a team as a code owner October 2, 2020 14:28
@ivanpauno ivanpauno self-assigned this Oct 2, 2020
@ivanpauno ivanpauno requested review from dabonnie and jikawa-az and removed request for a team October 2, 2020 14:28
@ivanpauno
Copy link
Member Author

ivanpauno commented Oct 2, 2020 •
edited
Loading

  • Linux with libc++ Build Status

mabelzhang and others added 12 commits October 2, 2020 11:34
@mabelzhang @ivanpauno
change workflow to autotest feature branch
4ee8e4e 
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang @ivanpauno
revert workflow
dc2fb5a 
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang @ivanpauno
add -Wunused-value for pybind clang error
9ed1b47 
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang @ivanpauno
add PYBIND11_EXPORT macro for symbol visibility
2425f24 
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang @ivanpauno
add pragma ignore -Wunused-value for pybind.h. Revert 2 previous atte…
4210ee9 
…mpts

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang @ivanpauno
add pragma ignore -Wunused-value to _storage
a49aa70 
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang @ivanpauno
disable tests if clang
78bb0d2 
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@ivanpauno
Fix clang bug
1c7f21f 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Revert random changes
f7a92cb 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
delete unneeded files
0047da0 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Please linters
c640355 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
pycache
e1b814e 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/fix_issue_505_linux branch from 0c0a9a1 to e1b814e Compare October 2, 2020 14:34
@ivanpauno
readd ament ignore
a952c5e 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

readded AMENT_IGNORE now that CI passed

mabelzhang
mabelzhang reviewed Oct 2, 2020
View reviewed changes
Copy link
Contributor

@mabelzhang mabelzhang 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 taking over this issue! I don't think I would have gotten this far. rtti is beyond me.

I think Windows CI has to pass too before we remove AMENT_IGNORE (I think that's what you meant too)?

rosbag2_py/test/test_sequential_reader.py Outdated Show resolved Hide resolved
rosbag2_py/test/test_sequential_writer.py Outdated Show resolved Hide resolved
rosbag2_py/CMakeLists.txt Show resolved Hide resolved
@ivanpauno
pybind11 include file
b8438fb 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
jacobperron
jacobperron approved these changes Oct 2, 2020
View reviewed changes
rosbag2_py/src/rosbag2_py/_writer.cpp Outdated Show resolved Hide resolved
@ivanpauno
nit
02d1e64 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
nit
68ecc4b 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
use private pybind11.hpp include eveywhere
0c76b14 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/fix_issue_505_linux branch from 6c1e230 to 0c76b14 Compare October 2, 2020 20:55
@ivanpauno
Copy link
Member Author

Force pushed to please the DCO checker.

@ivanpauno ivanpauno mentioned this pull request Oct 2, 2020
Merged
@mabelzhang
Copy link
Contributor

I just realized the CI above wasn't in debug mode. The problem was exposed only in debug mode. Can we rerun in debug mode? These are the parameters https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/513/

@ivanpauno
linters
72d3b64 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

I just realized the CI above wasn't in debug mode. The problem was exposed only in debug mode. Can we rerun in debug mode? These are the parameters https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/513/

It shouldn't make a difference, but trying again in debug mode too:

  • Linux with libc++ Build Status

This job is testing directly #531, which is this + fixes on Windows (this PR still has the AMENT_IGNORE file, so it's safe to merge without direct CI).

@ivanpauno
Copy link
Member Author

Merging this one, if there's any further comment I can address them in #531.

@ivanpauno ivanpauno merged commit 5150a73 into master Oct 5, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/fix_issue_505_linux branch October 5, 2020 13:08
@nuclearsandwich nuclearsandwich mentioned this pull request Oct 8, 2020
Closed
@nuclearsandwich
Copy link
Member

@ivanpauno I think this PR may have introduced the warnings in #537. Would you be able to check that and address them if that turns out to be the case?

@ivanpauno
Copy link
Member Author

@ivanpauno I think this PR may have introduced the warnings in #537

Yeah, this seems to be the root cause.

Would you be able to check that and address them if that turns out to be the case?

Yes, thanks for bringing this up.

emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
@ivanpauno @mabelzhang
Fix rosbag2_py bug when using libc++ (#529)
b63aa3b 
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Co-authored-by: Mabel Zhang <mabel@openrobotics.org>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
@ivanpauno @mabelzhang
Fix rosbag2_py bug when using libc++ (#529)
312713d 
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Co-authored-by: Mabel Zhang <mabel@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mabelzhang mabelzhang mabelzhang left review comments

@jacobperron jacobperron jacobperron approved these changes

@dabonnie dabonnie Awaiting requested review from dabonnie

@jikawa-az jikawa-az Awaiting requested review from jikawa-az

@emersonknapp emersonknapp Awaiting requested review from emersonknapp

Assignees

@ivanpauno ivanpauno

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rosbag2_py build failure in clang+libcxx nightly builds
5 participants
@ivanpauno @mabelzhang @nuclearsandwich @emersonknapp @jacobperron