fix: add flag for overriding classic Python search values #4195
Conversation
|
Actually I think it's our stuff around FindPython that's breaking, not this one. Will make a new PR just in case I'm wrong and this history is useful. |
|
No, I take that back, this is running the old one - where are these values being set? Not by FindPythonInterp as far as I can tell... |
tools/FindPythonLibsNew.cmake
Outdated
| @@ -95,9 +95,24 @@ if(NOT PythonLibsNew_FIND_VERSION) | |||
| set(PythonLibsNew_FIND_VERSION "3.6") | |||
| endif() | |||
|
|
|||
| # Save variables that get set by PythonInterp | |||
There was a problem hiding this comment.
From my viewpoint as someone not knowing much about cmake, copying the title of this PR here, ideally even with a link to this PR, would seem super helpful. Otherwise I'd give too much credit to this code looking fresh. Maybe:
# For PyPy, broken FindPythonInterp values need to be overridden (see PR #4195).
# This macro saves variables that get set by PythonInterp.
There was a problem hiding this comment.
This isn't working, so I'm going to need to address this differently anyway. I'll probably do one of two things. If I can stop these from being set at all, then the current form is fine. If not, I can introduce a toggle and then you could override all or none. Something like that. :/
There was a problem hiding this comment.
(So far sill have no idea who is (incorrectly) setting these values, it's not FindPythonInterp and I'm pretty sure we don't run FindPythonLibs)
3e31783 to
c2eb69f
Compare
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
c2eb69f to
e0b0766
Compare
|
@rwgk or @Skylion007 What do you think? This reverts the 2.10 default change and instead makes it opt-in for now. The problem is that currently tools like scikit-build reuse the same cache dir for multiple builds, which causes the "promote to cache" that's done here to override new values with old ones on new runs. Scikit-build really should make per-interpreter cached directories; that would help, but for now, this seems like the simplest fix. However, I'm torn - I'm also fine to say it's scikit-build's problem, and it needs include the interpreter string in the build path that gets cahced. Scikit-build-core will probably be designed this way from the start (currently, it just doesn't support caching at all - caching build directories is hard 😆). But that will make fixing scikit_build_example ugly for a bit until a proper fix lands in scikit-build. But this might motivate me. ;) What do you think? Option a) (this PR) or option b) (declare it not broken and working as expected). |
* fix: PyPy needs to overrite broken FindPythonInterp values Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * fix: add flag to opt-in to new (cross-compile) behavior Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * Apply suggestions from code review Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Description
Should fix pybind/scikit_build_example#45.
Suggested changelog entry:
* CMake: revert overwrite behavior, now opt-in with ``PYBIND11_PYTHONLIBS_OVERRWRITE OFF``