From efddce85910503a52fb303bea97a172d6c1bfbc4 Mon Sep 17 00:00:00 2001 From: SYMSCAE Date: Tue, 3 Nov 2026 10:13:20 -0800 Subject: [PATCH 1/4] Fix lifetime management regressions in Atom bindings --- news/fix-atom-scatteringpower-lifetime.rst | 24 ++++++++++++++++++++++ src/extensions/nb_atom.cpp | 8 +++++--- 2 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 news/fix-atom-scatteringpower-lifetime.rst diff --git a/news/fix-atom-scatteringpower-lifetime.rst b/news/fix-atom-scatteringpower-lifetime.rst new file mode 100644 index 0000000..90042c5 --- /dev/null +++ b/news/fix-atom-scatteringpower-lifetime.rst @@ -0,0 +1,24 @@ +**Added:** + +* + +**Changed:** + +* + +**Deprecated:** + +* + +**Removed:** + +* + +**Fixed:** + +* Preserve scattering power lifetimes in nanobind Atom construction, + copying, and initialization to prevent use-after-free crashes. + +**Security:** + +* diff --git a/src/extensions/nb_atom.cpp b/src/extensions/nb_atom.cpp index 8516522..6450029 100644 --- a/src/extensions/nb_atom.cpp +++ b/src/extensions/nb_atom.cpp @@ -56,7 +56,7 @@ void wrap_atom(nb::module_& m) .def("__init__", [](Atom* self, const Atom& old) { new (self) Atom(old); - }, nb::arg("old")) + }, nb::arg("old"), nb::keep_alive<1,2>()) .def("__init__", [](Atom* self, REAL x, REAL y, REAL z, const std::string& name, nb::object pow, REAL popu) { @@ -64,7 +64,8 @@ void wrap_atom(nb::module_& m) new (self) Atom(x, y, z, name, sp, popu); }, nb::arg("x"), nb::arg("y"), nb::arg("z"), nb::arg("name"), - nb::arg("pow").none(), nb::arg("popu") = (REAL)1.0) + nb::arg("pow").none(), nb::arg("popu") = (REAL)1.0, + nb::keep_alive<1,6>()) .def("Init", [](Atom& self, REAL x, REAL y, REAL z, const std::string& name, nb::object pow, REAL popu) { @@ -72,7 +73,8 @@ void wrap_atom(nb::module_& m) self.Init(x, y, z, name, sp, popu); }, nb::arg("x"), nb::arg("y"), nb::arg("z"), nb::arg("name"), - nb::arg("pow").none(), nb::arg("popu") = (REAL)1.0) + nb::arg("pow").none(), nb::arg("popu") = (REAL)1.0, + nb::keep_alive<1,6>()) .def("GetMass", &Atom::GetMass) .def("GetRadius", &Atom::GetRadius) .def("IsDummy", &Atom::IsDummy) From 12b9087c5bd5baf7a872bcbecdf8b24af8e8b638 Mon Sep 17 00:00:00 2001 From: Vincent Favre-Nicolin Date: Wed, 4 Nov 2026 08:42:20 +0100 Subject: [PATCH 2/4] test: add Atom/ScatteringPower lifetime tests (#123) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/test_atom.py | 118 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 tests/test_atom.py diff --git a/tests/test_atom.py b/tests/test_atom.py new file mode 100644 index 0000000..38f0e18 --- /dev/null +++ b/tests/test_atom.py @@ -0,0 +1,118 @@ +#!/usr/bin/env python +"""Lifetime tests for Atom and its ScatteringPower (see issue #123). + +An Atom only stores a raw pointer to its ScatteringPower and calls +``DeRegisterClient`` on it in its destructor, so the Python Atom must keep +the Python ScatteringPower alive. +""" + +import gc +import os +import subprocess +import sys +import textwrap + +import pytest + +from pyobjcryst.atom import Atom +from pyobjcryst.scatteringpower import ScatteringPowerAtom + + +def _refcount(obj): + gc.collect() + return sys.getrefcount(obj) + + +# The refcount tests below never let a ScatteringPower die before its Atom, +# so that they fail cleanly (instead of crashing pytest) without the fix. + + +def test_constructor_keeps_scattering_power_alive(): + sp = ScatteringPowerAtom("Ni", "Ni") + before = _refcount(sp) + atom = Atom(0, 0, 0, "Ni", sp) + delta = _refcount(sp) - before + del atom + assert delta >= 1 + + +def test_init_keeps_scattering_power_alive(): + sp1 = ScatteringPowerAtom("Ni", "Ni") + sp2 = ScatteringPowerAtom("O", "O") + atom = Atom(0, 0, 0, "Ni", sp1) + before = _refcount(sp2) + atom.Init(0.1, 0.2, 0.3, "O", sp2) + delta = _refcount(sp2) - before + symbol = atom.GetScatteringPower().GetSymbol() + del atom + assert delta >= 1 + assert symbol == "O" + + +def test_copy_constructor_keeps_original_alive(): + sp = ScatteringPowerAtom("Ni", "Ni") + atom = Atom(0, 0, 0, "Ni", sp) + before = _refcount(atom) + copy = Atom(atom) + delta = _refcount(atom) - before + symbol = copy.GetScatteringPower().GetSymbol() + del copy, atom + assert delta >= 1 + assert symbol == "Ni" + + +@pytest.mark.parametrize( + "script", + [ + # Atom constructed from a temporary ScatteringPower. + """ + atom = Atom(0, 0, 0, "Ni", ScatteringPowerAtom("Ni", "Ni")) + gc.collect() + assert atom.GetScatteringPower().GetSymbol() == "Ni" + del atom + """, + # Atom re-initialized with a temporary ScatteringPower. + """ + atom = Atom(0, 0, 0, "Ni", ScatteringPowerAtom("Ni", "Ni")) + atom.Init(0, 0, 0, "O", ScatteringPowerAtom("O", "O")) + gc.collect() + assert atom.GetScatteringPower().GetSymbol() == "O" + del atom + """, + # Copy outliving both the original Atom and the ScatteringPower. + """ + sp = ScatteringPowerAtom("Ni", "Ni") + atom = Atom(0, 0, 0, "Ni", sp) + copy = Atom(atom) + del sp, atom + gc.collect() + assert copy.GetScatteringPower().GetSymbol() == "Ni" + del copy + """, + # Dummy atom (no ScatteringPower) must still work. + """ + atom = Atom(0, 0, 0, "dummy", None) + assert atom.IsDummy() + assert atom.GetScatteringPower() is None + atom.Init(0, 0, 0, "dummy2", None) + assert atom.IsDummy() + del atom + """, + ], +) +def test_scattering_power_outlived_by_atom(script): + # Run in a subprocess so that a use-after-free crash is reported as a + # test failure rather than killing pytest. Freed memory is scribbled + # (macOS / glibc) to make such a crash deterministic. + code = ( + "import gc\n" + "from pyobjcryst.atom import Atom\n" + "from pyobjcryst.scatteringpower import ScatteringPowerAtom\n" + + textwrap.dedent(script) + + "gc.collect()\n" + ) + env = dict(os.environ, MallocScribble="1", MALLOC_PERTURB_="85") + result = subprocess.run( + [sys.executable, "-c", code], capture_output=True, text=True, env=env + ) + assert result.returncode == 0, result.stderr From 2b74b6ffdcc997a6508209475a9f9722643eb477 Mon Sep 17 00:00:00 2001 From: SYMSCAE Date: Sat, 7 Nov 2026 18:13:01 -0800 Subject: [PATCH 3/4] build: build self-contained PyPI wheels with cibuildwheel The published wheels needed libObjCryst/libboost_python (Linux, macOS, Windows) from a conda environment: the old shared wheel workflow ran auditwheel and then removed the bundled libraries and pointed RPATH at the conda prefix. The nanobind build links ObjCryst++, cctbx and newmat statically and Boost is header-only, so build the wheels with cibuildwheel: - add [tool.cibuildwheel] (cp312-cp315; manylinux x86_64, macOS native, Windows AMD64; tests run on every wheel) - use the cibuildwheel shared workflow from SYMSCAE/release-scripts until it is released in scikit-package/release-scripts@v0 - derive the version from git tags with setuptools-scm instead of the static 2026.2.0rc0 placeholder - declare packaging as a runtime dependency (used by powderpattern) - trim unused ObjCryst++ submodule parts from the sdist - guard the GCC-only -O0 flag and add MSVC settings in CMakeLists.txt - ship the licenses of the statically linked code in the wheel Co-Authored-By: Claude Opus 5.5 --- .../workflows/build-wheel-release-upload.yml | 7 ++- CMakeLists.txt | 27 ++++++++-- nanobind_migration_notes.md | 19 ++++--- news/cibuildwheel-wheels.rst | 25 +++++++++ news/nanobind-packaging.rst | 1 - pyproject.toml | 54 ++++++++++++++++--- requirements/pip.txt | 1 + 7 files changed, 113 insertions(+), 21 deletions(-) create mode 100644 news/cibuildwheel-wheels.rst diff --git a/.github/workflows/build-wheel-release-upload.yml b/.github/workflows/build-wheel-release-upload.yml index 42b91f7..63897b1 100644 --- a/.github/workflows/build-wheel-release-upload.yml +++ b/.github/workflows/build-wheel-release-upload.yml @@ -23,8 +23,13 @@ jobs: # # Choose this for open-source packages distributed via PyPI and/or # conda-forge where broad public availability is the goal. + # + # Wheels are built with cibuildwheel using [tool.cibuildwheel] in + # pyproject.toml. + # TODO: switch back to scikit-package/release-scripts@v0 once the + # cibuildwheel change is released there. build-release: - uses: scikit-package/release-scripts/.github/workflows/_build-wheel-release-upload.yml@v0 + uses: SYMSCAE/release-scripts/.github/workflows/_build-wheel-release-upload.yml@cibuildwheel with: project: pyobjcryst submodules: "true" diff --git a/CMakeLists.txt b/CMakeLists.txt index 26b23b8..1106534 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -152,10 +152,21 @@ target_compile_definitions(ObjCryst PUBLIC target_link_libraries(ObjCryst PUBLIC cctbx newmat) # Put VFNDebug.cpp first so debug-level constants are defined before use -set_source_files_properties( - "${OBJCRYST_SRC}/Quirks/VFNDebug.cpp" - PROPERTIES COMPILE_FLAGS "-O0" -) +if(NOT MSVC) + set_source_files_properties( + "${OBJCRYST_SRC}/Quirks/VFNDebug.cpp" + PROPERTIES COMPILE_FLAGS "-O0" + ) +endif() + +# MSVC: same settings as the libobjcryst Windows build (/EHsc, M_PI etc.), +# plus /bigobj for the large binding translation units. +if(MSVC) + foreach(_tgt newmat cctbx ObjCryst) + target_compile_definitions(${_tgt} PUBLIC _USE_MATH_DEFINES NOMINMAX) + target_compile_options(${_tgt} PUBLIC /EHsc /bigobj) + endforeach() +endif() # --------------------------------------------------------------------------- # nanobind extension: _pyobjcryst @@ -178,3 +189,11 @@ target_link_libraries(_pyobjcryst PRIVATE ObjCryst) # Install into pyobjcryst package install(TARGETS _pyobjcryst LIBRARY DESTINATION pyobjcryst) + +# Licenses of the statically linked third-party code ship with the wheel. +install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/extern/objcryst/LICENSE.txt" + DESTINATION pyobjcryst/licenses/objcryst) +install(FILES "${CCTBX_SRC}/LICENSE_2_0.txt" "${CCTBX_SRC}/COPYRIGHT_2_0.txt" + DESTINATION pyobjcryst/licenses/cctbx) +install(FILES "${NEWMAT_SRC}/readme.txt" "${NEWMAT_SRC}/nm11.htm" + DESTINATION pyobjcryst/licenses/newmat) diff --git a/nanobind_migration_notes.md b/nanobind_migration_notes.md index 57eb662..5f2c75c 100644 --- a/nanobind_migration_notes.md +++ b/nanobind_migration_notes.md @@ -40,15 +40,18 @@ Known, accepted gaps — real, not oversights, but still open: The `Molecule`↔`MolAtom` cycle is fixed. `Crystal`↔`Scatterer`/`Molecule`/`ScatteringPower` cycles remain (56 leaked instances in the current full-suite run) — not safe to fix the same way, see [keep_alive Reference Cycles](#keep_alive-reference-cycles). Not blocking functionality. -- **CI has not been exercised against this build.** `.github/workflows/` is inherited from `main` - unmodified (it calls a shared reusable workflow); it has never actually run against the - nanobind/CMake build because nothing on this branch has been pushed yet. -- **Cross-platform**: everything above has been built and tested on Linux only. macOS/Windows are - unverified. +- **Wheel builds**: PyPI wheels are built with `cibuildwheel` by the shared release workflow, + configured in `[tool.cibuildwheel]` in `pyproject.toml`. The old shared workflow stripped the + libraries `auditwheel` had bundled, so those wheels only worked inside a conda environment. + The cibuildwheel version of the shared workflow is not released yet: + `build-wheel-release-upload.yml` points at `SYMSCAE/release-scripts@cibuildwheel` for now and + must be switched back to `scikit-package/release-scripts@v0` once it is. +- **Cross-platform**: Linux, macOS (x86_64/arm64) and Windows wheels are built and tested by + cibuildwheel in the release workflow. - **Performance**: not benchmarked against Boost.Python. -- **Versioning**: `pyproject.toml` uses a static version (`2026.2.0rc0`) instead of - `main`'s dynamic tag-based versioning (`setuptools-git-versioning`) — scikit-build-core needs a - different mechanism for that; flagged as a `TODO` in `pyproject.toml` itself, not yet resolved. +- **Versioning**: the version comes from the git tag via `setuptools-scm` + (`metadata.version.provider = "scikit_build_core.metadata.setuptools_scm"`), replacing + `main`'s `setuptools-git-versioning`. Builds need the tags (`fetch-depth: 0` in CI). None of the above blocks day-to-day use of the library from Python — every method that was reachable under Boost.Python is reachable under nanobind (modulo the override caveat above, which diff --git a/news/cibuildwheel-wheels.rst b/news/cibuildwheel-wheels.rst new file mode 100644 index 0000000..87862f2 --- /dev/null +++ b/news/cibuildwheel-wheels.rst @@ -0,0 +1,25 @@ +**Added:** + +* + +**Changed:** + +* Build the PyPI wheels with ``cibuildwheel`` (manylinux, macOS x86_64/arm64 and Windows AMD64), configured in ``[tool.cibuildwheel]`` in ``pyproject.toml``. +* The package version is now derived from the git tag via ``setuptools-scm``. + +**Deprecated:** + +* + +**Removed:** + +* + +**Fixed:** + +* PyPI wheels are now self-contained and no longer need ``libObjCryst`` or ``libboost_python`` from a conda environment; previously ``import pyobjcryst`` failed in a plain virtual environment. +* Declare ``packaging`` as a runtime dependency (used by ``pyobjcryst.powderpattern``). + +**Security:** + +* diff --git a/news/nanobind-packaging.rst b/news/nanobind-packaging.rst index 6419c7f..6d6fa3f 100644 --- a/news/nanobind-packaging.rst +++ b/news/nanobind-packaging.rst @@ -5,7 +5,6 @@ **Changed:** * Boost headers are now taken from the PyPI ``libboost-headers`` package when building with pip, falling back to system/conda Boost headers (or ``-DBOOST_INCLUDE_DIR=``). -* Set package version to ``2026.2.0rc0``. **Deprecated:** diff --git a/pyproject.toml b/pyproject.toml index ea0624b..d70a14b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,14 +1,16 @@ [build-system] -requires = ["scikit-build-core>=0.9", "nanobind", "numpy", "libboost-headers"] +requires = [ + "scikit-build-core>=0.10", + "setuptools-scm>=8", + "nanobind", + "numpy", + "libboost-headers", +] build-backend = "scikit_build_core.build" [project] name = "pyobjcryst" -# TODO: main uses setuptools-git-versioning (dynamic, tag-based) with -# setuptools.build_meta; scikit-build-core needs a different dynamic-version -# provider to match that exactly. Pinned as a static placeholder for now -- -# revisit before release, see nanobind_migration_notes.md. -version = "2026.2.0rc0" +dynamic = ["version"] authors = [ { name="Christopher Farrow" }, { name="Pavol Juhás" }, @@ -22,7 +24,7 @@ description = "Python bindings to the ObjCryst++ library." keywords = ['objcryst', 'atom structure crystallography', 'powder diffraction'] readme = "README.rst" requires-python = ">=3.12, <3.16" -dependencies = ["numpy"] +dependencies = ["numpy", "packaging"] classifiers = [ 'Development Status :: 5 - Production/Stable', 'Environment :: Console', @@ -52,8 +54,46 @@ Issues = "https://github.com/diffpy/pyobjcryst/issues/" pyobjcryst = "pyobjcryst.pyobjcryst_app:main" [tool.scikit-build] +minimum-version = "build-system.requires" cmake.build-type = "Release" wheel.packages = ["src/pyobjcryst"] +sdist.cmake = true +sdist.exclude = [ + "build", + "dist", + ".pytest_cache", + # Parts of the ObjCryst++ submodule not needed to build the extension. + "extern/objcryst/Fox", + "extern/objcryst/test", + "extern/objcryst/ObjCryst/doc", + "extern/objcryst/ObjCryst/example", +] +metadata.version.provider = "scikit_build_core.metadata.setuptools_scm" + +[tool.setuptools_scm] +version_scheme = "no-guess-dev" + +# PyPI wheels: ObjCryst++, cctbx and newmat are built from extern/ and linked +# statically, Boost is header-only and nanobind is linked statically, so the +# wheels install into any environment without conda or system libraries. +[tool.cibuildwheel] +build = ["cp312-*", "cp313-*", "cp314-*", "cp315-*"] +skip = ["*-musllinux_*"] +test-requires = ["pytest"] +# The tests open tests/testdata/* relative to the working directory. +test-sources = ["tests"] +test-command = "pytest tests" + +[tool.cibuildwheel.linux] +archs = ["x86_64"] + +[tool.cibuildwheel.macos] +archs = ["native"] +environment = { MACOSX_DEPLOYMENT_TARGET = "11.0" } + +# No Windows ARM64 wheels. +[tool.cibuildwheel.windows] +archs = ["AMD64"] [tool.codespell] exclude-file = ".codespell/ignore_lines.txt" diff --git a/requirements/pip.txt b/requirements/pip.txt index e1eb255..f70a1e2 100644 --- a/requirements/pip.txt +++ b/requirements/pip.txt @@ -1,3 +1,4 @@ setuptools numpy +packaging libboost-headers From 7951366742008735c4447f917efe124b6f1aac79 Mon Sep 17 00:00:00 2001 From: SYMSCAE Date: Sun, 8 Nov 2026 16:26:08 -0800 Subject: [PATCH 4/4] ci: update workflow --- .github/workflows/build-wheel-release-upload.yml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/build-wheel-release-upload.yml b/.github/workflows/build-wheel-release-upload.yml index 63897b1..375e248 100644 --- a/.github/workflows/build-wheel-release-upload.yml +++ b/.github/workflows/build-wheel-release-upload.yml @@ -24,16 +24,17 @@ jobs: # Choose this for open-source packages distributed via PyPI and/or # conda-forge where broad public availability is the goal. # - # Wheels are built with cibuildwheel using [tool.cibuildwheel] in - # pyproject.toml. + # build_c_ext_with_cibw builds the wheels with cibuildwheel using + # [tool.cibuildwheel] in pyproject.toml. # TODO: switch back to scikit-package/release-scripts@v0 once the - # cibuildwheel change is released there. + # cibw change is released there. build-release: - uses: SYMSCAE/release-scripts/.github/workflows/_build-wheel-release-upload.yml@cibuildwheel + uses: SYMSCAE/release-scripts/.github/workflows/_build-wheel-release-upload.yml@cibw with: project: pyobjcryst submodules: "true" c_extension: true + build_c_ext_with_cibw: true maintainer_github_username: sbillinge secrets: PYPI_TOKEN: ${{ secrets.PYPI_TOKEN }}