-
Notifications
You must be signed in to change notification settings - Fork 345
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
Fixes to get Qt5 working on Windows. #872
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -419,11 +419,14 @@ win32-g++ { | |
expat: PKGCONFIG += expat | ||
cairo: PKGCONFIG += cairo fontconfig | ||
equals(QT_MAJOR_VERSION, 5) { | ||
shiboken: INCLUDEPATH += $$PYTHON_SITE_PACKAGES/PySide2/include/shiboken2 | ||
pyside: INCLUDEPATH += $$PYTHON_SITE_PACKAGES/PySide2/include/PySide2 | ||
pyside: INCLUDEPATH += $$PYTHON_SITE_PACKAGES/PySide2/include/PySide2/QtCore | ||
pyside: INCLUDEPATH += $$PYTHON_SITE_PACKAGES/PySide2/include/PySide2/QtGui | ||
pyside: INCLUDEPATH += $$PYTHON_SITE_PACKAGES/PySide2/include/PySide2/QtWidgets | ||
shiboken: INCLUDEPATH += $$system(pkg-config --variable=includedir shiboken2) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that makes pkg-config a build requirement. Will that still be possible if we switch to - say - vcpkg? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There already is a dependency on pkg-config. I'm basically doing the same thing that the Qt4 block is doing below. I'm not really familiar with vcpkg but I suspect it would be trivial to get similar info from whatever it uses to describe where the package include directory is. FWIW, as a side project, I've been trying to get various Natron components to build with the Conan package manager (https://conan.io/). I have prototypes of openfx-misc and openfx-io building on Windows, Linux, & Mac all from the same source. Part of why I was trying to get Qt5 working is because I didn't want to port Qt4 to conan and I knew you folks were trying to migrate to Qt5 anyways. For conan, at least, it does support pkg-config and will allow you to actually build it from source if needed. Conan will automatically generate the .pc files for all your dependencies if needed. This is usually only required for automake/make builds though because it uses more direct means with cmake. |
||
PYSIDE_INCLUDEDIR = $$system(pkg-config --variable=includedir pyside2) | ||
pyside: INCLUDEPATH += $$PYSIDE_INCLUDEDIR | ||
pyside: INCLUDEPATH += $$PYSIDE_INCLUDEDIR/QtCore | ||
pyside: INCLUDEPATH += $$PYSIDE_INCLUDEDIR/QtGui | ||
pyside: INCLUDEPATH += $$PYSIDE_INCLUDEDIR/QtWidgets | ||
shiboken: PKGCONFIG += shiboken2 | ||
pyside: PKGCONFIG += pyside2 | ||
} | ||
equals(QT_MAJOR_VERSION, 4) { | ||
shiboken: PKGCONFIG += shiboken-py$$PYV | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid double curly braces, which may confuse indenteds. I would prefer having two separate function definitions (will make the change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you think about something like this instead
My thinking here is that it keeps the main logic that is common in a single place and keeps the Qt version differences minimal and in their appropriate #if sections. I don't have a strong feeling here, just trying to address your concern and avoid duplicate code. It's too bad I can't use std::optional. It would be a little cleaner looking.