search

pybind / pybind11

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

fix(cmake): skip empty PYBIND11_PYTHON_EXECUTABLE_LAST for the first cmake run #4856

Closed ilya-lavrenov closed 9 months ago

ilya-lavrenov commented 1 year ago

Description

Closes https://github.com/pybind/pybind11/issues/4854

Suggested changelog entry:

Skip empty ``PYBIND11_PYTHON_EXECUTABLE_LAST`` for the first cmake run
henryiii commented 1 year ago

A value won’t ever be equal to an empty string. Though this avoids the undefined variable usage warning if you enable that, is that why? Didn’t think we were very close to passing that.

ilya-lavrenov commented 1 year ago

A value won’t ever be equal to an empty string. Though this avoids the undefined variable usage warning if you enable that, is that why? Didn’t think we were very close to passing that.

The value of PYBIND11_PYTHON_EXECUTABLE_LAST is empty on first cmake run

henryiii commented 1 year ago

Ah, I see, this is the reverse of what I was thinking. I'm pretty sure we are using the empty value on first run to fill this if needed. If you aren't manually setting PYTHON_MODULE_EXTENSION, it needs to be set, right?

henryiii commented 1 year ago

Ahh, no, the block is shorter than I thought. Hmm, this might (mostly) work. In your case, if you did have the interpreter change, then you'd still lose the manually set value.

henryiii commented 1 year ago

The only issue here, then, is if PYBIND11_PYTHON_EXECUTABLE_LAST is unset, it will never be set.

ilya-lavrenov commented 1 year ago

Yes, I agree with current state. Thanks for noticing it

ilya-lavrenov commented 11 months ago

ping

henryiii commented 10 months ago

I think this is fine to go in, haven't been able to get anyone else to review it.

Thanks!

henryiii commented 10 months ago

(I'll merge after CMake 3.27.8 gets uploaded and un-breaks our CI. There's a narrow window (<1 hour) between a release and the binaries being uploaded that we get failing CI runs in).

ilya-lavrenov commented 9 months ago

Could you please merge the PR?