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

Build type support + Windows Debug fix #785

Conversation

HamedSabri-adsk
Copy link
Contributor

This commit aims at supporting various build types as well as fixing python errors during Debug build on Windows.

  • The build script now specifies 3 different variants options ( --build-debug, --build-release, --build-relwithdebug ). TBB, BOOST, CMAKE_BUILD_TYPE are also now set with the same Variant type. If these flags are not set the default build type will be RelWithDebInfo

  • Fix Debug build on Windows by hiding the definition of _DEBUG during the inclusion of Python.h

Hamed Sabri added 2 commits March 6, 2019 18:04
…Info)

Conflicts:
	build_scripts/build_usd.py
	cmake/macros/Public.cmake
…the inclusion of Python.h

Other libraries such as boost library also use a similar technique by using "wrap_python.hpp" which is basically a wrapper around <Python.h> which allows it to be compiled under win32 so that a program can be compiled in debug mode without requiring a special debugging build of the Python library.

Conflicts:
	pxr/usdImaging/lib/usdImaging/pch.h
@marktucker
Copy link
Contributor

This change makes it impossible to build against a debug python library, which just flips the current problem to its opposite. Wouldn't it make more sense to actually have a python wrapper header instead of including Python.h directly everywhere? Or maybe even use the boost python wrapper instead of including Python.h directly? Boost's python wrapper checks the BOOST_DEBUG_PYTHON define to decide whether or not the clear/restore the _DEBUG define.

I would think the goal here is to let the person building USD decide between debug/release USD independently from choosing between debug/release python (preferably using a CMake option).

@HamedSabri-adsk
Copy link
Contributor Author

Sure, it does makes sense. At the time we made this change, it was important for us to be able to debug into USD library itself and debugging python was not as urgent. We also made sure that dependency libraries ( e.g TBB, Boost ) and even CMake Build type itself follow the same build type as you can see in the changes.

Yes I have looked at "wrap_python.hpp" before and read on python debugging build which led me to make the changes in those ~50 files. I do agree with you that a better option would be to just use boost's wrap_python.hpp instead since it comes with "BOOST_DEBUG_PYTHON" flag that one can pass through to the script and ultimately let people decide to use it:

https:/nesbox/boost_1_68-android/blob/master/x86/include/boost-1_68/boost/python/detail/wrap_python.hpp

https://www.boost.org/doc/libs/1_60_0/libs/python/doc/html/building/python_debugging_builds.html

@meshula
Copy link
Member

meshula commented Mar 7, 2019

I like where you are going with this change, and also agree that supporting Debug Python builds via a cmake flag (whether using the boost mechanism or not) would be an improvement. I also like that you’ve solved the TBB debug link issues which is something I’d been hacking around. Didn’t know about those settings!

…ant to build USD with python debug/release.

We use boost python wrapper header that comes with the "BOOST_DEBUG_PYTHON" flag instead of including plain Python.h directly. This flag can now be set via build_usd.py script as well as Vanilla CMake.

- CMake -DPXR_DEFINE_BOOST_DEBUG_PYTHON_FLAG

- build_usd.py --boost-debug-python

I have also refactored some of the code inside RunCMake function and fixed couple of bugs in my previous changes.
@jilliene
Copy link

jilliene commented Mar 7, 2019

Filed as internal issue #USD-5128

@HamedSabri-adsk
Copy link
Contributor Author

HamedSabri-adsk commented Mar 7, 2019

Thank you for your feedback. With the new changes, it would be possible for people to decide if they want to build USD with python debug/release. We decided to go with using boost python wrapper header that comes with the "BOOST_DEBUG_PYTHON" flag instead of just including plain Python.h directly.

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

I have also refactored some of the codes inside RunCMake function and fixed couple of bugs in my previous changes.

@c64kernal
Copy link
Contributor

Hi @HamedSabri-adsk, thanks for the PR! We don't currently have a record of a CLA from you. Please submit one or ask that your company amend a corporate CLA to include you.

@HamedSabri-adsk
Copy link
Contributor Author

Hi @c64kernal, I was recently told that I am included in corporate CLA.

Thank you.

@c64kernal
Copy link
Contributor

Thanks @HamedSabri-adsk -- confirmed that we got the CLA, thanks again. We'll update this PR when we've had a chance to look at it in more detail -- unfortunately I don't have an ETA at the moment.

@c64kernal
Copy link
Contributor

Hi @HamedSabri-adsk -- thanks for your patience with this. Unfortunately we couldn't take this PR as-is because it modifies pch.h files which are programmatically generated by an internal script. We actually had to modify the original includes that caused these direct Python.h files to appear in the pch.h files. We also added an indirection file in tf, called tf/pySafePython.h which we will use instead of Python.h from now on. Finally we had to modify our pch-generating script to work with this new python include. The good news is that we're going to land all of that in dev after the current 20.02 release is finalized. After that point, if you'd like to see the rest of the changes you've made here still be integrated, please either make a new PR or rebase this one to exclude all the pch.h files.

Thanks again, and hope that the fixes we put in will also work for you once they land in dev and you get a chance to try them out.

@c64kernal c64kernal added the blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed label Jan 11, 2020
@HamedSabri-adsk
Copy link
Contributor Author

Hi @c64kernal, great to hear that. Looking forward to see the new changes!

We also more polished changes for full python debug support and will make a new PR after current 20.02 release is finalized.

Cheers,
Hamed

@c64kernal
Copy link
Contributor

Perfect, thanks @HamedSabri-adsk!

@HamedSabri-adsk
Copy link
Contributor Author

Hi @c64kernal, I was wondering if you could tell me roughly when the new changes will be available in dev now that 20.02 is officially released?

Thanks again.

pixar-oss pushed a commit that referenced this pull request Feb 1, 2020
We should no longer include Python.h directly and instead use the new
pySafePython.h file. Including Python.h directly is known to cause
problems on certain platforms with certain build configurations.

A follow up change will replace all current includes of Python.h.

See #785
See #1006

(Internal change: 2033910)
pixar-oss pushed a commit that referenced this pull request Feb 1, 2020
tf/pySafePython.h instead.

See #785
See #1006

(Internal change: 2034270)
pixar-oss pushed a commit that referenced this pull request Feb 1, 2020
Fixes #1006
Fixes #785

(Internal change: 2034642)
@sunyab
Copy link
Contributor

sunyab commented Feb 1, 2020

Hi @HamedSabri-adsk, these changes are in the dev branch now. Let us know how this works for you!

@HamedSabri-adsk
Copy link
Contributor Author

@sunyab Perfect! I see them now. I was searching for tf/pySafePython.h in dev branch on github quickly forgot that github only searches things in master branch only. sigh....

The reason, I asked is because we are preparing a new PR to address some of these issues with python Debug and we wanted to sync up with these new changes.

AdamFelt pushed a commit to autodesk-forks/USD that referenced this pull request Apr 16, 2024
AdamFelt pushed a commit to autodesk-forks/USD that referenced this pull request Apr 16, 2024
AdamFelt pushed a commit to autodesk-forks/USD that referenced this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants