-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix(cmake): correctly detect FindPython policy and better warning #4806
Conversation
Signed-off-by: Henry Schreiner <[email protected]>
We don't really test this, since you need to include the repo as a submodule, etc - @polmes, is it possible for you to test this locally? What I'm doing here is a little weird, so would be nice to see if it works. |
Just tested this locally. It doesn't work as implemented because of the line
that comes after setting the CMP0148 policy to the root project's value. After that line, the value is reset and if you try to retrieve its value (i.e., However, moving the following code block you added
below the
Having said that, this made me think. Can't we go a step further and just get rid of all
calls in pybind11's CMake code altogether? This way we would not be blindly setting policies and overriding other values from the root project. Instead, we can deal with each individual policy as needed (just like it was done here for CMP0148). The advantage from this switch would be that we no longer need to worry about "upgrading" or keeping track of the maximum CMake version supported anywhere, but I might be missing some other downside. It is also a bit odd to keep that line that sets the maximum CMake policy version to 3.26 when we are actually supporting features from 3.27. (Keep in mind that a quick search in pybind11 reveals no other uses of I can make a PR with these changes if you want to review what that would look like. |
Ahh, yes, I put this too high up. Thanks! Will move.
I suggested this originally, but was a bit more worried about doing it, since it's unusual, as far as I know, to not have a " Usually, if a new policy that affects us pops up, then this protects our code to keep it still working, even if a project including us updates their max (or min) version. The downside is that it becomes inconsistent; if a user sets max to 3.19, for example, they probably want everything to follow that, while we will still follow 3.26 (currently). But a user doesn't have to update this value if it breaks something. Given there are downsides to both, I think what we are doing now is more common, so probably the least surprising. I think. If there was any indication that others are doing it the "only set if main project" way, we could switch, I just don't really want to be first in this. :)
This absolutely should be updated to 3.27 (like in #4786), but best to do one thing at a time. |
* fix: Use lowercase builtin collection names (pybind#4833) * Update render for buffer sequence and handle (pybind#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 7861dcf. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]> * fix: Missing typed variants of `iterator` and `iterable` (pybind#4832) * Fix small bug introduced with PR pybind#4735 (pybind#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 (pybind#4806) * fix(cmake): support DEBUG_POSTFIX correctly (pybind#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#4699 * style: pre-commit fixes * fix(cmake): support postfix for old FindPythonInterp mode too Signed-off-by: Henry Schreiner <[email protected]> --------- Signed-off-by: Henry Schreiner <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Henry Schreiner <[email protected]> * Avoid copy in iteration by using const auto & (pybind#4861) This change is fixing a Coverity AUTO_CAUSES_COPY issues. * Add 2 missing `throw error_already_set();` (pybind#4863) Fixes oversights in PR pybind#4570. * MAINT: Include `numpy._core` imports (pybind#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 <[email protected]> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]> * MAINT: Remove np.int_ (pybind#4867) * chore(deps): update pre-commit hooks (pybind#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 <[email protected]> --------- Signed-off-by: Henry Schreiner <[email protected]> Co-authored-by: Sergei Izmailov <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Henry Schreiner <[email protected]> Co-authored-by: László Papp <[email protected]> Co-authored-by: Oleksandr Pavlyk <[email protected]> Co-authored-by: Mateusz Sokół <[email protected]>
Description
This is a fix for #4785. We were not detecting the status of this policy correctly.
I'll likely update documentation a bit too later (not this PR).
Suggested changelog entry: