Build Windows wheels and fix the DLL the loader picks - #131
Merged
Conversation
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.
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
1 participant
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.
Now that #129 has the vanilla Windows build producing a DLL and a linked
openmc.exe, this adds the packaging to turn that into an actual wheel artifact, and fixes the bug that would have made any such wheel fail on import.The bug the pip install probe found
get_core_libraries()globslibopenmc*overlib,lib64andbinin that order. On Windows the loadable DLL is a RUNTIME artifact inbin, but the import library is an ARCHIVE artifact namedlibopenmc.libsitting inlib. It matches the same pattern, and becauselibis searched first it is returned first.openmc/lib/__init__.py:21takes element zero of that list and passes it toCDLL:Observed from the actual install layout:
This is what the pip install probe job was for. It also shows that adding
binto the search path in 033fa82 was necessary but not sufficient, and that this had nothing to do with HDF5 bundling: the import failed identically with the HDF5 DLLs on the path, which is why that step was split in two.The pattern is now extension-specific on Windows so the import library can never be chosen. Verified against both real layouts, with POSIX output identical before and after.
Without this, cibuildwheel's own
test-commandwould fail on every Windows wheel, so there would be no usable artifact to publish.A missing import, found alongside
openmc/__init__.pyusessys.platforminget_extra_libraries()but never importssys. It only resolves becausefrom openmc.material import *leaks the name, that module doingimport sysand having no__all__. That is fragile, it would break the momentmaterial.pygained an__all__, and the new code needssystoo, so the import is now explicit.Packaging
cp312,cp313,cp314win_amd64added to[tool.cibuildwheel] build. Worth calling out: that table is an allowlist, so without this nothing is built for Windows regardless of what else is configured. It is the easiest piece to overlook.[tool.cibuildwheel.windows], vanilla and serial.delvewheelis the auditwheel equivalent: it bundles the HDF5 DLLs intoopenmc.libsand injects theos.add_dll_directorycalls that a plain build does not.get_extra_libraries()needs no change, since its non-darwin branch already resolves toopenmc.libs, which is delvewheel's default output directory.build-wheels-windows.yml, mirroring the Linux workflow, uploading awindows-wheelsartifact withif-no-files-found: errorso a silent no-build cannot look like success.Serial for now:
/openmp:llvmstill rejects four constructs insrc/, so threaded Windows is separate work. This means single-threaded transport, which is a real limitation for a Monte Carlo code and worth being explicit about, but it is what makes a first wheel shippable.The Windows
test-commanddeliberately omitsopenmc --version, because the console script written bycmake/GenerateScript.cmakeis POSIX only. That remains open.Testing
cibuildwheel already runs
test-commandinside the build job, but that runs on the machine that has vcpkg and the source tree. The separatetestjob installs the repaired wheel into a clean interpreter across 3.12, 3.13 and 3.14 with no checkout and no vcpkg, which is what actually proves delvewheel bundled HDF5 rather than the runner happening to have it. It then reads back aconstflag and a namespaced global through ctypes:Those are precisely the two export cases that were broken before #129 and #130, so they are worth asserting rather than just importing.
Linux and macOS configuration is untouched.