Skip to content

Conversation

@wjakob
Copy link
Member

@wjakob wjakob commented Sep 29, 2020

On very incomplete python installations (e.g. within Docker), it's
possible that distutils is not installed. In that case, the
execute_command statement that queries distutils for the Python
module extension fails, and pybind11 uses the empty string. This commit
adds an extra check that causes a CMake failure with more actionable
information (just spent a lot of time trying to track down this problem :))

On very incomplete python installations (e.g. within Docker), it's
possible that distutils is not installed. In that case, the
``execute_command`` statement that queries distutils for the Python
module extension fails, and pybind11 uses the empty string. This commit
adds an extra check that causes a CMake failure with more actionable
information (just spent a lot of time trying to track down this problem :))
@wjakob wjakob requested a review from henryiii September 29, 2020 09:22
@henryiii
Copy link
Collaborator

henryiii commented Sep 29, 2020

Sort-of related, on Windows, can we just set pyd? It's done that way here: https://gitlab.kitware.com/cmake/cmake/-/blob/39677de5e209445c8cbc5957c1e79088d5d2a03a/Modules/FindPython/Support.cmake#L3136-3138

I think we should print the error string if it fails. Nevermind, you added that.

@henryiii henryiii added this to the v2.6.0 milestone Sep 29, 2020
@wjakob
Copy link
Member Author

wjakob commented Sep 30, 2020

Are you saying that we don't set the .pyd extension on Windows at the moment? Not sure that makes sense to me because extensions appear correctly named when building on Windows.

@henryiii
Copy link
Collaborator

No, I meant I don't think we need to query for the extension on Windows; it is always .pyd. So even if distutils was missing/incomplete, it would still work on Windows. Since CMake's FindPython seems to assume .pyd on Windows, I think it might be safe.

(or that could just be a fallback on Windows if this fails).

@wjakob
Copy link
Member Author

wjakob commented Sep 30, 2020

But _PYTHON_MODULE_EXTENSION_ERR contains more than just the filename extension, no? (e.g. .cpython-37m-darwin.so)

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

I don't think there's any reason to hold this up on not skipping Windows; and Windows might be so likely to always work here that it's not a worthwhile optimization to make.

@henryiii henryiii merged commit 3232e59 into master Sep 30, 2020
@henryiii henryiii deleted the findpython-distutils-check branch September 30, 2020 21:49
@henryiii
Copy link
Collaborator

Yes, in the CMake code, that's added as part of the "WITH_SOABI" option below. Okay, this is ideal, then. :)

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.

3 participants