Conversation
Implicit declarations silently truncate pointer-returning libc calls to 32-bit int — exactly the bug that produced the Linux x64 Debug segfaults fixed in 553ec21. Make the build fail loudly the next time a POSIX/libc call sneaks into a translation unit without a visible prototype. Cross-platform flags: - gcc / clang (Linux, Darwin): -Werror=implicit-function-declaration - MSVC (Windows): /we4013 (promotes warning C4013 — "function undefined; assuming extern returning int" — to error) C++ is unaffected: implicit declarations are already illegal in the language, so the flag is C-only (added to CMAKE_C_FLAGS only). Also fix src/legacy/output/CMakeLists.txt with C_EXTENSIONS ON so swmm_output.c's fseeko()/ftello() are visible under glibc strict ANSI — mirrors the legacy-engine fix in 553ec21. Without this the Werror would block the Linux build. Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Honors the documented project policy (PHASE2_HANDOFF_2026-04-26.md:66-68):
-fno-fast-math is non-negotiable, regression tolerance is +/-0.001 abs
and +/-0.1% rel per timestep, not bit-exact across architectures. The
preset changes here are policy-preserving — no numerics change — and
fix three inconsistencies the audit surfaced.
1. Replace -ffloat-store (Linux only, an x87 holdover that is a no-op
on x86_64) with -ffp-contract=off (Linux + Darwin). FMA fusion is
the real source of cross-platform drift today; -ffp-contract=off is
the modern cross-vendor gate. MSVC /fp:precise already covers it.
2. Add -fno-math-errno on gcc/clang. Lets libm calls lower to compiler
builtins (often inlined / vectorized). Bit-identical math results;
only post-libm errno semantics change. Apple clang has this default
already; making it explicit aligns gcc.
3. Add /Gy /Gw to Windows Release for section-stripping parity with
-fdata-sections -ffunction-sections on Linux/Darwin. The matching
/OPT:REF /OPT:ICF linker flags are already in place — they had
nothing below the OBJ level to strip until now.
Also reorganizes each preset so CMAKE_C_FLAGS carries config-invariant
policy (warnings, FP, visibility) and CMAKE_C_FLAGS_<CONFIG> carries
only optimization (O2/LTO/section-split or O0/g/DDEBUG). Eliminates the
"both fields hold the full flag list" duplication that caused
-ffloat-store to silently end up Linux-only in the first place. Net
compile line is unchanged for policy flags; only the structure differs.
Out of scope and intentionally not added: -O3, -march=native,
-funroll-loops, -fno-signed-zeros, /fp:strict. Reasons enumerated in
/Users/calebbuahin/.claude/plans/i-want-the-compiler-virtual-popcorn.md.
Verification: macOS arm64 Darwin-debug rebuilt clean (no
argument-unused / no implicit-decl errors), CMakeCache reflects new
flags, build.ninja shows the new flags on the legacy swmm5.c compile
line, full 42-test unit suite green.
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the build/packaging toolchain to produce more self-contained installs (and wheels) by bundling runtime shared-library dependencies, and refactors CI to build Python wheels via cibuildwheel across more platforms.
Changes:
- Add CMake helpers to bundle/install runtime dependencies for CLI/engine installs and to stage runtime deps for test executables.
- Switch Python wheel CI to cibuildwheel (and adjust wheel metadata: version tag, supported Python versions).
- Update presets/workflows to tighten FP/implicit-decl policies and to enable GeoPackage in test/build matrices.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Adds runtime-dependency bundling helpers and reorders test/benchmark subdir inclusion. |
CMakePresets.json |
Updates compiler flags/preset descriptions; adds new *-tests-release presets and enables geopackage in test presets. |
src/engine/CMakeLists.txt |
Expands install RPATH search paths and installs selected imported runtime deps into bin/. |
src/cli/CMakeLists.txt |
Bundles runtime deps at install time; stages runtime DLLs for build-tree execution on Windows. |
src/legacy/cli/CMakeLists.txt |
Aligns install destination, adds bundling at install time, and stages runtime DLLs on Windows. |
src/legacy/output/CMakeLists.txt |
Enables C extensions for this target to expose fseeko/ftello under strict C17 builds. |
tests/CMakeLists.txt |
Introduces openswmm_stage_test_runtime_deps() for staging runtime deps next to test executables. |
tests/unit/engine/CMakeLists.txt |
Calls runtime-dep staging helper for unit test executables (and selected extra targets). |
tests/unit/legacy/engine/CMakeLists.txt |
Calls runtime-dep staging helper for legacy solver unit tests. |
tests/unit/legacy/output/CMakeLists.txt |
Calls runtime-dep staging helper for legacy output unit test. |
tests/regression/CMakeLists.txt |
Uses the staging helper for regression test runtime deps (replaces prior inline Windows-only logic). |
python/pyproject.toml |
Updates Python version support and cibuildwheel configuration (manylinux_2_28 + vcpkg bootstrap in-container). |
python/CMakeLists.txt |
Adds an sdist “metadata-only” guard with an actionable fatal error message. |
python/scripts/repair_wheel_windows.py |
Removes bespoke Windows wheel repair script (superseded by cibuildwheel config). |
.github/workflows/unit_testing.yml |
Enables geopackage feature/flag in unit-testing CI and verifies bundled runtime libs after install. |
.github/workflows/regression_testing.yml |
Runs tests in Release (via new presets), verifies bundled runtime libs, adds sdist job. |
.github/workflows/unit_testing_python.yml |
Replaces bespoke wheel build flow with cibuildwheel matrix; adds separate sdist job. |
.github/workflows/deployment.yml |
Switches wheel build to cibuildwheel matrix; adds sdist job and updates release dependencies. |
.github/workflows/scorecard.yml |
Updates push branch trigger. |
.github/workflows/documentation.yml |
Updates branch triggers. |
.github/workflows/codeql.yml |
Updates branch triggers. |
tests/unit/legacy/engine/data/hotstart/*.rpt |
Updates timestamp/elapsed-time lines in report outputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+181
to
+183
| set(_script_in "${CMAKE_SOURCE_DIR}/cmake/BundleRuntimeDeps.cmake.in") | ||
| set(_script_out "${CMAKE_CURRENT_BINARY_DIR}/BundleRuntimeDeps-${TARGET_NAME}.cmake") | ||
| configure_file("${_script_in}" "${_script_out}" @ONLY) |
Comment on lines
+224
to
+225
| /host/vcpkg/vcpkg install \ | ||
| --triplet "$(uname -m)-linux" \ |
Comment on lines
+35
to
+39
| "Wheels are published for Python 3.9-3.13 on Linux x86_64, macOS " | ||
| "(arm64 and x86_64), and Windows x64. If pip fell back to this " | ||
| "sdist, no wheel matched your platform/interpreter -- check the " | ||
| "available files at https://pypi.org/project/openswmm/#files and, " | ||
| "if needed, switch to a supported Python version.\n" |
Comment on lines
+314
to
+316
| Analysis begun on: Sat May 23 20:06:20 2026 | ||
| Analysis ended on: Sat May 23 20:06:20 2026 | ||
| Total elapsed time: < 1 sec No newline at end of file |
Comment on lines
8
to
10
| Analysis begun on: Sat May 23 20:06:20 2026 | ||
| Analysis ended on: Sat May 23 20:06:20 2026 | ||
| Total elapsed time: < 1 sec No newline at end of file |
Comment on lines
+8
to
+9
| Analysis begun on: Sat May 23 20:06:21 2026 | ||
| Analysis ended on: Sat May 23 20:06:21 2026 |
Comment on lines
+314
to
+315
| Analysis begun on: Sat May 23 20:06:28 2026 | ||
| Analysis ended on: Sat May 23 20:06:28 2026 |
Comment on lines
314
to
316
| Analysis begun on: Sat May 23 20:06:21 2026 | ||
| Analysis ended on: Sat May 23 20:06:21 2026 | ||
| Total elapsed time: < 1 sec No newline at end of file |
| else() | ||
| # Unix: copy each imported third-party shared lib (when defined) | ||
| # next to the exe. Targets that don't exist in this configuration | ||
| # are silently skipped via $<TARGET_NAME_IF_EXISTS:…>. |
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
…vcpkg triplet/cmake pin, Windows repair helper)
- CMakeLists.txt: PRE_EXCLUDE_REGEXES alternation converted to char-class
form. CMake regex is case-sensitive but PE imports report system DLLs
uppercase (ACLUI.dll, KERNEL32.DLL), so the earlier lowercase pattern
silently failed to filter them. Verified with cmake -P that all case
variants now match and engine/SUNDIALS/HDF5 DLLs still pass through.
- python/pyproject.toml [tool.cibuildwheel.windows]:
- repair-wheel-command: replaced {package}/scripts/... with the literal
relative path python/scripts/cibw_repair_windows.py. cibuildwheel
only substitutes {wheel} and {dest_dir} for repair — verified by
reading cibuildwheel source. {package} and {project} pass through
literally and Python fails with [Errno 2] No such file.
- version: 6.0.0a1 -> 6.0.0.dev2
- python/scripts/cibw_repair_windows.py: docstring documents the
verified placeholder rules so this trap doesn't recur.
The bundle template substituted OPENSWMM_BUNDLE_PRE_EXCLUDES (a bracket-
quoted list with semicolons) directly into file(GET_RUNTIME_DEPENDENCIES
PRE_EXCLUDE_REGEXES ...). CMake passed the entire bracketed string as
ONE argument, treating it as a single giant regex that matched nothing.
Result: api-ms-*, libc.*, all Windows system DLL excludes have been
silently inactive since the bundle script was written. This is why
'unresolved runtime dep: api-ms-...' kept appearing in CI even though
api-ms-.* was in the exclude list, and why the Windows aclui.dll
'Multiple conflicting paths' error persisted across multiple regex fix
attempts.
Fix: in the template, capture the @-substituted value into a CMake
variable first, then dereference with ${...} when passing to file().
CMake's list-aware expansion at the dereference site splits on ; into
separate arguments, which file() then applies as individual regexes.
Verified end-to-end by running the configure_file'd generated script
against a binary with libc as a dep PLUS a duplicate libc in the
staging dir (mimicking the exact aclui-in-staging conflict scenario).
Bracket-quoted form: libc resolves through, no filter. Fixed form:
libc filtered, no conflict.
The bundle template's file(GET_RUNTIME_DEPENDENCIES) only searched
${CMAKE_INSTALL_PREFIX}/{bin,lib}. Transitive deps of openswmm.engine.dll
(sundials_cvode.dll, sundials_core.dll, hdf5.dll, plus their own runtime
deps) live in vcpkg's installed/<triplet>/bin and were never findable, so
they showed up as 'unresolved runtime dep' warnings and never made it into
the cpack zip's bin/.
Fix: capture VCPKG_INSTALLED_DIR/<triplet>/bin at configure time and
substitute it into the bundle template's DIRECTORIES via a new
@OPENSWMM_BUNDLE_EXTRA_DIRS@ placeholder, dereferenced through a CMake
list variable so the path gets passed as its own argument to file().
Fallback to $ENV{VCPKG_ROOT}/installed/<triplet>/bin if manifest-mode
vars aren't set. Empty list (no-op) for non-vcpkg builds — Linux/macOS
typically resolve these via rpath anyway.
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Templates have C++ linkage by definition and cannot be declared inside an extern "C" block — GCC rejects with 'error: template with C linkage'. Move the anonymous-namespace block containing for_each_pattern_name_ref above the extern "C" opening so the helper has its proper C++ linkage while the C-API exports retain theirs.
Windows refuses to delete a file while any handle is open — POSIX
unlink-on-open does not apply. Both TagsTest.InpWriterEmitsTagsSection
and TagsTest.NoTagsSectionWhenAllEmpty opened an ifstream and then
called fs::remove while the stream was still in scope, causing:
The process cannot access the file because it is being used by
another process.
Move the ifstream into an inner block so the handle is released before
the remove. Harmless on Linux/macOS.
…ing pattern test - test_report_section.cpp: DisabledWritesSummaryOk had the same Windows bug as TagsTest — std::ifstream still in scope when std::remove ran. Fixed by scoping the ifstream into an inner block so its handle is released before remove. Same pattern as commit e6956eb. - tests/unit/engine/CMakeLists.txt + test_pattern_mutation_api.cpp: the CMakeLists entry was added without committing the .cpp source. Adding both together so CI can find the new test.
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Windows CI was failing in deployment.yml's build job and the unit_testing_python.yml/cibuildwheel Windows wheel job at: CMake Error ... Could NOT find SQLite3 src/engine/input/geopackage/CMakeLists.txt:18 (find_package) Root cause: OPENSWMM_WITH_GEOPACKAGE defaulted ON in CMakeLists.txt but sqlite3 was gated behind the 'geopackage' vcpkg feature which is not in default-features. Only unit_testing.yml passed VCPKG_MANIFEST_FEATURES=geopackage explicitly; every other workflow ran a vcpkg install without sqlite3 and CMake's find_package(SQLite3 REQUIRED) failed. Linux/macOS happened to work because system sqlite is available; Windows runners have no system sqlite. Move sqlite3[rtree] out of the geopackage feature and into top-level 'dependencies' so it's installed by every vcpkg invocation. The geopackage feature still declares the same dependency (vcpkg dedups) so unit_testing.yml's existing VCPKG_MANIFEST_FEATURES setting continues to work unchanged.
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.com>
Signed-off-by: cbuahin <caleb.buahin@gmail.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
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.
No description provided.