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

Copy windows debug fixes for pybind11 #681

Merged
merged 6 commits into from
Feb 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions rclpy/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,73 @@ find_package(rcutils REQUIRED)
find_package(rmw REQUIRED)
find_package(rmw_implementation_cmake REQUIRED)

# Find python before pybind11
find_package(python_cmake_module REQUIRED)
find_package(PythonExtra REQUIRED)
if(WIN32 AND CMAKE_BUILD_TYPE STREQUAL "Debug")
# Set the python debug interpreter.
# pybind11 will setup the build for debug now.
set(PYTHON_EXECUTABLE "${PYTHON_EXECUTABLE_DEBUG}")
endif()

find_package(pybind11 REQUIRED)

if(WIN32 AND CMAKE_BUILD_TYPE STREQUAL "Debug")
# pybind11 logic for setting up a debug build when both a debug and release
# python interpreter are present in the system seems to be pretty much broken.
# This works around the issue.
set(PYTHON_LIBRARIES "${PYTHON_DEBUG_LIBRARIES}")
endif()

function(clean_windows_flags target)
# Hack to avoid pybind11 issue.
#
# TODO(ivanpauno):
# This can be deleted when we update `pybind11_vendor` to a version including
# https://github.com/pybind/pybind11/pull/2590.
#
# They are enabling /LTCG on Windows to reduce binary size,
# but that doesn't play well with MSVC incremental linking (default for Debug/RelWithDebInfo).
#
# See:
# - https://docs.microsoft.com/en-us/cpp/build/reference/incremental-link-incrementally?view=vs-2019
# - https://docs.microsoft.com/en-us/cpp/build/reference/ltcg-link-time-code-generation?view=vs-2019

if(MSVC AND "${CMAKE_BUILD_TYPE}" STREQUAL "RelWithDebInfo")
get_target_property(target_link_libraries ${target} LINK_LIBRARIES)
list(REMOVE_ITEM target_link_libraries "$<$<NOT:$<CONFIG:Debug>>:-LTCG>")
set_target_properties(${target} PROPERTIES LINK_LIBRARIES "${target_link_libraries}")

get_target_property(target_compile_options ${target} COMPILE_OPTIONS)
list(REMOVE_ITEM target_compile_options "$<$<NOT:$<CONFIG:Debug>>:/GL>")
set_target_properties(${target} PROPERTIES COMPILE_OPTIONS "${target_compile_options}")
endif()
endfunction()

# pybind11_add_module sets a compile definition Py_DEBUG, but the CPython
# pyconfig.h header on windows also sets Py_DEBUG when _DEBUG is set.
# This leadds to a C4005 compiler warning for a duplicate definition of
# Py_DEBUG.
# The pybind11.h header has logic to work around that, but not all of these
# CPython extensions are using pybind11.
# This function strips the Py_DEBUG compile definition.
# It should only be used if pybind11_add_module is used but the module does not
# include pybind11.h.
function(remove_py_debug_definition_on_win32 target)
if(WIN32 AND CMAKE_BUILD_TYPE STREQUAL "Debug")
get_target_property(compile_definitions "${target}" COMPILE_DEFINITIONS)
set(compile_definitions_to_keep "")
foreach(definition ${compile_definitions})
if("Py_DEBUG" STREQUAL definition)
# Skip!
continue()
endif()
list(APPEND compile_definitions_to_keep "${definition}")
endforeach()
set_target_properties("${target}" PROPERTIES COMPILE_DEFINITIONS "${compile_definitions_to_keep}")
endif()
endfunction()

find_package(pybind11_vendor REQUIRED)
find_package(pybind11 REQUIRED)

Expand Down Expand Up @@ -97,6 +164,8 @@ target_link_libraries(_rclpy PRIVATE
rcutils::rcutils
)
configure_build_install_location(_rclpy)
clean_windows_flags(_rclpy)
remove_py_debug_definition_on_win32(_rclpy)

# Extension with code for Action servers and clients
pybind11_add_module(_rclpy_action SHARED
Expand All @@ -119,6 +188,8 @@ target_link_libraries(_rclpy_logging PRIVATE
rcutils::rcutils
)
configure_build_install_location(_rclpy_logging)
clean_windows_flags(_rclpy_logging)
remove_py_debug_definition_on_win32(_rclpy_logging)

# Signal handling library
pybind11_add_module(_rclpy_signal_handler SHARED
Expand All @@ -130,12 +201,15 @@ target_link_libraries(_rclpy_signal_handler PRIVATE
rcutils::rcutils
)
configure_build_install_location(_rclpy_signal_handler)
clean_windows_flags(_rclpy_signal_handler)
remove_py_debug_definition_on_win32(_rclpy_signal_handler)

# Pycapsule library
pybind11_add_module(_rclpy_pycapsule SHARED
src/rclpy/_rclpy_pycapsule.cpp
)
configure_build_install_location(_rclpy_pycapsule)
clean_windows_flags(_rclpy_pycapsule)

# Handle library, used to keep dependencies between C objects.
pybind11_add_module(_rclpy_handle SHARED
Expand All @@ -146,6 +220,8 @@ target_link_libraries(_rclpy_handle PRIVATE
rcutils::rcutils
)
configure_build_install_location(_rclpy_handle)
clean_windows_flags(_rclpy_handle)
remove_py_debug_definition_on_win32(_rclpy_handle)

if(NOT WIN32)
ament_environment_hooks(
Expand Down
1 change: 1 addition & 0 deletions rclpy/package.xml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<author email="william@openrobotics.org">William Woodall</author>

<buildtool_depend>ament_cmake</buildtool_depend>
<buildtool_depend>python_cmake_module</buildtool_depend>

<build_depend>pybind11_vendor</build_depend>
<build_depend>rcutils</build_depend>
Expand Down
1 change: 0 additions & 1 deletion rclpy/src/rclpy/_rclpy_action.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include <Python.h>
#include <pybind11/pybind11.h>

#include <rcl/error_handling.h>
Expand Down
1 change: 0 additions & 1 deletion rclpy/src/rclpy/_rclpy_pycapsule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include <Python.h>
#include <pybind11/pybind11.h>

namespace py = pybind11;
Expand Down