Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cmake/defaults/CXXDefaults.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ _add_define(GL_GLEXT_PROTOTYPES)
_add_define(GLX_GLXEXT_PROTOTYPES)

# Python bindings for tf require this define.
_add_define(BOOST_PYTHON_NO_PY_SIGNATURES)
if (NOT PXR_BUILD_PYTHON_DOCUMENTATION)
_add_define(BOOST_PYTHON_NO_PY_SIGNATURES)
endif()
Comment on lines +45 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the documentation builds also build TF, I'm thinking that this comment may now be out of date, and we can remove the define unconditionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The setting of the BOOST_PYTHON_NO_PY_SIGNATURES define has moved here: https://github.com/PixarAnimationStudios/OpenUSD/blob/dev/pxr/external/boost/python/CMakeLists.txt#L327

I tried to port the same behavior, like so:

--- a/pxr/external/boost/python/CMakeLists.txt
+++ b/pxr/external/boost/python/CMakeLists.txt
@@ -321,11 +321,13 @@ pxr_library(python
     DISABLE_PRECOMPILED_HEADERS
 )
 
-# Disable insertion of C++ signatures in docstrings. We generate this
-# information separately via our Python doc build process.
-target_compile_definitions(python
-    PUBLIC PXR_BOOST_PYTHON_NO_PY_SIGNATURES
-)
+if (NOT PXR_BUILD_PYTHON_DOCUMENTATION)
+    # Disable insertion of C++ signatures in docstrings. We generate this
+    # information separately via our Python doc build process.
+    target_compile_definitions(python
+        PUBLIC PXR_BOOST_PYTHON_NO_PY_SIGNATURES
+    )
+endif()

Unfortunately, this leads to a build error that I don't understand.

/Users/chad/dev/USD/pxr/external/boost/python/type_id.hpp:88:9: error: 'typeid' of incomplete type 'pxrInternal_v0_25_8__pxrReserved__::ArAsset'
   88 |         typeid(T)
      |         ^
/Users/chad/dev/USD/pxr/external/boost/python/converter/registered.hpp:111:31: note: in instantiation of function template specialization 'pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::type_id<const volatile pxrInternal_v0_25_8__pxrReserved__::ArAsset &>' requested here
  111 |       return registry::lookup(type_id<T&>());
      |                               ^
/Users/chad/dev/USD/pxr/external/boost/python/converter/registered.hpp:118:14: note: in instantiation of function template specialization 'pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::converter::detail::registry_lookup2<const volatile pxrInternal_v0_25_8__pxrReserved__::ArAsset>' requested here
  118 |       return registry_lookup2((T(*)())0);
      |              ^
/Users/chad/dev/USD/pxr/external/boost/python/converter/registered.hpp:129:64: note: in instantiation of function template specialization 'pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::converter::detail::registry_lookup1<const volatile pxrInternal_v0_25_8__pxrReserved__::ArAsset &>' requested here
  129 |   registration const& registered_base<T>::converters = detail::registry_lookup1(type<T>());
      |                                                                ^
/Users/chad/dev/USD/pxr/external/boost/python/to_python_value.hpp:117:54: note: in instantiation of function template specialization 'pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::detail::shared_ptr_to_python_value<const std::shared_ptr<pxrInternal_v0_25_8__pxrReserved__::ArAsset> &>::get_pytype<pxrInternal_v0_25_8__pxrReserved__::ArAsset>' requested here
  117 |       PyTypeObject const* get_pytype() const {return get_pytype((type<argument_type>*)0);}
      |                                                      ^
/Users/chad/dev/USD/pxr/external/boost/python/detail/caller.hpp:137:98: note: in instantiation of member function 'pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::detail::shared_ptr_to_python_value<const std::shared_ptr<pxrInternal_v0_25_8__pxrReserved__::ArAsset> &>::get_pytype' requested here
  137 |         return create_result_converter((PyObject*)0, (ResultConverter *)0, (ResultConverter *)0).get_pytype();
      |                                                                                                  ^
/Users/chad/dev/USD/pxr/external/boost/python/detail/caller.hpp:159:61: note: (skipping 6 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
  159 |         , &detail::converter_target_type<result_converter>::get_pytype
      |                                                             ^
/Users/chad/dev/USD/pxr/external/boost/python/make_function.hpp:148:20: note: in instantiation of function template specialization 'pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::detail::make_function_aux<std::shared_ptr<pxrInternal_v0_25_8__pxrReserved__::ArAsset> (pxrInternal_v0_25_8__pxrReserved__::ArResolver::*)(const pxrInternal_v0_25_8__pxrReserved__::ArResolvedPath &) const, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::default_call_policies, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::detail::type_list<std::shared_ptr<pxrInternal_v0_25_8__pxrReserved__::ArAsset>, pxrInternal_v0_25_8__pxrReserved__::ArResolver &, const pxrInternal_v0_25_8__pxrReserved__::ArResolvedPath &>, std::integral_constant<int, 1>>' requested here
  148 |     return detail::make_function_aux(
      |                    ^
/Users/chad/dev/USD/pxr/external/boost/python/class.hpp:458:13: note: in instantiation of function template specialization 'pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::make_function<std::shared_ptr<pxrInternal_v0_25_8__pxrReserved__::ArAsset> (pxrInternal_v0_25_8__pxrReserved__::ArResolver::*)(const pxrInternal_v0_25_8__pxrReserved__::ArResolvedPath &) const, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::default_call_policies, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::detail::keywords<1>, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::detail::type_list<std::shared_ptr<pxrInternal_v0_25_8__pxrReserved__::ArAsset>, pxrInternal_v0_25_8__pxrReserved__::ArResolver &, const pxrInternal_v0_25_8__pxrReserved__::ArResolvedPath &>>' requested here
  458 |           , make_function(
      |             ^
/Users/chad/dev/USD/pxr/external/boost/python/class.hpp:527:15: note: in instantiation of function template specialization 'pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::class_<pxrInternal_v0_25_8__pxrReserved__::ArResolver, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::noncopyable>::def_impl<pxrInternal_v0_25_8__pxrReserved__::ArResolver, std::shared_ptr<pxrInternal_v0_25_8__pxrReserved__::ArAsset> (pxrInternal_v0_25_8__pxrReserved__::ArResolver::*)(const pxrInternal_v0_25_8__pxrReserved__::ArResolvedPath &) const, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::detail::def_helper<pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::detail::keywords<1>>>' requested here
  527 |         this->def_impl(
      |               ^
/Users/chad/dev/USD/pxr/external/boost/python/class.hpp:205:15: note: in instantiation of function template specialization 'pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::class_<pxrInternal_v0_25_8__pxrReserved__::ArResolver, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::noncopyable>::def_maybe_overloads<std::shared_ptr<pxrInternal_v0_25_8__pxrReserved__::ArAsset> (pxrInternal_v0_25_8__pxrReserved__::ArResolver::*)(const pxrInternal_v0_25_8__pxrReserved__::ArResolvedPath &) const, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::detail::keywords<1>>' requested here
  205 |         this->def_maybe_overloads(name, a1, a2, &a2);
      |               ^
/Users/chad/dev/USD/pxr/usd/ar/wrapResolver.cpp:97:10: note: in instantiation of function template specialization 'pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::class_<pxrInternal_v0_25_8__pxrReserved__::ArResolver, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::noncopyable>::def<std::shared_ptr<pxrInternal_v0_25_8__pxrReserved__::ArAsset> (pxrInternal_v0_25_8__pxrReserved__::ArResolver::*)(const pxrInternal_v0_25_8__pxrReserved__::ArResolvedPath &) const, pxrInternal_v0_25_8__pxrReserved__::pxr_boost::python::detail::keywords<1>>' requested here
   97 |         .def("OpenAsset", &This::OpenAsset,
      |          ^
/Users/chad/dev/USD/pxr/usd/ar/resolver.h:25:7: note: forward declaration of 'pxrInternal_v0_25_8__pxrReserved__::ArAsset'
   25 | class ArAsset;
      |       ^
1 error generated.
gmake[2]: *** [pxr/usd/ar/CMakeFiles/_ar.dir/build.make:233: pxr/usd/ar/CMakeFiles/_ar.dir/wrapResolver.cpp.o] Error 1

My goal in not setting BOOST_PYTHON_NO_PY_SIGNATURES is to allow boost to add its representation of the python signatures to the python docstrings.

In an ideal world, instead of adding these to the __doc__ attribute of each object we would write them to disk as a build artifact that can be read by the custom docstring generator. Now that we have our own fork of boost that might actually be on the table, but either way this build error needs to be resolved.

Unfortunately this has reached a level of complexity that I won't be able to solve it on my own, which means I can't make stubs for any versions of USD after the switch to pxr_boost. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure how far on the journey I can go, but this particular error is saying that wrapResolver.cpp, with that option enabled, requires a complete declaration of ArAsset, whereas resolver.h is only forward declaring it. So I think you just need to include asset.h in wrapResolver.cpp


# Parts of boost (in particular, boost::hash) rely on deprecated features
# of the STL that have been removed from some implementations under C++17.
Expand Down
114 changes: 83 additions & 31 deletions docs/python/doxygenlib/cdWriterDocstring.py
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,18 @@ def __getPythonObjectAndPath(self, parentPath, overloads):

(obj, pypath, jumped) = self.__getPythonObjectByPath(pypath)

if jumped and overloads[0].isFunction():
# We found a module-level function in our module through a permissive
# search (jumped=True). Do some further vetting.
if obj and hasattr(obj, "__module__") and \
not parentPath[-1].name.startswith(obj.__module__.split(".")[1]):
# The doxygen object clearly indicates it's from another module.
obj = None
elif type(obj).__module__ != 'Boost.Python':
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 the module name changed with pxr_boost, but seeing this reminds me that you didn't explicitly confirm if you've tested this with pxr_boost yet...

# The function we found is not a boost function, so it can't correspond
# to our doxygen object
obj = None

# check for the property by either possible name (since there
# are two possible naming conventions for boolean properties)
ppypath = ppypath1
Expand Down Expand Up @@ -687,49 +699,89 @@ def __getFullDoc(self, pyname, pyobj, doxy):
# make the doxy element static if it is tagged as such
if ATTR_STATIC_METHOD in doxy.doc['tags']:
doxy.static = 'yes'

lines = self.__getShortDescription(pyname, pyobj, doxy)
if doxy.isFunction() and type(pyobj) != property:
lines += self.__getSignatureDescription(pyname, pyobj, doxy)
lines.append('')
description = self.__getSignatureDescription(pyname, pyobj, doxy)
if description is not None:
lines += description
lines.append('')
lines += self.__getDocumentation(pyname, pyobj, doxy)
lines.append('')
return lines

def __getOutputFormat(self, pypath, pyobj, overloads):
"""Return the line that installs the docstring into the namespace."""
@classmethod
def __stripBoostSig(cls, doc):
def looksLikeBoostSig(l):
return bool(l and (l[0].isalnum() or l[0] == "_") and ' -> ' in l)

lines = doc.strip().splitlines()
if len(lines) and looksLikeBoostSig(lines[0]):
# boost signature has been prepended. that means if there is
# an existing description provided in the C++ wrapper it will be
# indented. strip the signature and dedent the description.
Comment on lines +720 to +722
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 an example before / after docstring (either real or made up) would greatly help understanding here. I had to re-read this a few times before I thought I understood what's going on, as my initial assumption about what these would look like was wrong.

found = False
newLines = []
for line in lines:
if not found and line.startswith(' '):
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also add a and line.strip() check here? ie, if we have several lines of indented-but-empty lines preceeding the start of the non-whitespace section, we'll probably want to strip those too.

Would also (help) prevent us from returning a result that's nothing but whitespace-only lines. (We'd also need to check if found=False after the loop).

found = True
if found:
newLines.append(line)
return textwrap.dedent('\n'.join(newLines))
Comment on lines +725 to +730
Copy link
Contributor

@pmolodo pmolodo Jul 14, 2025

Choose a reason for hiding this comment

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

So, basically we're finding the first indented line, grabbing it and all following, and then dedenting.

I'm assuming you haven't encountered the situation where unindented text follows indented, ie:

A
B
    C
    D
E
    F

...but we should consider what to do if it happens. Current behavior would be to return:

    C
    D
E
    F

...which is likely not desired.

Simplest solution is to just error if we're fairly sure this should never happen, and/or can't make any guesses at what the best behavior would be if it does. Other options would be to return None, or one of:

C
D
F

or

C
D
E
F

...all of which feel more likely to be correct.

Comment on lines +725 to +730
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we return None if found = False, or error?

else:
# None indicates that the existing __doc__ attr should be left as-is
return None

def __getDocstring(self, pypath, pyobj, overloads):
"""Return the docstring."""

# is there an existing python docstring? we don't want to overwrite
# this because it may be custom authored in the C++ wrap files.
# However, we always override the module doc string for now...
docString = ''

# boost auto-generates function signatures that can be useful in some
# contexts (such as generating pyi type stubs) because they more
# accurately reflect changes made to the API within the C++ wrap files,
# but for our docstrings we strip them out and replace them with our
# own function signatures. After stripping out the boost signatures,
# if there is a custom authored docstring in the C++ wrap files we honor
# it.
if hasattr(pyobj, '__doc__') and pyobj.__doc__ is not None:
doc = pyobj.__doc__.strip()
if len(doc) > 0 and not doc.startswith("C++ signature:") \
and not overloads[0].isModule():
Debug("Docstring exists for %s - skipping" % pypath)
return None
if len(doc) > 0 and not overloads[0].isModule():
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to add the old we always override the module doc string for now... comment back here

newDoc = self.__stripBoostSig(doc)
if newDoc is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some cases where __stripBoostSig() could return a non-None string that's either empty or nothing but whitespace. We should either clean those up, or change this check to if newDoc is None or not newDoc.strip():, or both (an effectively empty docstring is probably never the correct thing to return, I'm guessing).

# None indicates that the existing __doc__ attr should be
# left as is
Debug("Docstring exists for %s - skipping" % pypath)
return None
docString = newDoc

if not docString.strip():
# get the full docstring that we want to output
lines = []
pyname = pypath.split('.')[-1]

# get the full docstring that we want to output
lines = []
pyname = pypath.split('.')[-1]
docString = ''

if len(overloads) == 1:
lines += self.__getFullDoc(pyname, pyobj, overloads[0])
if overloads[0].isStatic():
docString = LABEL_STATIC # set the return type to static
else:
for doxy in overloads:
if doxy.isStatic():
if len(overloads) == 1:
lines += self.__getFullDoc(pyname, pyobj, overloads[0])
if overloads[0].isStatic():
docString = LABEL_STATIC # set the return type to static

desc = self.__getFullDoc(pyname, pyobj, doxy)
if lines and desc:
lines.append('-'*70)
if desc:
lines += desc
docString += '\n'.join(lines)
else:
for doxy in overloads:
if doxy.isStatic():
docString = LABEL_STATIC # set the return type to static

desc = self.__getFullDoc(pyname, pyobj, doxy)
if lines and desc:
lines.append('-'*70)
if desc:
lines += desc
docString += '\n'.join(lines)

def __getOutputFormat(self, pypath, pyobj, overloads):
"""Return the line that installs the docstring into the namespace."""

docString = self.__getDocstring(pypath, pyobj, overloads)
if docString is None:
return None
# work out the attribute to set to install this docstring
words = pypath.split('.')
cls = words[0]
Expand Down
17 changes: 9 additions & 8 deletions pxr/base/tf/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
# and newer. These interpreters don't search for DLLs in the path anymore, you
# have to provide a path explicitly. This re-enables path searching for USD
# dependency libraries
import platform, sys
import os, platform, sys
if sys.version_info >= (3, 8) and platform.system() == "Windows":
import contextlib

Expand Down Expand Up @@ -109,15 +109,16 @@ def PreparePythonModule(moduleName=None):
except KeyError:
pass

try:
module = importlib.import_module(".__DOC", f_locals["__name__"])
module.Execute(f_locals)
if os.environ.get("PXR_USD_PYTHON_DISABLE_DOCS", "false").lower() not in ("1", "true", "yes"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Tf.GetEnvBool() also returns true for "on" - we should probably follow that convention:

https://github.com/PixarAnimationStudios/OpenUSD/blob/v25.05.01/pxr/base/tf/getenv.h#L48

try:
del f_locals["__DOC"]
except KeyError:
module = importlib.import_module(".__DOC", f_locals["__name__"])
module.Execute(f_locals)
try:
del f_locals["__DOC"]
except KeyError:
pass
except Exception:
pass
except Exception:
pass

finally:
del frame
Expand Down
6 changes: 5 additions & 1 deletion pxr/base/tf/pyModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,12 @@ class Tf_ModuleProcessor {

inline object ReplaceFunctionOnOwner(char const *name, object owner, object fn)
{
object fnDocstring = fn.attr("__doc__");
object newFn = DecorateForErrorHandling(name, owner, fn);
PyObject_DelAttrString(owner.ptr(), name);
objects::function::add_to_namespace(owner, name, newFn);
// add_to_namespace removes docstrings, so we restore them here
newFn.attr("__doc__") = fnDocstring;
return newFn;
}

Expand Down Expand Up @@ -431,7 +434,8 @@ void Tf_PyInitWrapModule(

// Disable docstring auto signatures.
boost::python::docstring_options docOpts(true /*show user-defined*/,
false /*show signatures*/);
true /*show py signatures*/,
false /*show cpp signatures*/);

// Do the wrapping.
wrapModule();
Expand Down