Skip to content

Conversation

@liaboveall
Copy link
Contributor

Fix build issues and improve code consistency

Overview

This PR addresses compilation and installation issues while improving code consistency across Pyfhel modules. The changes focus on fixing build system compatibility with modern Python environments and harmonizing internal attribute handling patterns.

Changes Summary

1. PyPoly.pyx Improvements

  • Added imports for consistency:

    • from .Pyfhel import Pyfhel for type checking
    • from Pyfhel.Pyfhel cimport to_Scheme_t for unified enum conversion
  • Unified _scheme attribute handling:

    • Getter: Now returns to_Scheme_t(self._scheme) for consistency with PyCtxt.pyx and PyPtxt.pyx
    • Setter: Converts input via to_Scheme_t(new_scheme) then stores new_scheme.value to avoid Python-level dependency on C enum types
    • Deleter: Sets self._scheme = Scheme_t.none.value
  • Fixed Cython syntax issues:

    • Corrected indentation for property getter/setter/deleter function bodies and docstrings
    • Resolved "Expected an increase in indentation level" compilation errors
  • Impact: External API behavior remains unchanged; only internal _scheme handling is harmonized with other modules for better consistency and maintainability.

2. Build System Updates (pyproject.toml)

  • Updated setuptools dependency:

    • Changed from setuptools<=60.9 to setuptools>=68
    • Fixes compatibility with Python 3.12+ build chain
    • Resolves errors related to deprecated pkg_resources interface
  • Added CMake policy configuration:

    • Added CMAKE_POLICY_VERSION_MINIMUM = '3.5' to SEAL cmake options
    • Resolves CMake policy compatibility issues with third-party zlib/zstd libraries
    • Stabilizes the SEAL build process

3. Documentation Updates (README.md)

  • Added system dependency installation instructions:
    • Added conda-based installation: conda install -y -c conda-forge libxcrypt
    • Added apt-based alternative: sudo apt-get install -y libxcrypt-dev
    • Provides crypt.h header files required for extension compilation

Environment & Dependencies

The changes address common build failures on modern Linux distributions where the libxcrypt system library is required but not automatically available. The updated installation instructions in the README provide clear guidance for users to install the necessary system dependencies.

Testing

  • Builds successfully with Python 3.12
  • PyPoly module maintains API compatibility
  • SEAL backend compiles without CMake policy warnings
  • Installation works with provided dependency instructions

Fixes

  • Resolves build failures on systems missing libxcrypt headers
  • Fixes Cython compilation errors in PyPoly module
  • Addresses setuptools compatibility with modern Python versions
  • Eliminates CMake policy warnings during SEAL compilation

This PR ensures Pyfhel can be successfully built and installed in modern Python environments while maintaining backward compatibility and improving internal code consistency.

@ibarrond
Copy link
Owner

ibarrond commented Aug 20, 2025

Hi @liaboveall , it seems you're trying to make the MacOS installation work. I'm gonna merge the other PR to bump to Python 3.12 and at least have a working version for Windows & Unix. Can you rebase this PR once that change is done?

@liaboveall
Copy link
Contributor Author

Hi!@ibarrond,I’ve reverted the MacOS fix attempt. I’ll rebase once the Python 3.12 PR is merged.

@ibarrond
Copy link
Owner

@liaboveall merged! Feel free to rebase, and I strongly encourage you to try and debug the MacOS issue. If needed, I can grant you SSH access to the VM in the CI that runs the MacOS version.

@liaboveall
Copy link
Contributor Author

@ibarrond Thanks a lot for merging! I’ll rebase my PR on top of the latest main. I’d really appreciate SSH access to the MacOS CI VM — that would be very helpful for debugging the installation issue. Thanks again for your support!

@liaboveall
Copy link
Contributor Author

liaboveall commented Aug 21, 2025

@ibarrond I’ve managed to fix the installation issue on MacOS and have opened a new PR with the changes: [#265](#265). Thanks again for your support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants