Making wheel 3 - #70
Open
shimwell wants to merge 225 commits into
Open
Conversation
…into scikit_build_core_wheel
…into scikit_build_core_wheel
manylinux_2_28 ships patchelf 0.17.2. When auditwheel repair used it to rewrite the small non-PIE bin/openmc executable, it emitted a new PT_LOAD whose filesz stopped 0x30 bytes short of the relocated .dynamic section: LOAD vaddr 0x3ff000 filesz 0x1098 DYNAMIC vaddr 0x4000c8 size 0x310 <- past the end of that LOAD The dynamic loader then read .dynamic from unmapped memory, so the binary segfaulted inside dl_main before main() ran and "openmc --version" died with SIGSEGV. libopenmc.so was rewritten correctly, which is why "import openmc.lib" kept working and only the CLI smoke test failed. The bug is layout sensitive, so it only started biting once the develop merge changed the executable slightly. Verified in the manylinux container that 0.14.5, 0.15.0 and 0.16.1 all rewrite this executable correctly while 0.17.2 corrupts it, so pin 0.16.1 and overwrite the copy auditwheel resolves from PATH.
12 tasks
Covers the first three items of the Windows wheel support issue. None of this changes the Linux or macOS build. MSVC previously built libopenmc as a static library, which openmc.lib can never load because it uses ctypes.CDLL. Build a shared library instead and set WINDOWS_EXPORT_ALL_SYMBOLS so the 204 openmc_* C API functions do not need __declspec(dllexport) annotations. The five globals read through ctypes in_dll are all declared extern "C", so they should be exported unmangled, though data exports are the weak spot of WINDOWS_EXPORT_ALL_SYMBOLS and may still need a hand-written .def file. Search "bin" for the core library, since Windows installs a DLL to CMAKE_INSTALL_BINDIR as a RUNTIME artifact while .so and .dylib are LIBRARY artifacts in CMAKE_INSTALL_LIBDIR. The Windows import library is an ARCHIVE artifact and still lands in lib, so lib_path is unchanged. Add a scratch windows-latest workflow to establish a feedback loop, since Windows cannot be reproduced locally. It sets continue-on-error so it can never block a merge, and its matrix tests /openmp:llvm against a serial build to determine which OpenMP setting is viable, then checks the five data symbols through ctypes. Verified that the Linux build is unaffected: with an unchanged HEAD, so the embedded commit hash is constant, libopenmc.so and the openmc executable are byte-identical before and after (sha256 b75b26fb.. and c346351b..), the 4222 entry dynamic symbol table is unchanged, and cmake configure output is identical. A control rebuild confirmed the build is reproducible, so those comparisons are meaningful. The "bin" addition was checked against a real staged install and resolves to the same single library.
MSVC accepts the alternative operator tokens (and, or, not and friends) only when /permissive- is passed, which is not the default for a CMake-driven build. A single one of these in a widely included header therefore breaks the entire Windows build. Because the failure is a parse error, the cost is out of proportion to the cause. Two occurrences of 'and' in mesh.h (fixed in openmc-dev#4048) produced over a hundred cascade errors spread across unrelated translation units, including complaints about members of openmc::simulation and about MSVC's own internal _Cosh and _Exp names, none of which pointed at the actual line. clang-format does not rewrite these tokens, so nothing in CI catches them today. This adds a check that strips comments and string literals before scanning, so it does not trip on prose or on identifiers such as n_and and order. It reports file, line and the offending source line, and skips vendored code under src/external/. The script takes optional path arguments so it can be run directly during development: python tools/ci/check_alternative_tokens.py
CMake's WINDOWS_EXPORT_ALL_SYMBOLS generates an export table for
functions but does not emit global data, so variables read from outside
libopenmc are absent from the DLL. The Windows build probe hit this in
two places at once:
* 4 of the globals openmc.lib reads through ctypes in_dll were missing
from the export table, while n_coord_levels was present
* linking openmc.exe failed with 4 unresolved externals, all data:
settings::run_mode, settings::solver_type, mpi::master and
openmc_err_msg
The discriminator is const, not name mangling. Checking a Linux build
confirms it: DAGMC_ENABLED, UWUW_ENABLED, LIBMESH_ENABLED and
STRICT_FP_ENABLED are the only four in read-only data (section R in nm
output), and they are exactly the four that were dropped. bindexplib
emits mutable data but not read-only data.
This adds include/openmc/export.h with an OPENMC_API macro that expands
to dllexport while building the library, dllimport for anything linking
it, and nothing at all on POSIX or in a static MSVC build. The relevant
declarations now carry it.
Two related notes found while inventorying the symbols:
openmc.lib depends on 24 in_dll globals, not the 5 previously assumed.
Fourteen of them are behind the _DLLGlobal descriptor in
openmc/lib/core.py rather than in a literal in_dll call, which is why
they were missed. The probe now checks all of them and fails the step on
any that are absent, rather than reporting and exiting zero as before.
settings::run_mode and settings::rel_max_lost_particles are declared
without extern "C" and so are name-mangled, but openmc/lib looks them up
unmangled. Both therefore raise on every platform, not just Windows.
That is a pre-existing bug and is left alone here; the probe records them
as expected absences so they do not mask a real export regression.
openmc.lib reads both of these through ctypes in_dll using their plain
names, but both were declared in namespace openmc::settings without
extern "C", so the symbols are name-mangled and the lookup has never
resolved. On a Linux build:
>>> ctypes.c_int.in_dll(dll, 'run_mode')
ValueError: openmc/lib/libopenmc.so: undefined symbol: run_mode
nm shows why: _ZN6openmc8settings8run_modeE and
_ZN6openmc8settings22rel_max_lost_particlesE. So
openmc.lib.settings.run_mode raises through its property getter, and
openmc.lib.settings.rel_max_lost_particles raises through the _DLLGlobal
descriptor. Every other global in this file that openmc.lib reads is
already declared extern "C", including max_lost_particles on the line
immediately above rel_max_lost_particles.
This is not platform specific. The two symbols are mangled on Linux,
macOS and Windows alike.
rel_max_lost_particles has been broken since it was introduced.
d763806 renamed it from relative_max_lost_particles on both the C++
and Python sides to make the names match, but with no extern "C" the
symbol never matched under either spelling.
After the change, both resolve and read the correct values, with
rel_max_lost_particles giving the 1.0e-6 default from settings.cpp:
run_mode = 0 (RunMode::UNSET)
rel_max_lost_particles = 1e-06
solver_type is left alone since openmc.lib does not read it.
Job-level continue-on-error keeps the workflow run green but the job is still reported as failed to the checks API, so the openmp-llvm leg put a red check on the PR despite the probe being explicitly not required to pass. Tolerating the failure at step level instead, gated on a new experimental matrix flag, lets that leg keep reporting its findings in the log without doing that. The openmp-off leg is marked not experimental, since it is the configuration a first vanilla wheel would ship and a failure there is a real regression. /openmp:llvm turns out to get much further than the plain /openmp the matrix was written around. It accepts collapse and atomic capture, and rejects only four things: 'seq_cst' on atomic in shared_array.h, a non-conforming collapse bound in plot.cpp, and a reduction on a non-static member in flat_source_domain.cpp and random_ray_simulation.cpp. Recorded in the matrix comment so the remaining work is written down rather than rediscovered. Also requires run_mode and rel_max_lost_particles now that they have C linkage, taking the checked set to all 24 globals openmc.lib reads.
windows: export the data symbols that cross the library boundary
Adds the packaging pieces for a vanilla Windows wheel and fixes the bug
that would have made any such wheel fail on import.
The bug: get_core_libraries globs "libopenmc*" over lib, lib64 and bin in
that order. On Windows the DLL is a RUNTIME artifact in bin but the
import library is an ARCHIVE artifact named libopenmc.lib in lib, so it
matches the same pattern and, because lib is searched first, it is
returned first. openmc/lib takes element zero of that list and hands it
to CDLL, which cannot load an import library:
OSError: [WinError 193] %1 is not a valid Win32 application
Found by the pip install probe job, which is also what showed that
adding bin to the search path was necessary but not sufficient. The
pattern is now extension-specific on Windows so the import library can
never be selected. POSIX behaviour is unchanged, verified against both
layouts.
Also adds the import of sys that openmc/__init__.py has been missing.
sys.platform was already used in get_extra_libraries and only resolved
because "from openmc.material import *" happens to leak the name, that
module having no __all__. Relying on that is fragile and the new code
needs sys too.
Packaging:
* cp312, cp313 and cp314 win_amd64 added to [tool.cibuildwheel] build.
That table is an allowlist, so without this nothing is built for
Windows no matter what else is configured.
* a [tool.cibuildwheel.windows] section building vanilla and serial, with
delvewheel as the auditwheel equivalent to bundle the HDF5 DLLs and
inject the os.add_dll_directory calls a plain build does not.
* build-wheels-windows.yml, mirroring the Linux workflow, uploading a
windows-wheels artifact.
The Windows test-command deliberately omits "openmc --version", since
the console script generated by cmake/GenerateScript.cmake is POSIX only.
The separate test job installs the repaired wheel into a clean
interpreter with no vcpkg and no source tree present, which is what
actually proves delvewheel bundled HDF5 rather than the machine having
it, and asserts the two export cases that were broken before #129 and
#130 read back through ctypes.
The first Windows wheel run compiled libopenmc.dll cleanly and then failed linking the Catch2 unit tests, with 21 LNK2019 errors across test_mesh, test_ray, test_region and test_tally. They reference openmc::model globals directly (meshes, mesh_map, surfaces, surface_map, filter_map, root_universe, universe_level_counts, n_coord_levels), and importing data across a DLL boundary requires dllimport on the declaration, which WINDOWS_EXPORT_ALL_SYMBOLS cannot supply. Being present in the export table is not sufficient for data: n_coord_levels is exported and still failed to link, because a plain data reference has no thunk to go through. OPENMC_BUILD_TESTS defaults to ON and the Windows environment did not turn it off, unlike the probe job which has always passed -DOPENMC_BUILD_TESTS=OFF. A wheel has no use for the C++ test binaries anyway, so this is a fix rather than a workaround, and it drops several minutes of Catch2 compilation from every wheel build. Building the C++ tests on Windows would need those model globals annotated with OPENMC_API in the same way as #129 did for the four export cases main.cpp needed. That is separate work and is not required for a wheel.
The check step reported success while one of its commands was raising:
AttributeError: module 'openmc.lib' has no attribute 'DAGMC_ENABLED'
Two separate mistakes. The flags are not module attributes, they are read
through accessor functions (openmc.lib._dagmc_enabled and friends),
which is how openmc's own test suite does it, see tests/conftest.py. And
the step ran three "python -c" calls under pwsh, where only the last
native command's exit code is inspected, so the first two failing was
invisible and the step still passed.
Now a single python process under bash, which GitHub invokes with -e, so
anything raising fails the step. Explicit $LASTEXITCODE guards added to
the pwsh install step for the same reason.
While rewriting, the assertions were made worth having:
* the loaded path must end in .dll, which is the regression that the
extension-specific search pattern fixes. Asserting the import
succeeded is not enough, since loading the import library is exactly
what used to happen and it fails at CDLL rather than silently.
* all three const bool flags are checked, not just DAGMC_ENABLED. These
are the read-only data case that WINDOWS_EXPORT_ALL_SYMBOLS drops.
* rel_max_lost_particles is checked against its 1.0e-6 default from
src/settings.cpp rather than merely for the absence of an exception, so
a symbol resolving to the wrong storage would be caught.
run_mode is printed but not asserted on. It reads back as None rather
than a string because the library is uninitialised, so RunMode::UNSET is
absent from openmc.lib's mapping. Resolving without raising is the point;
before the extern "C" fix it was a ValueError about an undefined symbol.
Build Windows wheels and fix the DLL the loader picks
Add CI guard against C++ alternative operator tokens
The Windows wheel test only imported openmc and read openmc.lib symbols, which passes even when the openmc executable can not be launched at all. The Mac OS test ran openmc --version but no simulation, and the Linux test ran openmc --version with a || echo that discarded the exit code. Adds minimal_test_csg.py, a CSG only simulation that uses the nuclear data already committed for minimal_test.py, so it can run on the vanilla Windows wheel which has no DAGMC. It goes through model.run() so the openmc executable and the console script that launches it are covered. Windows and Mac OS now run the CLI check and the simulation, Mac OS also runs the DAGMC minimal_test.py, and the Linux Dockerfile no longer swallows the exit code of openmc --version.
The launcher was a plain script file generated by cmake/GenerateScript.cmake that called os.execv on a path with no .exe suffix. On Windows the packaging tools install a plain script file without any extension, which Windows can not execute, so nothing named openmc was resolvable on the PATH and both the command line and openmc.run() failed with WinError 2. os.execv is also wrong on Windows as it does not replace the calling process, so a caller waiting on it would carry on immediately. Declaring openmc as a console script entry point gets an openmc.exe launcher generated on Windows and an ordinary script elsewhere. GenerateScript.cmake is removed as the entry point replaces it. Also restores openmc --version to the Windows cibuildwheel test-command, checks the output of openmc --version rather than only its exit status, since the previous bash check passed while printing nothing, and adds diagnostics that run the executable directly and list its DLL dependencies to explain why it exits without any output.
quiet_dll called sys.stdout.fileno() to redirect the output of the shared library. In a Jupyter notebook sys.stdout is an ipykernel OutStream, which raises io.UnsupportedOperation from fileno() unless the kernel is capturing output at the file descriptor level, so anything going through quiet_dll raised. Model.plot() does, which made it fail in a notebook on Mac OS while working on Linux. Suppressing the output is only cosmetic so it is now skipped when there is no file descriptor to redirect, rather than raising.
The only conflict was in include/openmc/settings.h, where develop gained the same C linkage for run_mode and rel_max_lost_particles from openmc-dev#4050 that this branch added for the Windows DLL exports. Resolved by keeping develop's linkage together with this branch's OPENMC_API annotations and the openmc/export.h include, which develop does not have.
libopenmc.dll imports hdf5.dll and hdf5_hl.dll, which are bundled into the wheel by delvewheel and placed in a folder beside the package. The injected os.add_dll_directory calls only apply to the Python process, so importing openmc.lib works while the executable, which runs as a separate process, exits without writing anything at all. That is why the error was an empty output and a non zero status rather than a message. The console script now passes an environment with those folders on the PATH, and tools/ci/check_windows_wheel.py reports the layout, the DLL imports of each binary and where each one resolves to, so the next failure of this kind is readable from the build log. It replaces the Windows test-command so the diagnostics are printed even when the checks fail, which matters because a failing test-command stops cibuildwheel producing a wheel for the test job to look at.
The Windows wheel did not contain HDF5 at all. There was no openmc.libs folder, because delvewheel only analyses the extension modules in a wheel by default and openmc has none, it is pure Python calling libopenmc.dll through ctypes, so delvewheel had nothing to look at and vendored nothing. --analyze-existing makes it inspect the DLLs already in the wheel. This went unnoticed because h5py bundles its own copy of HDF5 and adds it to the DLL search path of the process, so importing openmc.lib worked and happened to bind libopenmc.dll against h5py's HDF5. The openmc executable is a separate process that gets none of that and failed with 0xC0000135 STATUS_DLL_NOT_FOUND, writing nothing at all. The check script now reports what was vendored and where h5py's HDF5 is, so this is visible rather than inferred, and declares the return types of the kernel32 calls it uses. Without them ctypes truncated the 64 bit module handles to c_int and every DLL was reported at the same empty path.
Brings in the wheel tests that run the command line interface and a simulation on all three platforms, the openmc console script entry point, the HDF5 bundling fix for the Windows wheel and the quiet_dll fix for notebooks.
Owner
Author
Follow up: the macOS wheel reports
|
| Platform | openmc --version |
|---|---|
| Linux | OpenMC version 0.15.3-dev613 |
| Windows | OpenMC version 0.15.3-dev613 |
| macOS ARM | OpenMC version 0.0.0 |
Only the reported version is wrong — the macOS wheel itself is fine, it passes the CLI check, the CSG simulation and the DAGMC minimal_test.py, and its tritium production result agrees with Linux and Windows to ~15 significant figures.
Likely cause: build-wheels-macos.yml builds with python -m build directly rather than through cibuildwheel, and its actions/checkout has no fetch-depth: 0. setuptools_scm then sees a shallow clone with no tags and falls back, where the Windows workflow does set fetch-depth: 0 # needed for setuptools_scm. Worth checking whether the C++ side takes its version from the same place.
Why it matters a little: users see this in openmc --version output and paste it into bug reports, so a 0.0.0 makes issues harder to triage.
Owner
Author
The Windows and macOS wheels are single threadedRaising this as a follow up. Both non-Linux wheels are built with OpenMP off, so every simulation run from them is serial:
Only Windows has a stated reason, in the comment above
macOS has no accompanying note, so it may just be inherited rather than deliberate. The usual cause there is that AppleClang ships no OpenMP runtime by default and needs Measured impact. This is not theoretical. The neutronics-workshop CI runs the same notebooks on all three platforms, and several simulations that finish comfortably on Linux exceed a 300 second per-cell timeout on Windows and macOS. Three notebooks timed out (task 9 dose plotting, task 12 notebooks 2 and 3), which is what led to this being noticed. Why it matters. Users on a laptop with 8 or 10 cores get roughly a core's worth of a machine, so a run that takes a couple of minutes on Linux can take most of an hour. For a teaching workshop where people run notebooks interactively, that is the difference between a usable and an unusable session. Suggest splitting into two pieces of work, since the causes are unrelated:
Worth checking |
Upstream replaced the exported C API globals with accessor functions, which supersedes most of the Windows symbol visibility work on this branch. openmc/lib no longer reads a single global through ctypes in_dll, so only functions cross the library boundary now. Seven conflicts, resolved as follows. CMakeLists.txt: keep develop's libopenmc_objects object library, but build libopenmc SHARED on MSVC as well, because openmc.lib loads it with ctypes.CDLL and cannot load a static library. WINDOWS_EXPORT_ALL_SYMBOLS is enough now that no data symbol is exported. The branch's SKBUILD install layout, RPATH handling and console script wiring are kept. include/openmc/capi.h: take develop. openmc_err_msg is gone and openmc_get_err_msg replaces it. include/openmc/settings.h: take develop. run_mode and solver_type are reached through openmc_setting_get_int32 rather than read directly. include/openmc/dagmc.h, mesh.h and output.h: take develop. DAGMC_ENABLED, UWUW_ENABLED, LIBMESH_ENABLED and STRICT_FP_ENABLED are replaced by openmc_get_feature_enabled. openmc/lib/__init__.py: develop's ctypes imports and feature_enabled, with the branch's openmc.lib[0] loader kept, because the wheel installs the library under openmc/core rather than openmc/lib.
include/openmc/export.h existed because openmc.lib read 24 globals across the library boundary with ctypes in_dll, and WINDOWS_EXPORT_ALL_SYMBOLS emits functions and mutable data but not read-only data, so the const flags were silently absent from the export table. Upstream removed the reason for it. openmc_get_feature_enabled replaced the four const bool flags, openmc_get_err_msg replaced the openmc_err_msg buffer, and openmc_setting_get_* and set_* replaced everything openmc/lib/settings.py reached through the _DLLGlobal descriptor. There is no in_dll call left anywhere in openmc/, and src/main.cpp is now a single call to openmc_main, so no data symbol crosses the boundary at all. The C++ unit tests link libopenmc_test, a static library over the same objects, so they do not cross it either. With every annotated declaration reverted to its upstream form by the merge, export.h had no users left. Keeping it wired would have been worse than deleting it: libopenmc has no sources of its own now, so OPENMC_BUILDING_LIBOPENMC on that target compiles nothing, and moving it to libopenmc_objects would propagate a PUBLIC OPENMC_DLL through libopenmc_test into every Catch2 test unit and make statically linked test code see __declspec(dllimport). Also updates the two Windows CI checks that tested the old contract. The wheel diagnostics called openmc.lib._dagmc_enabled and friends, which no longer exist and would have raised AttributeError under a step that GitHub runs with -e. The probe checked 24 data symbols by name, ten of which are gone and the rest of which are name mangled C++ globals; it now checks that the C API functions openmc.lib depends on are exported, which is the contract that actually has to hold. The pyproject rationale for OPENMC_BUILD_TESTS=OFF described an LNK2019 failure that upstream openmc-dev#4062 made impossible.
The pin was still 0.15.3 while this branch sits 247 commits past the v0.16.0 tag, so every wheel built here was stamped with a released version number it did not contain. pip compares version strings, not file contents, so a rebuilt 0.15.3 wheel published to an index looks like no upgrade at all to anyone who already installed one. 0.16.1.dev0 is what setuptools_scm guess-next-dev would produce after v0.16.0. It sorts above 0.16.0 and below a future 0.16.1.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We have made a PR to openmc with this wheel building and while that is considered / reviewed there is a need for wheels both to test the production of wheels but also we need wheels with the latest develop branch changes to run our simulations.
So I keep this branch upto date with the latest develop changes. The actions on this branch build wheels for Linux, Mac and Windows. Periodically I upload a new wheel from the actions to an extra pypi index here that other people who want to pip install openmc using the wheels can test it https://github.com/shimwell/wheels