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

[docs] fixes for python docstring builds on windows #2574

Conversation

pmolodo
Copy link
Contributor

@pmolodo pmolodo commented Aug 8, 2023

Description of Change(s)

Allows python docstring generation on windows

NOTE:
This PR is one of several targeting the python docstring generation process. To see all these PRs in a branch, see here.

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@jesschimein
Copy link
Contributor

Filed as internal issue #USD-8569

@dsyu-pixar
Copy link

dsyu-pixar commented Oct 2, 2023

@pmolodo again, thanks so much for doing this PR (and apologies for the delay in reviewing it).

I tried this PR on a Win10 instance using the following build_usd.py command (to a clean/empty USD-install dir):

py C:\Users\dsyu\temp\USD\build_scripts\build_usd.py C:\Users\dsyu\temp\USD\USD-install --docs --no-embree --python --python-docs --no-debug-python --no-draco --no-prman --no-materialx --no-alembic --no-hdf5 --no-opencolorio --no-openimageio --no-usdview --no-openvdb --ptex --no-imaging

and I'm getting the following log error with no __DOC.py files getting produced:

File "C:\Users\dsyu\temp\USD\docs\python\convertDoxygen.py", line 70, in <module>
  os.add_dll_directory(path)
File "C:\Python39\lib\os.py", line 1111, in add_dll_directory
  cookie = nt._add_dll_directory(path)
FileNotFoundError: [WinError 3] The system cannot find the path specified: 'C:\\Users\\dsyu\\temp\\USD\\USD-install\\plugin\\usd'

If I take out ${CMAKE_INSTALL_PREFIX}/plugin/usd from the list of libPaths in Public.cmake, things build correctly. I think(?) what might be happening is that the plugin\usd dir isn't getting created/built until after the python docs are built? Can you confirm this behavior?

@pmolodo pmolodo force-pushed the pr/python-docstrings-windows-fixes branch from c8392df to 3bd68fc Compare October 17, 2023 19:54
@pmolodo
Copy link
Contributor Author

pmolodo commented Oct 17, 2023

Oh, good find @dsyu-pixar !
I think the issue here is that with your build options, none of the usd plugins are built, so, as you surmised, in this case, that directory isn't created until after the documentation runs (when the plugInfo.json file is created).

I fixed this by just adding a check that the path exists before calling os.add_dll_directory. I think simply removing it might create problems for builds that actually create those plugins.

Confirmed that it works, with your options, after this change.

@pmolodo pmolodo force-pushed the pr/python-docstrings-windows-fixes branch from 3bd68fc to a623c9f Compare October 17, 2023 20:19
@pmolodo
Copy link
Contributor Author

pmolodo commented Oct 17, 2023

Rebased and fixed merge conflict...

@dsyu-pixar
Copy link

dsyu-pixar commented Oct 20, 2023

Thanks for the fix @pmolodo! Confirmed it's working on my end on Windows.

@dsyu-pixar
Copy link

@pmolodo a question came up in a review of this change -- do we need the LD_LIBRARY_PATH change for Linux and MacOS? As in, were you running into cases on Linux or MacOS where the modules weren't loading? Additionally, if there is an issue on MacOS, it was pointed out that we might need to use DYLD_LIBRARY_PATH instead of LD_LIBRARY_PATH.

If the issue was only occurring on Windows, would it be possible to update the change to only get applied if the build is happening on Windows? E.g. only add to the commandline in Public.cmake if we detect we're building on Windows, rename the libPath argument to something Windows-specific, like windowsLibPath, for better clarity, and drop the LD_LIBRARY_PATH portion of the change in convertDoxygen.py? Let me know what you think.

@pmolodo pmolodo force-pushed the pr/python-docstrings-windows-fixes branch from a623c9f to e5f720d Compare November 15, 2023 00:52
@pmolodo pmolodo force-pushed the pr/python-docstrings-windows-fixes branch from e5f720d to e6c0cf8 Compare November 15, 2023 02:22
@pmolodo
Copy link
Contributor Author

pmolodo commented Nov 15, 2023

I checked and confirmed that the --libPath change wasn't needed for us on Linux, so I've done as you suggested, and made it windows-only.

@pixar-oss pixar-oss merged commit 48ea005 into PixarAnimationStudios:dev Nov 28, 2023
3 of 5 checks passed
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.

4 participants