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

Full debug (with debug python) build support #1478

Merged

Conversation

seando-adsk
Copy link
Contributor

Description of Change(s):

Full debug python support and portable USD builds:

  • Use @loader_path instead of absolute path for plugins and libraries. On Mac @loader_path is equivalent to $ORIGIN on Linux.
  • With the new changes, it would be possible for people to decide if they want to build USD with python debug/release.
  • One can now set this flag via both build_usd python script as well as CMake.
    • CMake flag is: -DPXR_DEFINE_BOOST_DEBUG_PYTHON_FLAG
    • build_usd flag is: --debug-python
  • Full python debug support.
    • Must build boost with python debugging flag.
    • On Windows when using debug python the libraries must have _d in name.
  • Option to not put full path of python executable in python shebang. For portable USD builds.

Added ability to use custom python

  • With new flag --build-python-info you can pass in which python to use rather than automatically finding system one.
  • Added ability to use custom PYSIDEUICBINARY.

Fixes Issue(s):

  • Allows full debug builds, built with debug python.
  • Makes builds portable.

pxr/imaging/hd/strategyBase.h Outdated Show resolved Hide resolved
@@ -17,7 +17,7 @@ pxr_library(tf
${TBB_tbb_LIBRARY}

INCLUDE_DIRS
${PYTHON_INCLUDE_DIR}
${PYTHON_INCLUDE_DIRS}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistent use of the "...DIRS" (vs DIR) variable.

Comment on lines +2111 to +2078
# Special case - we are given the PYSIDEUICBINARY as cmake arg.
usdBuildArgs = context.GetBuildArguments(USD)
given_pysideUic = 'PYSIDEUICBINARY' in " ".join(usdBuildArgs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pyside-uic has been deprecated by Qt. So added ability to pass in what to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is pyside-uic deprecated? I thought it was just getting complicated to distribute with py2 vs py3 and pyside1 vs pyside2.

The actual source of pyside-uic/pyside2-uic/etc is pretty small. The latest version I have just imports an object called uic and executes it. Older versions in pyside and pyside2 were pretty minimal as well, though not as short. I wonder if we could include our own pyside-uic with USD's build scripts and run it instead of making a user install extra packages and find their executable name. If we did that we could also get rid of all the name checking we do below this comment line.

This issue is related #1419

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes as Hamed mentions below uic is now capable of generating python code. Please see my change in "cmake/macros/Private.cmake" where I add the python generator if using uic. There is no more pyside2-uic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since pyside2-uic appears to be deprecated (although the latest PyPI package still seems to include it?), it seems like the build should recognize uic without it having to be passed in via a special build flag. So how about just adding uic to the list of programs in pyside2Uic instead of checking PYSIDEUICBINARY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we could add it to the list of executables to search for, but having a way to pass in a pyside executable to use is very helpful. This code assumes the one you want to use is in the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added 'uic' to list of pyside executables.

Comment on lines -1963 to -1948
if IsMayaPython():
if context.buildUsdview:
PrintError("Cannot build usdview when building against Maya's version "
"of Python. Maya does not provide access to the 'OpenGL' "
"Python module. Use '--no-usdview' to disable building "
"usdview.")
sys.exit(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At Autodesk we are using the python from Maya to build USD on all platforms. Maya has now added pip, so the missing OpenGL module needs to be installed before building USD.

Comment on lines 1623 to 1630
group.add_argument("--build-debug", dest="build_debug", action="store_true",
help="Build in Debug mode")

group.add_argument("--build-release", dest="build_release", action="store_true",
help="Build in Release mode")

group.add_argument("--build-relwithdebug", dest="build_relwithdebug", action="store_true",
help="Build in RelWithDebInfo mode")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra flags to build all the various config types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are mutually exclusive, could we move these settings into a mutually exclusive group and represent them with an enum-like value? We could follow the pattern for the --build-shared/--build-monolithic settings that are a few lines below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes good suggestion.

Comment on lines +1635 to +1620
group.add_argument("--build-python-info", type=str, nargs=4, default=[],
metavar=('PYTHON_EXECUTABLE', 'PYTHON_INCLUDE_DIR', 'PYTHON_LIBRARY', 'PYTHON_VERSION'),
help=("Specify a custom python to use during build"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flag to specify a custom python. We use this at Autodesk to pass in the Python from Maya, which from latest Maya can be either py2 or py3. I've seen other requests in usd interest to build with a custom python.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is running the script with Maya's Python insufficient? Or was this a way to get away from the Maya Python-specific code throughout the script?

I'm a bit wary of adding these variables here because I'd like to move USD's build away from the deprecated FindPythonLibs/FindPythonInterp modules in CMake and to the new FindPython2/FindPython3 modules, and I'm not sure these settings are supported or translatable to the new modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No unfortunately not. Maya 2022 is dual-python containing both python 2.7 and 3.7 and mayapy/mayapy2. The internal function IsMayaPython() no longer works with those and since it was marked with comments about not having any special Maya knowledge I decided not to modify it. But rather create a more generic ability to specify the python to build with via a new flag --build-python-info.

This allows a user to call the build_usd.py with any python (such as system) but pass in the python that you want USD to use to build the python bindings with.

I even recall a thread (usd-interest/github?) where someone asked for something like this. Unfortunately I cannot find it.

In MayaUsd repo we are using find_package(Python 3.7) and also have the same mechanism to set the python to use (via build flag).

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, understood. The only thing I'd ask then is if we could put a bit of documentation about this in the program description section around lines 1559 or so. A brief blurb with exactly what you said about letting users specify the Python for USD to use and a list of the four parameters to pass to --build-python-info would be perfect.

Comment on lines -682 to -696
if Windows():
# Unfortunately Boost build scripts require the Python folder
# that contains the executable on Windows
pythonPath = os.path.dirname(pythonInfo[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might have been with older versions of boost. It is certainly not the case with the boost version used now. We are building USD on: Windows, Linux & OSX.

Comment on lines +723 to +741
projectFile.write('using python : %s\n' % pythonInfo[3])
projectFile.write(' : "%s"\n' % pythonInfo[0].replace("\\","/"))
projectFile.write(' : "%s"\n' % pythonInfo[2].replace("\\","/"))
projectFile.write(' : "%s"\n' % os.path.dirname(pythonInfo[1]).replace("\\","/"))
if context.buildDebug and context.debugPython:
projectFile.write(' : <python-debugging>on\n')
projectFile.write(' ;\n')
b2_settings.append("--user-config=python-config.jam")

if context.buildDebug and context.debugPython:
b2_settings.append("python-debugging=on")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra flags when building with debug python.

@meshula
Copy link
Member

meshula commented Mar 19, 2021

Out of curiosity, what does "Donnels" stand for?

@rstelzleni Did you have any similar work outstanding?

@seando-adsk The existing code mentions that Apple's System Integrity Protection was preventing relative paths from working. @loader_path is used in other Python bindings, such as OpenTimelineIO's bindings, so I'm wondering if there was another factor implicating SIP, perhaps versus usdview or something. Are you on a macOS with SIP enabled? If no, that's something that should be verified as functional now. I would definitely prefer that the USD build is relocatable!

@seando-adsk seando-adsk changed the title Donnels/debug build support Full debug (with debug python) build support Mar 19, 2021
@seando-adsk
Copy link
Contributor Author

@meshula Oops sorry I forgot to edit the title (I fixed that). donnels is my username.

This work is a group effort (not just changes made by me). My colleague @HamedSabri-adsk was the one who made the @loader_path change, so I'll let him answer your question.

@HamedSabri-adsk
Copy link
Contributor

Hi @meshula,

Some of the changes that was brought in this PR dates back to almost 2 years ago. I do remember the change and at the time did my research to find any real evidence or official documents that proves the validity of the comment but couldn't find anything. If my memory helps I started with USD on "El Capitan".

Yes, I always have SIP enabled on my development machine and also other MacOS that we have used here.

Screen Shot 2021-03-20 at 12 28 04 PM

We have been building USD with these changes all the way from 10.11 "El Capitan" to 11 "Big Sur" and don't recall a single incident report as a result of this change to the best of my knowledge.

Speaking of relocatable, there was a big effort at the time to make sure both USD and MayaUSD builds are relocatable.

Very early on we realized that the set of utility function in "Private.cmake" for handling run-time search path(e.g _pxr_init_rpath(), _pxr_add_rpath(), _pxr_install_rpath() ) quite useful and made it very convenient for us to use them in places we needed them.

Hope this clarifies things.

Cheers,

Hamed

@meshula
Copy link
Member

meshula commented Mar 21, 2021

@HamedSabri-adsk, I remember SIP being a problem at the time, but unfortunately the specifics escape me. Thanks for confirming you've got SIP on now.

At this point, I think several teams are using installnametool based scripts to manage relocation on macos; @loader_path will allow that sort of thing to fall by the wayside.

Copy link
Contributor

@rstelzleni rstelzleni left a comment

Choose a reason for hiding this comment

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

I don't have any pending work related to building python in debug. I added a few comments about related issues with some of the other changes here. Sorry for making it a review! Apparently once you click the review button instead of comment you can't cancel :)

CMakeLists.txt Outdated
CACHE
STRING
"Replacement path for Python #! line."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's another PR targetting this same issue a different way #1445

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at that change. It is good if you are using the system python (either 2 or 3). In my changes, I allow passing in the python to use and then extract the py shebang from that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think the explicit specification of the shebang as in #1445 is a bit clearer than having special logic that does this in certain cases, i.e. if PYTHON_EXECUTABLE is supplied.

If we merged that PR (or something like it) in, would it be sufficient for your use cases? I'd imagine since you're supplying the Python executable directly to the build script, you could also specify PXR_PYTHON_SHEBANG with the executable name when you run the build script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is actually better. I'll remove the changes to this file.

Comment on lines +2111 to +2078
# Special case - we are given the PYSIDEUICBINARY as cmake arg.
usdBuildArgs = context.GetBuildArguments(USD)
given_pysideUic = 'PYSIDEUICBINARY' in " ".join(usdBuildArgs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is pyside-uic deprecated? I thought it was just getting complicated to distribute with py2 vs py3 and pyside1 vs pyside2.

The actual source of pyside-uic/pyside2-uic/etc is pretty small. The latest version I have just imports an object called uic and executes it. Older versions in pyside and pyside2 were pretty minimal as well, though not as short. I wonder if we could include our own pyside-uic with USD's build scripts and run it instead of making a user install extra packages and find their executable name. If we did that we could also get rid of all the name checking we do below this comment line.

This issue is related #1419

@jtran56
Copy link

jtran56 commented Mar 22, 2021

Filed as internal issue #USD-6615

@HamedSabri-adsk
Copy link
Contributor

Hi @rstelzleni,

Is pyside-uic deprecated? I thought it was just getting complicated to distribute with py2 vs py3 and pyside1 vs pyside2.

Yes, starting Qt 5.14, both "uic" and "rcc" will support generating "python code" directly.

@sunyab
Copy link
Contributor

sunyab commented Aug 11, 2021

I'm looking into merging this PR. As a first step I've pulled out the fix to pxr/imaging/hd/strategyBase.h into a separate commit and checked that in internally, since it's unrelated to the meat of the change here.

@seando-adsk
Copy link
Contributor Author

@sunyab Yes correct about strategyBase.h. Thanks.

Copy link
Contributor

@sunyab sunyab left a comment

Choose a reason for hiding this comment

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

I took a first pass at this change and left some questions and notes. Let me know what you think, thanks!

build_scripts/build_usd.py Outdated Show resolved Hide resolved
@@ -311,7 +320,7 @@ def FormatMultiProcs(numJobs, generator):
tag = "-j"
if generator:
if "Visual Studio" in generator:
tag = "/M:"
tag = "/M:" # This will build multiple projects at once. Also add /MP to CXX_FLAGS (below).
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned about oversubscription if we tell MSVC to build multiple projects and multiple files within each project with all processors. I don't think MSVC coordinates these jobs globally(*) so it seems like MSVC could wind up trying to use (numJobs * numJobs) CPUs.

I do think that parallelizing within projects is more useful than parallelizing projects (although I have no data showing that one is faster than the other). So what if we switch "/M:" to use "/p:CL_MPcount=" instead?

(*) It seems like MS has added some support for this in 2019: https://devblogs.microsoft.com/cppblog/improved-parallelism-in-msbuild/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had (incorrectly I guess) just assumed that both /M and /MP would both use up to the max number processors (based on current load, etc). But after reading the docs I think you are right and that both will use the max number of processors so we would end up oversubscribed.

I agree that parallelizing with a project will lead to better optimization as each project generally has many files, but the total number of projects is relatively low. So I agree with your suggestion about changing the option and then removing my /MP line below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did a quick test today on a Windows VM running MSVC 2015 with 24 virtual CPUs and got some surprising results:

USD build with /M: ~10 minutes
USD build with /p:CL_MPcount=: ~25 minutes (!)

I verified that all cores were being used during the build, so I don't know what's going on here. Did you notice a performance improvement on your side with your original change?

It might be worth pulling this out into a separate PR so we can dig into it further without holding up the rest of this change.

Comment on lines +1635 to +1620
group.add_argument("--build-python-info", type=str, nargs=4, default=[],
metavar=('PYTHON_EXECUTABLE', 'PYTHON_INCLUDE_DIR', 'PYTHON_LIBRARY', 'PYTHON_VERSION'),
help=("Specify a custom python to use during build"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is running the script with Maya's Python insufficient? Or was this a way to get away from the Maya Python-specific code throughout the script?

I'm a bit wary of adding these variables here because I'd like to move USD's build away from the deprecated FindPythonLibs/FindPythonInterp modules in CMake and to the new FindPython2/FindPython3 modules, and I'm not sure these settings are supported or translatable to the new modules.

Comment on lines 1623 to 1630
group.add_argument("--build-debug", dest="build_debug", action="store_true",
help="Build in Debug mode")

group.add_argument("--build-release", dest="build_release", action="store_true",
help="Build in Release mode")

group.add_argument("--build-relwithdebug", dest="build_relwithdebug", action="store_true",
help="Build in RelWithDebInfo mode")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are mutually exclusive, could we move these settings into a mutually exclusive group and represent them with an enum-like value? We could follow the pattern for the --build-shared/--build-monolithic settings that are a few lines below.

Comment on lines +2111 to +2078
# Special case - we are given the PYSIDEUICBINARY as cmake arg.
usdBuildArgs = context.GetBuildArguments(USD)
given_pysideUic = 'PYSIDEUICBINARY' in " ".join(usdBuildArgs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since pyside2-uic appears to be deprecated (although the latest PyPI package still seems to include it?), it seems like the build should recognize uic without it having to be passed in via a special build flag. So how about just adding uic to the list of programs in pyside2Uic instead of checking PYSIDEUICBINARY?

cmake/modules/FindPySide.cmake Show resolved Hide resolved
@@ -47,6 +47,7 @@ option(PXR_ENABLE_OSL_SUPPORT "Enable OSL (OpenShadingLanguage) based components
option(PXR_ENABLE_PTEX_SUPPORT "Enable Ptex support" ON)
option(PXR_ENABLE_OPENVDB_SUPPORT "Enable OpenVDB support" OFF)
option(PXR_ENABLE_NAMESPACES "Enable C++ namespaces." ON)
option(PXR_DEFINE_BOOST_DEBUG_PYTHON_FLAG "Define BOOST_DEBUG_PYTHON flag." OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this flag controls stuff beyond just setting the necessary boost defines, for example appending "_d" to library names on Windows. If we ever switched to pybind11 or something, we'd drop the boost defines but would still need to leave the library name behavior in place.

Because of that, it seems like this flag should be named more generally, like PXR_USE_DEBUG_PYTHON. This would match the --debug-python option in build_usd.py. We could also move this flag next to the other Python-related options at line 43-44.

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 the "_d" is a python thing, not a boost thing. I'm good with renaming this option and moving it with the other python ones.

Comment on lines 103 to 104
_add_define(BOOST_DEBUG_PYTHON)
_add_define(BOOST_LINKING_PYTHON)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: could we quote these defines to match the others in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes for sure. Sorry I missed that.

CMakeLists.txt Outdated
Comment on lines 22 to 28
# If we were given a python executable, we'll extract the name from that to use in
# the python shebang replacement. This will make these files more portable by not
# having some full path to a python executable which may not exist on another machine.
if (DEFINED PYTHON_EXECUTABLE)
get_filename_component(PYTHON_EXE_BASENAME ${PYTHON_EXECUTABLE} NAME)
endif()

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason why this set here in the middle of these includes? Naively I'd think this would go down next to where PXR_PYTHON_SHEBANG is set, so that we'd use whatever PYTHON_EXECUTABLE comes out of the Packages include.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it there so its above "include(Packages)" where the find python is called from. I thought it would be a good idea to only do this shebang replacement when the build was called with a specific PYTHON_EXECUTABLE (i.e. one from the new build_usd.py flag --build-python-info that I added).

Copy link
Contributor Author

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

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

@sunyab Thanks for looking at this PR and the comments. I've answered all your questions. How do we proceed now. Would you like me to make the changes and then push a new commit?

This PR is also stale now and has merge conflicts. How do we handle that? Should I merge in the latest dev, or do you prefer a rebase?

Thanks, Sean

CMakeLists.txt Outdated
Comment on lines 22 to 28
# If we were given a python executable, we'll extract the name from that to use in
# the python shebang replacement. This will make these files more portable by not
# having some full path to a python executable which may not exist on another machine.
if (DEFINED PYTHON_EXECUTABLE)
get_filename_component(PYTHON_EXE_BASENAME ${PYTHON_EXECUTABLE} NAME)
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it there so its above "include(Packages)" where the find python is called from. I thought it would be a good idea to only do this shebang replacement when the build was called with a specific PYTHON_EXECUTABLE (i.e. one from the new build_usd.py flag --build-python-info that I added).

@@ -311,7 +320,7 @@ def FormatMultiProcs(numJobs, generator):
tag = "-j"
if generator:
if "Visual Studio" in generator:
tag = "/M:"
tag = "/M:" # This will build multiple projects at once. Also add /MP to CXX_FLAGS (below).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had (incorrectly I guess) just assumed that both /M and /MP would both use up to the max number processors (based on current load, etc). But after reading the docs I think you are right and that both will use the max number of processors so we would end up oversubscribed.

I agree that parallelizing with a project will lead to better optimization as each project generally has many files, but the total number of projects is relatively low. So I agree with your suggestion about changing the option and then removing my /MP line below.

Comment on lines 1623 to 1630
group.add_argument("--build-debug", dest="build_debug", action="store_true",
help="Build in Debug mode")

group.add_argument("--build-release", dest="build_release", action="store_true",
help="Build in Release mode")

group.add_argument("--build-relwithdebug", dest="build_relwithdebug", action="store_true",
help="Build in RelWithDebInfo mode")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes good suggestion.

Comment on lines +1635 to +1620
group.add_argument("--build-python-info", type=str, nargs=4, default=[],
metavar=('PYTHON_EXECUTABLE', 'PYTHON_INCLUDE_DIR', 'PYTHON_LIBRARY', 'PYTHON_VERSION'),
help=("Specify a custom python to use during build"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No unfortunately not. Maya 2022 is dual-python containing both python 2.7 and 3.7 and mayapy/mayapy2. The internal function IsMayaPython() no longer works with those and since it was marked with comments about not having any special Maya knowledge I decided not to modify it. But rather create a more generic ability to specify the python to build with via a new flag --build-python-info.

This allows a user to call the build_usd.py with any python (such as system) but pass in the python that you want USD to use to build the python bindings with.

I even recall a thread (usd-interest/github?) where someone asked for something like this. Unfortunately I cannot find it.

In MayaUsd repo we are using find_package(Python 3.7) and also have the same mechanism to set the python to use (via build flag).

Comment on lines +2111 to +2078
# Special case - we are given the PYSIDEUICBINARY as cmake arg.
usdBuildArgs = context.GetBuildArguments(USD)
given_pysideUic = 'PYSIDEUICBINARY' in " ".join(usdBuildArgs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we could add it to the list of executables to search for, but having a way to pass in a pyside executable to use is very helpful. This code assumes the one you want to use is in the path.

@@ -47,6 +47,7 @@ option(PXR_ENABLE_OSL_SUPPORT "Enable OSL (OpenShadingLanguage) based components
option(PXR_ENABLE_PTEX_SUPPORT "Enable Ptex support" ON)
option(PXR_ENABLE_OPENVDB_SUPPORT "Enable OpenVDB support" OFF)
option(PXR_ENABLE_NAMESPACES "Enable C++ namespaces." ON)
option(PXR_DEFINE_BOOST_DEBUG_PYTHON_FLAG "Define BOOST_DEBUG_PYTHON flag." OFF)
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 the "_d" is a python thing, not a boost thing. I'm good with renaming this option and moving it with the other python ones.

Comment on lines 103 to 104
_add_define(BOOST_DEBUG_PYTHON)
_add_define(BOOST_LINKING_PYTHON)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes for sure. Sorry I missed that.

cmake/modules/FindPySide.cmake Show resolved Hide resolved
pixar-oss pushed a commit that referenced this pull request Aug 16, 2021
Originally part of PR #1478. Thanks to seando-adsk for
flagging this!

(Internal change: 2182870)
@sunyab
Copy link
Contributor

sunyab commented Aug 17, 2021

@seando-adsk I left a few more notes/comments. And yes, if you wouldn't mind making the changes and rebasing on top of the latest dev that would be great. Thank you!

* Use @loader_path instead of absolute path for plugins and
  libraries. On Mac @loader_path is equivalent to $ORIGIN on Linux.

* With the new changes, it would be possible for people to decide
  if they want to build USD with python debug/release.

* One can now set this flag via both build_usd python script as well
  as CMake.
** CMake flag is: -DPXR_DEFINE_BOOST_DEBUG_PYTHON_FLAG
** build_usd flag is: --debug-python

* Full python debug support.
** Must build boost with python debugging flag.
** On Windows when using debug python the libraries must have _d in name.
** Fixed a crash at exit time with python cleanup.
* Fix python cmake variable names (that come from find_package).
* Option to not put full path of python executable in python shebang.
  For portable USD builds.
* Support for maya dual-python build.
* Added ability to use custom PYSIDEUICBINARY.
* My change to 'strategyBase.h' is now gone since it was fixed in dev.
* Added 'uic' to list of pyside executables.
* Added quotes for boost defines.
* Renamed "PXR_DEFINE_BOOST_DEBUG_PYTHON_FLAG" -> "PXR_USE_DEBUG_PYTHON".
* Removed /MP flag.
* Made --build-{debug/release/relwithdebug} mutually exclusive.
* Added documentation for --build-python-info flag.
Copy link
Contributor Author

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

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

@sunyab Thanks for all the great comments. I've rebased my branch ontop of the latest dev and then added a new commit addressing all your code review comments.

My change to 'strategyBase.h' is now gone since it was fixed in dev.

When I added the /MP I'm sure I would have timed a full build (including all the downloaded dependencies). I do recall seeing an improvement (my machine is a dual-12 core), but given your results compiling USD I guess the improvement I saw would have only been in the extra build requirements. Probably because they would have less projects (perhaps only 1) and thus would have been entirely single-threaded. Internally we have switched to using ninja on all platforms so I'm fine with removing this change.

cmake/modules/FindPySide.cmake Show resolved Hide resolved
cmake/defaults/msvcdefaults.cmake Show resolved Hide resolved
@@ -41,6 +41,7 @@ option(PXR_ENABLE_MATERIALX_SUPPORT "Enable MaterialX support" OFF)
option(PXR_BUILD_DOCUMENTATION "Generate doxygen documentation" OFF)
option(PXR_ENABLE_PYTHON_SUPPORT "Enable Python based components for USD" ON)
option(PXR_USE_PYTHON_3 "Build Python bindings for Python 3" OFF)
option(PXR_USE_DEBUG_PYTHON "Build with debug python" OFF)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed "PXR_DEFINE_BOOST_DEBUG_PYTHON_FLAG" -> "PXR_USE_DEBUG_PYTHON" and moved up to location of other python options.

Comment on lines +2111 to +2078
# Special case - we are given the PYSIDEUICBINARY as cmake arg.
usdBuildArgs = context.GetBuildArguments(USD)
given_pysideUic = 'PYSIDEUICBINARY' in " ".join(usdBuildArgs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added 'uic' to list of pyside executables.

Comment on lines +1603 to +1613
(BUILD_DEBUG, BUILD_RELEASE, BUILD_RELWITHDEBUG) = (0, 1, 2)
subgroup = group.add_mutually_exclusive_group()
subgroup.add_argument("--build-debug", dest="build_variant",
action="store_const", const=BUILD_DEBUG, default=BUILD_RELWITHDEBUG,
help="Build in Debug mode")
subgroup.add_argument("--build-release", dest="build_variant",
action="store_const", const=BUILD_RELEASE,
help="Build in Release mode")
subgroup.add_argument("--build-relwithdebug", dest="build_variant",
action="store_const", const=BUILD_RELWITHDEBUG,
help="Build in RelWithDebInfo mode (default)")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made --build-{debug/release/relwithdebug} mutually exclusive.

Comment on lines +1561 to +1565
The flag --build-python-info allows calling the %(prog)s with any python (such as
system python) but pass in the python that you want USD to use to build the python
bindings with. This flag takes 4 arguments: python executable, python include directory
python library and python version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added documentation for --build-python-info flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants