chore(cmake): carry option-driven defines on pybind11_headers INTERFACE - #6130
chore(cmake): carry option-driven defines on pybind11_headers INTERFACE#6130henryiii wants to merge 1 commit into
Conversation
bdd7f79 to
7d1e36e
Compare
PYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION and PYBIND11_SIMPLE_GIL_MANAGEMENT were directory-scoped add_compile_definitions, which only reached the master-project test build. On the pybind11_headers INTERFACE (like PYBIND11_INTERNALS_VERSION) they also apply in add_subdirectory mode and in the exported/installed targets. Assisted-by: ClaudeCode:claude-fable-5
7d1e36e to
4673ad1
Compare
rwgk
left a comment
There was a problem hiding this comment.
Reviewed with codex gpt-5.6-sol ultra:
This change looks right to me: these configuration-dependent definitions belong on pybind11_headers as usage requirements, so they propagate through the target graph and survive export/install.
I'm approving, but I would strongly suggest adding regression coverage before merging. We already have focused CMake integration tests under tests/test_cmake_build, including small add_subdirectory and installed-target consumers, so this should fit naturally into the existing structure.
The current option-enabled CI configurations do not distinguish the old behavior from the new behavior: directory-scoped definitions already reached pybind11’s top-level test targets, while the separately configured consumer tests neither forward these options nor assert the resulting macros.
I think the existing subdirectory_target and installed_target tests could be extended by:
- forwarding the two enabled option values into the nested configurations;
- defining test-only “expect this macro” flags on the consumer targets; and
- adding conditional #ifndef checks to their shared main.cpp.
Existing CI jobs already enable each option and run test_cmake_build, so this should not require new workflow jobs. The resulting tests should fail on master and pass with this PR.
|
I'd push back on the test. Adding cmake tests are expensive (one push of this stack kills the ability to run anything on any pybind repo for several hours!), we don't do cmake tests that often, and it will get tested by the items later in the stack anyway. |
|
Oh, I just realized you have a proposal that extends an existing test, that's a bit more attractive. I can check. |
This is a general fix anyway, can stand on its own.
🤖 AI text below 🤖
Description
PYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATIONandPYBIND11_SIMPLE_GIL_MANAGEMENTwere applied with a directory-scopedadd_compile_definitions, so they only took effect in the master-project (test) build. Move them onto thepybind11_headersINTERFACE, the same pattern asPYBIND11_INTERNALS_VERSION. The options now also work inadd_subdirectorymode and ride the exported/installed targets.This is step 0 of the optional pre-compilation work: must-match configuration macros have to propagate through
pybind11::headersso a future precompiled static library and consumer modules always agree.Suggested changelog entry:
PYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATIONandPYBIND11_SIMPLE_GIL_MANAGEMENTnow apply inadd_subdirectorymode and to installed/exported targets, not only when pybind11 is the top-level project.