Skip to content
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

Merged
merged 2 commits into from
Sep 15, 2023

Conversation

henryiii
Copy link
Collaborator

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:

- Correctly detect CMake FindPython removal when used as a subdirectory.

@henryiii
Copy link
Collaborator Author

henryiii commented Aug 21, 2023

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.

@polmes
Copy link
Contributor

polmes commented Aug 22, 2023

Just tested this locally. It doesn't work as implemented because of the line

cmake_policy(VERSION 3.26)

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., cmake_policy(GET CMP0148 ...)), it will be empty.

However, moving the following code block you added

if(_pybind11_cmp0148)
  cmake_policy(SET CMP0148 ${_pybind11_cmp0148})
  unset(_pybind11_cmp0148)
endif()

below the cmake_policy version call does the trick. With this change, I was just able to confirm that in CMake 3.27 the following scenarios work as follows (which I believe is as intended):

  • If the root project only sets a CMake minimum version < 3.27 (and no maximum version), then you get the same warning as before
  • If the root project either sets the CMake maximum version to 3.27 via cmake_minimum_version(3.XX...3.27) or cmake_policy(VERSION 3.27), or alternatively just sets the CMP0148 policy to NEW, then the policy is propagated correctly
  • If the root project sets the CMP0148 policy to OLD, then the policy is propagated correctly as well.

Having said that, this made me think. Can't we go a step further and just get rid of all

if(${CMAKE_VERSION} VERSION_LESS 3.XX)
  cmake_policy(VERSION ${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION})
else()
  cmake_policy(VERSION 3.XX)
endif()

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 cmake_policy directly.)

I can make a PR with these changes if you want to review what that would look like.

@henryiii
Copy link
Collaborator Author

henryiii commented Aug 30, 2023

Ahh, yes, I put this too high up. Thanks! Will move.

Can't we go a step further and just get rid of all

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 "cmake_minimum_policy(VERSION 3.5....3.26)" statement (this block really is just that, but written out due to a bug in an old MSVC fork of CMake). This statement always blindly sets all policies. Then again, CMake wasn't really designed to use add_subdirectory to include projects like this, even though it's now become fairly common.

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. :)

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.

This absolutely should be updated to 3.27 (like in #4786), but best to do one thing at a time.

@henryiii henryiii merged commit 4fb111b into pybind:master Sep 15, 2023
82 checks passed
@henryiii henryiii deleted the henryiii/fix/cmakewarnfp branch September 15, 2023 21:59
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 15, 2023
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 27, 2024
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jun 12, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants