NatronGitHub / Natron

Open-source video compositing software. Node-graph based. Similar in functionalities to Adobe After Effects and Nuke by The Foundry.
http://NatronGitHub.github.io
GNU General Public License v2.0
4.56k stars 333 forks source link

Shiboken generation in QMake for Python bindings #803

Closed YakoYakoYokuYoku closed 2 years ago

YakoYakoYokuYoku commented 2 years ago

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)

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 commented 2 years ago

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

YakoYakoYokuYoku commented 2 years ago

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 commented 2 years ago

can you please:

devernay commented 2 years ago

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

YakoYakoYokuYoku commented 2 years ago
  • 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 commented 2 years ago

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

YakoYakoYokuYoku commented 2 years ago

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
YakoYakoYokuYoku commented 2 years ago

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

rodlie commented 2 years ago
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 commented 2 years ago

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

YakoYakoYokuYoku commented 2 years ago

PR #830 is up for reviews.