Merged
Conversation
- Add build-wheels.yml for automated wheel building: - Triggered on push to main, tags, and workflow_dispatch - Supports matching tags from cfd library releases - Builds wheels for Linux, macOS, and Windows - Uses cibuildwheel for cross-platform compatibility - Static links CFD library for self-contained wheels - Uploads artifacts for distribution - Publishes to PyPI on tagged releases
- Add CFD_USE_STABLE_ABI=ON to CIBW_ENVIRONMENT for stable ABI wheels - Expand PyPI publish condition to support: - GitHub release events - Direct tag pushes (v*) - workflow_dispatch on tags (triggered from cfd repo) This enables the cfd repo's version-release workflow to trigger Python package builds and PyPI publishing.
actions/checkout@v4 doesn't allow paths outside the workspace. Changed from `../cfd` to `cfd` subdirectory within workspace.
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive GitHub Actions CI/CD workflow for building, testing, and publishing Python wheels for the cfd-python package. The workflow automates the entire release process from building cross-platform wheels to publishing on PyPI and Test PyPI.
Key changes:
- Multi-platform wheel building (Ubuntu, Windows, macOS) with Python 3.8-3.12 support via cibuildwheel
- CFD library version coordination between the Python bindings and the C library sibling repository
- Automated testing pipeline that validates wheels across all supported platforms and Python versions
- Conditional publishing to PyPI (on releases/tags) and Test PyPI (on main branch pushes)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
.github/workflows/build-wheels.yml
Outdated
Comment on lines
+88
to
+110
| env: | ||
| CIBW_BUILD: cp38-* cp39-* cp310-* cp311-* cp312-* | ||
| CIBW_SKIP: "*-musllinux_* pp*" | ||
| CIBW_ARCHS_WINDOWS: AMD64 | ||
| CIBW_ARCHS_MACOS: x86_64 arm64 | ||
| CIBW_ARCHS_LINUX: x86_64 | ||
|
|
||
| # Environment for static linking and stable ABI | ||
| CIBW_ENVIRONMENT: "CMAKE_BUILD_TYPE=Release CFD_STATIC_LINK=ON CFD_USE_STABLE_ABI=ON" | ||
|
|
||
| # Test the built wheels | ||
| CIBW_TEST_COMMAND: > | ||
| python -c "import cfd_python; print('CFD Python version:', cfd_python.__version__); print('Solvers:', cfd_python.list_solvers())" | ||
|
|
||
| # macOS wheel repair (static linking handles most deps) | ||
| CIBW_REPAIR_WHEEL_COMMAND_MACOS: delocate-wheel --require-archs {delocate_archs} -w {dest_dir} {wheel} | ||
|
|
||
| # Linux wheel repair | ||
| CIBW_REPAIR_WHEEL_COMMAND_LINUX: auditwheel repair -w {dest_dir} {wheel} | ||
|
|
||
| # Windows - no repair needed with static linking | ||
| CIBW_REPAIR_WHEEL_COMMAND_WINDOWS: "" | ||
|
|
There was a problem hiding this comment.
The cibuildwheel configuration in this workflow file duplicates and overrides the settings already defined in pyproject.toml. The workflow defines CIBW_BUILD, CIBW_SKIP, architecture settings, and environment variables that are already configured in the pyproject.toml file (lines 88-138).
This creates a maintenance problem where the same configuration exists in two places and can get out of sync. It also means the workflow ignores the platform-specific before-build commands defined in pyproject.toml (lines 108-138) which build the CFD library, instead attempting to build it separately before running cibuildwheel.
Consider removing the duplicate CIBW_* environment variables (lines 89-109) and relying on the configuration in pyproject.toml, or if workflow-specific overrides are needed, document why they differ from the defaults.
Suggested change
| env: | |
| CIBW_BUILD: cp38-* cp39-* cp310-* cp311-* cp312-* | |
| CIBW_SKIP: "*-musllinux_* pp*" | |
| CIBW_ARCHS_WINDOWS: AMD64 | |
| CIBW_ARCHS_MACOS: x86_64 arm64 | |
| CIBW_ARCHS_LINUX: x86_64 | |
| # Environment for static linking and stable ABI | |
| CIBW_ENVIRONMENT: "CMAKE_BUILD_TYPE=Release CFD_STATIC_LINK=ON CFD_USE_STABLE_ABI=ON" | |
| # Test the built wheels | |
| CIBW_TEST_COMMAND: > | |
| python -c "import cfd_python; print('CFD Python version:', cfd_python.__version__); print('Solvers:', cfd_python.list_solvers())" | |
| # macOS wheel repair (static linking handles most deps) | |
| CIBW_REPAIR_WHEEL_COMMAND_MACOS: delocate-wheel --require-archs {delocate_archs} -w {dest_dir} {wheel} | |
| # Linux wheel repair | |
| CIBW_REPAIR_WHEEL_COMMAND_LINUX: auditwheel repair -w {dest_dir} {wheel} | |
| # Windows - no repair needed with static linking | |
| CIBW_REPAIR_WHEEL_COMMAND_WINDOWS: "" |
shaia
force-pushed
the
feature/ci-cd
branch
2 times, most recently
from
9f75323 to
7678151
Compare
- Remove duplicate cibuildwheel config, rely on pyproject.toml - Use CIBW_ENVIRONMENT_PASS_* to override CFD_ROOT for CI - Remove redundant CFD library build steps (handled by before-build) - Use pattern: wheels-* for explicit artifact download - Download wheels and sdist separately for clarity - Add id-token: write permission for Test PyPI - Replace smoke tests with full pytest suite
The manylinux images already have cmake and gcc installed, so the before-all step attempting to install them was failing with exit code 127 (command not found) due to incorrect shell logic.
Use CIBW_ENVIRONMENT instead of CIBW_ENVIRONMENT_PASS_* to properly override the CFD_ROOT setting from pyproject.toml. The PASS variant only passes through variables but doesn't override existing settings.
Remove shell default syntax ${VAR:-default} in favor of direct $VAR
usage since the environment is set by cibuildwheel. Add echo to
show CFD_ROOT value for debugging CI failures.
Use cmake's -S (source) and -B (build) flags instead of cd to avoid path issues. The previous approach failed because the second cd command was relative to the already-changed directory.
Multi-config generators (Visual Studio, Xcode) output libraries to build/lib/<Config>/, while single-config generators (Make, Ninja) output directly to build/lib/. This fix searches both paths to support all build systems in CI.
When the C extension exists but fails to import, now raises ImportError with a helpful message instead of silently falling back to development mode. This surfaces the actual error (e.g., missing dependencies, ABI incompatibility) instead of causing confusing AttributeErrors later.
These are build artifacts that shouldn't be tracked: - wheelhouse/ - built wheel files from cibuildwheel - src/cfd_lib/ - local copy of CFD library created during builds
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Instead of building against cfd master (which may have untested changes), the workflow now fetches the latest cfd release tag via GitHub API. This ensures: - Stability: builds against tested, released code - Reproducibility: exact cfd version is a known release - No surprises: master breaking changes won't affect builds Falls back to master only if no releases exist yet.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Comment on lines
+100
to
+118
| except ImportError as e: | ||
| # Check if this is a development environment (source checkout without built extension) | ||
| # vs a broken installation (extension exists but fails to load) | ||
| import os as _os | ||
| _package_dir = _os.path.dirname(__file__) | ||
|
|
||
| # Look for compiled extension files | ||
| _extension_exists = any( | ||
| f.startswith('cfd_python') and (f.endswith('.pyd') or f.endswith('.so')) | ||
| for f in _os.listdir(_package_dir) | ||
| ) | ||
|
|
||
| if _extension_exists: | ||
| # Extension file exists but failed to load - this is an error | ||
| raise ImportError( | ||
| f"Failed to load cfd_python C extension: {e}\n" | ||
| "The extension file exists but could not be imported. " | ||
| "This may indicate a missing dependency or ABI incompatibility." | ||
| ) from e |
There was a problem hiding this comment.
The new import error handling logic lacks test coverage. This code distinguishes between development environments (no extension built) and broken installations (extension exists but fails to load), but there are no tests verifying this behavior.
Consider adding tests that verify:
- The error message and behavior when an extension file exists but fails to import
- The fallback behavior in development mode when no extension exists
Using branch references like @release/v1 is a security risk as maintainers could push breaking changes or malicious code. Pin to specific version tag for security.
- Add tests for import error handling logic - Update conftest.py to fail in CI if extension not available (instead of silently skipping tests) - Update test_integration.py to handle dev mode gracefully - Clarify CIBW_ENVIRONMENT behavior in workflow comments
- Remove CMAKE_C_VISIBILITY_PRESET hidden setting that was causing PyInit_cfd_python to be hidden despite PyMODINIT_FUNC - Disable INTERPROCEDURAL_OPTIMIZATION which can strip symbols during link-time optimization This fixes "dynamic module does not define module export function" errors when importing the built wheel.
- Show wheel contents (looking for .so/.pyd) before installing - Add detailed extension import debugging in verify step - Try direct import of cfd_python.cfd_python to catch import errors
- Add wheel.py-api = "cp38" for abi3 wheel tags - Move CFD_USE_STABLE_ABI to cmake.define for consistency This should produce cp38-abi3 wheels that work with Python 3.8+ instead of version-specific cp38-cp38 wheels.
The extension was being named cfd_python.cpython-38-x86_64-linux-gnu.so but abi3 wheels need cfd_python.abi3.so for cross-version compatibility. Python 3.9+ couldn't find the module because it looks for either: - cfd_python.cpython-39-x86_64-linux-gnu.so (version-specific) - cfd_python.abi3.so (stable ABI) Now explicitly set: - Linux/macOS: .abi3.so suffix - Windows: .abi3.pyd suffix
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (1)
pyproject.toml:104
- [nitpick] The
environmentsetting at the global[tool.cibuildwheel]level definesCFD_ROOT = "../cfd", but this is overridden byCIBW_ENVIRONMENTin the GitHub Actions workflow (line 80). However, the platform-specific sections no longer define their ownenvironment, which could cause issues if cibuildwheel is run outside of GitHub Actions (e.g., locally). The previous version had platform-specific environment settings that have been removed. Consider either:
- Keeping the global default for local builds
- Or documenting that
CFD_ROOTmust be set when building locally
# Global environment - static linking for self-contained wheels
# CFD_ROOT can be set to override the default ../cfd location
# CFD_USE_STABLE_ABI is enabled for wheel builds to support multiple Python versions
environment = { CMAKE_BUILD_TYPE = "Release", CFD_STATIC_LINK = "ON", CFD_USE_STABLE_ABI = "ON", CFD_ROOT = "../cfd" }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Comment on lines
+125
to
+141
| def test_real_module_loads_successfully(self): | ||
| """Test that the real cfd_python module loads without error""" | ||
| # This verifies the actual installed module works | ||
| import cfd_python | ||
|
|
||
| # Should have version | ||
| assert hasattr(cfd_python, '__version__') | ||
| assert cfd_python.__version__ is not None | ||
|
|
||
| # Should have core functions | ||
| assert hasattr(cfd_python, 'list_solvers') | ||
| assert callable(cfd_python.list_solvers) | ||
|
|
||
| # list_solvers should return a non-empty list | ||
| solvers = cfd_python.list_solvers() | ||
| assert isinstance(solvers, list) | ||
| assert len(solvers) > 0 |
There was a problem hiding this comment.
The test test_real_module_loads_successfully assumes the extension is always built and available (line 141: assert len(solvers) > 0). However, this test could fail in development mode where the extension isn't built. Consider either:
- Making this test conditional on extension availability (like
test_integration.pydoes) - Or marking it to skip in dev mode with a decorator like
@pytest.mark.skipif(not hasattr(cfd_python, 'list_solvers'), reason="Extension not built")
Comment on lines
+143
to
+183
| def test_extension_detection_logic(self, tmp_path): | ||
| """Test that extension detection correctly identifies .pyd and .so files""" | ||
| test_dir = tmp_path / "test_detection" | ||
| test_dir.mkdir() | ||
|
|
||
| # Test with no extension files | ||
| files = list(test_dir.iterdir()) | ||
| has_extension = any( | ||
| f.name.startswith('cfd_python') and (f.name.endswith('.pyd') or f.name.endswith('.so')) | ||
| for f in files | ||
| ) | ||
| assert not has_extension | ||
|
|
||
| # Test with .pyd file | ||
| (test_dir / "cfd_python.cp311-win_amd64.pyd").touch() | ||
| files = list(test_dir.iterdir()) | ||
| has_extension = any( | ||
| f.name.startswith('cfd_python') and (f.name.endswith('.pyd') or f.name.endswith('.so')) | ||
| for f in files | ||
| ) | ||
| assert has_extension | ||
|
|
||
| # Clean and test with .so file | ||
| (test_dir / "cfd_python.cp311-win_amd64.pyd").unlink() | ||
| (test_dir / "cfd_python.cpython-311-x86_64-linux-gnu.so").touch() | ||
| files = list(test_dir.iterdir()) | ||
| has_extension = any( | ||
| f.name.startswith('cfd_python') and (f.name.endswith('.pyd') or f.name.endswith('.so')) | ||
| for f in files | ||
| ) | ||
| assert has_extension | ||
|
|
||
| # Test with unrelated .so file (should not match) | ||
| (test_dir / "cfd_python.cpython-311-x86_64-linux-gnu.so").unlink() | ||
| (test_dir / "other_module.so").touch() | ||
| files = list(test_dir.iterdir()) | ||
| has_extension = any( | ||
| f.name.startswith('cfd_python') and (f.name.endswith('.pyd') or f.name.endswith('.so')) | ||
| for f in files | ||
| ) | ||
| assert not has_extension |
There was a problem hiding this comment.
The extension detection test doesn't account for stable ABI extension filenames (.abi3.so and .abi3.pyd), which are set in CMakeLists.txt lines 122 and 125 when CFD_USE_STABLE_ABI is enabled. The detection logic in cfd_python/__init__.py (lines 107-110) checks for files ending with .pyd or .so, which would match .abi3.pyd and .abi3.so, but the test should verify this works correctly. Consider adding test cases for stable ABI extension filenames:
# Test with stable ABI .pyd file
(test_dir / "cfd_python.abi3.pyd").touch()
# ... verify detection works
# Test with stable ABI .so file
(test_dir / "cfd_python.abi3.so").touch()
# ... verify detection works| 1. Extension exists but fails to load (broken installation) | ||
| 2. No extension exists (development mode) | ||
| """ | ||
| import os |
There was a problem hiding this comment.
Import of 'os' is not used.
Suggested change
| import os |
| """ | ||
| import os | ||
| import sys | ||
| import tempfile |
There was a problem hiding this comment.
Import of 'tempfile' is not used.
Suggested change
| import tempfile |
| import os | ||
| import sys | ||
| import tempfile | ||
| import shutil |
There was a problem hiding this comment.
Import of 'shutil' is not used.
Suggested change
| import shutil |
Windows Python does not recognize .abi3.pyd as a valid extension suffix. Unlike Linux/macOS which support .abi3.so, Windows only looks for: - .cp38-win_amd64.pyd (version-specific) - .pyd (generic) The abi3 compatibility is indicated by the wheel tag (cp38-abi3-win_amd64), not the extension filename on Windows.
The "DLL load failed" error was caused by the extension depending on vcruntime DLLs that may not be present on all systems. - Build both CFD library and Python extension with static MSVC runtime (MultiThreaded instead of MultiThreadedDLL) - This makes the wheel self-contained without requiring VC++ redistributable
Windows wheel builds have complex MSVC runtime and DLL dependency issues that need more investigation. Focus on Linux and macOS first to get the CI pipeline working. TODO: Re-enable Windows builds after resolving: - Static MSVC runtime linking - python3.lib stable ABI linking - Potential vcruntime dependencies
Build and test only cp312-manylinux_x86_64 to isolate issues faster. Will expand to other platforms/versions once this works.
Since we're using wheel.py-api = "cp38" for stable ABI, we should build with cp38 to create a proper abi3 wheel that works on 3.8+. Test on both 3.8 (native) and 3.12 (abi3 compatibility) to verify.
Start fresh with a simple workflow: - Direct python -m build instead of cibuildwheel - Build CFD library explicitly before wheel - No stable ABI complexity - Python 3.8 on Linux only Previous configs backed up to backups/
The test was importing from the source checkout instead of the installed wheel. Fixed by: - Running import test from /tmp to avoid source shadowing - Using sparse checkout to only get tests directory - Running pytest from /tmp pointing to tests
Build and test wheels on both Linux and macOS.
- Add CFD_USE_STABLE_ABI option to CMakeLists.txt with Py_LIMITED_API - Configure .abi3.so extension suffix for Linux/macOS - Add wheel.py-api = "cp38" to pyproject.toml - Test wheel on both Python 3.8 and 3.12 to verify abi3 compatibility
- Add windows-latest to build and test matrices - Split build/test steps into Unix and Windows variants - Use PowerShell syntax for Windows (dir, $env:, Get-ChildItem) - Use cross-platform Python for wheel inspection
On Windows with stable ABI, we need to link against python3.lib instead of the version-specific python38.lib. CMake's FindPython doesn't handle this automatically, so we find and link it explicitly.
- Add MSVC_RUNTIME_LIBRARY=MultiThreaded to statically link MSVC runtime - Build CFD library with same static runtime to avoid runtime mismatch - This eliminates dependency on VC++ redistributable
Remove static runtime override - both CFD library and Python extension need to use the same runtime. The default /MD (DLL runtime) is standard for Python extensions on Windows.
Prevents duplicate runs when pushing to a branch with an open PR. Push triggers only for main/master, PRs trigger for any branch.
Both CFD library and Python extension now use /MT (static runtime) to eliminate dependency on MSVC redistributable DLLs.
MinGW (GCC) produces more portable Windows binaries without MSVC runtime dependencies. This should fix the DLL load failures.
The egor-tensin/setup-mingw action has a bug with MinGW 15.x. Use chocolatey directly to install MinGW.
Add -static and -static-libgcc flags to eliminate dependency on MinGW runtime DLLs (libgcc, libwinpthread, etc.)
MinGW causes ABI incompatibility with MSVC-built Python. Add dumpbin diagnostic step to identify missing DLLs.
Python_add_library with WITH_SOABI always links to version-specific python3X.lib. For stable ABI on Windows, manually create the module and link against python3.lib to get python3.dll dependency.
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.