From b322018e1571b42513de77d9ad9fde1d030c8cc6 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 8 Nov 2021 15:56:20 -0500 Subject: [PATCH 1/6] [pre-commit.ci] pre-commit autoupdate (#3449) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updates: - [github.com/PyCQA/isort: 5.9.3 → 5.10.0](https://github.com/PyCQA/isort/compare/5.9.3...5.10.0) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 706194646..6f0bb39b1 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -38,7 +38,7 @@ repos: - id: pyupgrade - repo: https://github.com/PyCQA/isort - rev: 5.9.3 + rev: 5.10.0 hooks: - id: isort From b11ff912a6b68edcd67770308ba4703e3a740e7b Mon Sep 17 00:00:00 2001 From: Antony Lee Date: Mon, 8 Nov 2021 22:27:32 +0100 Subject: [PATCH 2/6] fix(setup =_helpers): don't add -g0 CFLAGS sets -g (#3436) On Unix, setuptools prepends $CFLAGS and $CPPFLAGS to the compiler flags (they always come before extra_compile_args and anything else; see distutils.sysconfig.customize_compiler). In practice, the environment variables are useful e.g. to quickly generate a debug build (e.g. by setting CFLAGS=-g), but Pybind11Extension currently unconditionally overwrites this with -g0. Instead, check the environment variables and only insert -g0 if not overridden by them. --- pybind11/setup_helpers.py | 8 +++++++- tests/extra_setuptools/test_setuphelper.py | 14 +++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/pybind11/setup_helpers.py b/pybind11/setup_helpers.py index 4ff1a0cb3..d6d84f672 100644 --- a/pybind11/setup_helpers.py +++ b/pybind11/setup_helpers.py @@ -42,6 +42,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. import contextlib import os import platform +import shlex import shutil import sys import sysconfig @@ -143,7 +144,12 @@ class Pybind11Extension(_Extension): if WIN: cflags += ["/EHsc", "/bigobj"] else: - cflags += ["-fvisibility=hidden", "-g0"] + cflags += ["-fvisibility=hidden"] + env_cflags = os.environ.get("CFLAGS", "") + env_cppflags = os.environ.get("CPPFLAGS", "") + c_cpp_flags = shlex.split(env_cflags) + shlex.split(env_cppflags) + if not any(opt.startswith("-g") for opt in c_cpp_flags): + cflags += ["-g0"] if MACOS: cflags += ["-stdlib=libc++"] ldflags += ["-stdlib=libc++"] diff --git a/tests/extra_setuptools/test_setuphelper.py b/tests/extra_setuptools/test_setuphelper.py index c24f50af8..788f368b1 100644 --- a/tests/extra_setuptools/test_setuphelper.py +++ b/tests/extra_setuptools/test_setuphelper.py @@ -8,6 +8,7 @@ import pytest DIR = os.path.abspath(os.path.dirname(__file__)) MAIN_DIR = os.path.dirname(os.path.dirname(DIR)) +WIN = sys.platform.startswith("win32") or sys.platform.startswith("cygwin") @pytest.mark.parametrize("parallel", [False, True]) @@ -71,13 +72,20 @@ def test_simple_setup_py(monkeypatch, tmpdir, parallel, std): encoding="ascii", ) - subprocess.check_call( + out = subprocess.check_output( [sys.executable, "setup.py", "build_ext", "--inplace"], - stdout=sys.stdout, - stderr=sys.stderr, ) + if not WIN: + assert b"-g0" in out + out = subprocess.check_output( + [sys.executable, "setup.py", "build_ext", "--inplace", "--force"], + env=dict(os.environ, CFLAGS="-g"), + ) + if not WIN: + assert b"-g0" not in out # Debug helper printout, normally hidden + print(out) for item in tmpdir.listdir(): print(item.basename) From aebd21b53c6be5a817a50491327371393aa9d8de Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Wed, 10 Nov 2021 12:13:10 -0500 Subject: [PATCH 3/6] docs: rework CI a bit, more modern skipping (#3424) * docs: rework CI a bit, more modern skipping * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- .github/workflows/ci.yml | 9 +++------ docs/Doxyfile | 3 +-- docs/requirements.txt | 13 +++++-------- include/pybind11/pytypes.h | 8 ++++---- noxfile.py | 10 ++++++---- 5 files changed, 19 insertions(+), 24 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3694f1636..2888c420b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -704,14 +704,11 @@ jobs: - name: Install Doxygen run: sudo apt-get install -y doxygen librsvg2-bin # Changed to rsvg-convert in 20.04 - - name: Install docs & setup requirements - run: python3 -m pip install -r docs/requirements.txt - - name: Build docs - run: python3 -m sphinx -W -b html docs docs/.build + run: pipx run nox -s docs - name: Make SDist - run: python3 setup.py sdist + run: pipx run nox -s build -- --sdist - run: git status --ignored @@ -723,7 +720,7 @@ jobs: - name: Compare Dists (headers only) working-directory: include run: | - python3 -m pip install --user -U ../dist/* + python3 -m pip install --user -U ../dist/*.tar.gz installed=$(python3 -c "import pybind11; print(pybind11.get_include() + '/pybind11')") diff -rq $installed ./pybind11 diff --git a/docs/Doxyfile b/docs/Doxyfile index c8562952e..62c267556 100644 --- a/docs/Doxyfile +++ b/docs/Doxyfile @@ -18,6 +18,5 @@ ALIASES += "endrst=\endverbatim" QUIET = YES WARNINGS = YES WARN_IF_UNDOCUMENTED = NO -PREDEFINED = DOXYGEN_SHOULD_SKIP_THIS \ - PY_MAJOR_VERSION=3 \ +PREDEFINED = PY_MAJOR_VERSION=3 \ PYBIND11_NOINLINE diff --git a/docs/requirements.txt b/docs/requirements.txt index 8f293b5d3..b2801b1f0 100644 --- a/docs/requirements.txt +++ b/docs/requirements.txt @@ -1,8 +1,5 @@ -breathe==4.26.1 -# docutils 0.17 breaks HTML tags & RTD theme -# https://github.com/sphinx-doc/sphinx/issues/9001 -docutils==0.16 -sphinx==3.3.1 -sphinx_rtd_theme==0.5.0 -sphinxcontrib-moderncmakedomain==3.17 -sphinxcontrib-svg2pdfconverter==1.1.0 +breathe==4.31.0 +sphinx==3.5.4 +sphinx_rtd_theme==1.0.0 +sphinxcontrib-moderncmakedomain==3.19 +sphinxcontrib-svg2pdfconverter==1.1.1 diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index f54d5fad6..a08fd8ceb 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -287,10 +287,10 @@ protected: struct borrowed_t { }; struct stolen_t { }; -#ifndef DOXYGEN_SHOULD_SKIP_THIS // Issue in breathe 4.26.1 + /// @cond BROKEN template friend T reinterpret_borrow(handle); template friend T reinterpret_steal(handle); -#endif + /// @endcond public: // Only accessible from derived classes and the reinterpret_* functions @@ -1717,7 +1717,7 @@ public: #endif }; -#ifndef DOXYGEN_SHOULD_SKIP_THIS +/// @cond DUPLICATE inline memoryview memoryview::from_buffer( void *ptr, ssize_t itemsize, const char* format, detail::any_container shape, @@ -1745,7 +1745,7 @@ inline memoryview memoryview::from_buffer( throw error_already_set(); return memoryview(object(obj, stolen_t{})); } -#endif // DOXYGEN_SHOULD_SKIP_THIS +/// @endcond /// @} pytypes /// \addtogroup python_builtins diff --git a/noxfile.py b/noxfile.py index 53d87dbcb..4adffac2e 100644 --- a/noxfile.py +++ b/noxfile.py @@ -57,10 +57,10 @@ def docs(session: nox.Session) -> None: session.chdir("docs") if "pdf" in session.posargs: - session.run("sphinx-build", "-M", "latexpdf", ".", "_build") + session.run("sphinx-build", "-b", "latexpdf", ".", "_build") return - session.run("sphinx-build", "-M", "html", ".", "_build") + session.run("sphinx-build", "-b", "html", ".", "_build") if "serve" in session.posargs: session.log("Launching docs at http://localhost:8000/ - use Ctrl-C to quit") @@ -86,6 +86,8 @@ def build(session: nox.Session) -> None: session.install("build") session.log("Building normal files") - session.run("python", "-m", "build") + session.run("python", "-m", "build", *session.posargs) session.log("Building pybind11-global files (PYBIND11_GLOBAL_SDIST=1)") - session.run("python", "-m", "build", env={"PYBIND11_GLOBAL_SDIST": "1"}) + session.run( + "python", "-m", "build", *session.posargs, env={"PYBIND11_GLOBAL_SDIST": "1"} + ) From e450eb62c21c8518c1988c3b17bc5793ea347756 Mon Sep 17 00:00:00 2001 From: Guillaume Jacquenot Date: Fri, 12 Nov 2021 16:53:43 +0100 Subject: [PATCH 4/6] Removed duplicated word in docs/advanced/cast/eigen.rst (#3458) --- docs/advanced/cast/eigen.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/advanced/cast/eigen.rst b/docs/advanced/cast/eigen.rst index 80f101343..a5c11a3f1 100644 --- a/docs/advanced/cast/eigen.rst +++ b/docs/advanced/cast/eigen.rst @@ -52,7 +52,7 @@ can be mapped *and* if the numpy array is writeable (that is the passed variable will be transparently carried out directly on the ``numpy.ndarray``. -This means you can can write code such as the following and have it work as +This means you can write code such as the following and have it work as expected: .. code-block:: cpp From 270b11d50229e67e2d8d63e96464f9bcd12b527c Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Sat, 13 Nov 2021 21:32:58 -0500 Subject: [PATCH 5/6] Revert "style: drop pycln" (#3466) * Revert "style: drop pycln (#3397)" This reverts commit 606f81a966e56d1ec0f17b0c032e8d6679430d67. * Update .pre-commit-config.yaml --- .pre-commit-config.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6f0bb39b1..91047b328 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -61,6 +61,12 @@ repos: hooks: - id: remove-tabs +# Autoremoves unused imports +- repo: https://github.com/hadialqattan/pycln + rev: v1.1.0 + hooks: + - id: pycln + - repo: https://github.com/pre-commit/pygrep-hooks rev: v1.9.0 hooks: From afdc09deda50d82eba6cc2e48228df7d909650be Mon Sep 17 00:00:00 2001 From: Trigve Date: Mon, 15 Nov 2021 19:36:41 +0100 Subject: [PATCH 6/6] [master] Wrong caching of overrides (#3465) * override: Fix wrong caching of the overrides There was a problem when the python type, which was stored in override cache for C++ functions, was destroyed and the record wasn't removed from the override cache. Therefor, dangling pointer was stored there. Then when the memory was reused and new type was allocated at the given address and the method with the same name (as previously stored in the cache) was actually overridden in python, it would wrongly find it in the override cache for C++ functions and therefor override from python wouldn't be called. The fix is to erase the type from the override cache when the type is destroyed. * test: Pass by const ref instead of by value (clang-tidy) * test: Rename classes and move to different files Rename the classes and files so they're no too generic. Also, better place to test the stuff is in test_virtual_functions.cpp/.py as we're basically testing the virtual functions/trampolines. * Add TODO for erasure code * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- include/pybind11/pybind11.h | 10 ++++++ tests/test_embed/CMakeLists.txt | 2 +- tests/test_embed/test_interpreter.cpp | 49 +++++++++++++++++++++++++++ tests/test_embed/test_trampoline.py | 18 ++++++++++ tests/test_virtual_functions.cpp | 25 ++++++++++++++ tests/test_virtual_functions.py | 19 +++++++++++ 6 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 tests/test_embed/test_trampoline.py diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index b8b7c7a02..c2b88688a 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1979,6 +1979,16 @@ inline std::pair all_t // gets destroyed: weakref((PyObject *) type, cpp_function([type](handle wr) { get_internals().registered_types_py.erase(type); + + // TODO consolidate the erasure code in pybind11_meta_dealloc() in class.h + auto &cache = get_internals().inactive_override_cache; + for (auto it = cache.begin(), last = cache.end(); it != last; ) { + if (it->first == reinterpret_cast(type)) + it = cache.erase(it); + else + ++it; + } + wr.dec_ref(); })).release(); } diff --git a/tests/test_embed/CMakeLists.txt b/tests/test_embed/CMakeLists.txt index 3b89d6e58..edb8961a7 100644 --- a/tests/test_embed/CMakeLists.txt +++ b/tests/test_embed/CMakeLists.txt @@ -25,7 +25,7 @@ pybind11_enable_warnings(test_embed) target_link_libraries(test_embed PRIVATE pybind11::embed Catch2::Catch2 Threads::Threads) if(NOT CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_CURRENT_BINARY_DIR) - file(COPY test_interpreter.py DESTINATION "${CMAKE_CURRENT_BINARY_DIR}") + file(COPY test_interpreter.py test_trampoline.py DESTINATION "${CMAKE_CURRENT_BINARY_DIR}") endif() add_custom_target( diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index 20bcade0a..508975eb3 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -37,6 +37,22 @@ class PyWidget final : public Widget { std::string argv0() const override { PYBIND11_OVERRIDE_PURE(std::string, Widget, argv0); } }; +class test_override_cache_helper { + +public: + virtual int func() { return 0; } + + test_override_cache_helper() = default; + virtual ~test_override_cache_helper() = default; + // Non-copyable + test_override_cache_helper &operator=(test_override_cache_helper const &Right) = delete; + test_override_cache_helper(test_override_cache_helper const &Copy) = delete; +}; + +class test_override_cache_helper_trampoline : public test_override_cache_helper { + int func() override { PYBIND11_OVERRIDE(int, test_override_cache_helper, func); } +}; + PYBIND11_EMBEDDED_MODULE(widget_module, m) { py::class_(m, "Widget") .def(py::init()) @@ -45,6 +61,12 @@ PYBIND11_EMBEDDED_MODULE(widget_module, m) { m.def("add", [](int i, int j) { return i + j; }); } +PYBIND11_EMBEDDED_MODULE(trampoline_module, m) { + py::class_>(m, "test_override_cache_helper") + .def(py::init_alias<>()) + .def("func", &test_override_cache_helper::func); +} + PYBIND11_EMBEDDED_MODULE(throw_exception, ) { throw std::runtime_error("C++ Error"); } @@ -73,6 +95,33 @@ TEST_CASE("Pass classes and data between modules defined in C++ and Python") { REQUIRE(cpp_widget.the_answer() == 42); } +TEST_CASE("Override cache") { + auto module_ = py::module_::import("test_trampoline"); + REQUIRE(py::hasattr(module_, "func")); + REQUIRE(py::hasattr(module_, "func2")); + + auto locals = py::dict(**module_.attr("__dict__")); + + int i = 0; + for (; i < 1500; ++i) { + std::shared_ptr p_obj; + std::shared_ptr p_obj2; + + py::object loc_inst = locals["func"](); + p_obj = py::cast>(loc_inst); + + int ret = p_obj->func(); + + REQUIRE(ret == 42); + + loc_inst = locals["func2"](); + + p_obj2 = py::cast>(loc_inst); + + p_obj2->func(); + } +} + TEST_CASE("Import error handling") { REQUIRE_NOTHROW(py::module_::import("widget_module")); REQUIRE_THROWS_WITH(py::module_::import("throw_exception"), diff --git a/tests/test_embed/test_trampoline.py b/tests/test_embed/test_trampoline.py new file mode 100644 index 000000000..87c8fa44c --- /dev/null +++ b/tests/test_embed/test_trampoline.py @@ -0,0 +1,18 @@ +# -*- coding: utf-8 -*- + +import trampoline_module + + +def func(): + class Test(trampoline_module.test_override_cache_helper): + def func(self): + return 42 + + return Test() + + +def func2(): + class Test(trampoline_module.test_override_cache_helper): + pass + + return Test() diff --git a/tests/test_virtual_functions.cpp b/tests/test_virtual_functions.cpp index 6e06db9fc..f1a513180 100644 --- a/tests/test_virtual_functions.cpp +++ b/tests/test_virtual_functions.cpp @@ -214,6 +214,25 @@ static void test_gil_from_thread() { t.join(); } +class test_override_cache_helper { + +public: + virtual int func() { return 0; } + + test_override_cache_helper() = default; + virtual ~test_override_cache_helper() = default; + // Non-copyable + test_override_cache_helper &operator=(test_override_cache_helper const &Right) = delete; + test_override_cache_helper(test_override_cache_helper const &Copy) = delete; +}; + +class test_override_cache_helper_trampoline : public test_override_cache_helper { + int func() override { PYBIND11_OVERRIDE(int, test_override_cache_helper, func); } +}; + +inline int test_override_cache(std::shared_ptr const &instance) { return instance->func(); } + + // Forward declaration (so that we can put the main tests here; the inherited virtual approaches are // rather long). @@ -378,6 +397,12 @@ TEST_SUBMODULE(virtual_functions, m) { // .def("str_ref", &OverrideTest::str_ref) .def("A_value", &OverrideTest::A_value) .def("A_ref", &OverrideTest::A_ref); + + py::class_>(m, "test_override_cache_helper") + .def(py::init_alias<>()) + .def("func", &test_override_cache_helper::func); + + m.def("test_override_cache", test_override_cache); } diff --git a/tests/test_virtual_functions.py b/tests/test_virtual_functions.py index 0b550992f..4f25cac4a 100644 --- a/tests/test_virtual_functions.py +++ b/tests/test_virtual_functions.py @@ -439,3 +439,22 @@ def test_issue_1454(): # Fix issue #1454 (crash when acquiring/releasing GIL on another thread in Python 2.7) m.test_gil() m.test_gil_from_thread() + + +def test_python_override(): + def func(): + class Test(m.test_override_cache_helper): + def func(self): + return 42 + + return Test() + + def func2(): + class Test(m.test_override_cache_helper): + pass + + return Test() + + for _ in range(1500): + assert m.test_override_cache(func()) == 42 + assert m.test_override_cache(func2()) == 0