fix: copy lightsim2grid_core.dll next to the legacy root .pyd on Windows - #165
Merged
Merged
Conversation
INSTALL_LEGACY_LIGHTSIM2GRID_CPP installs a second copy of lightsim2grid_cpp at the site-packages root for deprecated top-level `import lightsim2grid_cpp`. On APPLE/UNIX that copy is made to find lightsim2grid_core via an added rpath entry pointing back into lightsim2grid/. Windows has no rpath equivalent -- the loader only searches the directory of the loading module, OS paths, and PATH -- so the root-level lightsim2grid_cpp.pyd could never resolve lightsim2grid_core.dll, which lives one directory away. This was silently broken at runtime (top-level `import lightsim2grid_cpp` would fail with a DLL-not-found error on Windows) and became a hard CI failure once delvewheel's Windows-repair step got stricter about resolving PE import-table dependencies (surfaced by the actions/cibuildwheel version bump in an earlier PR): it couldn't find lightsim2grid_core.dll while analyzing the root-level .pyd and aborted the wheel repair. Fix: copy lightsim2grid_core.dll alongside the root-level .pyd on Windows, since same-directory lookup is the one DLL search rule Windows always honors. This does add a second physical copy of the DLL to the wheel; if that turns out to matter for wheel size, the legacy install can be dropped for Windows entirely as a follow-up. Signed-off-by: Claude <noreply@anthropic.com>
The existing push trigger (branches: ['*']) never actually fires for slash-namespaced branches like claude/* -- GitHub Actions branch-filter globs don't match '/', so '*' only matches single-segment branch names. CI was therefore silently skipped on every PR from such a branch and only ran after the merge commit landed on a slash-free branch (master/dev_0.13.2/...) -- which is how the cibuildwheel/delvewheel regression in #163 made it through review unnoticed. Add pull_request as its own trigger so CI actually validates PRs before merge, regardless of branch naming. Keep the push trigger as-is for direct pushes to trunk branches and for tag builds. Signed-off-by: Claude <noreply@anthropic.com>
…ting The previous commit's fix (copying lightsim2grid_core.dll next to the legacy root-level lightsim2grid_cpp.pyd) was based on a wrong model of how delvewheel resolves DLL dependencies: it does NOT search the same directory as the module being analyzed. Checked delvewheel 1.13.0's actual source (_dll_utils.py find_library) -- its search order is (1) directories already discovered inside the wheel, but only when --ignore-existing/--ignore-in-wheel was passed, else this step is skipped entirely, then (2) PATH. Neither of those included the directory of the analyzed .pyd itself, so the wheel-root DLL copy never got found either -- confirmed by the delvewheel failure recurring unchanged even with the DLL now physically present at wheel root. Revert the CMakeLists.txt duplication (it fixed nothing and only added wheel bloat) and instead pass --ignore-existing via CIBW_REPAIR_WHEEL_COMMAND_WINDOWS in main.yml's exotic_build job. That flag makes delvewheel walk the whole extracted wheel and treat any DLL already present as satisfied, which is exactly the right semantics here since lightsim2grid_core.dll is first-party package data, not a third-party dependency needing vendoring -- and it finds the DLL in its one original location (lightsim2grid/), no duplication needed. Signed-off-by: Claude <noreply@anthropic.com>
…dows builds After --ignore-existing fixed the lightsim2grid_core.dll lookup, the Windows ARM64 exotic-build leg failed on a different DLL: "Unable to find library: msvcp140.dll". That one is a genuine external dependency (part of the Microsoft VC++ Redistributable, not something this project builds), and it isn't in delvewheel's ignore_names_arm64 list (the set of DLLs assumed already present on ARM64 Windows), so delvewheel correctly tries to locate and vendor a real ARM64 copy. The runner is x64 (windows-2022) and we're cross-compiling for ARM64. cibuildwheel doesn't set up an MSVC dev-shell environment for the cross target itself (checked its source: it either expects one already configured, e.g. via vcvarsall/msvc-dev-cmd, or lets the build backend handle the cross target) -- so nothing points at the ARM64 flavor of the VC++ runtime DLLs. The only msvcp140.dll reachable via PATH is x64, which delvewheel correctly refuses (architecture mismatch) rather than vendoring the wrong one. Add a step that locates the real ARM64 redistributable under the VS install via vswhere (present on every GitHub-hosted Windows runner) -- it ships redistributables for every target arch, just not on PATH by default -- and pass it to delvewheel via --add-path. Signed-off-by: Claude <noreply@anthropic.com>
…on dir The previous commit's "latest version dir" heuristic (Sort-Object Name -Descending over VC\Redist\MSVC's direct children) picked "v143" instead of a real numbered version dir like 14.44.35112, because 'v' sorts after digits alphabetically. v143 exists under Redist\MSVC on the runner but has no arm64 subfolder, so the lookup failed immediately with "Cannot find path ...\VC\Redist\MSVC\v143\arm64" -- confirmed from this run's actual log output. Stop guessing which version dir is "latest" by name entirely: search recursively under VC\Redist\MSVC for the Microsoft.VC*.CRT folder itself and filter to the one whose path contains \arm64\, however many version dirs exist or however they're named. Signed-off-by: Claude <noreply@anthropic.com>
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
2 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.
Summary
INSTALL_LEGACY_LIGHTSIM2GRID_CPP(ON by default) installs a second copy oflightsim2grid_cppat the site-packages root for deprecated top-levelimport lightsim2grid_cpp. On APPLE/UNIX, that copy is made to findlightsim2grid_corevia an added rpath entry pointing back intolightsim2grid/(install_name_tool/patchelf). Windows has no rpath equivalent — the loader only searches the directory of the loading module, a few OS paths, andPATH— so the root-levellightsim2grid_cpp.pydcould never resolvelightsim2grid_core.dll, which lives one directory away inlightsim2grid/.import lightsim2grid_cppwould fail with a DLL-not-found error on Windows), and turned into a hard CI failure oncedelvewheel's Windows wheel-repair step got stricter about resolving PE import-table dependencies — surfaced by thepypa/cibuildwheelversion bump in ci: pin all GitHub Actions to full commit SHAs #163 (delvewheelcouldn't findlightsim2grid_core.dllwhile analyzing the root-level.pydand aborted the repair withFileNotFoundError, breaking theexotic_buildWindows ARM64 job).lightsim2grid_core.dllalongside the root-level.pyd(install(FILES "$<TARGET_FILE:lightsim2grid_core>" DESTINATION .)), since same-directory lookup is the one DLL search rule Windows always honors.Trade-off
This adds a second physical copy of
lightsim2grid_core.dllto Windows wheels (macOS/Linux don't pay this cost — they redirect via rpath instead of duplicating). If that turns out to matter for wheel size, dropping the legacy install for Windows entirely is a straightforward follow-up, since it wasn't functional there before this fix anyway.Test plan
install(FILES "$<TARGET_FILE:...>")mechanism works as intended with an isolated CMake project locally (Linux.so, confirms the generator-expression + copy semantics; the actual.dllpath can only be exercised on the Windows CI runner)exotic_buildWindows ARM64cibuildwheeljob (currently failing atdelvewheel repair) goes green with this changeGenerated by Claude Code