Add pip-installable wheels for Linux and macOS - #151
Conversation
`pip install forefire` now works, providing both the pyforefire module and the forefire command-line interpreter with NetCDF vendored in. Closes forefireAPI#149. Packaging moves to the repository root and is driven by scikit-build-core, which runs the existing CMake build instead of assuming a pre-built libforefireL: an sdist rooted at bindings/python cannot reach ../../src. bindings/python/{setup.py,pyproject.toml} are removed. CMakeLists.txt becomes option-driven, keeping today's behaviour as the default for source builds while wheels get portable settings: FOREFIRE_ENABLE_MPI, FOREFIRE_NATIVE_ARCH, FOREFIRE_BUILD_PYTHON, FOREFIRE_STATIC_CORE, FOREFIRE_BUILD_TOOLS, FOREFIRE_CHECK_LFS Wheels disable MPI so a published artifact never depends on what happened to be installed on the builder (see forefireAPI#102), and disable -march=native, which was previously unconditional and made any redistributed binary — including the published Docker image — crash with SIGILL on older CPUs. NetCDF is now located with find_path/find_library behind a single forefire_netcdf interface target rather than bare link_libraries. This is what makes a macOS wheel possible: Homebrew names the library libnetcdf-cxx4, not libnetcdf_c++4, and /opt/homebrew is off the default search path. Missing NetCDF now fails at configure time with install hints. The core is an OBJECT library, not STATIC, when linked directly into the artifacts: propagation and flux models register themselves from static initialisers that nothing references by name, and a static archive lets the linker discard them, leaving the model table empty at runtime. Other changes: - version is parsed from src/include/Version.h, so project(VERSION), CPack and the wheel all track the file tools/increment-version.sh edits - .gitignore no longer ignores CMakeLists.txt, which had been dropping the build file from the sdist - Dockerfile builds its wheel from the root and installs the interpreter from that one wheel instead of copying a second binary - tests/python/test_wheel.py smoke-tests an installed wheel; every wheel is tested before upload
Project maturity is a maintainer's call, not something packaging should assert. Left unset rather than guessed.
|
Thanks for this @HugoFara — went through it with Claude helping me dig into the CMake/packaging side (I'm honestly not a C++ build expert, so wanted a second pair of eyes on that part specifically). Overall take: solid work — the OBJECT library split, the NetCDF detection covering the Homebrew naming difference, single-source-of-truth versioning off One thing before merging: Simplest fix is probably setting I'm traveling this week with limited bandwidth, back around Aug 10th. Happy to hop on a call then to close this out — or if you get a chance to fix the macOS target in the meantime, I'll do a final pass and merge once CI's green. Either works for me. |
The macos-14 arm64 wheel failed to repair: every Homebrew bottle on that runner (netcdf, hdf5, netcdf-cxx4, zstd, libaec, libsz) has a minimum target of 14.0, so delocate refused to relocate them into a wheel tagged for 13.0. The right value follows the runner image rather than the project, so it moves from pyproject.toml into the workflow matrix: 14.0 on macos-14, 13.0 on macos-13. Apple Silicon wheels now require macOS 14.
|
Thanks for the review, and for digging into the CMake side. Fixed in 347a720. One small correction on the diagnosis: So 13.0 was not too low in general, it was too low for that particular runner image. I tried to keep a single source-of-truth number in Two things though:
No rush to merge, enjoy your trip! If you are okay with this PR, we can discuss it through about its next adjustments. I want to keep it minimal for readability, but there are more edits I'd like to make (old NetCDF + unnecessary dependencies in the wheel), so it'll be a good occasion to have a chat about its specific aspects. Let me know! |
Closes #149.
Makes
pip install forefirework. Wheels for Linux and macOS ship thepyforefiremodule, theforefirecommand-line interpreter, and a vendored NetCDF. No NetCDF install, noNETCDF_HOME, no prior CMake build.pip install forefire forefire -v python -c "import pyforefire; print(pyforefire.ForeFire())"The PyPI distribution is named
forefire, as the issue asked. The import name stayspyforefire, so existing scripts keep working.The three obstacles from @antonio-leblanc's reply
1.
setup.pyassumed a pre-builtlibforefireL.Replaced with scikit-build-core, which drives the existing
CMakeLists.txt. The wheel build now compiles the C++ core itself. Packaging moved to the repository root, because an sdist rooted atbindings/python/cannot reach../../src.bindings/python/setup.pyandbindings/python/pyproject.tomlare deleted.2. NetCDF vendoring.
On Linux,
tools/devops/install-netcdf-manylinux.shinstalls NetCDF C and HDF5 from EPEL inside the manylinux_2_28 container. It then buildsnetcdf-cxx4from source with a pinned checksum, since no RHEL-family package provides the C++4 API.auditwheelcopies the result into the wheel. macOS uses Homebrew plusdelocate.3. The MPI axis (#102).
Wheel builds set
FOREFIRE_ENABLE_MPI=OFFexplicitly. A published wheel can no longer depend on whatever happened to be installed on the build machine. Users who need coupling build from source withFOREFIRE_ENABLE_MPI=ON. This is now documented rather than implicit.CMake changes
The build is option-driven. Defaults preserve the previous behaviour for ordinary source builds:
FOREFIRE_ENABLE_MPIONOFFFOREFIRE_NATIVE_ARCHONOFFFOREFIRE_BUILD_PYTHONOFFONFOREFIRE_STATIC_COREOFFONFOREFIRE_BUILD_TOOLSONOFFFOREFIRE_CHECK_LFSONOFFAlso changed:
find_pathandfind_library, replacing barelink_libraries(netcdf netcdf_c++4). This is what makes the macOS wheel possible. Homebrew names the librarylibnetcdf-cxx4, notlibnetcdf_c++4, and/opt/homebrewis not on the default search path on Apple Silicon. A missing NetCDF now fails at configure time with install instructions, instead of failing at link time.src/include/Version.h.project(VERSION), CPack and the wheel version now track the filetools/increment-version.shedits.project()previously said1.0while the code saidv2.3.0.if(NETCDF_STATIC_LIBS)/if(MPI_FOUND)link blocks collapse into oneforefire_netcdfinterface target.bin/,lib/), the LFS check and CPack are skipped underSKBUILDonly. Native builds are unaffected.Two portability bugs fixed along the way
-march=nativewas unconditional. Any binary built on one machine and run on an older CPU dies with SIGILL. That includes the published Docker image, so theDockerfilenow passes-DFOREFIRE_NATIVE_ARCH=OFF. Native builds keep the flag on by default../bindings/python, which is no longer a project. It now builds from the root and takes theforefirebinary from that single wheel, rather than copying a second one.CI
New
.github/workflows/wheels.yml, running on pull requests and pushes to master:ubuntu-latest(x86_64),ubuntu-24.04-arm(aarch64, native runner, no QEMU),macos-14(arm64) andmacos-13(x86_64), for CPython 3.9 to 3.13.Every wheel is smoke-tested before upload.
tests/python/test_wheel.pyimports the module, runs an isotropic 1000 s simulation, and asserts the front grew.forefire -vthen checks the shipped interpreter.Verification
Built in a real
manylinux_2_28_x86_64container with podman, then installed and tested on a Fedora 44 host with no NetCDF installed at all. That second step is the meaningful one: the test machine is not the build machine, andglibc differs (2.43 against 2.28).
forefire.libs/lddon the extensionlibrtandlibresolvfrom the systemforefire -vfrom any directoryv2.3.0tests/runff/run.ffThat last row runs a full Rothermel simulation over the 4 MB
tests/runff/data.nc, so the vendored HDF5 and NetCDF are genuinely exercised rather than merely loaded.The plain
cmake -S . -B buildpath is unchanged: sharedlib/libforefireL.so,bin/forefireandbin/ANN_test,-march=nativestill on, LFS check still running. The native binary and the wheel produce byte-identical output onrun.ff. The sdist is 402 KB and containsCMakeLists.txt, which.gitignorehad been silently excluding.Two bugs were caught by that local run rather than by review, which is why the smoke test now asserts on them:
addLayer("propagation", "Iso", ...)printedmodel Iso is not recognized, and the next call segfaulted. Fixed with an OBJECT library.tests/python/test_wheel.pynow fails loudly instead of crashing.Threads::Threadslinked explicitly. The shared library had been providing it implicitly.macOS wheels are not verified locally, I may do it later on. Both the Linux and macOS jobs run
forefire -vagainst the installed wheel. Ifdelocatedoes not relocate the interpreter binary the wayauditwheeldoes, CI fails rather than publishing something broken. Worth watching on the first run.Before this can publish
The
forefireproject on PyPI needs to exist, I'll leave it to you. Until then the workflow builds and tests wheels, and the publish job never runs. It is gated ongithub.event_name == 'release'.Packaging choices worth a look
The root
pyproject.tomlreplaces a much smallerbindings/python/pyproject.toml. Some values in it are forced by the toolchain, others are policy. The policy ones:>=3.8>=3.92025.2src/include/Version.h, currently2.3.0pybind11{ file = "LICENSE" }"GPL-3.0-or-later"OS Independentforefire.univ-corse.frtests/,docs/,paper/excluded, smoke test keptTwo suggestions rather than changes:
Development Statusclassifier is set.5 - Production/Stablewould fit v2.3.0 and the JOSS paper, if you want the maturity advertised.skiplist.Notes for reviewers
netcdf_c++4for it, and the sources use<sys/time.h>andpopen, so it needs a porting effort rather than a packaging one. Windows users fall back to the sdist.bindings/python/README.mdis rewritten aroundpip install. It is what PyPI renders as the project page.MACOSX_DEPLOYMENT_TARGETlives in the workflow matrix, notpyproject.toml, because the correct value is a property of the runner image rather than of the project. It will need revisiting when these runners are replaced.