-
Notifications
You must be signed in to change notification settings - Fork 202
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
EMSUSD-282 - MayaUsd: build with Qt directly from Maya devkit/runtime #3298
EMSUSD-282 - MayaUsd: build with Qt directly from Maya devkit/runtime #3298
Conversation
* New logic in build.py (during configure stage) to "setup Maya Qt" by looking for Qt headers in Maya devkit. If not found, extract either just the header from "qt*-include.zip" for Qt5 or the entire zipfile for Qt6. * Deprecated the "--qt-location" flag from build.py. * New cmake module "FindMaya_Qt" used for Qt5. * Use versionless Qt targets.
Chorus detected one or more security issues with this pull request. See the Checks tab for more details. As a reminder, please follow the secure code review process as part of the Secure Coding Non-Negotiable requirement. |
This look awesome, @seando-adsk ! Thank you for putting this together. It'll really help streamline builds |
CMakeLists.txt
Outdated
@@ -156,27 +156,49 @@ endif() | |||
|
|||
if(DEFINED QT_LOCATION) |
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.
Still support setting a custom Qt location, but I think after this PR is merged we should consider deprecating this cmake variable and outputting a warning/error if its used. MayaUsd plugin is required to run from Maya which has Qt, so building/using the Qt from Maya is the proper thing and will make sure there are no incompatible libraries.
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.
After this PR is merged I will fix our internal builds (preflight included) to no longer set this cmake variable. And thus our builds will fall into the else below.
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.
@dgovil What do you think about this section where a user is providing a custom built Qt to build MayaUsd with. Should I just remove this section and only use my new section below (currently the else). I can't think of a case where someone would want to build with their own Qt anymore now that we can build with the Qt from Maya devkit. I'm thinking of removing this section and outputting a cmake warning if QT_LOCATION is set, saying that flag is deprecated and not needed anymore.
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.
I agree with you. Having a custom Qt build has enough gotchas that I doubt anyone would use it, especially now that they can use the version inside Maya.
set(CMAKE_PREFIX_PATH "${MAYA_DEVKIT_LOCATION}/Qt") | ||
find_package(Qt6 6.5 COMPONENTS Core Gui Widgets QUIET) |
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.
The Maya devkit has been "fixed" for the switch to Qt6 by including a Qt zip that has all of Qt packaged up for use by cmake builds. All we need to do now is set the CMAKE_PREFIX_PATH and then call the normal find_package.
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.
I use QUIET here since this find is allowed to fail, meaning there is no Qt6 and we'll fall below and search for Qt5.
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.
This also relies on the new Qt.tar.gz
file being extracted this I do in build.py
.
CMakeLists.txt
Outdated
# Minimum version required is 5.15 (Maya 2022/2023/2024). | ||
# This will find Qt in the Maya devkit using a custom find package. | ||
# So the version will match Maya we are building for. | ||
find_package(Maya_Qt 5.15 COMPONENTS Core Gui Widgets QUIET) |
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.
For older Maya using Qt 5, I took the cmake rules that Dhruv wrote for issue #1070 and created a new find "Maya_Qt" module. This also relies on qt_5*-include.{zip/tar.gz}
being extracted which I do in build.py
. I again used QUIET here which means if the user building isn't using our build.py
and hasn't extracted the qt zip file it will fail to find Qt and will output a warning message (below).
CMakeLists.txt
Outdated
set(BUILD_WITH_QT_6 OFF) | ||
set(Qt_FOUND TRUE) | ||
else() | ||
message(WARNING "Could not find Qt in Maya devkit directory: ${MAYA_DEVKIT_LOCATION}. Building with Qt features will be disabled.") |
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 do you think about warning vs error here? I am open to changing this to error which means you either use our build.py
(which automatically extracts qt for you) or you have to manually extract Qt.
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.
I think an error would be better here. Now that it can find the Qt from the Maya versions, I don't see a scenario where someone would prefer to build without it (and also can't just modify the file).
I think an error would prevent any quiet discovery issues
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.
@dgovil Thanks I agree. I've removed the QT_LOCATION flag (no need to support building with custom Qt anymore) and output cmake error if Qt cannot be found in Maya devkit.
@@ -359,6 +362,99 @@ def RunMakeZipArchive(context): | |||
PrintError("Failed to write to directory {pkgDir} : {exp}".format(pkgDir=pkgDir,exp=exp)) | |||
sys.exit(1) | |||
|
|||
def SetupMayaQt(context): | |||
def haveQtHeaders(rootPath): |
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.
Helper function to search (recursively) to find the qt headers (only the 3 components we use). If found then we know the qt zip was extracted.
build.py
Outdated
files = [n for n in archive.namelist() | ||
if (n.startswith('QtCore/') or n.startswith('QtGui/') or n.startswith('QtWidgets/'))] |
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.
I'm only extracting the headers for the 3 components we use.
# Then we can simply use find_package(Qt6) on it. | ||
for dirToSearch in dirsToSearch: | ||
# Qt archive has same name on all platforms. | ||
qtArchive = os.path.join(dirToSearch, 'Qt.tar.gz') |
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.
For Qt6 extract the entire archive. This one is new and contains everything needed to build with Qt.
@@ -49,7 +49,7 @@ target_compile_definitions(${PROJECT_NAME} | |||
$<$<BOOL:${IS_LINUX}>:GL_GLEXT_PROTOTYPES> | |||
$<$<BOOL:${IS_LINUX}>:GLX_GLXEXT_PROTOTYPES> | |||
$<$<BOOL:${CMAKE_WANT_MATERIALX_BUILD}>:WANT_MATERIALX_BUILD> | |||
$<$<OR:$<BOOL:${Qt5_FOUND}>,$<BOOL:${Qt6_FOUND}>>:WANT_QT_BUILD> | |||
$<$<BOOL:${Qt_FOUND}>:WANT_QT_BUILD> |
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.
In CMakeLists.txt I set a generic Qt found variable.
else() | ||
set_target_properties(Qt5::Core PROPERTIES INTERFACE_COMPILE_DEFINITIONS QT_NO_KEYWORDS) | ||
endif() | ||
set_target_properties(Qt::Core PROPERTIES INTERFACE_COMPILE_DEFINITIONS QT_NO_KEYWORDS) |
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.
Since Qt 5.15 there are versionless targets.
@@ -0,0 +1,190 @@ | |||
# Module to find Qt in Maya devkit. |
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.
I documented this module, so I won't add extra comments here. Most of this came from what Dhruv wrote. I just made it generic and working on all platforms.
@dgovil Would you mind code reviewing the changes and trying it out on your build pipeline? |
Yep, will do. I won’t be able to get to it till Monday though, but at a
cursory glance, this all looks good to me.
…On Fri, Aug 25, 2023 at 2:01 PM Sean Donnelly ***@***.***> wrote:
@dgovil <https://github.com/dgovil> Would you mind code reviewing the
changes and trying it out on your build pipeline?
—
Reply to this email directly, view it on GitHub
<#3298 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABB4XUVK6RHLZTYURXOL6GLXXEHBDANCNFSM6AAAAAA365YCU4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Thanks. No rush. I've been working on these changes for quite a while now. Also working internally to get the devkit updated for Qt6. |
* Fix for security/bandit B202: tarfile_unsafe_members
This looks to be working well for me. I didn't have any code comments either other than agreeing the an error might be better than a warning if no Qt is found. Thanks again for putting this together :-) |
* Code review feedback - remove QT_LOCATION completely and cmake error if Qt cannot be found in Maya devkit.
if(NOT Qt_FOUND) | ||
message(SEND_ERROR "Could not find Qt in Maya devkit directory: ${MAYA_DEVKIT_LOCATION}.") | ||
if(MAYA_APP_VERSION VERSION_GREATER 2024) | ||
message(STATUS " You must extract Qt.tar.gz") | ||
else() | ||
if (IS_WINDOWS) | ||
message(STATUS " In Maya devkit you must extract include/qt_5.15.2_vc14-include.zip") | ||
else() | ||
message(STATUS " In Maya devkit you must extract include/qt_5.15.2-include.tar.gz") | ||
endif() | ||
message(STATUS "Setting Qt version to Qt ${QT_VERSION}") | ||
endif() | ||
set(CMAKE_PREFIX_PATH "${QT_LOCATION}") | ||
if(BUILD_WITH_QT_6) | ||
find_package(Qt6 ${QT_VERSION} COMPONENTS Core Gui Widgets REQUIRED) | ||
if(Qt6_FOUND) | ||
message(STATUS "Building with Qt ${QT_VERSION} features enabled.") | ||
endif() | ||
else() | ||
find_package(Qt5 ${QT_VERSION} COMPONENTS Core Gui Widgets REQUIRED) | ||
if(Qt5_FOUND) | ||
message(STATUS "Building with Qt ${QT_VERSION} features enabled.") | ||
endif() | ||
endif() | ||
else() | ||
message(STATUS "QT_LOCATION not set. Building Qt features will be disabled.") | ||
message(FATAL_ERROR "Cannot build MayaUsd without Qt.") |
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.
As per discussion with Dhruv if Qt cannot be found in Maya devkit, output a cmake error (stopping build). Also removed the QT_LOCATION flag as there is no need to build with a custom Qt anymore as we can always build with Qt from Maya devkit.
* Fixed python scripting error.
8f79042
to
547740d
Compare
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.
LGTM!
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.
Great work!
EMSUSD-282 - MayaUsd: build with Qt directly from Maya devkit/runtime
This will solve issue #1070 by finding and building with Qt from Maya devkit for all supported versions of Maya.