Fix Windows builds and rely on MinGW64 (#28)

* Do static builds instead of shared * shared on unix and static on win * fix lib name * openblas * Allow setting to build static * Optionally build static for Unix * more openblas * var * move import * move again * Build win wheels * always --import-mode=importlib * xml cov * No more static builds... need to be shared for ctypes * revert * move * editable install * link against numpy * style * dll * Use numpy * no copy * missing va * no numpy * load openbloas * fix * win * old * move * fix syntax * use full_path * full path * use bin * joinpath * just copy in ci * echo * load from C:/msys64/mingw64/bin * no pfapack/libopenblas.dll * Add common BLAS paths * Update README
basnijholt · Dec 4, 2024 · 7e3f3c9 · 7e3f3c9
1 parent ba556e7
commit 7e3f3c9
Show file tree

Hide file tree

Showing 7 changed files with 117 additions and 108 deletions.
diff --git a/.github/workflows/build_wheels.yml b/.github/workflows/build_wheels.yml
@@ -14,8 +14,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        # TODO: Fix `windows-latest`
-        os: [ubuntu-latest, macos-13, macos-14]
+        os: [ubuntu-latest, macos-13, macos-14, windows-latest]
 
     steps:
       - uses: actions/checkout@v4

diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml
@@ -62,11 +62,11 @@ jobs:
 
     - name: Install test dependencies
       run: |
-        pip install ".[test]"
+        pip install -e ".[test]"
 
     - name: Run Python tests
       run: |
-        pytest tests --cov --cov-append --cov-report=term-missing
+        pytest tests
 
     - name: Upload coverage to Codecov
       if: matrix.os == 'ubuntu-latest' && matrix.python-version == '3.13'

diff --git a/README.md b/README.md
@@ -11,17 +11,18 @@ Code and algorithms are taken from [arXiv:1102.3440](https://arxiv.org/abs/1102.
 [![Ruff](https://img.shields.io/endpoint?url=https://raw.githubusercontent.com/astral-sh/ruff/main/assets/badge/v2.json)](https://github.com/astral-sh/ruff)
 
 ### Install
-Recommended way (because it includes faster C/FORTRAN bindings)
+
 ```bash
-conda install -c conda-forge pfapack
+pip install pfapack
 ```
 
-Alternatively use
+Or using conda:
 ```bash
-pip install pfapack
+conda install -c conda-forge pfapack
 ```
 
 ## Usage
+
 ```python
 from pfapack import pfaffian as pf
 import numpy.matlib
@@ -31,32 +32,33 @@ A = A - A.T
 pfa1 = pf.pfaffian(A)
 pfa2 = pf.pfaffian(A, method="H")
 pfa3 = pf.pfaffian_schur(A)
-
 print(pfa1, pfa2, pfa3)
 ```
 
-If installed with `conda`, C/FORTRAN code is included with Python bindings, use it like:
+The package includes optimized C/FORTRAN implementations that can be used for better performance:
 ```python
 from pfapack.ctypes import pfaffian as cpf
-
 pfa1 = cpf(A)
 pfa2 = cpf(A, method="H")
-
 print(pfa1, pfa2)
 ```
 
+> [!WARNING]
+> On Windows, the C bindings require MSYS2 to be installed with the MinGW64 toolchain. The current Windows build system has some limitations and requires external dependencies. We welcome contributions to improve the Windows build system, such as using Microsoft's toolchain (MSVC) directly or finding better ways to handle the OpenBLAS dependency.
+
 ## Citing
+
 If you have used `pfapack` in your research, please cite it using the following `bib` entry:
 ```
 @article{wimmer2012algorithm,
-  title={Efficient numerical computation of the pfaffian for dense and banded skew-symmetric matrices},
-  author={Michael Wimmer},
-  journal={ACM Transactions on Mathematical Software (TOMS)},
-  volume={38},
-  number={4},
-  pages={1--17},
-  year={2012},
-  publisher={ACM New York, NY, USA}
+    title={Efficient numerical computation of the pfaffian for dense and banded skew-symmetric matrices},
+    author={Michael Wimmer},
+    journal={ACM Transactions on Mathematical Software (TOMS)},
+    volume={38},
+    number={4},
+    pages={1--17},
+    year={2012},
+    publisher={ACM New York, NY, USA}
 }
 ```
 

diff --git a/meson.build b/meson.build
@@ -9,63 +9,60 @@ project('pfapack',
   version: run_command('pfapack/_version.py', check: true).stdout().strip(),
 )
 
-# Get compilers
+# === Compilers and basic dependencies ===
 c_compiler = meson.get_compiler('c')
 fortran_compiler = meson.get_compiler('fortran')
-
-# Dependencies
 thread_dep = dependency('threads')
 m_dep = c_compiler.find_library('m', required: false)  # math library
 
+# === Python setup ===
+py = import('python').find_installation(pure: false)
+
+# === Platform specific BLAS/LAPACK setup ===
 if host_machine.system() == 'darwin'
+  # Use Apple's Accelerate framework
   add_project_link_arguments('-framework', 'Accelerate', language: ['c', 'fortran'])
   lapack_dep = declare_dependency()
   blas_dep = declare_dependency()
+
 elif host_machine.system() == 'windows'
   # On Windows, find OpenBLAS which includes LAPACK
   openblas_lib = dependency('openblas', required: false)
   if not openblas_lib.found()
     # Fallback to manual detection
-    compiler = meson.get_compiler('c')
-    openblas_lib = compiler.find_library('libopenblas',
+    openblas_lib = c_compiler.find_library('libopenblas',
       dirs: ['C:/msys64/mingw64/lib'],
       required: true)
   endif
   # OpenBLAS includes both BLAS and LAPACK
   lapack_dep = declare_dependency(dependencies: [openblas_lib])
   blas_dep = declare_dependency(dependencies: [openblas_lib])
+else # Linux
+  # Try multiple methods to find OpenBLAS
+  openblas_dep = dependency('openblas', required: false)
+  if not openblas_dep.found()
+      # Fallback to manual detection
+      openblas_dep = c_compiler.find_library('openblas', required: false)
+  endif
+
+  if openblas_dep.found()
+      # OpenBLAS includes both BLAS and LAPACK
+      blas_dep = declare_dependency(dependencies: [openblas_dep])
+      lapack_dep = declare_dependency(dependencies: [openblas_dep])
+  else
+      # Fall back to separate BLAS and LAPACK
+      blas_dep = c_compiler.find_library('blas', required: true)
+      lapack_dep = c_compiler.find_library('lapack', required: true)
+  endif
 
-else
   # Add rt library for clock_gettime
   rt_dep = c_compiler.find_library('rt', required: false)
   if rt_dep.found()
     add_project_link_arguments('-lrt', language: ['c', 'fortran'])
   endif
-
-    # Add rt library for clock_gettime
-    rt_dep = c_compiler.find_library('rt', required: false)
-    if rt_dep.found()
-        add_project_link_arguments('-lrt', language: ['c', 'fortran'])
-    endif
-
-    # Try multiple methods to find OpenBLAS
-    openblas_dep = dependency('openblas', required: false)
-    if not openblas_dep.found()
-        # Fallback to manual detection
-        openblas_dep = c_compiler.find_library('openblas', required: false)
-    endif
-
-    if openblas_dep.found()
-        # OpenBLAS includes both BLAS and LAPACK
-        blas_dep = declare_dependency(dependencies: [openblas_dep])
-        lapack_dep = declare_dependency(dependencies: [openblas_dep])
-    else
-        # Fall back to separate BLAS and LAPACK
-        blas_dep = c_compiler.find_library('blas', required: true)
-        lapack_dep = c_compiler.find_library('lapack', required: true)
-    endif
 endif
 
+# === Source files ===
 pfapack_dir = 'original_source'
 
 # Fortran sources in correct order matching the Makefile.FORTRAN
@@ -159,15 +156,13 @@ c_sources = files(
   join_paths(pfapack_dir, 'c_interface', 'skbtrd.c'),
 )
 
-# Import Python module
-py = import('python').find_installation(pure: false)
-
+# === Build libraries ===
 # Build Fortran library
 libpfapack = shared_library('pfapack',
   fortran_sources,
   dependencies: [lapack_dep, blas_dep, thread_dep, m_dep],
   install: true,
-  install_dir: py.get_install_dir() / 'pfapack',  # Install in package directory
+  install_dir: py.get_install_dir() / 'pfapack',
 )
 
 # Include directories
@@ -183,22 +178,25 @@ libcpfapack = shared_library('cpfapack',
   link_with: libpfapack,
   dependencies: [lapack_dep, blas_dep, thread_dep, m_dep],
   install: true,
-  install_dir: py.get_install_dir() / 'pfapack',  # Install in package directory
+  install_dir: py.get_install_dir() / 'pfapack',
 )
 
-# Python module
+# === Python module installation ===
+py_sources = [
+  'pfapack/__init__.py',
+  'pfapack/_version.py',
+  'pfapack/ctypes.py',
+  'pfapack/exceptions.py',
+  'pfapack/pfaffian.py',
+]
+
 py.install_sources(
-  ['pfapack/__init__.py',
-   'pfapack/_version.py',
-   'pfapack/ctypes.py',
-   'pfapack/exceptions.py',
-   'pfapack/pfaffian.py'],
+  py_sources,
   pure: false,
   subdir: 'pfapack'
 )
 
-
-# Build test executables
+# === Tests ===
 if get_option('build_tests').allowed()
   # C interface tests
   c_test_sources = files(

diff --git a/pfapack/ctypes.py b/pfapack/ctypes.py
@@ -8,7 +8,6 @@
 from __future__ import annotations
 
 import ctypes
-import sys
 import os
 from pathlib import Path
 from typing import Final
@@ -24,57 +23,65 @@
 
 
 def _find_library() -> ctypes.CDLL:
-    """Find and load the PFAPACK C library.
-
-    Returns
-    -------
-    ctypes.CDLL
-        The loaded library.
-
-    Raises
-    ------
-    OSError
-        If the library cannot be found or loaded.
-    """
+    """Find and load the PFAPACK C library."""
     _folder: Final = Path(__file__).parent
     _build_folder: Final = _folder.parent / "build"
 
-    if sys.platform == "darwin":
-        lib_name = "libcpfapack.dylib"
-    elif sys.platform == "win32":
-        lib_name = "libcpfapack.dll"
-    else:
-        lib_name = "libcpfapack.so"
+    # On Windows, ensure OpenBLAS and its dependencies can be found
+    if os.name == "nt":
+        # Common locations for OpenBLAS on Windows
+        possible_blas_paths = [
+            Path("C:/msys64/mingw64/bin"),  # MSYS2 MinGW64
+            Path("C:/msys64/ucrt64/bin"),  # MSYS2 UCRT64
+            Path("C:/msys64/clang64/bin"),  # MSYS2 Clang64
+            Path(os.environ.get("OPENBLAS_PATH", "")).parent
+            / "bin",  # Custom installation
+            Path(os.environ.get("CONDA_PREFIX", "")) / "Library" / "bin",  # Conda
+        ]
 
-    # List of all possible paths
-    possible_paths = [_folder / lib_name]  # Regular install
-
-    # Add build directories for editable install
-    if _build_folder.exists():
-        for p in _build_folder.glob("*"):
-            if p.is_dir():
-                possible_paths.append(p / lib_name)
-
-    if sys.platform == "win32":
-        # Add all paths to DLL search path
-        for path in possible_paths:
-            if path.parent.exists():
+        # Try to find and load OpenBLAS from any of these locations
+        blas_loaded = False
+        for path in possible_blas_paths:
+            if path.exists():
                 try:
-                    os.add_dll_directory(str(path.parent))
-                except OSError:
-                    pass  # Ignore if directory can't be added
-
-        # Try loading just by filename first (Windows-specific behavior)
-        try:
-            return ctypes.CDLL(lib_name)
-        except OSError:
-            pass
+                    os.add_dll_directory(str(path))  # type: ignore[attr-defined]
+                    openblas_path = path / "libopenblas.dll"
+                    if openblas_path.exists():
+                        ctypes.CDLL(str(openblas_path))
+                        blas_loaded = True
+                        break
+                except OSError as e:
+                    print(f"Warning: Failed to load OpenBLAS from {path}: {e}")
+
+        if not blas_loaded:
+            print("Warning: Could not load OpenBLAS from any known location")
+
+    # Try all possible library names
+    lib_names = [
+        "cpfapack.dll",
+        "libcpfapack.dll",
+        "libcpfapack.so",
+        "libcpfapack.dylib",
+    ]
 
-    # Try all possible full paths
+    # List of all possible paths
+    possible_paths = []
+    for lib_name in lib_names:
+        possible_paths.append(_folder / lib_name)
+        # Add build directories for editable install
+        if _build_folder.exists():
+            for p in _build_folder.glob("*"):
+                if p.is_dir():
+                    possible_paths.append(p / lib_name)
+
+    # Try all possible paths
     errors = []
     for path in possible_paths:
         try:
-            return ctypes.CDLL(str(path))
+            if path.exists():
+                return ctypes.CDLL(str(path))
+            else:
+                errors.append(f"{path}: File does not exist")
         except OSError as e:
             errors.append(f"{path}: {e}")
             continue
@@ -84,10 +91,10 @@ def _find_library() -> ctypes.CDLL:
         [
             "Could not load PFAPACK library.",
             "Attempted paths:",
-            *[f"  {e}" for e in errors],
+            *[f" {e}" for e in errors],
             f"Current directory: {os.getcwd()}",
             f"Package directory: {_folder}",
-            f"Python path: {sys.path}",
+            f"Files in package directory: {list(_folder.glob('*'))}",
         ]
     )
     raise OSError(error_msg)

diff --git a/pyproject.toml b/pyproject.toml
@@ -79,10 +79,13 @@ ignore_errors = true
 [tool.pytest.ini_options]
 addopts = """
     --durations=5
+    --cov
     --cov=pfapack
     --cov-append
     --cov-fail-under=30
     --cov-report=term-missing
+    --cov-report=xml
+    --import-mode=importlib
 """
 norecursedirs = ["docs"]
 

diff --git a/tests/test_ctypes_detailed.py b/tests/test_ctypes_detailed.py
@@ -15,7 +15,7 @@
     with_ctypes = False
 
 
-# @pytest.mark.skipif(not with_ctypes, reason="the libs might not be installed")
+@pytest.mark.skipif(not with_ctypes, reason="the libs might not be installed")
 def test_ctypes_real_different_sizes():
     """Test real matrices of different sizes."""
     for n in [2, 4, 8, 16, 32]: