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

[BUG] regression when mixing installed pybind11 with local copy #2709

Closed
henryiii opened this issue Dec 1, 2020 · 4 comments · Fixed by #2716
Closed

[BUG] regression when mixing installed pybind11 with local copy #2709

henryiii opened this issue Dec 1, 2020 · 4 comments · Fixed by #2716
Milestone

Comments

@henryiii
Copy link
Collaborator

henryiii commented Dec 1, 2020

Issue description

Hi all, since the recent refactor of CMake files (pull request #2370), when setting up include paths, pybind11_add_module puts priority on the already installed version of pybind11 (PYTHON_INCLUDE_DIRS) instead of the local copy (pybind11_INCLUDE_DIR). As an effect, wrong header files are included.

I understand that the documentation of pybind11_add_module says that pybind11 must be properly installed for it to work, but this nevertheless looks like a bug -- why add pybind11_INCLUDE_DIR if you expect it to be ignored?

Any idea how to get the old behavior? I found a hacky solution by modifying pybind11_add_module to do

target_link_libraries(${target_name} PRIVATE pybind11::headers pybind11::module)

instead of

target_link_libraries(${target_name} PRIVATE pybind11::module)

but I hope to find something better. Thanks!

From @ikicic on Gitter.

@henryiii
Copy link
Collaborator Author

henryiii commented Dec 1, 2020

I probably never thought about this situation, when two copies of pybind11 exist and you want the local one to overide the system one pulled in via the Python directories. The local include property is preceding the target (newer versions of CMake might support this with SYSTEM, but we should fix properly). I believe what we should do here is make a Python::Headers IMPORTED INTERFACE target, and stick them on that. Noting this here so I don't forget.

@henryiii henryiii added this to the v2.6.2 milestone Dec 1, 2020
@ikicic
Copy link

ikicic commented Dec 2, 2020

Just to note that my hacky solution may not actually work, as the order of dependencies is not guaranteed (?). For the same reason, would it make sense to simply move PYTHON_INCLUDE_DIRS from pybind11::pybind11 to pybind11::headers, to force the order of includes?
(Edit: Maybe I misunderstood your proposal at first. You want to effectively do the same, a single target with both include paths?)

@henryiii
Copy link
Collaborator Author

henryiii commented Dec 2, 2020

The point of pybind11::headers is it is only the headers, no PYTHON_INCLUDE_DIRS. I think we should make a target pybind11::python_headers that has the python includes, and include that on pybind11::pybind11. Targets do respect order, though if you have SYSTEM includes those go to then end but otherwise still respect order (IIRC, and requires your CMake min version to be set above something, I think).

@henryiii
Copy link
Collaborator Author

henryiii commented Dec 8, 2020

@ikicic Assuming tests pass and I didn't make a mess of it, can you check my PR and see if that fixes it for you? Mismatched installed vs. local pybind11 not something that's very easy to test in CI.

henryiii added a commit to henryiii/pybind11 that referenced this issue Dec 11, 2020
henryiii added a commit to henryiii/pybind11 that referenced this issue Dec 16, 2020
henryiii added a commit that referenced this issue Dec 16, 2020
* fix: regression with installed pybind11 overriding discovered one

Closes #2709

* docs: wording incorrect
henryiii added a commit that referenced this issue Jan 18, 2021
* CI: Intel icc/icpc via oneAPI

Add testing for Intel icc/icpc via the oneAPI images.
Intel oneAPI is in a late beta stage, currently shipping
oneAPI beta09 with ICC 20.2.

CI: Skip Interpreter Tests for Intel

Cannot find how to add this, neiter the package `libc6-dev` nor
`intel-oneapi-mkl-devel` help when installed to solve this:
```
-- Looking for C++ include pthread.h
-- Looking for C++ include pthread.h - not found
CMake Error at /__t/cmake/3.18.4/x64/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:165 (message):
  Could NOT find Threads (missing: Threads_FOUND)
Call Stack (most recent call first):
  /__t/cmake/3.18.4/x64/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:458 (_FPHSA_FAILURE_MESSAGE)
  /__t/cmake/3.18.4/x64/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindThreads.cmake:234 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  tests/test_embed/CMakeLists.txt:17 (find_package)
```

CI: libc6-dev from GCC for ICC

CI: Run bare metal for oneAPI

CI: Ubuntu 18.04 for oneAPI

CI: Intel +Catch -Eigen

CI: CMake from Apt (ICC tests)

CI: Replace Intel Py with GCC Py

CI: Intel w/o GCC's Eigen

CI: ICC with verbose make

[Debug] Find core dump

tests: use arg{} instead of arg() for Intel

tests: adding a few more missing {}

fix: sync with @tobiasleibner's branch

fix: try ubuntu 20-04

fix: drop exit 1

docs: Apply suggestions from code review

Co-authored-by: Tobias Leibner <tobias.leibner@googlemail.com>

Workaround for ICC enable_if issues

Another workaround for ICC's enable_if issues

fix error in previous commit

Disable one test for the Intel compiler in C++17 mode

Add back one instance of py::arg().noconvert()

Add NOLINT to fix clang-tidy check

Work around for ICC internal error in PYBIND11_EXPAND_SIDE_EFFECTS in C++17 mode

CI: Intel ICC with C++17

docs: pybind11/numpy.h does not require numpy at build time. (#2720)

This is nice enough to be mentioned explicitly in the docs.

docs: Update warning about Python 3.9.0 UB, now that 3.9.1 has been released (#2719)

Adjusting `type_caster<std::reference_wrapper<T>>` to support const/non-const propagation in `cast_op`. (#2705)

* Allow type_caster of std::reference_wrapper<T> to be the same as a native reference.

Before, both std::reference_wrapper<T> and std::reference_wrapper<const T> would
invoke cast_op<type>. This doesn't allow the type_caster<> specialization for T
to distinguish reference_wrapper types from value types.

After, the type_caster<> specialization invokes cast_op<type&>, which allows
reference_wrapper to behave in the same way as a native reference type.

* Add tests/examples for std::reference_wrapper<const T>

* Add tests which use mutable/immutable variants

This test is a chimera; it blends the pybind11 casters with a custom
pytype implementation that supports immutable and mutable calls.

In order to detect the immutable/mutable state, the cast_op needs
to propagate it, even through e.g. std::reference<const T>

Note: This is still a work in progress; some things are crashing,
which likely means that I have a refcounting bug or something else
missing.

* Add/finish tests that distinguish const& from &

Fixes the bugs in my custom python type implementation,
demonstrate test that requires const& and reference_wrapper<const T>
being treated differently from Non-const.

* Add passing a const to non-const method.

* Demonstrate non-const conversion of reference_wrapper in tests.

Apply formatting presubmit check.

* Fix build errors from presubmit checks.

* Try and fix a few more CI errors

* More CI fixes.

* More CI fixups.

* Try and get PyPy to work.

* Additional minor fixups. Getting close to CI green.

* More ci fixes?

* fix clang-tidy warnings from presubmit

* fix more clang-tidy warnings

* minor comment and consistency cleanups

* PyDECREF -> Py_DECREF

* copy/move constructors

* Resolve codereview comments

* more review comment fixes

* review comments: remove spurious &

* Make the test fail even when the static_assert is commented out.

This expands the test_freezable_type_caster a bit by:
1/ adding accessors .is_immutable and .addr to compare identity
from python.
2/ Changing the default cast_op of the type_caster<> specialization
to return a non-const value. In normal codepaths this is a reasonable
default.
3/ adding roundtrip variants to exercise the by reference, by pointer
and by reference_wrapper in all call paths.  In conjunction with 2/, this
demonstrates the failure case of the existing std::reference_wrpper conversion,
which now loses const in a similar way that happens when using the default cast_op_type<>.

* apply presubmit formatting

* Revert inclusion of test_freezable_type_caster

There's some concern that this test is a bit unwieldly because of the use
of the raw <Python.h> functions. Removing for now.

* Add a test that validates const references propagation.

This test verifies that cast_op may be used to correctly detect
const reference types when used with std::reference_wrapper.

* mend

* Review comments based changes.

1. std::add_lvalue_reference<type> -> type&
2. Simplify the test a little more; we're never returning the ConstRefCaster
type so the class_ definition can be removed.

* formatted files again.

* Move const_ref_caster test to builtin_casters

* Review comments: use cast_op and adjust some comments.

* Simplify ConstRefCasted test

I like this version better as it moves the assertion that matters
back into python.

ci: drop pypy2 linux, PGI 20.7, add Python 10 dev (#2724)

* ci: drop pypy2 linux, add Python 10 dev

* ci: fix mistake

* ci: commented-out PGI 20.11, drop 20.7

fix: regression with installed pybind11 overriding local one (#2716)

* fix: regression with installed pybind11 overriding discovered one

Closes #2709

* docs: wording incorrect

style: remove redundant instance->owned = true (#2723)

which was just before set to True in instance->allocate_layout()

fix: also throw in the move-constructor added by the PYBIND11_OBJECT macro, after the argument has been moved-out (if necessary) (#2701)

Make args_are_all_* ICC workarounds unconditional

Disable test_aligned on Intel ICC

Fix test_aligned on Intel ICC

Skip test_python_alreadyset_in_destructor on Intel ICC

Fix test_aligned again

ICC CI: Downgrade pytest

pytest 6 does not capture the `discard_as_unraisable` stderr and
just writes a warning with its content instead.

* refactor: simpler Intel workaround, suggested by @laramiel

* fix: try version with impl to see if it is easier to compile

* docs: update README for ICC

Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
Co-authored-by: Henry Schreiner <henryschreineriii@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants