-
Notifications
You must be signed in to change notification settings - Fork 10
Harden vendored f90wrap path for Qt/QGIS compatibility #1051
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
8aebf0f to
69d1aa1
Compare
9b4a960 to
0adaaed
Compare
Additional Fix: Early Return for Fatal ErrorsAdded Before: ErrorHint set the error state, but execution continued to After: Subroutine returns immediately after setting error state, allowing Python to catch This completes the fix for #1035 - the SIGINT handler removal ensures Qt compatibility, and this change ensures fatal errors exit cleanly before hitting undefined math operations. |
- Remove SIGINT handler modification that conflicts with Qt signal handlers - Add vendored f2py_f90wrap.py with Qt-safe error handling - Add headless Qt tests for error handling validation - Add GitHub Actions workflow for Windows QGIS CI testing The f90wrap SIGINT handler (`PyOS_setsig`) conflicts with Qt's signal handlers on Windows, causing QGIS to crash when SUEWS errors occur. This fix removes the SIGINT modification while preserving the setjmp/longjmp error handling mechanism. Trade-off: Ctrl+C during Fortran calls must wait for call to return (~ms), but Qt/QGIS compatibility is gained.
For pull_request and workflow_dispatch triggers, build supy from source to validate fixes before TestPyPI release. Only use TestPyPI for workflow_run (after nightly builds).
- Add test_pyqgis_error_handling.py for testing inside actual QGIS context - Merge QGIS tests into build-publish workflow (tests actual UMEP wheel) - Remove standalone test-qgis-windows.yml (superseded by integrated job)
- Remove redundant Qt-only tests (test_qt_*.py) - Keep only test_pyqgis_error_handling.py (most comprehensive) - Add --ignore for test/qgis in cibuildwheel (run separately) - Simplify test_qgis job to single test step The PyQGIS test uses actual QgsApplication context, making it more realistic than standalone Qt tests for validating GH-1035. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
OSGeo4W doesn't expose pip directly in PATH. Use python -m pip instead to install the UMEP wheel. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
OSGeo4W Python doesn't include pip by default. Use ensurepip to bootstrap pip before installing the UMEP wheel. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Set PYTHONPATH to include QGIS Python bindings (OSGeo4W v2 has no qgis_env.bat) - Add qgis_test_only workflow input for fast iteration using TestPyPI wheels instead of building from source - Skip builds when qgis_test_only is enabled 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add standalone test script that initialises QGIS in headless mode and verifies SUEWS error handling works correctly without crashing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Separate workflow that uses TestPyPI wheels directly - no building. Trigger via Actions > Test QGIS Windows Compatibility > Run workflow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use Qgis.version() instead of QgsApplication.version() - the latter doesn't exist in QGIS 3.x API. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Set QT_QPA_PLATFORM=offscreen at shell level before running Python, ensuring Qt sees the variable before any initialization. More defensive than relying solely on Python os.environ setting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use quoted set syntax for proper empty string handling in Windows CMD. The quoted form `set "VAR=value"` correctly handles empty values. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add early exit in SUEWS_cal_multitsteps when supy_error_flag is set - Fix SUEWSKernelError being caught as RuntimeError subclass - Re-raise kernel errors without wrapping in run_supy_ser - Scan all timesteps for errors (handles early exit case) Previously, fatal errors would repeat for every remaining timestep (288 times for a 1-day simulation). Now exits after first error. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove commented-out debugging code: - Removed commented print statement for temp directory path - Removed commented pdb.set_trace() in pack_var function - Removed extensive debug comparison code in pack_grid_dict - Removed redundant else:pass clause 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The UMEP wheel already has numpy>=1.22,<2.0 baked into its metadata from the build action, so explicit constraint is unnecessary. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The main test file uses Qgis.version() which works on all platforms. The macOS variant was created during debugging with the older Qgis.QGIS_VERSION attribute and is no longer needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The PyQGIS error handling test is really about UMEP wheel compatibility, not QGIS itself. Consolidate with existing UMEP tests for cleaner organisation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove test_imports.py: All imports already tested in other modules - Merge test_postprocessor.py into test_processor.py: Single test for config output path fits better with processor tests - Update __init__.py to reflect consolidated structure Reduces test files from 7 to 5 and removes ~90 lines of redundancy. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Merge test_environment.py, test_preprocessor.py, and test_processor.py into test_umep_api.py. All three test files share the same skip logic (Windows + Python 3.12 only) and test UMEP API compatibility. Final structure (4 files, 474 lines): - conftest.py: Shared skip logic - test_umep_api.py: All UMEP API tests (skipped unless Windows + Py3.12) - test_pyqgis_error_handling.py: PyQGIS error handling (runs in CI) Reduces from 660 to 474 lines (-186 lines). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Each test module now has its own pytestmark for skip logic. Removes conftest.py dependency and simplifies structure. Final structure (3 files, 449 lines): - __init__.py: Package docstring (5 lines) - test_umep_api.py: All UMEP API tests (331 lines) - test_pyqgis_error_handling.py: PyQGIS error handling (113 lines) Total reduction: 660 → 449 lines (-32%) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
OSGeo4W environment doesn't have pytest installed. The script runs standalone in CI (python test/umep/test_pyqgis_error_handling.py), so pytest import must be optional. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The `int | None` type hint syntax requires Python 3.10+. Adding `from __future__ import annotations` allows using this syntax in Python 3.9. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add SUEWS vendoring documentation to f2py_f90wrap.py explaining the SIGINT handler removal for Qt/QGIS compatibility - Extract QGIS_LTR_PYTHON_VERSION constant to test/umep/__init__.py for easier updates when QGIS LTR version changes - Update test skip reasons to use dynamic version string 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The sys module is still needed for tests that manipulate stdout/stderr. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The Fortran supy_error_flag persists between calls and must be reset before each kernel invocation. Without this reset, an error from a previous call would cause subsequent calls to exit immediately on the first timestep check. This was causing test failures where test_pyqgis_error_handling (which intentionally triggers an error) left the error flag set, causing subsequent tests using sample_config.yml to fail unexpectedly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add supy_version input to allow specifying exact version for qgis_test_only - Skip QGIS test gracefully when no UMEP variant available on TestPyPI - Replace emoji with ASCII in workflow output (FAIL/OK/WARN) - Optimise _check_supy_error_from_state to walk backwards from last timestep 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add test/umep/test_stop_interception.py to verify gfortran STOP handlers are present in the compiled extension - Add symbol inspection step to build-publish_to_pypi.yml and test-qgis.yml workflows - Checks for: suews_stop_string, suews_stop_numeric, _gfortran_stop_string, _gfortran_stop_numeric, f90wrap_abort_ Part of GH-1035 validation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When zzd < 0, ErrorHint sets the error state but execution continued to the next line which computes LOG(zzd/z0m). LOG of negative values causes a floating-point exception that crashes the process before Python can catch the SUEWSKernelError. This fix adds RETURN after the ErrorHint call to exit cal_Stab subroutine immediately, allowing the error state to propagate back to Python and be raised as SUEWSKernelError. Fixes #1035 (QGIS crash when SUEWS error occurs) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ErrorHint now calls f90wrap_abort for fatal errors, which triggers longjmp back to the Python/C wrapper. This ensures: 1. Fatal errors jump directly to Python (skip entire Fortran call stack) 2. Python catches RuntimeError instead of process crash 3. No need for callers to add RETURN after ErrorHint This properly reintroduces the nonstopf2py approach from 2018, where Fortran errors are converted to Python exceptions via setjmp/longjmp. Also keeps the defensive RETURN after ErrorHint(32) in cal_Stab as an additional safety measure. Fixes #1035 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
7c00b9f to
6df4e9c
Compare
The vendored f90wrap containing STOP interception code was not being used during build. Three issues prevented it: 1. Missing __init__.py in vendored f90wrap package directory 2. meson.build used find_program() which found PyPI f90wrap 3. run_f2py.py couldn't execute .py scripts directly Changes: - Add __init__.py to src/supy/_vendor/f90wrap_src/f90wrap/ - Update meson.build to use f2py-f90wrap-patched.py directly - Update run_f2py.py to handle .py scripts with sys.executable This ensures STOP handler symbols (_suews_stop_string, _gfortran_stop_*, for_stop_core) are included in the compiled extension, enabling proper error propagation from Fortran to Python without crashes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Previously the test only warned when critical STOP intercept symbols were missing, allowing CI to pass even if vendored f90wrap wasn't used. Now the test: - FAILS if tools available AND critical symbols missing - SKIPS only if inspection tools (dumpbin/nm) unavailable - Returns exit code 2 for build errors in standalone mode This ensures CI catches build configuration errors where the vendored f90wrap with STOP handlers is not properly integrated. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Move test/umep/test_stop_interception.py to test/core/test_build_symbols.py - Make test cross-platform (nm works on macOS/Linux/Windows) - Remove Windows-only skip - now validates on ALL platforms - Mark as @pytest.mark.core for inclusion in cibw test step - Remove redundant RETURN in suews_phys_atmmoiststab.f95 (f90wrap_abort handles it) - Update CI workflow references to new test location The symbol validation now runs automatically via CIBW_TEST_COMMAND after each wheel build on all platforms, catching build issues early. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…-1035) The longjmp mechanism in f90wrap_abort may fail in Qt/QGIS context due to Qt's signal handling. Without RETURN, execution continues to LOG(negative) which causes a floating-point exception crash before Python catches the error. RETURN serves as a safety net - normally longjmp prevents reaching it, but when longjmp fails (Qt event loop interference), RETURN prevents the LOG(negative) crash. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The CI workflow runs this script directly in QGIS environment where pytest may not be installed. Make pytest import optional so main() can run standalone for symbol validation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…-1035) When f90wrap_abort triggers longjmp from Fortran, the C wrapper raises RuntimeError directly. However, Fortran sets the error state before calling f90wrap_abort. This fix checks that error state in the except block and converts the exception to SUEWSKernelError when present. This ensures the test_pyqgis_error_handling.py test can catch the error as SUEWSKernelError instead of RuntimeError. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The test triggered error code 14 from SUEWS_cal_RoughnessParameters instead of error code 32 from stability calculation. Both errors indicate the same invalid z < zdm configuration, so the test now accepts either error message. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use error code and simpler string patterns to avoid character encoding issues when comparing error messages on Windows. The '<' character may be encoded differently in Fortran strings passed through f90wrap on different platforms. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Root cause: Fortran I/O operations (WRITE statements, including internal writes) crash QGIS GUI due to incompatibility with Qt's event loop. Solution: - In cal_Stab: bypass ErrorHint entirely and call set_supy_error directly - In ErrorHint: wrap internal WRITE statements with #ifdef wrf guards - Remove all WRITE(*,*) statements from non-WRF error handling path - Use string concatenation instead of internal WRITE for StopMessage The error is now properly caught and returned to Python via the supy_error_flag mechanism without crashing QGIS. Tested successfully in QGIS 3.44.5 on Windows. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Improves the vendored f90wrap code generation to ensure Qt/QGIS compatibility by hardening the module path handling and removing conflicting SIGINT handler modifications.
Changes
Quick Reproduction
Fixes #1035