Skip to content

Conversation

rhaschke
Copy link
Contributor

Fixes #5813

@rhaschke rhaschke requested a review from henryiii as a code owner August 24, 2025 11:11
@henryiii henryiii requested a review from Copilot September 8, 2025 14:21
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enforces the use of find_package(Python) by removing conditional checks that would skip Python finding when Python was already found. The change ensures that Python detection logic runs unconditionally rather than being bypassed when Python variables are already set.

  • Removes the outer conditional wrapper around Python finding logic
  • Ensures Python detection always executes regardless of existing Python_FOUND or Python3_FOUND variables

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +21 to +23
if(NOT DEFINED Python_FIND_IMPLEMENTATIONS)
set(Python_FIND_IMPLEMENTATIONS CPython PyPy)
endif()
Copy link
Preview

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

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

With the removal of the outer conditional check, this Python configuration will now run unconditionally. Consider whether this could overwrite existing Python_FIND_IMPLEMENTATIONS settings or if additional guards are needed to prevent reconfiguration when Python is already properly configured.

Copilot uses AI. Check for mistakes.

@henryiii
Copy link
Collaborator

henryiii commented Sep 8, 2025

We don't want to call FindPython if it's already been called with the right components present. Or if the user is using the old PythonInterp/Libs support. So I think the check can't be removed, but should instead check for everything that is needed, and trigger the find if everything isn't there.

@rhaschke
Copy link
Contributor Author

rhaschke commented Sep 8, 2025

We don't want to call FindPython if it's already been called with the right components present.

What's the problem in doing so? Do you fear performance issues? As far a I know, cmake's find functions cache previous results. So that shouldn't be a big deal.
On the other hand, find_package might have been called with different search-path arguments before, yielding a Python version < 3.8, which would not be compatible to pybind11. I think, pybind11 should ensure its required version and components itself.

Or if the user is using the old PythonInterp/Libs support.

I think this is not relevant here, as the old mode is handled in pybind11Tools.cmake, not pybind11NewTools.cmake.

So I think the check can't be removed, but should instead check for everything that is needed, and trigger the find if everything isn't there.

This would require moving the logic to figure out the required components out of the if.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: Python_ADD_LIBRARY: target Python::Module not defined
2 participants