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: regression with installed pybind11 overriding local one #2716

Merged
merged 2 commits into from
Dec 16, 2020

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Dec 8, 2020

Description

This was not always including the discovered pybind11 first. Closes #2709.

Suggested changelog entry:

* CMake: mixing local and installed pybind11's would prioritize the installed one over the local one (regression in 2.6.0)

@henryiii henryiii changed the title fix: regression with installed pybind11 overriding discovered one fix: regression with installed pybind11 overriding local one Dec 8, 2020
set_property(
TARGET pybind11::pybind11
APPEND
PROPERTY INTERFACE_INCLUDE_DIRECTORIES $<BUILD_INTERFACE:${${_Python}_INCLUDE_DIRS}>)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May have fixed a possible issue if using new FindPython and if Python's install dir had a space in it.

@ikicic
Copy link

ikicic commented Dec 8, 2020

@henryiii The patch works for me. Thanks!

@henryiii
Copy link
Collaborator Author

Quick description of what this fixes and why:

Say I install pybind11 2.5.0. Knowing me, I probably did a brew install pybind11, but how it got installed doesn't really matter, as long as it sits in the same place that Python.h will be coming from. Then I grab a project that has a built-in pybind11 submodule using a recent master. It suddenly matters if I include Python's include directory before pybind11's, because there is a sneaky installed copy of pybind11 next to Python, so if I put pybind11's local directory after, I might as well not put it in at all, the compiler will always get the installed copy even though I never ask for it.

We always add the local include directory first in the CMakeLists, though, so what's the problem? Well, when CMake builds the targets, it is just combining properties. The way it ended up in 2.6.{0,1}, pybind11::pybind11 had pybind11's local include included via a target, pybind11::headers, but Python's include via a the includes property - which always comes first over the linked targets property!

By also making pybind11::python_headers and then including it that way, it's now on even footing with pybind11::headers, in the exact same linked targets property, and it comes after it, so the include order is correct again.

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooof, that that order of includes matters is quite ... ugly, but nothing we can do about fundamentally.

Thanks a lot for the extra explanation, @henryiii! Surely this will also be useful towards the future.

# Only add Python for build - must be added during the import for config
# since it has to be re-discovered.
#
# This needs to be an target to it is included after the local pybind11
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something must have gone wrong rewriting, here

@henryiii henryiii merged commit ffb113d into pybind:master Dec 16, 2020
@henryiii henryiii deleted the fix/2709 branch December 16, 2020 02:55
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 16, 2020
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Dec 22, 2020
henryiii added a commit that referenced this pull request 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 this pull request may close these issues.

[BUG] regression when mixing installed pybind11 with local copy
3 participants