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

Add Python 3 support #502

Merged
merged 2 commits into from
May 20, 2020
Merged

Add Python 3 support #502

merged 2 commits into from
May 20, 2020

Conversation

HamedSabri-adsk
Copy link
Contributor

PR Description

This PR fixes various issues when building MayaUsd with Python 3. All changes are Python 2 and 3 compatible.

…Python 3. All changes are Python 2 and 3 compatible.
@HamedSabri-adsk HamedSabri-adsk added the build Related to building maya-usd repository label May 19, 2020
@HamedSabri-adsk HamedSabri-adsk requested review from kxl-adsk and mattyjams and removed request for kxl-adsk May 19, 2020 15:10
Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

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

A few tiny gripes, but otherwise looks good to me!

Comment on lines 71 to 82
if (DEFINED PYTHON_INCLUDE_DIR AND DEFINED PYTHON_LIBRARIES AND DEFINED Python_EXECUTABLE)
SET(PYTHON_INCLUDE_DIRS "${PYTHON_INCLUDE_DIR}")
SET(PYTHONLIBS_FOUND TRUE)
find_package(Python 2.7 EXACT REQUIRED COMPONENTS Interpreter)
# Use the Python module to find the python lib.
if(BUILD_WITH_PYTHON_3)
find_package(Python 3.7 EXACT REQUIRED COMPONENTS Interpreter)
else()
find_package(Python 2.7 EXACT REQUIRED COMPONENTS Interpreter)
endif()
if(NOT Python_Interpreter_FOUND)
set(PYTHONLIBS_FOUND FALSE)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that we've got find_package() calls both here and in cmake/python.cmake. Do we need all of this here? Could everything from lines 71-86 be replaced with simply include(cmake/python.cmake)? Or could it at least all be moved into cmake/python.cmake to avoid the redundancy?

I'd be ok with it going in as is for now, but it'd be nice to see that in a follow-up PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This block here from lines 71-76 is for our internal Maya build where we build maya-usd inside. We are passing in the 3 python vars (line 71) and then calling find_package() which doesn't really do anything since all the vars were set. Unless something bad happens in which case we reset (line 75). We cannot remove this, but it could probably be moved inside of cmake/python.cmake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point @mattyjams , I am planning to revisit python.cmake and some of the logic here. There is definitely some redundancy here but let's leave it for another PR.

Comment on lines +54 to +57
# QT_NO_KEYWORDS prevents Qt from defining the foreach, signals, slots and emit macros.
# this avoids overlap between Qt macros and boost, and enforces using Q_ macros.
set_target_properties(Qt5::Core PROPERTIES INTERFACE_COMPILE_DEFINITIONS QT_NO_KEYWORDS)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not obvious to me how these Qt changes are related to Python 3 support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Python 3 headers, there are certain identifiers such as 'signals' and/or 'slots' that conflict with Qt's identifiers. To get around this one needs to replace all special Qt keywords ( signals, slots, emit ) with their Q_ macro counterparts.

This is a known issue. Please see Qt's help ( Using Qt with 3rd Party Signals and Slots )

https://doc.qt.io/qt-5/signalsandslots.html#using-qt-with-3rd-party-signals-and-slots

Here is the error that one gets before the fix:

C:\Python37\include\object.h(448): error C2059: syntax error: ';'
error C2238: unexpected token(s) preceding ';'

and here is the line in question is PyType_Slot slots; / terminated by slot==0. */

typedef struct{
    const char* name;
    int basicsize;
    int itemsize;
    unsigned int flags;
    PyType_Slot *slots; /* terminated by slot==0. */
} PyType_Spec;

Comment on lines 19 to 21
# Python 2 and 3: forward-compatible
from builtins import range

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you should need this. range() exists in Python 2, it's just not a generator like it is in Python 3, and for the purposes of this test, the difference shouldn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I don't think it is a biggie but will get rid of line 20 in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see d10a527

@@ -20,6 +20,7 @@
Helper functions regarding Maya that will be used throughout the test.
"""

from __future__ import print_function
Copy link
Contributor

Choose a reason for hiding this comment

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

For the way print() is used below, I don't think you need this. It was only needed in build.py because of the parameter-style usage: print("Message:", msg).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Matt, I am not sure if I see the connection with build.py.

This is a separate python file and without this change almost all ufe tests fail.

Perhaps there is something I don't see?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that I don't think you should need this import statement. You do need the changes below to use print() as a function.

I didn't mean to imply that there was a connection to build.py. I just meant that because of the way build.py uses print(), you do need the from __future__ import print_function statement there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh I see now. My apologies for the confusion.

For the way print() is used below, I don't think you need this

which print() are you referring to? There are two in here:

print(sys.exc_info()[1])
print("Unable to load %s" % pluginName)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to highlight the difference in the single parameter version of print() vs. the multiple-parameter version, and how the behavior there is different between Python 2 and Python 3.

Single vs. Multi-parameter In Python 2:

> python --version
Python 2.7.10
> python -c "msg = 'This is a string'; print('Message: %s' % msg)"
Message: This is a string
> python -c "msg = 'This is a string'; print('Message:', msg)"
('Message:', 'This is a string')

Single vs. Multi-parameter In Python 3:

> python3 --version
Python 3.7.7
> python3 -c "msg = 'This is a string'; print('Message: %s' % msg)"
Message: This is a string
> python3 -c "msg = 'This is a string'; print('Message:', msg)"
Message: This is a string

Notice that the multi-parameter version in Python 2 prints tuple-like output, which we don't want. That's what requires importing from the future:

In Python 2:

> python -c "from __future__ import print_function; msg = 'This is a string'; print('Message:', msg)"
Message: This is a string

So since build.py is using the multi-parameter version, we need to import from the future to get the right output. Since mayaUtils.py is not, we should not need to import from the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattyjams Ughh, I didn't know this. Thanks, you convinced me.

Done. See d10a527

@kxl-adsk kxl-adsk merged commit 0681396 into dev May 20, 2020
@kxl-adsk kxl-adsk deleted the sabrih/MAYA-104403/add_support_python3 branch May 20, 2020 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to building maya-usd repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants