Windows Support - Githubissues

rlei-weta commented 4 years ago

It would be useful for USDPluginExamples to support Windows:

Update code and validate build & runtime.
Update documentation with Windows-specific procedures.
Automated CI tests (can we find any images or pre-built binaries?)

cole-clark commented 3 years ago

Looked into this a little bit on Windows. It's definitely going to be a project to get this working. But, I was able to get compiled and linked .dlls, but I had to make the following changes:

In FindUSD.cmake: Changed: NAMES "${PXR_LIB_PREFIX}usd${CMAKE_SHARED_LIBRARY_SUFFIX}" To: NAMES "usd${CMAKE_SHARED_LIBRARY_SUFFIX}"

Then with the cmake-generated Visual Studio project, I compared the dancing cubes example project with the one cmake produces for this, and had compiling / linking errors until I applied the following changes in the project properties:

Set unspecified compiler C++ Language Standard ISO C++14 Standard (/std:c++14)

Added include directory: D:\usd\builds\v21.02\build\USD\include

Added Preprocessor Definitions: NDEBUG;PXR_PYTHON_ENABLED=1;MFB_PACKAGE_NAME=usdDancingCubesExample;MFB_ALT_PACKAGE_NAME=usdDancingCubesExample;MFB_PACKAGE_MODULE=UsdDancingCubesExample;PXR_BUILD_LOCATION=usd;PXR_PLUGIN_BUILD_LOCATION=../plugin/usd;PXR_PYTHON_MODULES_ENABLED=1;USDDANCINGCUBESEXAMPLE_EXPORTS=1;PXR_OPENVDB_SUPPORT_ENABLED;_CRT_SECURE_NO_WARNINGS;_SCL_SECURE_NO_WARNINGS;NOMINMAX;YY_NO_UNISTD_H;BOOST_ALL_DYN_LINK;OPENEXR_DLL;GL_GLEXT_PROTOTYPES;GLX_GLXEXT_PROTOTYPES;BOOST_PYTHON_NO_PY_SIGNATURES;CMAKE_INTDIR="Release";usdDancingCubesExample_EXPORTS;%(PreprocessorDefinitions)

Added Linker Additional Dependencies: arch.lib trace.lib work.lib kind.lib ar.lib pcp.lib

So, there's clearly some things missing from the CMake-generated projects that Windows needs that are being covered from the USD build tree. I copied over the missing things naively just to get it to work, I doubt all of this is needed for each project. Also haven't had time yet to actually validate if this actually works at runtime.

rlei-weta commented 3 years ago

Hey @colevfx - thanks for looking into this!

It's awesome that you got this building on Windows :-)

I copied over the missing things naively just to get it to work, I doubt all of this is needed for each project.

That's right - ideally we'd extract the minimal set of required build procedures into this project.

If you have a chance to validate that things are working during runtime, we'd love to see a pull-request with the proposed changes.

If you also happen to be savvy with GitHub workflows - it'd be great to find existing prebuilt binaries or docker image to set-up a job to test against.

Thanks again for the help!

cole-clark commented 3 years ago

Thanks @rlei-weta, I'd be happy to keep looking into it! 😄 I'll take another pass and try to get things working properly in cmake and validate the runtime now that we sort of know what's not correct in Visual Studio.

I'm not that savvy, but also happy to look into setting up a job to build / test against the NVIDIA binaries after we get things basically working.

cole-clark commented 3 years ago

I got a lot further, this is a little dirty but will build and install on Windows reliably through cmake:

https://github.com/colevfx/USDPluginExamples/tree/20.11_windows_compat

There's still one blocker before I can put together a PR. At runtime, I can see the Tri USDViewPlugin, The triangle prim, with a correct bounding box, and the "Tri" render delegate, but I'm having an issue actually seeing the triangle in the viewport because of an error on both 20.11 and 21.02

ERROR: Usdview encountered an error while rendering. Error in 'pxrInternal_v0_21pxrReserved::PlugPlugin::_Load' at line 258 in file D:\usd\src\USD\pxr\base\plug\plugin.cpp : 'Failed to load plugin 'usdTriImaging': The specified module could not be found. in 'c:/Program Files/USDPluginExamples/plugin/usdTriImaging.dll'' Error in 'pxrInternal_v0_21pxrReserved::UsdImagingAdapterRegistry::ConstructAdapter' at line 167 in file D:\usd\src\USD\pxr\usdImaging\usdImaging\adapterRegistry.cpp : '[PluginLoad] PlugPlugin could not be loaded for TfType 'UsdTriImagingTriangleAdapter'

Have you seen this kind of error before while developing? I'm a little stuck on it right now.

rlei-weta commented 3 years ago

@colevfx - that was fast!

Have you seen this kind of error before while developing? I'm a little stuck on it right now.

Hmm, does c:/Program Files/USDPluginExamples/plugin/usdTriImaging.dll exist on disk at that location?

To provide more diagnostic info, could you try setting the environment variable TF_DEBUG to a value of PLUG_*, then attach the output (as a .txt file) from launching usdview with that scene? I'd be keen to take a look - thanks!

rlei-weta commented 3 years ago

Hey @colevfx - I checked out your branch to build on Windows - and was able to reproduce the same error.

usdTri.dll is a required runtime dependency ofusdTriImaging.dll, but it didn't seem like usdTri.dll was being installed under %USDPLUGINEXAMPLES_INSTALL_ROOT%/lib.

I think the issue is that we're missing a RUNTIME DESTINATION property in the install command here: https://github.com/colevfx/USDPluginExamples/blob/20.11_windows_compat/cmake/USDPluginTools.cmake#L441

Adding RUNTIME DESTINATION ${LIBRARY_INSTALL_PREFIX} just below the LIBRARY DESTINATION results in usdTri.dll being installed.

Then in the runtime environment I had set %USDPLUGINEXAMPLES_INSTALL_ROOT%/lib to PATH for usdTri.dll to be discovered and loaded before usdTriImaging.dll loads. This was enough to get a triangle imaged in the viewport.

Hope this helps!

cole-clark commented 3 years ago

@rlei-weta , you're a legend! I've never been so happy to see a triangle in Embree before 😄 . Interestingly, usdTri.lib (windows static library / imports library) on my system was actually being installed, and not a dll without that line of code--but clearly these should all be shared objects.

I'm going to put in a PR, but I'd like to get your opinion on something. I figured out that this program MUST include "NOMINMAX;" and CXX_STANDARD=14 properties for MSVC to compile, but there's actually there's a lot of other specific definitions in the USD project:

https://github.com/PixarAnimationStudios/USD/blob/release/cmake/defaults/msvcdefaults.cmake https://github.com/PixarAnimationStudios/USD/blob/release/cmake/defaults/CXXDefaults.cmake

Do you think I should copy over from here what makes sense, with the idea that the plugin env is comparable to USD, or do we keep it as minimal as possible, knowing that people may need to in the future copy these over themselves?

rlei-weta commented 3 years ago

Hey @colevfx - glad that the triangle is alive!

Do you think I should copy over from here what makes sense, with the idea that the plugin env is comparable to USD, or do we keep it as minimal as possible, knowing that people may need to in the future copy these over themselves?

That's a great question. The current preference is to keep the platform-specific flags as minimal as possible for this project to be functional. It would be useful to add some comments/docs to note the difference in flags and mention the referenced links for the full set of flags.

Long term - it would be cool if some of these mandatory properties / flags could be target-based and transitive, as part of modern CMake practices. This would be a change required on the USD side (I don't think there's an issue open for this yet).

Thanks again for pushing this along :-)

cole-clark commented 3 years ago

No problem, I'm really thankful that this project is open source--there's a lot of amazing things to be done in this space!

You're right let's keep the defines really minimal for now 😄 I'm going to finish the PR tomorrow probably, just looking into seeing if I can get the test code to run as expected, I think the set_tests_properties needs a little tweaking but not totally sure.

rlei-weta commented 3 years ago

@colevfx - I've pushed up a CI job matrix for building and testing against NVIDIA pre-built binaries on the Linux platform: https://github.com/wetadigital/USDPluginExamples/pull/24

In case you ever wanted to take a stab at adding the Windows job variant, that could be a good place to start :-)

cole-clark commented 3 years ago

@rlei-weta yay! That's awesome, thanks for adding those! 😄

I'd love to take a stab at adding the Windows testing counterparts! Only thing is I first have to actually get the windows-side testing to pass. Gotta poke around and see why my current plugins / tests complain about:

File line 1, in from . import _usdTri ImportError: cannot import name '_usdTri' from 'usdpluginexamples.UsdTri'

If you have ever seen this kind of thing before while building USD Python (I'm guessing something is missing in the cmake that maybe boost/python expects) I'd love to know! Otherwise I was just going to rip open a USD/Python project and see what's missing. 🕵️

rlei-weta commented 3 years ago

Hey hey @colevfx!

It looks like the _usdTri python module is being installed as:

%USDPLUGINEXAMPLES_INSTALL_ROOT%/lib/python/usdpluginexamples/UsdTri/_usdTri.dll

I compared that to the USD installation and on Windows the expected file suffix for python modules seems to be .pyd instead of .dll. Manually renaming _usdTri.dll -> _usdTri.pyd rectified the import error.

Reference to Pixar USD where this suffix is set: https://github.com/PixarAnimationStudios/USD/blob/dev/cmake/macros/Private.cmake#L963

cole-clark commented 3 years ago

Awesome! Thanks for finding that!

I'm going to try fixing this and see if I can get the Windows testing working as soon as I can. 😄

cole-clark commented 3 years ago

So I think I got to the bottom of this issue! There's two actually, one which is the .pyd suffix issue that you identified that's pretty easy to fix. The second is that the windows tests will still fail even though the install will run error-free, probably because the tests need an extra lib path or two added to "PYTHONPATH" and maybe "PATH". I got it working from CLI, but gonna integrate into CMake and PR as soon as I can 😄

cole-clark commented 3 years ago

Just wanted to update; now that we have MSVC ctests working in the most recent merge, I'm happy to start looking into getting the Windows-equivalent git tests to work!

rlei-weta commented 3 years ago

@colevfx thanks so much for your continued help with this project. Really excited to see the windows tests online as we move forward - 🍻 !

cole-clark commented 3 years ago

Apologies for the delay, I've been taking a look at this and have hit a roadblock. It seems like the NVIDIA-supplied Windows binaries have some issues that aren't present on the Linux ones.

The Binaries are just a giant flat list of files with no structure or subdirectories. This doesn't match the structure that I'm seeing on Linux or on my versions of Usd compiled from source.
The number of items is suspiciously low--Windows 11,534 items vs. Linux Prebuilt Binary size of 19455 items
The Binaries appear to be compiled against a non-supported version of Python (3.6, instead of 3.7 as specified by VERSIONS.md)

For these reasons, the FindUSD.cmake as written won't work with these binaries.

I'm a little sketched out by using these since it's so different. Not sure it's a good idea to tweak FindUSD.cmake to make it work--if that's even possible which I'm not sure. A few ideas:

We ship our own pre-built Windows binaries to test against.
We compile from source. This is obviously very slow, so we could do it in a separate workflow perhaps?

I'd love to know what you think!

rlei-weta commented 3 years ago

Hi @colevfx - thanks for taking a stab at getting the Windows tests up and running.

The Binaries are just a giant flat list of files with no structure or subdirectories. This doesn't match the structure that I'm seeing on Linux or on my versions of Usd compiled from source.

I downloaded https://developer.nvidia.com/usd-21-02-binary-windows-python-3.6 and opened the zip (on Ubuntu). It seems to have the same directory hierarchy as the linux variant. Could there be an option to preserve directory structure when extracting on windows?

The Binaries appear to be compiled against a non-supported version of Python (3.6, instead of 3.7 as specified by VERSIONS.md)

Yes, that is indeed different from USD's specifications, although I think it should still work (one of USDPluginExamples' linux CI job runs against the py3.6 based binaries)

Hopefully you're able to extract the windows binaries into the directory structure as prescribed in the zip - although I wouldn't be surprised if there might be further tweaks needed for FindUSD.cmake. Would be happy to think further along with your suggestions if those NVIDIA binaries still prove difficult to build against. Let me know how that goes! Thanks

cole-clark commented 3 years ago

Ah, you are right! Awesome--I can't believe I missed that! Was able to get much further today, thanks for the help.

rlei-weta commented 3 years ago

Thanks @colevfx for all your excellent work and dedication to seeing this through. With the windows CI job in I think we can finally call this issue done!!!

rlei-weta commented 3 years ago

Tagging associated pull requests #23 #28 #30

cole-clark commented 3 years ago

No problem @rlei-weta ! Glad I could help 😄

wetadigital / USDPluginExamples

Windows Support #6