From 6a492e2127a3b53e0348e78ca9f694da07525d85 Mon Sep 17 00:00:00 2001 From: Vincent Palancher Date: Fri, 9 Jun 2023 20:29:29 +0200 Subject: [PATCH 01/10] Bumps version to v3.1.4.dev0. --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 6e048cc..3c9d70d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,7 +7,7 @@ build-backend = "scikit_build_core.build" [project] name = "qi" description = "LibQi Python bindings" -version = "3.1.3" +version = "3.1.4.dev0" readme = "README.rst" requires-python = ">=3.7" license = { "file" = "COPYING" } From 82b7aa5d3afc3ef55e03ab3e52f39163a6272177 Mon Sep 17 00:00:00 2001 From: Vincent PALANCHER Date: Thu, 27 Jul 2023 12:25:42 +0000 Subject: [PATCH 02/10] Add Gitlab CI to build wheels. This patch adds Gitlab CI to the project to build wheels for Linux for each supported version of Python. This build can be manually triggered for any commit, but is automatic when a "qi-python-vXXX" tag is pushed. Furthermore, when a tag is pushed on GitLab, wheels are published in the package registry and a release is created. --- .gitlab-ci.yml | 55 ++++++++++++++++++++ CMakeLists.txt | 94 +++++++++++++++++------------------ ci/cibuildwheel_before_all.sh | 37 ++++++++++++++ ci/conan/global.conf | 2 + ci/conan/profiles/default | 8 +++ pyproject.toml | 19 +++++++ 6 files changed, 168 insertions(+), 47 deletions(-) create mode 100644 .gitlab-ci.yml create mode 100755 ci/cibuildwheel_before_all.sh create mode 100644 ci/conan/global.conf create mode 100644 ci/conan/profiles/default diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml new file mode 100644 index 0000000..61b2b8c --- /dev/null +++ b/.gitlab-ci.yml @@ -0,0 +1,55 @@ +stages: + - build + - publish + - release + +.if-tag: &if-tag + if: "$CI_COMMIT_TAG =~ /^qi-python-v/" + +build-wheel: + stage: build + rules: + # Allow manually building on a Git commit on any branch. + - if: $CI_COMMIT_BRANCH + when: manual + # Always build on a Git tag. + - <<: *if-tag + tags: + - docker + image: python:3.8 + variables: + LIBQI_REPOSITORY_URL: "https://gitlab-ci-token:$CI_JOB_TOKEN@$CI_SERVER_HOST/qi/libqi" + script: + - curl -sSL https://get.docker.com/ | sh + - pip install cibuildwheel==2.14.1 + - cibuildwheel --output-dir wheelhouse + artifacts: + paths: + - wheelhouse/ + +publish-wheel: + stage: publish + image: python:latest + rules: + - <<: *if-tag + needs: + - build-wheel + script: + - pip install build twine + - python -m build + - TWINE_PASSWORD="${CI_JOB_TOKEN}" + TWINE_USERNAME=gitlab-ci-token + python -m twine upload + --repository-url "${CI_API_V4_URL}/projects/${CI_PROJECT_ID}/packages/pypi" + wheelhouse/* + +create-release: + stage: release + rules: + - <<: *if-tag + script: + - echo "Releasing $CI_COMMIT_TAG." + release: + tag_name: $CI_COMMIT_TAG + name: 'lib$CI_COMMIT_TITLE' + description: '$CI_COMMIT_TAG_MESSAGE' diff --git a/CMakeLists.txt b/CMakeLists.txt index b2a5f22..064b39a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -66,16 +66,14 @@ endif() ############################################################################## # Find Python ############################################################################## -# First search for Python with Interpreter+Devel components, to allow the module -# to look for the interpreter that goes with the Python development library. -# Then if it could not find both, try looking for the development component -# only, but this time force the module to find it or fail (REQUIRED). -# Some of our toolchains (when crosscompiling for instance) have the Python -# library but not an interpreter that can run on the host. -find_package(Python COMPONENTS Development Interpreter) -if(NOT Python_FOUND) - find_package(Python REQUIRED COMPONENTS Development) -endif() +find_package( + Python REQUIRED + COMPONENTS + Development.Module + OPTIONAL_COMPONENTS + Interpreter + Development.Embed +) ############################################################################## # Find Boost @@ -323,47 +321,49 @@ if(BUILD_TESTING) qi::qi ) - add_executable(test_qipython) - target_sources( - test_qipython - PRIVATE - tests/common.hpp - tests/test_qipython.cpp - tests/test_guard.cpp - tests/test_types.cpp - tests/test_signal.cpp - tests/test_property.cpp - tests/test_object.cpp - tests/test_module.cpp - ) - target_link_libraries( - test_qipython + if(Python_Development.Embed_FOUND) + add_executable(test_qipython) + target_sources( + test_qipython + PRIVATE + tests/common.hpp + tests/test_qipython.cpp + tests/test_guard.cpp + tests/test_types.cpp + tests/test_signal.cpp + tests/test_property.cpp + tests/test_object.cpp + tests/test_module.cpp + ) + target_link_libraries( + test_qipython + PRIVATE + qi_python_objects + cxx_standard + Python::Python + Boost::headers + pybind11::pybind11 + GTest::gmock + ) + gtest_discover_tests(test_qipython) + + add_executable(test_qipython_local_interpreter) + target_sources( + test_qipython_local_interpreter + PRIVATE + tests/test_qipython_local_interpreter.cpp + ) + target_link_libraries( + test_qipython_local_interpreter PRIVATE - qi_python_objects - cxx_standard Python::Python - Boost::headers pybind11::pybind11 + qi_python_objects + cxx_standard GTest::gmock - ) - gtest_discover_tests(test_qipython) - - add_executable(test_qipython_local_interpreter) - target_sources( - test_qipython_local_interpreter - PRIVATE - tests/test_qipython_local_interpreter.cpp - ) - target_link_libraries( - test_qipython_local_interpreter - PRIVATE - Python::Python - pybind11::pybind11 - qi_python_objects - cxx_standard - GTest::gmock - ) - gtest_discover_tests(test_qipython_local_interpreter) + ) + gtest_discover_tests(test_qipython_local_interpreter) + endif() if(NOT Python_Interpreter_FOUND) message(WARNING "tests: a compatible Python Interpreter was NOT found, Python tests are DISABLED.") diff --git a/ci/cibuildwheel_before_all.sh b/ci/cibuildwheel_before_all.sh new file mode 100755 index 0000000..72ea5e3 --- /dev/null +++ b/ci/cibuildwheel_before_all.sh @@ -0,0 +1,37 @@ +#!/bin/sh +set -x -e + +PACKAGE=$1 + +pip install 'conan>=2' 'cmake>=3.23' ninja + +# Perl dependencies required to build OpenSSL. +yum install -y perl-IPC-Cmd perl-Digest-SHA + +# Install Conan configuration. +conan config install "$PACKAGE/ci/conan" + +# Clone and export libqi to Conan cache. +# Possible improvement: +# Avoid duplicating the version number here with the version +# defined in conanfile.py. +GIT_SSL_NO_VERIFY=true \ + git clone --depth=1 \ + --branch qi-framework-v4.0.1 \ + "$LIBQI_REPOSITORY_URL" \ + /work/libqi +conan export /work/libqi --version=4.0.1 + +# Install dependencies of libqi-python from Conan, including libqi. +# Only use the build_type as a variable for the build folder name, so +# that the generated CMake preset is named "conan-release". +# +# Build everything from sources, so that we do not reuse precompiled binaries. +# This is because the GLIBC from the manylinux images are often older than the +# ones that were used to build the precompiled binaries, which means the binaries +# cannot by executed. +conan install "$PACKAGE" \ + --build="*" \ + -c tools.build:skip_test=true \ + -c tools.cmake.cmaketoolchain:generator=Ninja \ + -c tools.cmake.cmake_layout:build_folder_vars=[\"settings.build_type\"] diff --git a/ci/conan/global.conf b/ci/conan/global.conf new file mode 100644 index 0000000..bbcc54a --- /dev/null +++ b/ci/conan/global.conf @@ -0,0 +1,2 @@ +core:default_profile=default +core:default_build_profile=default diff --git a/ci/conan/profiles/default b/ci/conan/profiles/default new file mode 100644 index 0000000..4ca0f95 --- /dev/null +++ b/ci/conan/profiles/default @@ -0,0 +1,8 @@ +[settings] +arch=x86_64 +build_type=Release +compiler=gcc +compiler.cppstd=gnu17 +compiler.libcxx=libstdc++ +compiler.version=10 +os=Linux diff --git a/pyproject.toml b/pyproject.toml index 3c9d70d..2c98a36 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -47,3 +47,22 @@ maintainers = [ [project.urls] repository = "https://github.com/aldebaran/libqi-python" + +[tool.scikit-build] +# Rely only on CMake install, not on Python packages detection. +wheel.packages = [] + +[tool.cibuildwheel] +build = "cp*manylinux*x86_64" +build-frontend = "build" + +environment-pass = ["LIBQI_REPOSITORY_URL"] + +[tool.cibuildwheel.linux] +# Build using the manylinux2014 image +manylinux-x86_64-image = "manylinux2014" + +before-all = ["ci/cibuildwheel_before_all.sh {package}"] + +[tool.cibuildwheel.linux.config-settings] +"cmake.args" = ["--preset=conan-release"] From 9dbe864482b99b15a6236b4f91189d9331bf270d Mon Sep 17 00:00:00 2001 From: Vincent Palancher Date: Thu, 27 Jul 2023 14:27:49 +0200 Subject: [PATCH 03/10] Upgrade pybind11 to any 2.9+ version. --- conanfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conanfile.py b/conanfile.py index 6777ae0..9d5e3a5 100644 --- a/conanfile.py +++ b/conanfile.py @@ -54,7 +54,7 @@ class QiPythonConan(ConanFile): requires = [ "boost/[~1.78]", - "pybind11/[~2.9]", + "pybind11/[^2.9]", "qi/4.0.1", ] From f2e6d0aa3886f21f65eaf41ca154104b42fc08cf Mon Sep 17 00:00:00 2001 From: Vincent Palancher Date: Thu, 21 Sep 2023 16:57:08 +0200 Subject: [PATCH 04/10] Replaces some `pybind11::handle` uses by type deduction. This change is done to prevent slicing `pybind11::object` values (or its subclasses) into `pybind11::handle`, which may result into handles to garbage collected objects. --- src/pyobject.cpp | 4 ++-- src/pystrand.cpp | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/pyobject.cpp b/src/pyobject.cpp index ddd1a4f..4038ed3 100644 --- a/src/pyobject.cpp +++ b/src/pyobject.cpp @@ -333,7 +333,7 @@ Object toObject(const ::py::object& obj) : ObjectThreadingModel_SingleThread); const auto attrKeys = ::py::reinterpret_steal<::py::list>(PyObject_Dir(obj.ptr())); - for (const ::py::handle& pyAttrKey : attrKeys) + for (const auto& pyAttrKey : attrKeys) { QI_ASSERT_TRUE(pyAttrKey); QI_ASSERT_FALSE(pyAttrKey.is_none()); @@ -342,7 +342,7 @@ Object toObject(const ::py::object& obj) const auto attrKey = pyAttrKey.cast(); auto memberName = attrKey; - const ::py::object attr = obj.attr(pyAttrKey); + const auto attr = obj.attr(pyAttrKey); if (attr.is_none()) { qiLogVerbose() << "The object attribute '" << attrKey diff --git a/src/pystrand.cpp b/src/pystrand.cpp index 7da98c5..0e1b2d8 100644 --- a/src/pystrand.cpp +++ b/src/pystrand.cpp @@ -35,15 +35,15 @@ bool isUnboundMethod(const ::py::function& func, const ::py::object& object) GILAcquire lock; const auto dir = ::py::reinterpret_steal<::py::list>(PyObject_Dir(object.ptr())); - for (const ::py::handle attrName : dir) + for (const auto& attrName : dir) { - const ::py::handle attr = object.attr(attrName); + const auto attr = object.attr(attrName); QI_ASSERT_TRUE(attr); if (!PyMethod_Check(attr.ptr())) continue; - const ::py::handle unboundMethod = PyMethod_GET_FUNCTION(attr.ptr()); + const auto unboundMethod = ::py::reinterpret_borrow<::py::object>(PyMethod_GET_FUNCTION(attr.ptr())); if (func.is(unboundMethod)) return true; } From 95df33474131cf6c21d6473110c4d6bfd232dbc1 Mon Sep 17 00:00:00 2001 From: Vincent PALANCHER Date: Tue, 7 Nov 2023 15:08:25 +0000 Subject: [PATCH 05/10] Reworks GIL related types to prevent some forms of program termination. This change is a rework of some GIL related types in the library to prevent and avoid some unexpected crashes and program terminations, which would occur most often at process exit: - A new exception type named `InterpreterFinalizingException` is introduced. It is thrown by some functions in the library when an operation fails because the interpreter is currently finalizing. - `GILAcquire` constructor now throws a `InterpreterFinalizingException` when the interpreter is finalizing and therefore the GIL could not be acquired. - `GILRelease` destructor now does not reacquire the GIL when the interpreter is finalizing. - The `Guarded` and `GILGuardedObject` types are removed, and replaced with a `SharedObject` type. This type now wraps a Python object value as a shared reference-counted value that does not require the GIL to copy or move. The last copy of a `SharedObject` value will acquire the GIL to release the object, if possible, or do nothing (and leak it) if the GIL cannot be acquired. - The `DeleteInOtherThread` deleter type is removed. Its implementation was incorrect and it has become unneeded. - Destructors of `Application` and `ApplicationSession` do not use the `DeleterInOtherThread` deleter type anymore. The hypothesis is that the `DeleteInOtherThread` deleter was previously needed because `Application` values were wrapped in shared pointers and could be destroyed in callbacks executed by native threads. This is not the case anymore and instead, they destroy the application in the same thread that the Python `Application` object is garbage collected, which most likely is the interpreter main thread (due to `Application` being a singleton in the Python code, and its destruction is scheduled in an `atexit` callback). - A new `invokeCatchPythonError` is introduced. It acquires the GIL, then invokes a function with the given arguments, but catches any `pybind11::error_already_set` exception thrown during this invocation, and throws a `std::runtime_error` instead. - The `currentThreadHoldsGil` function now returns an optional denoting if the interpreter is initialized and not finalizing. Another function `gilExistsAndCurrentThreadHoldsIt` is introduced that returns just a bool. - The `ObjectDecRef` internal type is removed, as it is unused. This change mitigates the problem observed in issue #SW-4658. --- CMakeLists.txt | 1 + qipython/common.hpp | 24 ++ qipython/common.hxx | 37 +++ qipython/pyguard.hpp | 290 ++++++++++++---------- src/pyapplication.cpp | 5 +- src/pyasync.cpp | 20 +- src/pyfuture.cpp | 17 +- src/pyobject.cpp | 12 +- src/pysignal.cpp | 6 +- src/pytypes.cpp | 10 - tests/test_guard.cpp | 118 ++++----- tests/test_qipython_local_interpreter.cpp | 69 ++++- 12 files changed, 366 insertions(+), 243 deletions(-) create mode 100644 qipython/common.hxx diff --git a/CMakeLists.txt b/CMakeLists.txt index 064b39a..ddba9e4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -115,6 +115,7 @@ target_sources( PUBLIC qipython/common.hpp + qipython/common.hxx qipython/pyapplication.hpp qipython/pyasync.hpp qipython/pyclock.hpp diff --git a/qipython/common.hpp b/qipython/common.hpp index 55fbf78..3335f66 100644 --- a/qipython/common.hpp +++ b/qipython/common.hpp @@ -125,6 +125,28 @@ inline boost::optional interpreterIsFinalizing() #endif } +/// Acquires the GIL, then invokes a function with the given arguments, but +/// catches any `pybind11::error_already_set` exception thrown during this +/// invocation, and throws a `std::runtime_error` instead. This is useful to +/// avoid the undefined behavior caused by the `what` member function of +/// `pybind11::error_already_set` when the GIL is not acquired. +template +auto invokeCatchPythonError(F&& f, Args&&... args); + +/// Exception type thrown when an operation fails because the interpreter is +/// finalizing. +/// +/// Some operations are forbidden during interpreter finalization as they may +/// terminate the thread they are called from. This exception allows stopping +/// the flow of execution in this case before such functions are called. +struct InterpreterFinalizingException : std::exception +{ + inline const char* what() const noexcept override + { + return "the interpreter is finalizing"; + } +}; + } // namespace py } // namespace qi @@ -203,4 +225,6 @@ namespace detail PYBIND11_DECLARE_HOLDER_TYPE(T, boost::shared_ptr); +#include + #endif // QIPYTHON_COMMON_HPP diff --git a/qipython/common.hxx b/qipython/common.hxx new file mode 100644 index 0000000..2618b47 --- /dev/null +++ b/qipython/common.hxx @@ -0,0 +1,37 @@ +/* +** Copyright (C) 2023 Aldebaran Robotics +** See COPYING for the license +*/ + +#pragma once + +#ifndef QIPYTHON_COMMON_HXX +#define QIPYTHON_COMMON_HXX + +#include +#include +#include + +namespace qi +{ +namespace py +{ + +template +auto invokeCatchPythonError(F&& f, Args&&... args) +{ + GILAcquire acquire; + try + { + return std::invoke(std::forward(f), std::forward(args)...); + } + catch (const pybind11::error_already_set& err) + { + throw std::runtime_error(err.what()); + } +} + +} // namespace py +} // namespace qi + +#endif // QIPYTHON_COMMON_HXX diff --git a/qipython/pyguard.hpp b/qipython/pyguard.hpp index 33ea618..f20ee1b 100644 --- a/qipython/pyguard.hpp +++ b/qipython/pyguard.hpp @@ -10,6 +10,7 @@ #include #include +#include #include namespace qi @@ -17,10 +18,25 @@ namespace qi namespace py { -/// Returns whether or not the GIL is currently held by the current thread. -inline bool currentThreadHoldsGil() +/// Returns whether or not the GIL is currently held by the current thread. If +/// the interpreter is not yet initialized or has been finalized, returns an +/// empty optional, as there is no GIL available. +inline boost::optional currentThreadHoldsGil() { - return PyGILState_Check() == 1; + // PyGILState_Check() returns 1 (success) before the creation of the GIL and + // after the destruction of the GIL. + if (Py_IsInitialized() == 1) { + const auto gilAcquired = PyGILState_Check() == 1; + return boost::make_optional(gilAcquired); + } + return boost::none; +} + +/// Returns whether or not the GIL exists (i.e the interpreter is initialized +/// and not finalizing) and is currently held by the current thread. +inline bool gilExistsAndCurrentThreadHoldsIt() +{ + return currentThreadHoldsGil().value_or(false); } /// DefaultConstructible Guard @@ -35,98 +51,6 @@ ka::ResultOf invokeGuarded(F&& f, Args&&... args) namespace detail { -// Guards the copy, construction and destruction of an object, and only when -// in case of copy construction the source object evaluates to true, and in case -// of destruction, the object itself evaluates to true. -// -// Explanation: -// pybind11::object only requires the GIL to be locked on copy and if the -// source is not null. Default construction does not require the GIL either. -template -class Guarded -{ - using Storage = - typename std::aligned_storage::type; - Storage _object; - - Object& object() { return reinterpret_cast(_object); } - const Object& object() const { return reinterpret_cast(_object); } - -public: - template::value, int> = 0> - explicit Guarded(Arg0&& arg0, Args&&... args) - { - Guard g; - new(&_object) Object(std::forward(arg0), std::forward(args)...); - } - - Guarded() - { - // Default construction, no GIL needed. - new(&_object) Object(); - } - - Guarded(Guarded& o) - { - boost::optional optGuard; - if (o.object()) - optGuard.emplace(); - new(&_object) Object(o.object()); - } - - Guarded(const Guarded& o) - { - boost::optional optGuard; - if (o.object()) - optGuard.emplace(); - new(&_object) Object(o.object()); - } - - Guarded(Guarded&& o) - { - new(&_object) Object(std::move(o.object())); - } - - ~Guarded() - { - boost::optional optGuard; - if (object()) - optGuard.emplace(); - object().~Object(); - } - - Guarded& operator=(const Guarded& o) - { - if (this == &o) - return *this; - - { - boost::optional optGuard; - if (o.object()) - optGuard.emplace(); - object() = o.object(); - } - return *this; - } - - Guarded& operator=(Guarded&& o) - { - if (this == &o) - return *this; - - object() = std::move(o.object()); - return *this; - } - - Object& operator*() { return object(); } - const Object& operator*() const { return object(); } - Object* operator->() { return &object(); } - const Object* operator->() const { return &object(); } - explicit operator Object&() { return object(); } - explicit operator const Object&() const { return object(); } -}; - /// G == pybind11::gil_scoped_acquire || G == pybind11::gil_scoped_release template void pybind11GuardDisarm(G& guard) @@ -141,37 +65,52 @@ void pybind11GuardDisarm(G& guard) } // namespace detail /// RAII utility type that guarantees that the GIL is locked for the scope of -/// the lifetime of the object. +/// the lifetime of the object. If the GIL cannot be acquired (for example, +/// because the interpreter is finalizing), throws an `InterpreterFinalizingException` +/// exception. /// /// Objects of this type (or objects composed of them) must not be kept alive /// after the hand is given back to the interpreter. /// /// This type is re-entrant. /// -/// postcondition: `GILAcquire acq;` establishes -/// `(interpreterIsFinalizing() && *interpreterIsFinalizing()) || currentThreadHoldsGil()` +/// postcondition: `GILAcquire acq;` establishes `gilExistsAndCurrentThreadHoldsIt()` struct GILAcquire { inline GILAcquire() { - const auto optIsFinalizing = interpreterIsFinalizing(); - const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing; - // `gil_scoped_acquire` is re-entrant by itself, so we don't need to check - // whether or not the GIL is already held by the current thread. - if (!definitelyFinalizing) - _acq.emplace(); - QI_ASSERT(definitelyFinalizing || currentThreadHoldsGil()); + if (gilExistsAndCurrentThreadHoldsIt()) + return; + + const auto isFinalizing = interpreterIsFinalizing().value_or(false); + if (isFinalizing) + throw InterpreterFinalizingException(); + + _state = ::PyGILState_Ensure(); + QI_ASSERT(gilExistsAndCurrentThreadHoldsIt()); + } + + inline ~GILAcquire() + { + // Even if releasing the GIL while the interpreter is finalizing is allowed, it does + // require the GIL to be currently held. But we have no guarantee that this is the case, + // because the GIL may have been released since we acquired it, and we could not + // reacquire it after that (maybe the interpreter is in fact finalizing). + // Therefore, only release the GIL if it is currently held by this thread. + if (_state && gilExistsAndCurrentThreadHoldsIt()) + ::PyGILState_Release(*_state); } + GILAcquire(const GILAcquire&) = delete; GILAcquire& operator=(const GILAcquire&) = delete; private: - boost::optional _acq; + boost::optional _state; }; -/// RAII utility type that guarantees that the GIL is unlocked for the scope of -/// the lifetime of the object. +/// RAII utility type that (as a best effort) tries to ensure that the GIL is +/// unlocked for the scope of the lifetime of the object. /// /// Objects of this type (or objects composed of them) must not be kept alive /// after the hand is given back to the interpreter. @@ -179,16 +118,44 @@ struct GILAcquire /// This type is re-entrant. /// /// postcondition: `GILRelease rel;` establishes -/// `(interpreterIsFinalizing() && *interpreterIsFinalizing()) || !currentThreadHoldsGil()` +/// `interpreterIsFinalizing().value_or(false) || +/// !gilExistsAndCurrentThreadHoldsIt()` struct GILRelease { inline GILRelease() { - const auto optIsFinalizing = interpreterIsFinalizing(); - const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing; - if (!definitelyFinalizing && currentThreadHoldsGil()) + // Even if releasing the GIL while the interpreter is finalizing is allowed, + // it does require the GIL to be currently held. However, reacquiring the + // GIL is forbidden if finalization started. + // + // It may happen that we try to release the GIL from a function ('F') called + // by the Python interpreter, while it is holding the GIL. In this case, if + // we try to release it, then fail to reacquire it and return the hand to + // the interpreter, the interpreter will most likely terminate the process, + // as it expects to still be holding the GIL. Failure to reacquire the GIL + // can only happen if the interpreter started finalization, either before we + // released it, or while it was released. + // + // Fortunately, in this case, because the interpreter is busy executing the + // function 'F', it is not possible for it to start finalization until 'F' + // returns. This means that the only possible reason of failure of + // reacquisition of the GIL is because the interpreter was finalizing + // *before* calling 'F', therefore before we released to GIL. Therefore, to + // prevent this failure, we check if the interpreter is finalizing before + // releasing the GIL, and if it is, we do nothing, and the GIL stays held. + const auto isFinalizing = interpreterIsFinalizing().value_or(false); + if (!isFinalizing && gilExistsAndCurrentThreadHoldsIt()) _release.emplace(); - QI_ASSERT(definitelyFinalizing || !currentThreadHoldsGil()); + QI_ASSERT(isFinalizing || !gilExistsAndCurrentThreadHoldsIt()); + } + + inline ~GILRelease() + { + // Reacquiring the GIL is forbidden when the interpreter is finalizing, as + // it may terminate the current thread. + const auto isFinalizing = interpreterIsFinalizing().value_or(false); + if (_release && isFinalizing) + detail::pybind11GuardDisarm(*_release); } GILRelease(const GILRelease&) = delete; @@ -198,11 +165,83 @@ struct GILRelease boost::optional _release; }; -/// Wraps a pybind11::object value and locks the GIL on copy and destruction. +/// Wraps a Python object as a shared reference-counted value that does not +/// require the GIL to copy, move or assign to. /// -/// This is useful for instance to put pybind11 objects in lambda functions so -/// that they can be copied around safely. -using GILGuardedObject = detail::Guarded; +/// On destruction, it releases the object with the GIL acquired. However, if +/// the GIL cannot be acquired, the object is leaked. This is most likely +/// mitigated by the fact that if the GIL is not available, it means the object +/// already has been or soon will be garbage collected by interpreter +/// finalization. +template +class SharedObject +{ + static_assert(std::is_base_of_v, + "template parameter T must be a subclass of pybind11::object"); + + struct State + { + std::mutex mutex; + T object; + }; + std::shared_ptr _state; + + struct StateDeleter + { + inline void operator()(State* state) const + { + auto handle = state->object.release(); + delete state; + + // Do not lock the GIL if there is nothing to release. + if (!handle) + return; + + try + { + GILAcquire acquire; + handle.dec_ref(); + } + catch (const qi::py::InterpreterFinalizingException&) + { + // Nothing, the interpreter is finalizing. + } + } + }; + +public: + SharedObject() = default; + + inline explicit SharedObject(T object) + : _state(new State { std::mutex(), std::move(object) }, StateDeleter()) + { + } + + /// Copies the inner Python object value by incrementing its reference count. + /// + /// @pre: If the inner value is not null, the GIL must be acquired. + T inner() const + { + QI_ASSERT_NOT_NULL(_state); + std::scoped_lock lock(_state->mutex); + return _state->object; + } + + /// Takes the inner Python object value and leaves a null value in its place. + /// + /// Any copy of the shared object will now have a null inner value. + /// + /// This operation does not require the GIL as the reference count of the + /// object is preserved. + T takeInner() + { + QI_ASSERT_NOT_NULL(_state); + std::scoped_lock lock(_state->mutex); + /// Hypothesis: Moving a `pybind11::object` steals the object and preserves + /// its reference count and therefore does not require the GIL. + return ka::exchange(_state->object, {}); + } +}; /// Deleter that deletes the pointer outside the GIL. /// @@ -218,23 +257,6 @@ struct DeleteOutsideGIL } }; -/// Deleter that delays the destruction of an object to another thread. -struct DeleteInOtherThread -{ - template - void operator()(T* ptr) const - { - GILRelease unlock; - // `std::async` returns an object, that unless moved from will block when - // destroyed until the task is complete, which is unwanted behavior here. - // Therefore we just take the result of the `std::async` call into a local - // variable and then ignore it. - auto fut = std::async(std::launch::async, [](std::unique_ptr) {}, - std::unique_ptr(ptr)); - QI_IGNORE_UNUSED(fut); - } -}; - } // namespace py } // namespace qi diff --git a/src/pyapplication.cpp b/src/pyapplication.cpp index 315d842..e9a0ed0 100644 --- a/src/pyapplication.cpp +++ b/src/pyapplication.cpp @@ -83,7 +83,7 @@ void exportApplication(::py::module& m) GILAcquire lock; - class_>( + class_( m, "Application") .def(init(withArgcArgv<>([](int& argc, char**& argv) { GILRelease unlock; @@ -93,8 +93,7 @@ void exportApplication(::py::module& m) .def_static("run", &Application::run, call_guard()) .def_static("stop", &Application::stop, call_guard()); - class_>( + class_( m, "ApplicationSession") .def(init(withArgcArgv( diff --git a/src/pyasync.cpp b/src/pyasync.cpp index 01b6310..f39ca2e 100644 --- a/src/pyasync.cpp +++ b/src/pyasync.cpp @@ -38,19 +38,15 @@ ::py::object async(::py::function pyCallback, const MicroSeconds delay(usDelay); - GILGuardedObject guardCb(pyCallback); - GILGuardedObject guardArgs(std::move(args)); - GILGuardedObject guardKwargs(std::move(kwargs)); - + SharedObject sharedCb(pyCallback); + SharedObject sharedArgs(std::move(args)); + SharedObject sharedKwArgs(std::move(kwargs)); auto invokeCallback = [=]() mutable { - GILAcquire lock; - auto res = ::py::object((*guardCb)(**guardArgs, ***guardKwargs)).cast(); - // Release references immediately while we hold the GIL, instead of waiting - // for the lambda destructor to relock the GIL. - *guardKwargs = {}; - *guardArgs = {}; - *guardCb = {}; - return res; + GILAcquire acquire; + return invokeCatchPythonError( + sharedCb.takeInner(), + *sharedArgs.takeInner(), + **sharedKwArgs.takeInner()).cast(); }; // If there is a strand attached to that callable, we use it but we cannot use diff --git a/src/pyfuture.cpp b/src/pyfuture.cpp index 9c473c9..dc4c6fb 100644 --- a/src/pyfuture.cpp +++ b/src/pyfuture.cpp @@ -53,8 +53,8 @@ template std::function(Args...)> toContinuation(const ::py::function& cb) { GILAcquire lock; - GILGuardedObject guardedCb(cb); - auto callGuardedCb = [=](Args... args) mutable { + SharedObject sharedCb(cb); + auto callSharedCb = [=](Args... args) mutable { GILAcquire lock; const auto handleExcept = ka::handle_exception_rethrow( exceptionLogVerbose( @@ -62,17 +62,18 @@ std::function(Args...)> toContinuation(const ::py::function& cb) "An exception occurred while executing a future continuation"), ka::type_t<::py::object>{}); const auto pyRes = - ka::invoke_catch(handleExcept, *guardedCb, std::forward(args)...); - // Release references immediately while we hold the GIL, instead of waiting - // for the lambda destructor to relock the GIL. - *guardedCb = {}; + ka::invoke_catch(handleExcept, [&] { + return invokeCatchPythonError( + sharedCb.takeInner(), + std::forward(args)...); + }); return castIfNotVoid(pyRes); }; auto strand = strandOfFunction(cb); if (strand) - return strand->schedulerFor(std::move(callGuardedCb)); - return futurizeOutput(std::move(callGuardedCb)); + return strand->schedulerFor(std::move(callSharedCb)); + return futurizeOutput(std::move(callSharedCb)); } void addCallback(Future fut, const ::py::function& cb) diff --git a/src/pyobject.cpp b/src/pyobject.cpp index 4038ed3..9bc1019 100644 --- a/src/pyobject.cpp +++ b/src/pyobject.cpp @@ -149,7 +149,7 @@ using GenericFunction = std::function<::py::object(::py::args)>; // @pre `cargs.size() > 0`, arguments must at least contain a `DynamicObject` // on which the function is to be called. AnyReference callPythonMethod(const AnyReferenceVector& cargs, - const GILGuardedObject& method) + const SharedObject<::py::function>& method) { auto it = cargs.begin(); const auto cargsEnd = cargs.end(); @@ -170,7 +170,7 @@ AnyReference callPythonMethod(const AnyReferenceVector& cargs, // Convert Python future object into a C++ Future, to allow libqi to unwrap // it. - const ::py::object ret = (*method)(*args); + const ::py::object ret = invokeCatchPythonError(method.inner(), *args); if (::py::isinstance(ret)) return AnyValue::from(ret.cast()).release(); return AnyReference::from(ret).content().clone(); @@ -259,7 +259,7 @@ boost::optional registerMethod(DynamicObjectBuilder& gob, return gob.xAdvertiseMethod(mmb, AnyFunction::fromDynamicFunction( boost::bind(callPythonMethod, _1, - GILGuardedObject(method)))); + SharedObject(method)))); } } // namespace @@ -393,11 +393,7 @@ Object toObject(const ::py::object& obj) // This is a useless callback, needed to keep a reference on the python object. // When the GenericObject is destroyed, the reference is released. - GILGuardedObject guardedObj(obj); - Object anyobj = gob.object([guardedObj](GenericObject*) mutable { - GILAcquire lock; - *guardedObj = {}; - }); + Object anyobj = gob.object([sharedObj = SharedObject(obj)](GenericObject*) {}); // If there was no ObjectUid stored in the python object, store a new one. if (!maybeObjectUid) diff --git a/src/pysignal.cpp b/src/pysignal.cpp index aa3a864..b6c3e9d 100644 --- a/src/pysignal.cpp +++ b/src/pysignal.cpp @@ -26,7 +26,7 @@ namespace constexpr static const auto asyncArgName = "_async"; -AnyReference dynamicCallFunction(const GILGuardedObject& func, +AnyReference dynamicCallFunction(const SharedObject<::py::function>& func, const AnyReferenceVector& args) { GILAcquire lock; @@ -34,7 +34,7 @@ AnyReference dynamicCallFunction(const GILGuardedObject& func, ::py::size_t i = 0; for (const auto& arg : args) pyArgs[i++] = castToPyObject(arg); - (*func)(*pyArgs); + invokeCatchPythonError(func.inner(), *pyArgs); return AnyValue::makeVoid().release(); } @@ -49,7 +49,7 @@ ::py::object connect(F&& connect, SignalSubscriber subscriber(AnyFunction::fromDynamicFunction( boost::bind(dynamicCallFunction, - GILGuardedObject(pyCallback), + SharedObject(pyCallback), _1)), strand.get()); subscriber.setCallType(MetaCallType_Auto); diff --git a/src/pytypes.cpp b/src/pytypes.cpp index 088ba13..fa03102 100644 --- a/src/pytypes.cpp +++ b/src/pytypes.cpp @@ -28,16 +28,6 @@ namespace py namespace { -struct ObjectDecRef -{ - ::py::handle obj; - void operator()() const - { - GILAcquire lock; - obj.dec_ref(); - } -}; - template boost::optional<::py::object> tryToCastObjectTo(ObjectTypeInterface* type, void* ptr) diff --git a/tests/test_guard.cpp b/tests/test_guard.cpp index bc16e48..d9d835c 100644 --- a/tests/test_guard.cpp +++ b/tests/test_guard.cpp @@ -39,70 +39,6 @@ TEST_F(InvokeGuardedTest, ExecutesFunctionWithGuard) EXPECT_EQ(52, r); } -using GuardedTest = GuardTest; - -TEST_F(GuardedTest, GuardsConstruction) -{ - using T = GuardedTest; - - struct Check - { - int i; - Check(int i) : i(i) { EXPECT_TRUE(T::guarded); } - explicit operator bool() const { return true; } - }; - - EXPECT_FALSE(T::guarded); - qi::py::detail::Guarded g(42); - EXPECT_FALSE(T::guarded); - EXPECT_EQ(42, g->i); -} - -TEST_F(GuardedTest, GuardsCopyConstruction) -{ - using T = GuardedTest; - - struct Check - { - int i = 0; - Check(int i) : i(i) {} - Check(const Check& o) - : i(o.i) - { EXPECT_TRUE(T::guarded); } - explicit operator bool() const { return true; } - }; - - EXPECT_FALSE(T::guarded); - qi::py::detail::Guarded a(42); - qi::py::detail::Guarded b(a); - EXPECT_FALSE(T::guarded); - EXPECT_EQ(42, a->i); - EXPECT_EQ(42, b->i); -} - -TEST_F(GuardedTest, GuardsCopyAssignment) -{ - using T = GuardedTest; - - struct Check - { - int i = 0; - Check(int i) : i(i) {} - Check& operator=(const Check& o) - { EXPECT_TRUE(T::guarded); i = o.i; return *this; } - explicit operator bool() const { return true; } - }; - - EXPECT_FALSE(T::guarded); - qi::py::detail::Guarded a(42); - qi::py::detail::Guarded b(37); - EXPECT_FALSE(T::guarded); - b = a; - EXPECT_FALSE(T::guarded); - EXPECT_EQ(42, a->i); - EXPECT_EQ(42, b->i); -} - TEST(GILAcquire, IsReentrant) { qi::py::GILAcquire acq0; QI_IGNORE_UNUSED(acq0); @@ -120,3 +56,57 @@ TEST(GILRelease, IsReentrant) qi::py::GILRelease rel2; QI_IGNORE_UNUSED(rel2); SUCCEED(); } + +struct SharedObject : testing::Test +{ + SharedObject() + { + // GIL is only required for creation of the inner object. + object = [&]{ + qi::py::GILAcquire lock; + return pybind11::capsule( + &this->destroyed, + [](void* destroyed){ + *reinterpret_cast(destroyed) = true; + } + ); + }(); + } + + ~SharedObject() + { + if (object) { + qi::py::GILAcquire lock; + object = {}; + } + } + + bool destroyed = false; + pybind11::capsule object; +}; + +TEST_F(SharedObject, KeepsRefCount) +{ + std::optional sharedObject = qi::py::SharedObject(std::move(object)); + ASSERT_FALSE(object); // object has been released. + EXPECT_FALSE(destroyed); // inner object is still alive. + { + auto sharedObjectCpy = *sharedObject; // copy the shared object locally. + EXPECT_FALSE(destroyed); // inner object is maintained by both copies. + sharedObject.reset(); + EXPECT_FALSE(destroyed); // inner object is maintained by the copy. + } + EXPECT_TRUE(destroyed); // inner object has been destroyed. +} + +TEST_F(SharedObject, TakeInnerStealsInnerRefCount) +{ + std::optional sharedObject = qi::py::SharedObject(std::move(object)); + auto inner = sharedObject->takeInner(); + EXPECT_FALSE(sharedObject->inner()); // inner object is null. + sharedObject.reset(); + EXPECT_FALSE(destroyed); // inner object is still alive. + qi::py::GILAcquire lock; // release local inner object, which requires the GIL. + inner = {}; + EXPECT_TRUE(destroyed); // inner object has been destroyed. +} diff --git a/tests/test_qipython_local_interpreter.cpp b/tests/test_qipython_local_interpreter.cpp index c1113f7..581255a 100644 --- a/tests/test_qipython_local_interpreter.cpp +++ b/tests/test_qipython_local_interpreter.cpp @@ -26,13 +26,80 @@ PYBIND11_EMBEDDED_MODULE(test_local_interpreter, m) { })); } -TEST(InterpreterFinalize, DoesNotSegfaultGarbageObjectDtorOutsideGIL) +TEST(InterpreterFinalize, GarbageObjectDtorOutsideGIL) { pybind11::scoped_interpreter interp; pybind11::globals()["qitli"] = pybind11::module::import("test_local_interpreter"); pybind11::exec("obj = qitli.ObjectDtorOutsideGIL()"); } +// This test checks that concurrent uses of GIL guards during finalization +// of the interpreter does not cause crashes. +// +// It uses 2 threads: a main thread, and a second native thread. +// +// The main thread starts an interpreter, starts the second thread and +// releases the GIL. The second thread acquires the GIL, then releases +// it, and waits for the interpreter to finalize. Once it is finalized, +// it tries to reacquire the GIL, and then release it, according to +// GIL guards destructors. +// +// Horizontal lines are synchronization points between the threads. +// +// main thread (T1) +// ~~~~~~~~~~~~~~~~ +// ▼ +// ╔═══════╪════════╗ +// ║ interpreter ║ +// ----------------------------------------------> start native thread T2 +// ║ ╎ ║ native thread (T2) +// ║ ╎ ║ ~~~~~~~~~~~~~~~~~~ +// ║ ╎ ║ ▼ +// ║ ╔═════╪══════╗ ║ ╎ +// ║ ║ GILRelease ║ ║ ╎ -> T1 releases the GIL +// ----------------------------------------------> GIL shift T1 -> T2 +// ║ ║ ╎ ║ ║ ╔═══════╪════════╗ +// ║ ║ ╎ ║ ║ ║ GILAcquire ║ -> T2 acquires the GIL +// ║ ║ ╎ ║ ║ ║ ╔═════╪══════╗ ║ +// ║ ║ ╎ ║ ║ ║ ║ GILRelease ║ ║ -> T2 releases the GIL +// ----------------------------------------------> GIL shift T2 -> T1 +// ║ ╚═════╪══════╝ ║ ║ ║ ╎ ║ ║ -> T1 acquires the GIL +// ╚═══════╪════════╝ ║ ║ ╎ ║ ║ -> interpreter starts finalizing +// ----------------------------------------------> interpreter finalized +// ╎ ║ ╚═════╪══════╝ ║ -> T2 tries to reacquire GIL but fails, it's a noop. +// ╎ ╚═══════╪════════╝ -> T2 tries to release GIL, but it's a noop. +// ╎ ╎ +TEST(InterpreterFinalize, GILReleasedInOtherThread) +{ + // Synchronization mechanism for the first GIL shift, otherwise T1 will release and reacquire the + // GIL instantly without waiting for T2. + qi::Promise shift1Prom; + auto shift1Fut = shift1Prom.future(); + // Second GIL shift does not require an additional synchronization + // mechanism. Waiting for interpreter finalization ensures that the + // GIL was acquired back by thread T1. + // Synchronization mechanism for interpreter finalization. + qi::Promise finalizedProm; + std::future asyncFut; + { + pybind11::scoped_interpreter interp; + asyncFut = std::async( + std::launch::async, + [finalizedFut = finalizedProm.future(), shift1Prom]() mutable { + qi::py::GILAcquire acquire; + // First GIL shift is done. + shift1Prom.setValue(nullptr); + qi::py::GILRelease release; + finalizedFut.value(); + }); + qi::py::GILRelease release; + shift1Fut.value(); + } + finalizedProm.setValue(nullptr); + // Join the thread, wait for the task to finish and ensure no exception was thrown. + asyncFut.get(); +} + int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv); From a174e5e1488960c7a868fbc78bf68d4a32db400b Mon Sep 17 00:00:00 2001 From: Vincent Palancher Date: Tue, 7 Nov 2023 18:17:31 +0100 Subject: [PATCH 06/10] conan: Uses gtest v1.14. The "cci.20210126" version of the "gtest" package is unmaintained on Conan center. This patch changes the dependency to the latest available package version. --- conanfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conanfile.py b/conanfile.py index 9d5e3a5..24b7c68 100644 --- a/conanfile.py +++ b/conanfile.py @@ -59,7 +59,7 @@ class QiPythonConan(ConanFile): ] test_requires = [ - "gtest/cci.20210126", + "gtest/[~1.14]", ] generators = "CMakeToolchain", "CMakeDeps" From 05a04e854dda53bd83288816365b2ecce78a6805 Mon Sep 17 00:00:00 2001 From: Vincent Palancher Date: Tue, 7 Nov 2023 18:19:37 +0100 Subject: [PATCH 07/10] Adds slight improvements to build, conan and CI scripts. --- ...ll.sh => cibuildwheel_linux_before_all.sh} | 17 ++++--------- ci/conan/global.conf | 5 ++++ conanfile.py | 25 ++++++++++--------- pyproject.toml | 4 +-- 4 files changed, 24 insertions(+), 27 deletions(-) rename ci/{cibuildwheel_before_all.sh => cibuildwheel_linux_before_all.sh} (57%) diff --git a/ci/cibuildwheel_before_all.sh b/ci/cibuildwheel_linux_before_all.sh similarity index 57% rename from ci/cibuildwheel_before_all.sh rename to ci/cibuildwheel_linux_before_all.sh index 72ea5e3..a20dcb0 100755 --- a/ci/cibuildwheel_before_all.sh +++ b/ci/cibuildwheel_linux_before_all.sh @@ -12,26 +12,19 @@ yum install -y perl-IPC-Cmd perl-Digest-SHA conan config install "$PACKAGE/ci/conan" # Clone and export libqi to Conan cache. -# Possible improvement: -# Avoid duplicating the version number here with the version -# defined in conanfile.py. +QI_VERSION=$(sed -nE '/^\s*requires\s*=/,/^\s*]/{ s/\s*"qi\/([^"]+)"/\1/p }' "$PACKAGE/conanfile.py") + GIT_SSL_NO_VERIFY=true \ git clone --depth=1 \ - --branch qi-framework-v4.0.1 \ + --branch "qi-framework-v${QI_VERSION}" \ "$LIBQI_REPOSITORY_URL" \ /work/libqi -conan export /work/libqi --version=4.0.1 +conan export /work/libqi --version="${QI_VERSION}" # Install dependencies of libqi-python from Conan, including libqi. -# Only use the build_type as a variable for the build folder name, so -# that the generated CMake preset is named "conan-release". # # Build everything from sources, so that we do not reuse precompiled binaries. # This is because the GLIBC from the manylinux images are often older than the # ones that were used to build the precompiled binaries, which means the binaries # cannot by executed. -conan install "$PACKAGE" \ - --build="*" \ - -c tools.build:skip_test=true \ - -c tools.cmake.cmaketoolchain:generator=Ninja \ - -c tools.cmake.cmake_layout:build_folder_vars=[\"settings.build_type\"] +conan install "$PACKAGE" --build="*" diff --git a/ci/conan/global.conf b/ci/conan/global.conf index bbcc54a..7ad81c5 100644 --- a/ci/conan/global.conf +++ b/ci/conan/global.conf @@ -1,2 +1,7 @@ core:default_profile=default core:default_build_profile=default +tools.build:skip_test=true +tools.cmake.cmaketoolchain:generator=Ninja +# Only use the build_type as a variable for the build folder name, so +# that the generated CMake preset is named "conan-release". +tools.cmake.cmake_layout:build_folder_vars=["settings.build_type"] diff --git a/conanfile.py b/conanfile.py index 24b7c68..bc74ea4 100644 --- a/conanfile.py +++ b/conanfile.py @@ -36,21 +36,22 @@ ] USED_BOOST_COMPONENTS = [ - "atomic", # required by thread - "chrono", # required by thread - "container", # required by thread - "date_time", # required by thread - "exception", # required by thread - "filesystem", # required by libqi - "locale", # required by libqi - "program_options", # required by libqi - "random", # required by libqi - "regex", # required by libqi - "system", # required by thread "thread", + # required by libqi + "filesystem", + "locale", + "program_options", + "random", + "regex", + # required by boost.thread + "atomic", + "chrono", + "container", + "date_time", + "exception", + "system", ] - class QiPythonConan(ConanFile): requires = [ "boost/[~1.78]", diff --git a/pyproject.toml b/pyproject.toml index 2c98a36..3ebf667 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -59,10 +59,8 @@ build-frontend = "build" environment-pass = ["LIBQI_REPOSITORY_URL"] [tool.cibuildwheel.linux] -# Build using the manylinux2014 image manylinux-x86_64-image = "manylinux2014" - -before-all = ["ci/cibuildwheel_before_all.sh {package}"] +before-all = ["ci/cibuildwheel_linux_before_all.sh {package}"] [tool.cibuildwheel.linux.config-settings] "cmake.args" = ["--preset=conan-release"] From 159d04bbb1cf171b8a9554013cffaf7dd5798a31 Mon Sep 17 00:00:00 2001 From: Vincent PALANCHER Date: Mon, 13 Nov 2023 10:55:51 +0000 Subject: [PATCH 08/10] Improve build procedure and document steps in README. This change fixes minor issues with the build system and the CI, and better document build steps and wheel construction in the README. --- CMakeLists.txt | 10 ++- README.rst | 97 ++++++++++++++++++++++++----- ci/cibuildwheel_linux_before_all.sh | 2 +- pyproject.toml | 4 ++ 4 files changed, 94 insertions(+), 19 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ddba9e4..db1acd8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -346,7 +346,10 @@ if(BUILD_TESTING) pybind11::pybind11 GTest::gmock ) - gtest_discover_tests(test_qipython) + gtest_discover_tests( + test_qipython + DISCOVERY_MODE PRE_TEST + ) add_executable(test_qipython_local_interpreter) target_sources( @@ -363,7 +366,10 @@ if(BUILD_TESTING) cxx_standard GTest::gmock ) - gtest_discover_tests(test_qipython_local_interpreter) + gtest_discover_tests( + test_qipython_local_interpreter + DISCOVERY_MODE PRE_TEST + ) endif() if(NOT Python_Interpreter_FOUND) diff --git a/README.rst b/README.rst index 2383130..8d35691 100644 --- a/README.rst +++ b/README.rst @@ -10,9 +10,30 @@ __ LibQi_repo_ Building ======== -The libqi-python project requires a compiler that supports C++17 to build. +To build the project, you need: -It is built with CMake >= 3.23. +- a compiler that supports C++17. + + - on Ubuntu: `apt-get install build-essential`. + +- CMake with at least version 3.23. + + - on PyPI (**recommended**): `pip install "cmake>=3.23"`. + - on Ubuntu: `apt-get install cmake`. + +- Python with at least version 3.7 and its development libraries. + + - On Ubuntu: `apt-get install libpython3-dev`. + +- a Python `virtualenv`. + + - On Ubuntu: + + .. code-block:: console + + apt-get install python3-venv + python3 -m venv ~/my-venv # Use the path of your convenience. + source ~/my-venv/bin/activate .. note:: The CMake project offers several configuration options and exports a set @@ -21,13 +42,13 @@ It is built with CMake >= 3.23. .. note:: The procedures described below assume that you have downloaded the project - sources and that your current working directory is the project sources root - directory. + sources, that your current working directory is the project sources root + directory and that you are working inside your virtualenv. Conan ^^^^^ -Additionally, libqi-python is available as a Conan 2 project, which means you +Additionally, `libqi-python` is available as a Conan 2 project, which means you can use Conan to fetch dependencies. You can install and/or upgrade Conan 2 and create a default profile in the @@ -46,11 +67,35 @@ Install dependencies from Conan and build with CMake The procedure to build the project using Conan to fetch dependencies is the following. -You must first install the project dependencies in Conan. +Most dependencies are available on Conan Center repository and should not +require any additional steps to make them available. However, you might need to +first get and export the `libqi` recipe into your local Conan cache. .. code-block:: console - conan install . --build=missing -s build_type=Debug + # GitHub is available, but you can also use internal GitLab. + QI_REPOSITORY="https://github.com/aldebaran/libqi.git" + QI_VERSION="4.0.1" # Checkout the version your project need. + QI_PATH="$HOME/libqi" # Or whatever path you want. + git clone \ + --depth=1 `# Only fetch one commit.` \ + --branch "qi-framework-v${QI_VERSION}" \ + "${QI_REPOSITORY}" \ + "${QI_PATH}" + conan export "${QI_PATH}" \ + --version "${QI_VERSION}" # Technically not required but some + # versions of libqi require it + # because of a bug. + +You can then install the `libqi-python` dependencies in Conan. + +.. code-block:: console + + conan install . \ + --build=missing `# Build dependencies binaries that are missing in Conan.` \ + -s build_type=Debug `# Build in debug mode.` \ + -c tools.build:skip_test=true `# Skip tests building for dependencies.` \ + -c '&:tools.build:skip_test=false' # Do not skip tests for the project. This will generate a build directory containing a configuration with a toolchain file that allows CMake to find dependencies inside the Conan cache. @@ -73,11 +118,14 @@ To start building, you need to configure with CMake and then build: cmake --preset conan-linux-x86_64-gcc-debug cmake --build --preset conan-linux-x86_64-gcc-debug -You can then invoke tests using CTest_: +Tests can now be invoked using CTest_, but they require a runtime environment +from Conan so that all dependencies are found: .. code-block:: console + source build/linux-x86_64-gcc-debug/generators/conanrun.sh ctest --preset conan-linux-x86_64-gcc-debug --output-on-failure + source build/linux-x86_64-gcc-debug/generators/deactivate_conanrun.sh Finally, you can install the project in the directory of your choice. @@ -85,8 +133,9 @@ The project defines a single install component, the ``Module`` component. .. code-block:: console - # "cmake --install" does not support preset sadly. - cmake --install build/linux-x86_64-gcc-debug + # `cmake --install` does not support presets sadly. + cmake \ + --install build/linux-x86_64-gcc-debug \ --component Module --prefix ~/my-libqi-python-install Wheel (PEP 517) @@ -101,21 +150,38 @@ dependencies, such as a toolchain generated by Conan: .. code-block:: console - conan install . --build=missing + conan install . \ + --build=missing `# Build dependencies binaries that are missing in Conan.` \ + -c tools.build:skip_test=true # Skip any test. You now can use the ``build`` Python module to build the wheel using PEP 517. .. code-block:: console - export CMAKE_TOOLCHAIN_FILE=$PWD/build/linux-x86_64-gcc-release/generators/conan_toolchain.cmake - python -m build + pip install -U build + python -m build \ + --config-setting cmake.define.CMAKE_TOOLCHAIN_FILE=$PWD/build/linux-x86_64-gcc-release/generators/conan_toolchain.cmake When built that way, the native libraries present in the wheel are most likely incomplete. You will need to use ``auditwheel`` or ``delocate`` to fix it. +.. note:: + `auditwheel` requires the `patchelf` utility program on Linux. You may need + to install it (on Ubuntu: `apt-get install patchelf`). + .. code-block:: console - auditwheel repair --plat manylinux_2_31_x86_64 dist/qi-*.whl + pip install -U auditwheel # or `delocate` on MacOS. + auditwheel repair \ + --strip `# Strip debugging symbols to get a lighter archive.` \ + `# The desired platform, which may differ depending on your build host.` \ + `# With Ubuntu 20.04, we can target manylinux_2_31. Newer versions of` \ + `# Ubuntu will have to target newer versions of manylinux.` \ + `# If you don't need a manylinux archive, you can also target the` \ + `# 'linux_x86_64' platform.` \ + --plat manylinux_2_31_x86_64 \ + `# Path to the wheel archive.` \ + dist/qi-*.whl # The wheel will be by default placed in a `./wheelhouse/` directory. Crosscompiling @@ -123,12 +189,11 @@ Crosscompiling The project supports cross-compiling as explained in the `CMake manual about toolchains`__. You may simply set the ``CMAKE_TOOLCHAIN_FILE`` variable to the -path of the CMake file in your toolchain. +path of the CMake file of your toolchain. __ CMake_toolchains_ .. _LibQi_repo: https://github.com/aldebaran/libqi .. _scikit-build: https://scikit-build.readthedocs.io/en/latest/ -.. _setuptools: https://setuptools.readthedocs.io/en/latest/setuptools.html .. _CMake_toolchains: https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html .. _CTest: https://cmake.org/cmake/help/latest/manual/ctest.1.html diff --git a/ci/cibuildwheel_linux_before_all.sh b/ci/cibuildwheel_linux_before_all.sh index a20dcb0..f033f37 100755 --- a/ci/cibuildwheel_linux_before_all.sh +++ b/ci/cibuildwheel_linux_before_all.sh @@ -12,7 +12,7 @@ yum install -y perl-IPC-Cmd perl-Digest-SHA conan config install "$PACKAGE/ci/conan" # Clone and export libqi to Conan cache. -QI_VERSION=$(sed -nE '/^\s*requires\s*=/,/^\s*]/{ s/\s*"qi\/([^"]+)"/\1/p }' "$PACKAGE/conanfile.py") +QI_VERSION=$(sed -nE '/^\s*requires\s*=/,/^\s*]/{ s/\s*"qi\/([^"]+)".*/\1/p }' "$PACKAGE/conanfile.py") GIT_SSL_NO_VERIFY=true \ git clone --depth=1 \ diff --git a/pyproject.toml b/pyproject.toml index 3ebf667..e6957a5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -52,6 +52,10 @@ repository = "https://github.com/aldebaran/libqi-python" # Rely only on CMake install, not on Python packages detection. wheel.packages = [] +[tool.scikit-build.cmake.define] +# Disable building tests by default when building a wheel. +BUILD_TESTING = "OFF" + [tool.cibuildwheel] build = "cp*manylinux*x86_64" build-frontend = "build" From 38de753f257a3c7bbe0642c67f2b8fb8d75a60fb Mon Sep 17 00:00:00 2001 From: Vincent Palancher Date: Mon, 4 Dec 2023 11:53:45 +0100 Subject: [PATCH 09/10] conan: Update libqi to v4.0.2. --- conanfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conanfile.py b/conanfile.py index bc74ea4..9e16c4c 100644 --- a/conanfile.py +++ b/conanfile.py @@ -56,7 +56,7 @@ class QiPythonConan(ConanFile): requires = [ "boost/[~1.78]", "pybind11/[^2.9]", - "qi/4.0.1", + "qi/4.0.2", ] test_requires = [ From cd66e1592a3b2a042bc2bacd286d8b5b5ddc9980 Mon Sep 17 00:00:00 2001 From: Vincent Palancher Date: Mon, 4 Dec 2023 12:05:28 +0100 Subject: [PATCH 10/10] Bump version to v3.1.4. --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index e6957a5..794715e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,7 +7,7 @@ build-backend = "scikit_build_core.build" [project] name = "qi" description = "LibQi Python bindings" -version = "3.1.4.dev0" +version = "3.1.4" readme = "README.rst" requires-python = ">=3.7" license = { "file" = "COPYING" }