Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: Not detecting the SUFFIX and DEBUG_POSTFIX for cmake correctly #4699

Open
2 of 3 tasks
lpapp-foundry opened this issue Jun 12, 2023 · 3 comments
Open
2 of 3 tasks
Labels
triage New bug, unverified

Comments

@lpapp-foundry
Copy link
Contributor

lpapp-foundry commented Jun 12, 2023

Required prerequisites

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

2.10.4

Problem description

pybind11 uses this to figure out the SUFFIX (extension) for cmake.

COMMAND
"${${_Python}_EXECUTABLE}" "-c"
"import sys, importlib; s = importlib.import_module('distutils.sysconfig' if sys.version_info < (3, 10) else 'sysconfig'); print(s.get_config_var('EXT_SUFFIX') or s.get_config_var('SO'))"

This is problematic because this can also contain the debug postfix ("_d" on Windows in debug mode) because of this:

https://github.com/python/cpython/blob/a8d69fe92c65d636fc454cfb1825c357eb2e6325/Python/dynload_win.c#L18

What should be done instead is splitting the result into SUFFIX and DEBUG_POSTFIX.

At the moment, we are trying to use pybind11 with a Python that sets the DEBUG_POSTFIX _d when it needs. This is great for all python libraries.

However, cmake combines DEBUG_POSTFIX and SUFFIX, so we end up with double _d, so we _d_d which then crashes since Windows in debug mode is looking for a single _d.

At the moment, as a consumer of pybind11, we have no easy solution to fix this, only to avoid using pybind11_add_module, but then we would need to reinvent quite a bit of boilerplate, which does not look sustainable.

So, the issue is that our Python package sets the DEBUG_POSTFIX and pybind11 sets the SUFFIX. These two get combined in debug mode. They do not overwrite each other.

pybind11_extension sets SUFFIX to _d.something and we set DEBUG_POSTFIX to _d.

_d + _d.something = _d_d.something

Reproducible example code

Call pybind11_add_module() from a consumer cmake extension module in the CMakeLists.txt file on Windows in debug mode.

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

Not a regression

@lpapp-foundry lpapp-foundry added the triage New bug, unverified label Jun 12, 2023
@friendlyanon
Copy link

friendlyanon commented Jun 12, 2023

pybind11 should instead use this code to derive the extension:

import sys
import importlib

s = importlib.import_module('distutils.sysconfig' if sys.version_info < (3, 10) else 'sysconfig')

print((s.get_config_var('EXT_SUFFIX') or s.get_config_var('SO')).replace('.', ';.', 1))

This will for example output _d;.cp311-win_amd64.pyd, which is a suitable CMake list with 2 elements: the value for DEBUG_POSTFIX and SUFFIX:

function(pybind11_extension name)
  list(GET PYTHON_MODULE_EXTENSION 0 debug_postfix)
  list(GET PYTHON_MODULE_EXTENSION 1 suffix)
  set_target_properties(${name} PROPERTIES PREFIX "" SUFFIX "${suffix}" DEBUG_POSTFIX "${debug_postfix}")
endfunction()

lpapp-foundry added a commit to lpapp-foundry/pybind11 that referenced this issue Jul 25, 2023
Into suffix and debug postfix. Pybind11 is currently treating both as
suffix, which is problematic when something else defines the
DEBUG_POSTFIX because they will be concatenated.

pybind11_extension sets SUFFIX to _d.something and if DEBUG_POSTFIX is
set to _d.

    _d + _d.something = _d_d.something

The issue has been reported at:

pybind#4699
henryiii added a commit that referenced this issue Sep 15, 2023
* cmake: split extension

Into suffix and debug postfix. Pybind11 is currently treating both as
suffix, which is problematic when something else defines the
DEBUG_POSTFIX because they will be concatenated.

pybind11_extension sets SUFFIX to _d.something and if DEBUG_POSTFIX is
set to _d.

    _d + _d.something = _d_d.something

The issue has been reported at:

#4699

* style: pre-commit fixes

* fix(cmake): support postfix for old FindPythonInterp mode too

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

---------

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <henryschreineriii@gmail.com>
@macdew
Copy link

macdew commented Oct 2, 2023

Are you sure that the fix also works for multi-config generators like Visual Studio? As far as I can tell from the code change, both PYTHON_MODULE_DEBUG_POSTFIX and PYTHON_MODULE_EXTENSION are being generated from the same single python.exe.

But in a multi-config generator, one would need to consider a single build with both python.exe and python_d.exe on Windows. And I don't see this reflected in the logic?

@friendlyanon
Copy link

FindPython does not find an interpreter with the python_d basename.

rwgk pushed a commit to google/pybind11clif that referenced this issue Oct 4, 2023
* fix: Use lowercase builtin collection names (#4833)

* Update render for buffer sequence and handle  (#4831)

* fix: Add capitalize render name of `py::buffer` and `py::sequence`

* fix: Render `py::handle` same way as `py::object`

* tests: Fix tests `handle` -> `object`

* tests: Test capitaliation of `py::sequence` and `py::buffer`

* style: pre-commit fixes

* fix: Render `py::object` as `Any`

* Revert "fix: Render `py::object` as `Any`"

This reverts commit 7861dcfabb78ac210b4c67c35a0d47fb67525a96.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ralf W. Grosse-Kunstleve <rwgk@google.com>

* fix: Missing typed variants of `iterator` and `iterable` (#4832)

* Fix small bug introduced with PR #4735 (#4845)

* Bug fix: `result[0]` called if `result.empty()`

* Add unit test that fails without the fix.

* fix(cmake): correctly detect FindPython policy and better warning (#4806)

* fix(cmake): support DEBUG_POSTFIX correctly (#4761)

* cmake: split extension

Into suffix and debug postfix. Pybind11 is currently treating both as
suffix, which is problematic when something else defines the
DEBUG_POSTFIX because they will be concatenated.

pybind11_extension sets SUFFIX to _d.something and if DEBUG_POSTFIX is
set to _d.

    _d + _d.something = _d_d.something

The issue has been reported at:

pybind/pybind11#4699

* style: pre-commit fixes

* fix(cmake): support postfix for old FindPythonInterp mode too

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

---------

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <henryschreineriii@gmail.com>

* Avoid copy in iteration by using const auto & (#4861)

This change is fixing a Coverity AUTO_CAUSES_COPY issues.

* Add 2 missing `throw error_already_set();` (#4863)

Fixes oversights in PR #4570.

* MAINT: Include `numpy._core` imports (#4857)

* MAINT: Include numpy._core imports

* style: pre-commit fixes

* Apply review comments

* style: pre-commit fixes

* Add no-inline attribute

* Select submodule name based on numpy version

* style: pre-commit fixes

* Update pre-commit check

* Add error_already_set and simplify if statement

* Update .pre-commit-config.yaml

Co-authored-by: Ralf W. Grosse-Kunstleve <rwgkio@gmail.com>

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ralf W. Grosse-Kunstleve <rwgkio@gmail.com>

* MAINT: Remove np.int_ (#4867)

* chore(deps): update pre-commit hooks (#4868)

* chore(deps): update pre-commit hooks

updates:
- [github.com/psf/black-pre-commit-mirror: 23.7.0 → 23.9.1](psf/black-pre-commit-mirror@23.7.0...23.9.1)
- [github.com/astral-sh/ruff-pre-commit: v0.0.287 → v0.0.292](astral-sh/ruff-pre-commit@v0.0.287...v0.0.292)
- [github.com/codespell-project/codespell: v2.2.5 → v2.2.6](codespell-project/codespell@v2.2.5...v2.2.6)
- [github.com/shellcheck-py/shellcheck-py: v0.9.0.5 → v0.9.0.6](shellcheck-py/shellcheck-py@v0.9.0.5...v0.9.0.6)
- [github.com/PyCQA/pylint: v3.0.0a7 → v3.0.0](pylint-dev/pylint@v3.0.0a7...v3.0.0)

* Update .pre-commit-config.yaml

* style: pre-commit fixes

* Update .pre-commit-config.yaml

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>

---------

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Co-authored-by: Sergei Izmailov <sergei.a.izmailov@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
Co-authored-by: László Papp <108480845+lpapp-foundry@users.noreply.github.com>
Co-authored-by: Oleksandr Pavlyk <oleksandr.pavlyk@intel.com>
Co-authored-by: Mateusz Sokół <8431159+mtsokol@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage New bug, unverified
Projects
None yet
Development

No branches or pull requests

3 participants