Skip to content
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

Simplify python binding include handling #3268

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

karliss
Copy link
Member

@karliss karliss commented Nov 21, 2023

Your checklist for this pull request

Detailed description

Looks like shiboken6 slightly changed the rules of bindings.txt and now include-path entry doesn't allow multiple include directories. Generating multiple include-path entries, one for each directory seems to work fine both for shiboken6 an shiboken2.

I am not too happy about the way I handled macOS include path, but I guess it's no worse than what was already there for qt5 (likely due to similar reasons).

Test plan (required)

  • CI builds still work
  • Arch Linux qt6 with bundled rizin
  • Arch Linux qt5 with bundled rizin *
  • Arch Linux qt6 with rizin installed in random user dir
  • macOS homebrew qt6 bundled rizin Qt6 Python bindings - failure to build on macOS  #3188
  • NixOS with qt6 as reported by @chayleaf
  • build attempts on any other distro or OS are welcome

* had to comment out Py_LIMITED_API seems like shiboken2 isn't tested against latest python . Otherwise it error on this code in shiboken2/pep384impl.h :

// This definition breaks the limited API a little, because it re-enables the
// buffer functions.
// But this is no problem as we check it's validity for every version.

#define PYTHON_BUFFER_VERSION_COMPATIBLE    (PY_VERSION_HEX >= 0x03030000 && \
                                             PY_VERSION_HEX <  0x030AFFFF)
#if !PYTHON_BUFFER_VERSION_COMPATIBLE
# error Please check the buffer compatibility for this python version!
#endif

Closing issues

closes #3188

* remove duplicate code introduced in rizinorg#2952
* specify include path using  multiple include-path lines instead  instead of single line with with directories separated by ; or :
@karliss karliss mentioned this pull request Nov 21, 2023
3 tasks
@chayleaf
Copy link

chayleaf commented Nov 22, 2023

this builds fine for me on NixOS with Pyside6. And yes, Shiboken2 requires Python 3.10

@karliss karliss marked this pull request as ready for review November 22, 2023 22:50
@karliss karliss requested a review from XVilka November 22, 2023 22:50
@chayleaf
Copy link

As @lilyinstarlight mentioned, this fixes the Nix build on Linux, but not on Darwin.

@karliss
Copy link
Member Author

karliss commented Nov 28, 2023

@lilyinstarlight Can you retest with Nix on macOS? Tested that homebrew still works on my system. It ended up being based on QT6_INSTALL_PREFIX anyway, but at least now it's closer to what pyside is doing.

Definition of QT_FRAMEWORK_INCLUDE_DIR
https://code.qt.io/cgit/pyside/pyside-setup.git/tree/sources/pyside6/cmake/PySideSetup.cmake?h=6.6.1#n134
Usage:
https://code.qt.io/cgit/pyside/pyside-setup.git/tree/sources/pyside6/cmake/Macros/PySideModules.cmake?h=6.6&id=v6.6.0#n146

@lilyinstarlight
Copy link

Can you retest with Nix on macOS?

Yep, the latest commit seems to work great for building with Qt6 from Nix now. Thank you! ❤️

@karliss karliss merged commit a6af291 into rizinorg:dev Nov 29, 2023
8 checks passed
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.

Qt6 Python bindings - failure to build on macOS
4 participants