-
Notifications
You must be signed in to change notification settings - Fork 221
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
Copy windows debug fixes for pybind11 #681
Conversation
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 is clearly an improvement, as Windows Debug no longer fails. However, there are still warnings present in the Windows Debug build. @sloretz any thoughts on what's causing that?
Implemented workaround to make sure Py_DEBUG on Anyways, if that's correct then this build should have no warnings: |
Heh, it got worse: 2 warnings -> 4 warnings :). |
So looking at the compile line for one of the files showing the problem, I see this (I split the lines for clarity):
It is clear from that command-line that Py_DEBUG is being set to an empty macro. The compile warning is this:
That warning comes from the
A few questions/observations then:
|
How'd you get the compile commands on Windows?
It looks like the compiler sets |
I did:
|
Yeah, can confirm with an MRE that windows complains when a compiler definition is set both on the compiler command line and in code. |
rclpy/CMakeLists.txt
Outdated
# Workaround to ensure rclpy_common has same definition for Py_DEBUG | ||
# TODO(sloretz) remove this workaround when using pybind11 2.6+ | ||
get_target_property(_rclpy_compile_definitions _rclpy COMPILE_DEFINITIONS) | ||
foreach(_rclpy_definition ${_rclpy_compile_definitions}) | ||
if("Py_DEBUG" STREQUAL _rclpy_definition) | ||
target_compile_definitions(rclpy_common PUBLIC Py_DEBUG) | ||
break() | ||
endif() | ||
endforeach() |
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.
Given what we now know, it seems like this isn't the correct workaround. That is, we specifically don't want to add Py_DEBUG
to the command-line, since that ends up with the warning. It kind of looks to me like we need to remove this, and then see if there is something else that is also adding Py_DEBUG
to the command-line. If it is pybind11, maybe the thing to do here is to augment clean_windows_flags
to also remove that flag?
Copies windows debug fixes from rosbag2_py for using Pybind11 on Windows debug and RelWithDebInfo. For more info see the original PRs ros2/rosbag2#538 ros2/rosbag2#531 Signed-off-by: Shane Loretz <sloretz@openrobotics.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
f9ab18e
to
0d84422
Compare
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Putting everything together, the Python interpreter has a special debug mode, and it uses the macro One might think, just don't let As long as One way to fix this is to include
This seems like the best fix, but it needs to be undone once the modules are using Pybind11. I made it in a separate macro in 1932bee so I could apply it to only the modules that have not yet been converted. |
Woohoo! I also tested this fix locally, and it seems to do the trick for me. I'll do a brief review here, and then we can merge. Thanks for the work. |
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.
The changes look OK to me, so approving. One question: should we open up an issue to track reverting these changes once everything is converted to PyBind11? Or are you going to track it in #665?
Hopefully fixes #680
Copies windows debug fixes from rosbag2_py for using Pybind11 on Windows
debug and RelWithDebInfo.
For more info see the original PRs
ros2/rosbag2#538
ros2/rosbag2#531
Signed-off-by: Shane Loretz sloretz@openrobotics.org
Signed-off-by: Shane Loretz sloretz@osrfoundation.org