[BUG]: module being stripped if no build type selected (2.10.2 regression)

[tttapa]

Required prerequisites

• Make sure you've read the documentation. Your issue may be addressed there.
• Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
• Consider asking first in the Gitter chat room or in a Discussion.

What version (or hash if on master) of pybind11 are you using?

2.10.3

Problem description

If no CMAKE_BUILD_TYPE is specified, the seemingly innocuous quotes that were added in 88b019a cause the pybind11_add_module function to strip the module, even for debug builds.

By relying on CMAKE_BUILD_TYPE at configure time rather than on $<CONFIG> at generation time, multi-configuration generators like Ninja Multi-Config cannot work correctly. It might be better to use a generator expression (e.g. https://stackoverflow.com/questions/45455350/cmake-how-to-add-a-custom-command-that-is-only-executed-for-one-configuration).

Reproducible example code

CMakeLists.txt

cmake_minimum_required(VERSION 3.20)
project(test-project)

# Use case-insensitive comparison to match the result of $<CONFIG:cfgs>
string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
if(NOT MSVC AND NOT ${uppercase_CMAKE_BUILD_TYPE} MATCHES DEBUG|RELWITHDEBINFO)
    # Strip unnecessary sections of the binary on Linux/macOS
    message(STATUS "Without quotes: strip")
else()
    message(STATUS "Without quotes: no strip")
endif()

# Use case-insensitive comparison to match the result of $<CONFIG:cfgs>
string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
if(NOT MSVC AND NOT "${uppercase_CMAKE_BUILD_TYPE}" MATCHES DEBUG|RELWITHDEBINFO)
    # Strip unnecessary sections of the binary on Linux/macOS
    message(STATUS "With quotes:    strip")
else()
    message(STATUS "With quotes:    no strip")
endif()

Run CMake without build type

cmake -Bbuild -S.

-- Without quotes: no strip
-- With quotes:    strip

Is this a regression? Put the last known working version here if it is.

1f04cc7

[Skylion007]

FYI @henryiii , is this a regression or something we forgot to mention in the changelog?

[henryiii]

How would you propose using a generator expression? You can't run a configure-time function based on a generator expression. I need to check to see if generator expressions can be used inside pybind11_strip. To fix the bug back to 2.10.0 behavior, I expect you could check to see if it's defined? I'm guessing the quotes cause it to "not match" (empty string) rather then being skipped.

[tttapa]

How would you propose using a generator expression? You can't run a configure-time function based on a generator expression.

Indeed, what I meant is changing the add_custom_command command itself.
For instance:

diff --git a/tools/pybind11Common.cmake b/tools/pybind11Common.cmake
index cc87ced5..fa7170cb 100644
--- a/tools/pybind11Common.cmake
+++ b/tools/pybind11Common.cmake
@@ -397,9 +397,21 @@ function(pybind11_strip target_name)
       set(x_opt -x)
     endif()
 
-    add_custom_command(
-      TARGET ${target_name}
-      POST_BUILD
-      COMMAND ${CMAKE_STRIP} ${x_opt} $<TARGET_FILE:${target_name}>)
+    if(CMAKE_VERSION VERSION_LESS 3.8)
+      message(WARNING "CMake 3.8+ required for correct strip behavior when using multi-config "
+                      "generators")
+      add_custom_command(
+        TARGET ${target_name}
+        POST_BUILD
+        COMMAND ${CMAKE_STRIP} ${x_opt} $<TARGET_FILE:${target_name}>)
+    else()
+      set(no_debug_info $<NOT:$<OR:$<CONFIG:Debug>,$<CONFIG:RelWithDebInfo>>>)
+      set(strip_cmd ${CMAKE_STRIP} ${x_opt} $<TARGET_FILE:${target_name}>)
+      add_custom_command(
+        TARGET ${target_name}
+        POST_BUILD
+        COMMAND "$<${no_debug_info}:${strip_cmd}>"
+        COMMAND_EXPAND_LISTS)
+    endif()
   endif()
 endfunction()

This would need some refactoring, though, because now pybind11_strip is a bit of a confusing name for the function, as it always strips, except when using a multi-config generator, then it strips conditionally.
Perhaps you could keep pybind11_strip as is for backwards compatibility, and add a new pybind11_strip_if_not_debug function that includes both the configure-time check from pybind11(New)Tools.cmake and the generator expressions from the patch above?

I would definitely be in favor of adding the if (DEFINED CMAKE_BUILD_TYPE) check in the meantime. I'll submit a PR.

[jasjuang]

Recently got bitten by this. The modification @tttapa did to the add_custom_command is required in order for the Xcode debugger to jump into the break points that is set in the source files that creates the python bindings when running debug on an application that uses the embedded interpreter to import the python bindings. Without @tttapa's patch, the debug symbols got stripped away in Xcode debug mode which causes the Xcode lldb debugger to unable to jump into the break points.

Looking forward to seeing @tttapa's fix making into the master branch in the near future.

[VA-GS]

Hello, I just found that issue. I trigger the NOT MSVC AND NOT "${uppercase_CMAKE_BUILD_TYPE}" MATCHES DEBUG|RELWITHDEBINFO check for a different reason: I have a custom build type (ASAN). Ideally I would like to be able to control stripping from pybind11_add_module instead.

[henryiii]

We should match on RELEASE-like builds only, not the other way around.