Copy windows debug fixes for pybind11 by sloretz · Pull Request #681 · ros2/rclpy

sloretz · 2021-02-16T22:29:58Z

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

sloretz · 2021-02-16T22:33:01Z

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

Linux
Linux-aarch64
macOS
Windows
Windows debug:

clalancette

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?

sloretz · 2021-02-17T17:02:42Z

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 rclpy_common matches setting on the Python Modules. This workaround should not be necessary on Pybind11 2.6 and beyond, because that adds Py_DEBUG as an interface compile definition to the pybind11::pybind11 target.

Anyways, if that's correct then this build should have no warnings:

Windows Debug

sloretz · 2021-02-17T17:52:18Z

Whoops, Windows debug again:

clalancette · 2021-02-17T20:34:54Z

Heh, it got worse: 2 warnings -> 4 warnings :).

sloretz · 2021-02-17T22:04:05Z

Windows Debug again

Made Py_DEBUG definition on rclpy_common PUBLIC so it's transitive - not sure why that would be needed.

clalancette · 2021-02-18T16:07:35Z

So looking at the compile line for one of the files showing the problem, I see this (I split the lines for clarity):

C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29333\bin\HostX64\x64\CL.exe
 /c /IC:\dev\ros2_ws\src\ros2\rclpy\rclpy\src\rclpy_common\include
 /IC:\dev\ros2_ws\install\include /IC:\Python38\include /Zi /nologo /W3 /WX- /diagnostics:column
 /Od /Ob0 /D WIN32 /D _WINDOWS /D RCLPY_COMMON_BUILDING_DLL /D Py_DEBUG /D RCUTILS_ENABLE_FAULT_INJECTION
 /D _CRT_SECURE_NO_WARNINGS /D DEFAULT_RMW_IMPLEMENTATION=rmw_cyclonedds_cpp /D "CMAKE_INTDIR=\"Debug\""
 /D rclpy_common_EXPORTS /D _WINDLL /D _MBCS /Gm- /RTC1 /MDd /GS /fp:precise /Zc:wchar_t /Zc:forScope
 /Zc:inline /Fo"rclpy_common.dir\Debug\\" /Fd"rclpy_common.dir\Debug\vc142.pdb" /Gd /TC
 /errorReport:queue C:\dev\ros2_ws\src\ros2\rclpy\rclpy\src\rclpy_common\src\common.c
 C:\dev\ros2_ws\src\ros2\rclpy\rclpy\src\rclpy_common\src\handle.c

It is clear from that command-line that Py_DEBUG is being set to an empty macro. The compile warning is this:

C:\Python38\include\pyconfig.h(321,1): warning C4005: 'Py_DEBUG': macro redefinition (compiling source file C:\dev\ros2_ws\src\ros2\rclpy\rclpy\src\rclpy_common\src\handle.c) [C:\dev\ros2_ws\build\rclpy\rclpy_common.vcxproj]
C:\dev\ros2_ws\src\ros2\rclpy\rclpy\src\rclpy_common\src\handle.c : message : see previous definition of 'Py_DEBUG' [C:\dev\ros2_ws\build\rclpy\rclpy_common.vcxproj]
C:\Python38\include\pyconfig.h(321,1): warning C4005: 'Py_DEBUG': macro redefinition (compiling source file C:\dev\ros2_ws\src\ros2\rclpy\rclpy\src\rclpy_common\src\common.c) [C:\dev\ros2_ws\build\rclpy\rclpy_common.vcxproj]
C:\dev\ros2_ws\src\ros2\rclpy\rclpy\src\rclpy_common\src\common.c : message : see previous definition of 'Py_DEBUG' [C:\dev\ros2_ws\build\rclpy\rclpy_common.vcxproj]

That warning comes from the pyconfig.h file is included, which has this bit in it:

#ifdef _DEBUG
#          define Py_DEBUG
#endif

A few questions/observations then:

Does Windows not allow redefining a macro, even if it is the same definition twice (as it appears to be in this case)?
What part of the system is setting _DEBUG on?
One way around this looks like to not define Py_DEBUG on the compile command-line. But that would seem to have been what we had earlier in this PR (before 35dfed7).

sloretz · 2021-02-18T17:20:23Z

How'd you get the compile commands on Windows?

What part of the system is setting _DEBUG on?

It looks like the compiler sets _DEBUG because /MDd is set: https://docs.microsoft.com/en-us/cpp/c-runtime-library/debug?view=msvc-160

clalancette · 2021-02-18T17:59:50Z

How'd you get the compile commands on Windows?

I did:

> set VERBOSE=1
> rd /Q /S build\rclpy
> python_d C:\Python38\Scripts\colcon.exe build --merge-install --cmake-args -DCMAKE_BUILD_TYPE=Debug --packages-select rclpy --event-handlers console_direct+

sloretz · 2021-02-20T05:02:40Z

Does Windows not allow redefining a macro, even if it is the same definition twice (as it appears to be in this case)?

Yeah, can confirm with an MRE that windows complains when a compiler definition is set both on the compiler command line and in code.

clalancette · 2021-02-22T15:26:40Z

rclpy/CMakeLists.txt

+# 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()


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>

sloretz · 2021-02-23T23:10:43Z

Windows debug is green 🎉

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.

Putting everything together, the Python interpreter has a special debug mode, and it uses the macro Py_DEBUG to enable or disable this mode. The source of the warning is pyconfig.h sets Py_DEBUG when _DEBUG is set, and pybind11_add_module sets Py_DEBUG via the command line when a debug Python interpreter is used. This definition being set in two places is why the warning comes up.

One might think, just don't let pybind11_add_module set Py_DEBUG and all would be fine, but Pybind11 has some extra logic to allow using the Python release interpreter when building in Debug mode. That logic also uses Py_DEBUG. If _DEBUG is set but Py_DEBUG is not, then it will undefine _DEBUG, include Python.h, then restore _DEBUG. If we remove Py_DEBUG then _DEBUG will get unset and CPython will link against release Python libs instead of the debug Python libs.

As long as Py_DEBUG is set and pybind11/pybind11.h was included instead of or before <Python.h>, then everything is happy. If code uses pybind11_add_module but includes Python.h, then there's the duplicate definition warning.

One way to fix this is to include pybind11/pybind11.h instead of Python.h, but that requires changing the language to C, and not everything is happy about that yet. Once all the PRs converting modules to use Pybind11 are in then this will be solved.

If it is pybind11, maybe the thing to do here is to augment clean_windows_flags to also remove that flag?

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.

clalancette · 2021-02-24T13:14:33Z

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.

clalancette

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?

sloretz · 2021-02-24T21:36:28Z

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

Linux
Linux-aarch64
macOS
Windows

sloretz · 2021-02-24T21:36:57Z

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?

I'll track it in #665. It should be removed in each PR as it's merged.

sloretz added the bug Something isn't working label Feb 16, 2021

sloretz self-assigned this Feb 16, 2021

clalancette reviewed Feb 17, 2021

View reviewed changes

clalancette mentioned this pull request Feb 19, 2021

Change ros_environment to the rolling branch ros2/ros2#1089

Merged

clalancette reviewed Feb 22, 2021

View reviewed changes

sloretz added 4 commits February 23, 2021 11:30

Workaround for consistent Py_DEBUG macro

df2e758

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

fix argument order

e3fc27c

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Does PUBLIC Py_DEBUG resolve differences?

0d84422

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz force-pushed the rclpy_pybind11_windows_debug_fixes branch from f9ab18e to 0d84422 Compare February 23, 2021 19:30

sloretz added 2 commits February 23, 2021 11:38

Don't include Python.h if including pybind11.h

efc9b6d

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Remove Py_DEBUG if target doesn't use pybind11

1932bee

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

clalancette approved these changes Feb 24, 2021

View reviewed changes

sloretz mentioned this pull request Feb 24, 2021

Use Pybind11 for rclpy Python Bindings #665

Closed

34 tasks

sloretz merged commit 66da648 into master Feb 24, 2021

delete-merged-branch bot deleted the rclpy_pybind11_windows_debug_fixes branch February 24, 2021 23:33

sloretz mentioned this pull request Feb 26, 2021

Use Pybind11 to generate _rclpy_logging #659

Merged

clalancette mentioned this pull request Feb 26, 2021

👨‍🌾 Warning in windows packaging jobs "ignoring '/INCREMENTAL' due to '/LTCG' specification" #687

Closed

sloretz mentioned this pull request Feb 27, 2021

Clean windows flags on _rclpy_pybind11 and _rclpy_action #688

Merged

nuclearsandwich mentioned this pull request Mar 1, 2021

msbuild warning in Windows debug nightlies since 2021-02-27 #689

Closed

Conversation

sloretz commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented Feb 16, 2021

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

sloretz commented Feb 17, 2021

Uh oh!

sloretz commented Feb 17, 2021

Uh oh!

clalancette commented Feb 17, 2021

Uh oh!

sloretz commented Feb 17, 2021

Uh oh!

clalancette commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented Feb 18, 2021

Uh oh!

clalancette commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented Feb 20, 2021

Uh oh!

clalancette Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

sloretz commented Feb 23, 2021

Uh oh!

clalancette commented Feb 24, 2021

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

sloretz commented Feb 24, 2021

Uh oh!

sloretz commented Feb 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sloretz commented Feb 16, 2021 •

edited

Loading

clalancette commented Feb 18, 2021 •

edited

Loading

clalancette commented Feb 18, 2021 •

edited

Loading