Skip to content

fix(cmake): carry option-driven defines on pybind11_headers INTERFACE - #6130

Merged
henryiii merged 2 commits into
masterfrom
chore/headers-interface-defines
Aug 17, 2026
Merged

fix(cmake): carry option-driven defines on pybind11_headers INTERFACE#6130
henryiii merged 2 commits into
masterfrom
chore/headers-interface-defines

Conversation

@henryiii

@henryiii henryiii commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

This is a general fix anyway, can stand on its own.

🤖 AI text below 🤖

Description

PYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION and PYBIND11_SIMPLE_GIL_MANAGEMENT were applied with a directory-scoped add_compile_definitions, so they only took effect in the master-project (test) build. Move them onto the pybind11_headers INTERFACE, the same pattern as PYBIND11_INTERNALS_VERSION. The options now also work in add_subdirectory mode 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::headers so a future precompiled static library and consumer modules always agree.

Suggested changelog entry:

  • The CMake options PYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION and PYBIND11_SIMPLE_GIL_MANAGEMENT now apply in add_subdirectory mode and to installed/exported targets

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
@henryiii
henryiii force-pushed the chore/headers-interface-defines branch from 7d1e36e to 4673ad1 Compare August 15, 2026 03:45

@rwgk rwgk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@henryiii

Copy link
Copy Markdown
Collaborator Author

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.

@henryiii

Copy link
Copy Markdown
Collaborator Author

Oh, I just realized you have a proposal that extends an existing test, that's a bit more attractive. I can check.

Extend subdirectory_target and installed_target to check that
PYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION and
PYBIND11_SIMPLE_GIL_MANAGEMENT propagate through the pybind11_headers
usage requirements. Fails without the parent commit.

Assisted-by: ClaudeCode:claude-fable-5
@henryiii

Copy link
Copy Markdown
Collaborator Author

🤖 AI text below 🤖

Extended the existing subdirectory_target and installed_target tests as suggested: the outer build forwards the two option values into the nested configurations, the consumer targets get EXPECT_<option> defines, and the shared main.cpp errors if the expected macro did not propagate. Verified locally that both tests fail with master's CMakeLists.txt and pass with this PR.

@henryiii
henryiii merged commit a49c4db into master Aug 17, 2026
78 checks passed
@henryiii
henryiii deleted the chore/headers-interface-defines branch August 17, 2026 04:50
@github-actions github-actions Bot added the needs changelog Possibly needs a changelog entry label Aug 17, 2026
@henryiii henryiii changed the title chore(cmake): carry option-driven defines on pybind11_headers INTERFACE fix(cmake): carry option-driven defines on pybind11_headers INTERFACE Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs changelog Possibly needs a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants