fix(cmake): support DEBUG_POSTFIX correctly #4761

lpapp-foundry · 2023-07-25T18:20:27Z

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

Description

Suggested changelog entry:

* Support DEBUG_POSFIX correctly for debug builds

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 · 2023-08-21T16:53:19Z

This only fixes the ~~classic FindPythonInterp, which was removed in CMake 3.27 (sort-of). Is a similar fix needed for the modern FindPython?~~ We try to keep both methods in sync. (Edit: I got this backwards, this is only fixing the new one).

lpapp-foundry · 2023-08-21T17:48:29Z

This only fixes the classic FindPythonInterp, which was removed in CMake 3.27 (sort-of). Is a similar fix needed for the modern FindPython? We try to keep both methods in sync.

Thanks @henryiii, would you be able to elaborate on this?

As far as I can see, we are making a generic fix for the suffix and debug postfix independently from FindPythonInterp and FindPython? As far as I know, FindPythonInterp does not deal with suffix related things - it is a standalone find module that only invoked python for version information.

How is this related to FindPythonInterp and FindPython?

Apologies if I am missing something fundamental.

Update: do you mean fixes in this file? https://github.com/pybind/pybind11/blob/master/tools/FindPythonLibsNew.cmake?

E.g.: https://github.com/pybind/pybind11/blob/master/tools/FindPythonLibsNew.cmake#L175?

I have personally not hit issues in that file, but that is likely because I have not used it myself, so it would be challenging for me to fix that without being able to test it.

henryiii · 2023-08-21T19:52:47Z

Ahh, due to the classic style naming, I think I had it backwards, but in general tools/pybind11NewTools.cmake and tools/pybind11Tools.cmake should stay in sync. The "common to both" file is tools/pybind11Common.cmake. The old one uses tools/FindPythonLibsNew.cmake.

If you use FindPython before pybind11, or set PYBIND11_FINDPYTHON, you get the new one, otherwise you get the old one.

henryiii · 2023-08-21T19:55:54Z

Ahh, okay, slightly less worried about it since it's the old one. I can try to make the same change there in a couple of days (remind me if not).

lpapp-foundry · 2023-08-21T20:22:37Z

Ahh, okay, slightly less worried about it since it's the old one. I can try to make the same change there in a couple of days (remind me if not).

Having had a quick look, to me, it looks like tools/pybind11Tools.cmake did not have this logic, so it looks like that we do not need to keep this in sync with regards to this patch. tools/pybind11Tools.cmake did not seem to have an internal fallback like that of tools/pybind11NewTools.cmake which is what this PR is trying to improve further. It looks like that tools/pybind11Tools.cmake just let the consumer to set this explicitly.

So, as far as I can see, at least, we are OK.

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

lpapp-foundry · 2023-08-21T22:01:23Z

@henryiii Thanks for the update. All looks great. Ready to go merge?

lpapp-foundry · 2023-08-25T11:58:50Z

@henryiii You asked me to follow up in a couple of days. How is it going? Ready to merge?

henryiii · 2023-08-30T16:02:10Z

Trying to get at least one more set of eyes on it, just in case. :)

lpapp-foundry · 2023-08-30T16:06:56Z

@friendlyanon already had an eye in the original ticket?

Would you like specifically another maintainer?

henryiii · 2023-08-30T17:02:16Z

Ideally I'd like another maintainer to okay it; according to our convention, since I didn't make the PR, my approval technically would be enough to merge it, but since I was involved in pushing to it, I'd like another maintainer just in case. :)

Thought someone else aware of the situation (and able to test it!) would be great. I don't have any way to test it, and our CI doesn't cover it, which is another reason a set of eyes looking at it that didn't write it would be nice.

friendlyanon · 2023-08-30T17:30:58Z

tools/FindPythonLibsNew.cmake

@@ -172,13 +172,20 @@ _pybind11_get_if_undef(_PYTHON_VALUES 0 _PYTHON_VERSION_LIST)
 _pybind11_get_if_undef(_PYTHON_VALUES 1 PYTHON_PREFIX)
 _pybind11_get_if_undef(_PYTHON_VALUES 2 PYTHON_INCLUDE_DIR)
 _pybind11_get_if_undef(_PYTHON_VALUES 3 PYTHON_SITE_PACKAGES)
-_pybind11_get_if_undef(_PYTHON_VALUES 4 PYTHON_MODULE_EXTENSION)
 _pybind11_get_if_undef(_PYTHON_VALUES 5 PYTHON_IS_DEBUG)
 _pybind11_get_if_undef(_PYTHON_VALUES 6 PYTHON_SIZEOF_VOID_P)
 _pybind11_get_if_undef(_PYTHON_VALUES 7 PYTHON_LIBRARY_SUFFIX)
 _pybind11_get_if_undef(_PYTHON_VALUES 8 PYTHON_LIBDIR)
 _pybind11_get_if_undef(_PYTHON_VALUES 9 PYTHON_MULTIARCH)


I noticed the banner comment at the top of the file is very much out of date considering all the variables that are set in this file. Not relevant here, just something to look out for.

friendlyanon · 2023-08-30T17:35:09Z

tools/FindPythonLibsNew.cmake

 _pybind11_get_if_undef(_PYTHON_VALUES 5 PYTHON_IS_DEBUG)
 _pybind11_get_if_undef(_PYTHON_VALUES 6 PYTHON_SIZEOF_VOID_P)
 _pybind11_get_if_undef(_PYTHON_VALUES 7 PYTHON_LIBRARY_SUFFIX)
 _pybind11_get_if_undef(_PYTHON_VALUES 8 PYTHON_LIBDIR)
 _pybind11_get_if_undef(_PYTHON_VALUES 9 PYTHON_MULTIARCH)

+list(GET _PYTHON_VALUES 4 _PYTHON_MODULE_EXT_SUFFIX)
+if(PYBIND11_PYTHONLIBS_OVERWRITE OR NOT DEFINED PYTHON_MODULE_DEBUG_POSTFIX)
+  get_filename_component(PYTHON_MODULE_DEBUG_POSTFIX "${_PYTHON_MODULE_EXT_SUFFIX}" NAME_WE)


This function is indeed more convenient doing the same thing than my original suggestion. Nice :)

Were you able to test this change? If so, I think it's safe to go in, though ideally another maintainer will review just for completeness.

Looks good to me, but I don't write a ton of CMake.

henryiii · 2023-08-31T02:25:20Z

It is unit tested locally against the original change. And it's cleaner, simpler, and implemented in both required places, instead of just one.

I don't want a half-working change. I'd rather take it slow and get it right. I'm sure another maintainer will have a look shortly.

Pybind11 has been used by thousands of projects over many years. I think waiting a couple more days for a small fix is okay.

lpapp-foundry requested a review from henryiii as a code owner July 25, 2023 18:20

style: pre-commit fixes

6d1d8df

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

8e3a451

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

henryiii changed the title ~~cmake: split extension~~ fix(cmake): support DEBUG_POSTFIX correctly Aug 21, 2023

henryiii added the build system label Aug 21, 2023

friendlyanon approved these changes Aug 30, 2023

View reviewed changes

Skylion007 approved these changes Sep 15, 2023

View reviewed changes

henryiii merged commit 5891867 into pybind:master Sep 15, 2023
85 checks passed

github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 15, 2023

henryiii removed the needs changelog Possibly needs a changelog entry label Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cmake): support DEBUG_POSTFIX correctly #4761

fix(cmake): support DEBUG_POSTFIX correctly #4761

lpapp-foundry commented Jul 25, 2023 •

edited by henryiii

henryiii commented Aug 21, 2023 •

edited

lpapp-foundry commented Aug 21, 2023 •

edited

henryiii commented Aug 21, 2023 •

edited

henryiii commented Aug 21, 2023

lpapp-foundry commented Aug 21, 2023 •

edited

lpapp-foundry commented Aug 21, 2023

lpapp-foundry commented Aug 25, 2023

henryiii commented Aug 30, 2023

lpapp-foundry commented Aug 30, 2023 •

edited

henryiii commented Aug 30, 2023

friendlyanon Aug 30, 2023

friendlyanon Aug 30, 2023

henryiii Aug 31, 2023

Skylion007 Aug 31, 2023

henryiii commented Aug 31, 2023

fix(cmake): support DEBUG_POSTFIX correctly #4761

fix(cmake): support DEBUG_POSTFIX correctly #4761

Conversation

lpapp-foundry commented Jul 25, 2023 • edited by henryiii

Description

Suggested changelog entry:

henryiii commented Aug 21, 2023 • edited

lpapp-foundry commented Aug 21, 2023 • edited

henryiii commented Aug 21, 2023 • edited

henryiii commented Aug 21, 2023

lpapp-foundry commented Aug 21, 2023 • edited

lpapp-foundry commented Aug 21, 2023

lpapp-foundry commented Aug 25, 2023

henryiii commented Aug 30, 2023

lpapp-foundry commented Aug 30, 2023 • edited

henryiii commented Aug 30, 2023

friendlyanon Aug 30, 2023

Choose a reason for hiding this comment

friendlyanon Aug 30, 2023

Choose a reason for hiding this comment

henryiii Aug 31, 2023

Choose a reason for hiding this comment

Skylion007 Aug 31, 2023

Choose a reason for hiding this comment

henryiii commented Aug 31, 2023

lpapp-foundry commented Jul 25, 2023 •

edited by henryiii

henryiii commented Aug 21, 2023 •

edited

lpapp-foundry commented Aug 21, 2023 •

edited

henryiii commented Aug 21, 2023 •

edited

lpapp-foundry commented Aug 21, 2023 •

edited

lpapp-foundry commented Aug 30, 2023 •

edited