Skip to content

Comments

setup.py: fix macOS dylib packaging and runtime linking#707

Merged
neworderofjamie merged 5 commits intogenn-team:masterfrom
Agrim-P777:feature/macos_package_data
Aug 28, 2025
Merged

setup.py: fix macOS dylib packaging and runtime linking#707
neworderofjamie merged 5 commits intogenn-team:masterfrom
Agrim-P777:feature/macos_package_data

Conversation

@Agrim-P777
Copy link
Contributor

Summary

This PR improves cross-platform packaging and installation for pygenn, with a particular focus on macOS support.

Changes Made

  • Platform-specific packaging
    • Unified package_data and depends handling with if / elif / else logic:
      • Windows.dll
      • macOS.dylib
      • Linux/others.so
    • Backend libraries now follow the same pattern, ensuring .dylibs are copied into the package on macOS.
  • Runtime linking improvements
    • macOS: Added Wl,-rpath,@loader_path so extensions can locate bundled .dylibs after installation.
    • Linux: Preserved $ORIGIN in runtime_library_dirs so .sos can be found next to the extensions.
    • Linux/macOS: Explicitly link against ffi from the active environment.
  • Coverage builds
    • Fixed the typo MAC -> MACOS
  • Backend Packaging Logic
    • Updated backend packaging logic to include macos by using if / elif / else for clarity.
  • Project URL
    • Fixed incorrect GitHub URL (genn_teamgenn-team).

Impact

  • Fixes missing .dylib files on macOS installs.
  • Ensures runtime linking works consistently across Windows, Linux, and macOS.
  • Simplifies platform-specific logic for easier maintenance.
  • Corrects metadata (URL) to point to the right repository.

Copy link
Contributor

@neworderofjamie neworderofjamie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great - just a few minor formatting suggestions

setup.py Outdated
Comment on lines 125 to 127
# --- Linux/macOS libffi linkage ---
if LINUX or MACOS:
genn_extension_kwargs["libraries"].append("ffi")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else clause above (starting at line 115) is basically LINUX or MACOS so I think it would be cleaner to put this logic in there rather than adding yet another if

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes updated it

Agrim-P777 and others added 4 commits August 28, 2025 00:09
Co-authored-by: neworderofjamie <neworderofjamie@gmail.com>
Co-authored-by: neworderofjamie <neworderofjamie@gmail.com>
Co-authored-by: neworderofjamie <neworderofjamie@gmail.com>
@neworderofjamie neworderofjamie merged commit 565e12c into genn-team:master Aug 28, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants