search

pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.39k stars 2.07k forks source link

fix(cmake): support DEBUG_POSTFIX correctly #4761

Closed lpapp-foundry closed 11 months ago

lpapp-foundry commented 1 year ago

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:

https://github.com/pybind/pybind11/issues/4699

Description

Suggested changelog entry:

* Support DEBUG_POSFIX correctly for debug builds
henryiii commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

lpapp-foundry commented 1 year ago

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

lpapp-foundry commented 1 year ago

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

henryiii commented 1 year ago

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

lpapp-foundry commented 1 year ago

@friendlyanon already had an eye in the original ticket?

Would you like specifically another maintainer?

henryiii commented 1 year ago

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.

henryiii commented 1 year ago

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.