Add preliminary Windows support

[mati-nvidia]

Just sharing the progress so far for #19. I just realized I was working off of the "main" branch, but I'll rebase onto "dev" once I have everything working.

I have the unf and pyUnf projects building on Windows and I was able to run the Python tests successfully on Windows. I'm still fiddling with the GTest projects so that they build on Windows and so that I can run them to verify that's all working too.

[mati-nvidia]

For GTest, I think I'm currently blocked by this: https://gitlab.kitware.com/cmake/cmake/-/issues/21453

Trying to run the test discovery during the post build event fails because the test executables can't find the required DLLs (usd_*.dll, unf.dll, gtest*.dll).

[buddly27]

For GTest, I think I'm currently blocked by this: https://gitlab.kitware.com/cmake/cmake/-/issues/21453

Trying to run the test discovery during the post build event fails because the test executables can't find the required DLLs (usd_.dll, unf.dll, gtest.dll).

Ah true, I've run into that issue before and solved it by monkey patching gtest_discover_tests as follows:

if (WIN32)
    # Expand functions to copy dependent DLLs in the same folder after
    # building target to ensure that tests can run properly on Windows.
    macro(gtest_discover_tests NAME)
        add_custom_command(
            TARGET ${NAME} POST_BUILD
            COMMAND ${CMAKE_COMMAND}
            -E copy_if_different
            $<TARGET_RUNTIME_DLLS:${NAME}>
            $<TARGET_FILE_DIR:${NAME}>
            COMMAND_EXPAND_LISTS
        )
        _gtest_discover_tests(${NAME} ${ARGV})
    endmacro()
endif()

The issue is that TARGET_RUNTIME_DLLS will require us to use CMake 3.21 as a minimum version, but it might not be too unreasonable at this point

[mati-nvidia]

For GTest, I think I'm currently blocked by this: https://gitlab.kitware.com/cmake/cmake/-/issues/21453
Trying to run the test discovery during the post build event fails because the test executables can't find the required DLLs (usd__.dll, unf.dll, gtest_.dll).

Ah true, I've run into that issue before and solved it by monkey patching gtest_discover_tests as follows:

if (WIN32)
    # Expand functions to copy dependent DLLs in the same folder after
    # building target to ensure that tests can run properly on Windows.
    macro(gtest_discover_tests NAME)
        add_custom_command(
            TARGET ${NAME} POST_BUILD
            COMMAND ${CMAKE_COMMAND}
            -E copy_if_different
            $<TARGET_RUNTIME_DLLS:${NAME}>
            $<TARGET_FILE_DIR:${NAME}>
            COMMAND_EXPAND_LISTS
        )
        _gtest_discover_tests(${NAME} ${ARGV})
    endmacro()
endif()

The issue is that TARGET_RUNTIME_DLLS will require us to use CMake 3.21 as a minimum version, but it might not be too unreasonable at this point

Thanks for the monkey patch. I gave it a try, but it only copied: python310.dll, unf.dll, unfTest.dll. It didn't copy USD, TBB, GTest. I'm thinking maybe it's because of the NOTE here: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#genex:TARGET_RUNTIME_DLLS

I was also thinking that maybe it'd be better to use -E env to just extend $PATH instead of copying the DLLs.

[davvid]

For GTest, I think I'm currently blocked by this: https://gitlab.kitware.com/cmake/cmake/-/issues/21453
Trying to run the test discovery during the post build event fails because the test executables can't find the required DLLs (usd__.dll, unf.dll, gtest_.dll).

Ah true, I've run into that issue before and solved it by monkey patching gtest_discover_tests as follows:
[...]

Does Windows support an environment variable that can be used to get it to find DLLs? Basically the semantic equivalent of Linux's LD_LIBRARY_PATH.

Is that variable just called PATH on Windows? (ref: https://stackoverflow.com/questions/2463243/dll-search-on-windows)

If so, I'd be curious if things run ok if you modify PATH in your shell environment to point to the appropriate directories on your filesystem. If so then that might be an approach that we can use to configure tests.

If PATH is an option then that might be something that can be handled by using set_tests_properties on each of the tests to have them run with a customized environment.

https://cmake.org/cmake/help/latest/prop_test/ENVIRONMENT_MODIFICATION.html

Each of the gtest_discover_tests(target) calls generate a list variable called <target>_TESTS so we should be able to use that variable to configure the generate tests:

# test/unit/CMakeLists.txt
if (WIN32)
    macro(unf_configure_test target)
        set_tests_properties(
            ${${target}_TESTS} PROPERTIES
            ENVIRONMENT_MODIFICATION  PATH=path_list_prepend:${CMAKE_BINARY_DIR}/src
    )    
    endmacro()
    unf_configure_tests(testUnitBroker)
    unf_configure_tests(testUnitDispatcher)
    unf_configure_tests(testUnitDispatcherPlugin)
    unf_configure_tests(testUnitTransaction)
endif()

That might only handle the unf libraries. Some additional tweaks might be needed to add gtest's and usd's directories if that's a viable solution.

[davvid]

For GTest, I think I'm currently blocked by this: https://gitlab.kitware.com/cmake/cmake/-/issues/21453
Trying to run the test discovery during the post build event fails because the test executables can't find the required DLLs (usd__.dll, unf.dll, gtest_.dll).

If it's test discovery that's failing then my sug to use ENVIRONMENT_MODIFICATION may not be a viable solution (since it may be failing earlier before those test targets are registered).

Would an option be to avoid gtest_discover_tests altogether and just wire them up as regular add_executable and add_test calls? ctest won't know about each gtest-level test, but it'll at least know about each executable (which should be good enough).

If we do that then we should have more control over the environment.

[buddly27]

Thanks for the monkey patch. I gave it a try, but it only copied: python310.dll, unf.dll, unfTest.dll. It didn't copy USD, TBB, GTest. I'm thinking maybe it's because of the NOTE here: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#genex:TARGET_RUNTIME_DLLS

Hmm true, TARGET_RUNTIME_DLLS wants shared imported libraries, so it wouldn't work unless we modify all module finder scripts, which is not reasonable. So updating the environment is probably a better option indeed.

If so, I'd be curious if things run ok if you modify PATH in your shell environment to point to the appropriate directories on your filesystem. If so then that might be an approach that we can use to configure tests.

I ran some tests by tweaking the monkey-patch to extend the PATH environment variable for all discovered tests and the linking error disappeared (but not all tests are passing yet):

if (WIN32)
    # Monkeypatch Gtest discovery function to extend environment so that
    # relevant DLLs can be found on Windows. We use 'set_tests_properties'
    # to prevent issue with escaped semi-colons when passing environment
    # to 'gtest_discover_tests'.
    macro(gtest_discover_tests NAME)

        set(DEPS
            $<TARGET_FILE_DIR:unf>
            $<TARGET_FILE_DIR:unfTest>
            $<TARGET_FILE_DIR:usd::usd>
            $<TARGET_FILE_DIR:usd::sdf>
            $<TARGET_FILE_DIR:usd::tf>
            $<TARGET_FILE_DIR:usd::plug>
            $<TARGET_FILE_DIR:usd::arch>
            $<TARGET_FILE_DIR:usd::vt>
            $<TARGET_FILE_DIR:TBB::tbb>
            $<TARGET_FILE_DIR:GTest::gtest>
            )
        list(REMOVE_DUPLICATES DEPS)

        string(REPLACE ";" "\;" DEPS "${DEPS}")
        string(REPLACE ";" "\;" ENV_PATH "$ENV{PATH}")

        gtest_add_tests(TARGET ${NAME} TEST_LIST allTests)
        set_tests_properties(
            ${allTests} PROPERTIES ENVIRONMENT
            "PATH=${DEPS}\;${ENV_PATH}")
    endmacro()
endif()

We could use the TARGET_RUNTIME_DLL_DIRS expression generator for convenience, but it would only work with CMake 3.27 so it might be a bit restrictive. I feel there might be a better way than listing all dependent targets though.

Also, somehow tbb.dll is installed in the 'bin' folder when running the build_usd.py script, so the PATH variable needs to be updated accordingly in the shell before running cmake. I started to work on a windows CI script, so we can better reproduce this setup.

[davvid]

We could use the TARGET_RUNTIME_DLL_DIRS expression generator for convenience, but it would only work with CMake 3.27 so it might be a bit restrictive

If it's tucked inside of a if (WIN32) conditional block and able to be opted-out of with some other option variable then I personally think requiring CMake 3.27 would be just fine.

The opt-out variable would be for folks that manage their environment through other means (e.g. the chocolatey package manager for Windows) and don't need that setup. The default behavior can be to leverage that feature so that it's easier for Windows users.

[mati-nvidia]

@buddly27 Can you share what you have with the monkey patch? I tried the latest snippet that you shared, but couldn't get it working.

[buddly27]

@mati-nvidia I pushed my tests on a dummy PR to my fork of the project if you want to check the results:
https://github.com/buddly27/unf/pull/1/checks

I've got 21 tests failing out of 53 in Release mode, but all tests are failing in Debug mode, so there is still something I'm missing. We could try to catch up this week to discuss it if you have time!

Also, I added set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS 1) for convenience, but we should explicitly export the symbols for unfTest instead, as you did for unf.

[asluk]

Thanks @buddly27 ! 👍🏼

[mati-nvidia]

Yes! Thank you @buddly27 !