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

Shiboken generation in QMake for Python bindings #803

Merged
merged 6 commits into from Jun 30, 2022
Merged

Conversation

YakoYakoYokuYoku
Copy link
Member

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:

PR Description

What type of PR is this? (Check one of the boxes below)

  • Bug fix (non-breaking change which fixes an issue)

What does this pull request do?

Currently for the Python bindings we're relying on an static generation of them, but this introduces many problems regarding version compatibility with Python, Qt5, Shiboken2 and PySide2 plus platform dependent issues as well.
Instead this PR lets QMake do the job for us by the usage of targets and generated sources. Dependency on both sources and headers is supported too in case any one of them had been modified.

Show a few screenshots (if this is a visual change)

N/A.

Have you tested your changes (if applicable)? If so, how?

By building Natron.

Futher details of this pull request

Not yet tested with Qt4. This might fail on Windows (specially MsBuild) due to the use of some used Make semantics. May supersede #800.

@devernay
Copy link
Member

If this breaks with Qt4/QMake4, this PR should be against RB-2.6, because Natron 2.5 is with Qt4 and Python3.

@YakoYakoYokuYoku
Copy link
Member Author

YakoYakoYokuYoku commented Jun 17, 2022

Quoting from #800

QMake 4 seems to not cooperate with it though it works just fine with the in repo sources

QMake 4 as of now uses the generated sources at Engine/Qt4/NatronEngine and Gui/Qt4/NatronGui. I've tested in a VM box locally and things continue to build (same with CI), though I haven't done testing in Windows with Visual Studio projects or NMake where it could break due to the usage of a glob specific to Make (%_wrapper.cpp), as I cannot use a list of files there because in QMake custom targets only process one file at the time (always gives me all the spaces escaped).

@devernay
Copy link
Member

can you please:

  • rebase to RB-2.5
  • fix runShiboken.sh and runShiboken2.sh so that they error if they have less than 2 parameters
  • update INSTALL_LINUX.md and INSTALL_MACOS.md shiboken2 instructions. You'll see that the new runs are commented, just uncomment and remove the old run.
  • SHIBOKEN is not set on MacPorts with Qt5. I didn't dive deeper. You'll find my shiboken run in INSTALL_MACOS in the "On MacPorts with qt5, py310-pyside2".

@devernay devernay self-requested a review June 18, 2022 00:09
@devernay
Copy link
Member

also please make sure that runPostShiboken.sh and runPostShiboken2.sh pass shellcheck:

  • use bash instead of sh
  • use $() instead of `...`
  • double-quote most uses of shell variables. shellcheck will tell you where
  • remove use of PYV in runPostShiboken.sh

Copy link
Member

@devernay devernay left a comment

Choose a reason for hiding this comment

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

see comments above

tools/utils/runPostShiboken.sh Outdated Show resolved Hide resolved
@YakoYakoYokuYoku
Copy link
Member Author

  • update INSTALL_LINUX.md and INSTALL_MACOS.md shiboken2 instructions. You'll see that the new runs are commented, just uncomment and remove the old run.

I've removed the sections of manual Qt5 generation as it's done by QMake per the PR.

  • SHIBOKEN is not set on MacPorts with Qt5. I didn't dive deeper. You'll find my shiboken run in INSTALL_MACOS in the "On MacPorts with qt5, py310-pyside2".

This surely was produced by the absence of the generator_location variable in the MacPorts py310-shiboken2 PkgConfig file. For example Arch's shiboken2.pc looks like this

prefix=/usr
exec_prefix=/usr
libdir=lib
includedir=/usr/include/shiboken2
generator_location=/usr/bin/shiboken2 # <----- Here
python_interpreter=/usr/bin/python
python_include_dir=/usr/include/python3.10

Name: shiboken2
Description: Support library for Python bindings created with the Shiboken2 generator.
Version: 5.15.4
Libs:  -L${libdir} -lshiboken2.cpython-310-x86_64-linux-gnu
Cflags: -I/usr/include/python3.10 -I${includedir}/

@devernay
Copy link
Member

macports package doesn't have a shiboken2.pc nor a pyside2.pc file. Probably a packaging issue

@YakoYakoYokuYoku
Copy link
Member Author

Then either the macports Portfile needs to ship those .pc or provide the following config.pri

shiboken: SHIBOKEN = /usr/bin/shiboken2 # or wherever is located
pyside:   INCLUDEPATH += /usr/include/PySide2
pyside:   INCLUDEPATH += /usr/include/PySide2/QtCore
pyside:   INCLUDEPATH += /usr/include/PySide2/QtGui
pyside:   INCLUDEPATH += /usr/include/PySide2/QtWidgets
pyside:   TYPESYSTEMPATH += /usr/share/PySide2/typesystems

@devernay devernay merged commit 54368ab into RB-2.5 Jun 30, 2022
@YakoYakoYokuYoku
Copy link
Member Author

I'm having my doubts why would someone run manually run Shiboken2 with this PR 🤔, though Thanks for merging anyways.

@YakoYakoYokuYoku YakoYakoYokuYoku deleted the shiboken-gen-qmake branch July 2, 2022 19:49
@rodlie
Copy link
Contributor

rodlie commented Jul 22, 2022

Engine\Engine.pro:501: Unknown replace function: absolute_path
Engine\Engine.pro:501: Unknown replace function: absolute_path
Engine\Engine.pro:501: Unknown replace function: absolute_path
Engine\Engine.pro:501: Unknown replace function: absolute_path
Engine\Engine.pro:509: Unknown replace function: shell_path
Engine\Engine.pro:512: Unknown replace function: relative_path
Engine\Engine.pro:527: Unknown replace function: shell_path
Gui\Gui.pro:433: Unknown replace function: absolute_path
Gui\Gui.pro:433: Unknown replace function: absolute_path
Gui\Gui.pro:433: Unknown replace function: absolute_path
Gui\Gui.pro:433: Unknown replace function: absolute_path
Gui\Gui.pro:433: Unknown replace function: absolute_path
Gui\Gui.pro:433: Unknown replace function: absolute_path
Gui\Gui.pro:441: Unknown replace function: shell_path
Gui\Gui.pro:445: Unknown replace function: relative_path
Gui\Gui.pro:460: Unknown replace function: shell_path

Please don't break the qt4 stuff.

@YakoYakoYokuYoku
Copy link
Member Author

Damn, you are right! We need to revert this (at least partially), opening a PR then.

@YakoYakoYokuYoku
Copy link
Member Author

YakoYakoYokuYoku commented Jul 22, 2022

PR #830 is up for reviews.

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.

None yet

3 participants