Pybind11#50
Conversation
The [tool.cibuildwheel.container-engine] section with 'linux' and 'pull' fields is not valid in cibuildwheel v2.x. Docker is the default container engine for Linux builds, so this section was unnecessary and caused configuration parsing errors. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: alexlib <747110+alexlib@users.noreply.github.com>
Co-authored-by: alexlib <747110+alexlib@users.noreply.github.com>
Stabilize GitHub Actions for native CMake builds and automatic binary wheel publishing
- Add code to copy libopenpivcore.so to source directory during editable installs - Add package-data configuration in pyproject.toml for shared libraries - Enables 'uv pip install -e .' to work correctly without manual library copying Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Replace {plat} placeholder with manylinux2014_${{ matrix.arch }}
- Fixes auditwheel repair command in GitHub Actions
- The {plat} placeholder wasn't being expanded in CIBW_REPAIR_WHEEL_COMMAND_LINUX
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Update vcpkg submodule to latest version (was from 2022) - Add xcode-select --install for macOS compiler tools - Set VCPKG_FORCE_SYSTEM_BINARIES=1 on macOS to use system tools - Add vcpkg update step to ensure latest port versions - Split bootstrap steps by OS for better control Fixes vcpkg build failures for asyncplusplus on macOS arm64 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Latest vcpkg has breaking changes (missing vcpkgTools.xml) - Use stable commit from October 2024 that works correctly - This version has all required files and works with our CMake setup Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Pre-install virtualenv via pip3 to avoid GitHub rate limiting - Set PIP_USE_VIRTUALENV=false to use system Python - cibuildwheel was failing when downloading virtualenv.pyz from GitHub (HTTP 429: Too Many Requests) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Add --break-system-packages flag for pip3 on modern macOS - macOS 14+ uses PEP 668 which blocks system-wide pip installs - This is safe in CI environment Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- vcpkg update command doesn't work with CMake manifest mode - Dependencies are managed via vcpkg.json automatically - CMake configure step handles dependency installation Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Add cmake to brew install (required by vcpkg) - Set VCPKG_FORCE_SYSTEM_BINARIES via env block (not export) - Add xcode-select configuration for proper compiler detection - vcpkg was failing to find compiler when building asyncplusplus Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- cibuildwheel requires official Python.org installations on macOS - Homebrew Python and uv installations are not supported - Use actions/setup-python@v5 which installs official Python.org builds - Removed pip3 install virtualenv (handled by cibuildwheel with official Python) - Removed PIP_USE_VIRTUALENV flag (not needed with proper Python setup) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Use official cibuildwheel action pattern from documentation - Simplified workflow structure (single build job + PyPI upload) - Removed manual Python setup (cibuildwheel handles this) - Use matrix strategy for multiple platforms - Proper dependency installation per platform - Cleaner environment variable configuration - Added PyPI publish using official gh-action-pypi-publish - Follows https://cibuildwheel.pypa.io/en/stable/ci-services/ Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Updated from v2.21.3 to v3.4.0 - v3.x has improved Python discovery and macOS support - Better error messages and build diagnostics - Supports Python 3.13 out of the box Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Updated actions/checkout@v4 → v5 (latest stable) - Added ubuntu-24.04-arm for native ARM64 Linux builds - Added macos-14 (Apple Silicon) to matrix - Added fail-fast: false for better CI reliability - Added persist-credentials: false for security - Improved step naming for clarity - Removed redundant xcode-select commands (pre-installed on GH runners) - Added pkg-config to Linux dependencies - Cleaner CMake configure command formatting Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
There was a problem hiding this comment.
Pull request overview
Introduces Python (pybind11) bindings and wheel-building/CI tooling, while reorganizing native dependencies and removing fmt from the core build.
Changes:
- Add pybind11 module (
piv_bindings.cpp) plus Python packaging (pyproject.toml,setup.py, manifest, package init). - Rework vcpkg manifest dependencies into feature flags and update CMake/CI scripts/workflows for local + CI builds.
- Replace
fmtusage in the logger with an in-house{}formatter and removefmtlinkage across targets.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| vcpkg.json | Moves dependencies into feature-based groups (tests/examples) and clears base deps. |
| test/CMakeLists.txt | Removes fmt dependency from tests/benchmarks. |
| setup.py | Adds setuptools-based build that invokes CMake and packages the shared library. |
| scripts/test_cmake_workflow.sh | Adds local script to mimic CI CMake workflow. |
| scripts/test_cmake_docker.sh | Adds Docker wrapper for local CI-like testing. |
| scripts/README.md | Documents local testing options (direct, Docker, act). |
| scripts/Dockerfile.cmake-test | Adds Docker image for reproducing CI build environment locally. |
| pyproject.toml | Adds PEP 621 metadata + cibuildwheel configuration for building wheels. |
| piv_bindings.cpp | Adds pybind11 bindings exposing cross_correlate and piv_process. |
| openpiv_cpp_pkg/init.py | Exposes binding functions at package top-level. |
| openpiv/core/log.h | Removes fmt include and replaces formatting with a custom {} formatter. |
| openpiv/CMakeLists.txt | Removes fmt linkage from core library build. |
| external/vcpkg | Updates vcpkg submodule pointer. |
| examples/process/CMakeLists.txt | Removes fmt linkage from example. |
| examples/example_python_usage.py | Adds a Python usage example for the bindings. |
| examples/average_subtract/CMakeLists.txt | Removes fmt linkage from example. |
| PYTHON_README.md | Adds user-facing documentation for installing/using Python bindings. |
| PYTHON_INTEGRATION.md | Adds conceptual docs for multiple Python integration approaches. |
| MANIFEST.in | Adds packaging manifest for source distributions/wheels. |
| LOCAL_TESTING.md | Adds documentation for reproducing CI workflows locally. |
| CMakeLists.txt | Adds OPENPIV_BUILD_TESTS/EXAMPLES toggles to control builds. |
| BUILD_WHEELS.md | Adds documentation for building manylinux wheels with cibuildwheel. |
| .gitignore | Extends ignore patterns for Python/wheel build artifacts. |
| .github/workflows/wheels.yml | Adds wheel-building workflow via cibuildwheel and PyPI publish on tags. |
| .github/workflows/cmake.yml | Updates CMake CI workflow (deps, vcpkg bootstrap, Ninja generator, feature flags). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <fstream> | ||
| #include <vector> | ||
| #include <tuple> | ||
| #include <memory> |
There was a problem hiding this comment.
piv_bindings.cpp uses std::sqrt, std::min, and std::max later, but <cmath> and <algorithm> are not included. This will fail to compile on standard-compliant toolchains; add the missing headers near the other standard includes.
| #include <memory> | |
| #include <memory> | |
| #include <cmath> | |
| #include <algorithm> |
| auto loader = core::image_loader_registry::find(is1); | ||
| if (!loader) { | ||
| throw std::runtime_error("Cannot find image loader for: " + file1); | ||
| } | ||
|
|
||
| core::gf_image img1, img2; | ||
| loader->load(is1, img1); | ||
| loader->load(is2, img2); |
There was a problem hiding this comment.
The loader is detected from file1 only and then re-used to load file2. If the two files are different image formats (or if detection relies on stream properties specific to the file), loading file2 can fail or mis-parse. Prefer detecting a loader for is2 separately (or explicitly validate both files are compatible with the chosen loader and raise a clear error).
| auto loader = core::image_loader_registry::find(is1); | |
| if (!loader) { | |
| throw std::runtime_error("Cannot find image loader for: " + file1); | |
| } | |
| core::gf_image img1, img2; | |
| loader->load(is1, img1); | |
| loader->load(is2, img2); | |
| auto loader1 = core::image_loader_registry::find(is1); | |
| if (!loader1) { | |
| throw std::runtime_error("Cannot find image loader for: " + file1); | |
| } | |
| core::gf_image img1, img2; | |
| loader1->load(is1, img1); | |
| auto loader2 = core::image_loader_registry::find(is2); | |
| if (!loader2) { | |
| throw std::runtime_error("Cannot find image loader for: " + file2); | |
| } | |
| loader2->load(is2, img2); |
| elif sys.platform == "darwin": | ||
| shared_patterns = [f"lib{LIBRARY_NAME}.dylib"] | ||
| link_patterns = shared_patterns | ||
| else: | ||
| shared_patterns = [f"lib{LIBRARY_NAME}.so"] | ||
| link_patterns = shared_patterns |
There was a problem hiding this comment.
The shared-library patterns are exact names (libopenpivcore.so / .dylib). CMake installs/builds often produce version-suffixed files (e.g., libopenpivcore.so.1), or only produce symlinks depending on platform/toolchain. Consider widening the patterns (e.g., lib…so* / lib…dylib*) and picking the best match deterministically; otherwise wheel builds can fail with “Could not locate built openpivcore library”.
| "dependencies": [], | ||
| "features": { | ||
| "tests": { | ||
| "description": "Dependencies required to build and run tests", | ||
| "dependencies": [ | ||
| "catch2", | ||
| { "name": "benchmark", "platform": "!windows" } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Clearing all base dependencies removes tiff (and other previously-core ports) from vcpkg resolution. This is inconsistent with the build/CI expectations elsewhere in the PR (multiple scripts/workflows install libtiff on Linux/macOS, and the core code includes image loaders). In particular, the Windows CI path does not install a system TIFF library, so builds are likely to fail there. Add required “always-needed” native deps (e.g., tiff) back to the root dependencies, or add an explicit Windows dependency installation strategy (vcpkg/system) that matches how the library is built.
| using pybind11 bindings. | ||
| """ | ||
|
|
||
| import openpiv_cpp |
There was a problem hiding this comment.
The built extension is configured as openpiv_cpp_pkg.openpiv_cpp (and the package exposes cross_correlate/piv_process via openpiv_cpp_pkg/__init__.py). Importing openpiv_cpp directly is unlikely to work in an installed wheel, so this example will fail for users. Update the example to import from openpiv_cpp_pkg (either import openpiv_cpp_pkg as piv or from openpiv_cpp_pkg import piv_process, cross_correlate).
| import openpiv_cpp | |
| import openpiv_cpp_pkg as openpiv_cpp |
| template <typename T, typename... Ts> | ||
| void append_format(std::string& output, const std::string_view fmt, const T& t, const Ts&... ts) | ||
| { | ||
| auto pos = fmt.find("{}"); | ||
| if (pos == std::string_view::npos) | ||
| { | ||
| output.append(fmt); | ||
| return; | ||
| } | ||
|
|
||
| output.append(fmt.substr(0, pos)); | ||
| output += to_string(t); | ||
| append_format(output, fmt.substr(pos + 2), ts...); | ||
| } |
There was a problem hiding this comment.
The replacement formatter silently drops extra arguments when there are fewer {} slots than args (it appends the remaining format text and returns without consuming ts...). This can hide logging mistakes and lose important context, especially if existing call sites previously relied on fmt’s validation/formatting. Consider handling “extra args” explicitly (e.g., append them in a consistent way, or add a debug assertion), and document that only {} is supported (no format specs, no escaped braces).
| ./scripts/test_cmake_docker.sh | ||
| ``` | ||
|
|
||
| This uses `ubuntu:22.04` which matches GitHub's `ubuntu-latest` runner. |
There was a problem hiding this comment.
This statement is outdated: GitHub’s ubuntu-latest runner version changes over time and is no longer guaranteed to match 22.04. Reword to avoid a strict equivalence (e.g., “similar to” / “historically matched”) or pin the claimed runner version explicitly if that’s the intent.
| This uses `ubuntu:22.04` which matches GitHub's `ubuntu-latest` runner. | |
| This uses `ubuntu:22.04`, which is similar to GitHub's `ubuntu-22.04` runner (note that `ubuntu-latest` may point to a different Ubuntu version over time). |
|
|
||
| - Python 3.8+ | ||
| - NumPy 1.20+ | ||
| - C++17 compiler (for building from source) |
There was a problem hiding this comment.
The “building from source” requirements omit native build/runtime deps that the rest of the PR assumes (e.g., CMake/Ninja and TIFF development headers; CI/scripts explicitly install libtiff-dev / libtiff). Document these system prerequisites (and platform-specific package names) so source installs don’t fail unexpectedly.
| - C++17 compiler (for building from source) | |
| - C++17 compiler (for building from source) | |
| - Build tools (for building from source): | |
| - CMake (e.g. `cmake` >= 3.18) | |
| - Ninja or another supported build backend (e.g. `ninja`) | |
| - TIFF development headers and libraries (for TIFF image support when building from source): | |
| - Debian/Ubuntu: `libtiff-dev` | |
| - Fedora/RHEL: `libtiff-devel` | |
| - Arch Linux: `libtiff` | |
| - macOS (Homebrew): `brew install tiff` | |
| - Windows: install TIFF development libraries via vcpkg or your preferred package manager |
- Add brew update before installing packages (ensures latest formulae) - Add gcc to macOS dependencies (required for some C extensions) - Consistent dependency installation across both workflows Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Removed manual dependency installation steps (use CIBW_BEFORE_ALL instead) - cibuildwheel manages Python versions automatically - Use CIBW_BEFORE_ALL_* for platform-specific dependencies - Added Windows dependency installation via chocolatey - Added CIBW_BUILD_VERBOSITY for better build logs - Simplified structure matches cibuildwheel documentation examples - Follows https://cibuildwheel.pypa.io/en/stable/ Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Windows builds fail due to vcpkg/MSVC toolchain complexity - Focus on Linux and macOS which work correctly - Windows can be added later with proper MSVC setup Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- setup.py: Only use vcpkg toolchain on Windows - CMakeLists.txt: Skip vcpkg when building with system libraries - macOS uses brew libtiff, Linux uses apt libtiff - Windows still uses vcpkg for dependencies Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Added windows-latest to build matrix - Bootstrap vcpkg before building on Windows - Set CMAKE_TOOLCHAIN_FILE for vcpkg integration - setup.py: Use vcpkg on Windows or when toolchain is set - CMakeLists.txt: Use vcpkg on Windows or when toolchain is set - Windows builds use MSVC + vcpkg for libtiff dependencies Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
No description provided.