[tf] fix for python dll loading order

Tf uses WindowsImportWrapper to add directories on the
PATH to the list of allowable directories to find .dll
files, so python modules relying on finding .dlls on
the PATH will continue to work.

However, it seems that os.add_dll_directory works in a
"most recently added wins" manner, so we need to reverse
the order that we call it for entries on the PATH.

pxr/base/tf/CMakeLists.txt

@@ -8,6 +8,101 @@ if(WIN32)
     set(WINLIBS Shlwapi.lib)
 endif()
 
+# Add a test that WindowsImportWrapper adds entries on the PATH in the correct order, to ensure
+# that the correct dlls needed by python modules are loaded
+function(add_py_dll_link_test)
+    if (NOT (PXR_ENABLE_PYTHON_SUPPORT AND PXR_BUILD_TESTS AND WIN32))
+        return()
+    endif()
+
+    # The structure we are going to make is:
+    #  - python test script: testTfPyDllLink.py
+    #  - python module: testTfPyDllLinkModule/__init__.py
+    #  - python c extension: testTfPyDllLinkModule/_testTfPyDllLinkModule.pyd
+    #  - cpp lib: testTfPyDllLinkImplementation.dll
+    #    - for the cpp lib that the python extension links to, we create two dlls with the same name
+    #      - a "Good" one which returns 0, and
+    #      - a "Bad" one which returns 0xBAD
+    #    - they are placed in subdirs, in both build and install dirs:
+    #      - ImplementationGood/testTfPyDllLink.dll
+    #      - ImplementationBad/testTfPyDllLink.dll
+    #    - this will allow us to test which is loading by adding different directories to the PATH
+
+    # Create the good/bad implementations: testTfPyDllLinkImplementation.dll
+    foreach(implementation_suffix Good Bad)
+        set(implementation_name "testTfPyDllLinkImplementation${implementation_suffix}")
+        add_library(${implementation_name} SHARED
+            "testenv/${implementation_name}.cpp"
+        )
+
+        set(directory_name "Implementation${implementation_suffix}")
+
+        set_target_properties(${implementation_name} PROPERTIES
+            OUTPUT_NAME "testTfPyDllLinkImplementation"
+            RUNTIME_OUTPUT_DIRECTORY "${directory_name}"  # for the windows .dll
+            ARCHIVE_OUTPUT_DIRECTORY "${directory_name}"  # for the windows .lib
+        )
+
+        install(
+            TARGETS ${implementation_name}
+            RUNTIME DESTINATION "tests/ctest/TfPyDllLinkTestEnv/testTfPyDllLinkModule/Implementation${implementation_suffix}"
+        )
+    endforeach()
+
+    # create the _testTfPyDllLinkModule.pyd compiled python module
+    add_library(testTfPyDllLinkModule SHARED
+        "testenv/testTfPyDllLinkModule.c"
+    )
+    add_dependencies(python testTfPyDllLinkModule)
+
+    set(module_name testTfPyDllLinkModule)
+    if (PXR_USE_DEBUG_PYTHON)
+        target_compile_definitions(testTfPyDllLinkModule PRIVATE PXR_USE_DEBUG_PYTHON)
+        # On Windows when compiling with debug python the library must be named with _d.
+        set(module_filename "_${module_name}_d")
+    else()
+        set(module_filename "_${module_name}")
+    endif()
+
+    set_target_properties(testTfPyDllLinkModule PROPERTIES
+        PREFIX ""
+        # Python modules must be suffixed with .pyd on Windows.
+        SUFFIX ".pyd"
+        OUTPUT_NAME ${module_filename}
+    )
+    target_include_directories(testTfPyDllLinkModule
+        SYSTEM
+        PRIVATE
+            ${PYTHON_INCLUDE_DIRS}
+    )
+
+    target_link_libraries(testTfPyDllLinkModule PRIVATE ${PYTHON_LIBRARIES} testTfPyDllLinkImplementation)
+
+    # tell our python module to link against the "good" implementation while building
+    # (When actually running the test, which dll is found is controlled by the PATH env var)
+    add_dependencies(testTfPyDllLinkModule testTfPyDllLinkImplementationGood)
+    target_link_directories(testTfPyDllLinkModule PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/ImplementationGood")
+
+    install(
+        TARGETS testTfPyDllLinkModule
+        RUNTIME DESTINATION "tests/ctest/TfPyDllLinkTestEnv/testTfPyDllLinkModule"
+    )
+
+    # install the testenv dir, which has the testTfPyDllLinkModule.py python module
+    pxr_install_test_dir(
+        SRC testenv/TfPyDllLinkTestEnv
+        DEST TfPyDllLinkTestEnv
+    )
+
+    # finally register the test itself
+    pxr_register_test(testTfPyDllLink
+        PYTHON
+        COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testTfPyDllLink"
+        TESTENV TfPyDllLinkTestEnv
+    )
+
+endfunction()
+
 pxr_library(tf
     LIBRARIES
         arch
@@ -291,6 +386,8 @@ if(PXR_ENABLE_PYTHON_SUPPORT)
         CPPFILES
             testenv/testTfPyResultConversions.cpp
     )
+
+    add_py_dll_link_test()
 endif()
 
 pxr_build_test(testTfSIGFPE
@@ -392,6 +489,7 @@ pxr_test_scripts(
     testenv/testTfNamedTemporaryFile.py
     testenv/testTfPathUtils.py
     testenv/testTfPython.py
+    testenv/testTfPyDllLink.py
     testenv/testTfPyNotice.py
     testenv/testTfPyOptional.py
     testenv/testTfPyScopeDescription.py

pxr/base/tf/__init__.py

@@ -40,7 +40,21 @@ def WindowsImportWrapper():
         import_paths = os.getenv('PXR_USD_WINDOWS_DLL_PATH')
         if import_paths is None:
             import_paths = os.getenv('PATH', '')
-        for path in import_paths.split(os.pathsep):
+        # the underlying windows API call, AddDllDirectory, states that:
+        #
+        # > If AddDllDirectory is used to add more than one directory to the
+        # > process DLL search path, the order in which those directories are
+        # > searched is unspecified.
+        #
+        # https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-adddlldirectory
+        #
+        # However, in practice, it seems that the most-recently-added ones
+        # take precedence - so, reverse the order of entries in PATH to give
+        # it the same precedence
+        #
+        # Note that we have a test (testTfPyDllLink) to alert us if this
+        # undefined behavior changes.
+        for path in reversed(import_paths.split(os.pathsep)):
             # Calling add_dll_directory raises an exception if paths don't
             # exist, or if you pass in dot
             if os.path.exists(path) and path != '.':

pxr/base/tf/testenv/TfPyDllLinkTestEnv/testTfPyDllLinkModule/__init__.py

@@ -0,0 +1,26 @@
+#
+# Copyright 2023 Pixar
+#
+# Licensed under the Apache License, Version 2.0 (the "Apache License")
+# with the following modification; you may not use this file except in
+# compliance with the Apache License and the following modification to it:
+# Section 6. Trademarks. is deleted and replaced with:
+#
+# 6. Trademarks. This License does not grant permission to use the trade
+#    names, trademarks, service marks, or product names of the Licensor
+#    and its affiliates, except as required to comply with Section 4(c) of
+#    the License and to reproduce the content of the NOTICE file.
+#
+# You may obtain a copy of the Apache License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the Apache License with the above modification is
+# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the Apache License for the specific
+# language governing permissions and limitations under the Apache License.
+#
+from pxr import Tf
+Tf.PreparePythonModule()
+del Tf

pxr/base/tf/testenv/testTfPyDllLink.py

@@ -0,0 +1,97 @@
+#!/pxrpythonsubst
+#
+# Copyright 2023 Pixar
+#
+# Licensed under the Apache License, Version 2.0 (the "Apache License")
+# with the following modification; you may not use this file except in
+# compliance with the Apache License and the following modification to it:
+# Section 6. Trademarks. is deleted and replaced with:
+#
+# 6. Trademarks. This License does not grant permission to use the trade
+#    names, trademarks, service marks, or product names of the Licensor
+#    and its affiliates, except as required to comply with Section 4(c) of
+#    the License and to reproduce the content of the NOTICE file.
+#
+# You may obtain a copy of the Apache License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the Apache License with the above modification is
+# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the Apache License for the specific
+# language governing permissions and limitations under the Apache License.
+#
+
+import os
+import subprocess
+import sys
+import unittest
+
+from typing import Dict, Iterable
+
+
+TEST_ROOT = os.getcwd()
+MODULE_NAME = "testTfPyDllLinkModule"
+MODULE_DIR = os.path.join(TEST_ROOT, MODULE_NAME)
+if not os.path.isdir(MODULE_DIR):
+    raise ValueError(f"expected module directory not found - testenv not copied correctly: {MODULE_DIR}")
+
+BAD_PATH = os.path.join(MODULE_DIR, "ImplementationBad")
+GOOD_PATH = os.path.join(MODULE_DIR, "ImplementationGood")
+
+for implementation_dir in (BAD_PATH, GOOD_PATH):
+    dll_path = os.path.join(implementation_dir, "testTfPyDllLinkImplementation.dll")
+    if not os.path.isfile(dll_path):
+        raise ValueError(f"expected dll not found: {dll_path}")
+
+
+def append_to_pathvar(env: Dict[str, str], var_name: str, new_entries: Iterable[str]):
+    """Appends the given new_entries to the end of the path variable given by var_name"""
+    path = env.get(var_name, "").split(os.pathsep)
+    for x in new_entries:
+        if x not in path:
+            path.append(x)
+    env[var_name] = os.pathsep.join(path)
+
+
+class TestPyDllLink(unittest.TestCase):
+    """Test that on windows, by modifying the PATH, we can import required
+    python modules, and their .dlls, and they are searched in the correct order.
+    """
+
+    # We can't run the tests inside this process, as we want to change the
+    # linkage we get when we import the same module name - so we need to shell
+    # out
+
+    def run_import_test(self, path: Iterable[str]) -> int:
+        new_env = dict(os.environ)
+        append_to_pathvar(new_env, "PATH", path)
+        append_to_pathvar(new_env, "PYTHONPATH", [TEST_ROOT])
+        cmd = f"import sys, {MODULE_NAME}; sys.exit({MODULE_NAME}.call_implementation())"
+        return subprocess.call([sys.executable, "-c", cmd], env=new_env)
+
+    def test_no_path(self):
+        # with nothing adding to the path, it should fail to find testTfPyDllLinkImplementation.dll, and
+        # error (with something other than 0xBAD)
+        self.assertNotIn(self.run_import_test([]), [0, 0xBAD])
+
+    def test_bad_path(self):
+        # with only the bad path, it should return 0xBAD
+        self.assertEqual(self.run_import_test([BAD_PATH]), 0xBAD)
+
+    def test_good_path(self):
+        # with only the good path, it should return 0
+        self.assertEqual(self.run_import_test([GOOD_PATH]), 0)
+
+    def test_bad_good_path(self):
+        # with both bad and good path, but bad path first, should return 0xBAD
+        self.assertEqual(self.run_import_test([BAD_PATH, GOOD_PATH]), 0xBAD)
+
+    def test_good_bad_path(self):
+        # with both good and bad path, but good path first, should return 0
+        self.assertEqual(self.run_import_test([GOOD_PATH, BAD_PATH]), 0)
+
+
+if __name__ == '__main__':
+    unittest.main()

pxr/base/tf/testenv/testTfPyDllLinkImplementationBad.cpp

@@ -0,0 +1,32 @@
+//
+// Copyright 2023 Pixar
+//
+// Licensed under the Apache License, Version 2.0 (the "Apache License")
+// with the following modification; you may not use this file except in
+// compliance with the Apache License and the following modification to it:
+// Section 6. Trademarks. is deleted and replaced with:
+//
+// 6. Trademarks. This License does not grant permission to use the trade
+//    names, trademarks, service marks, or product names of the Licensor
+//    and its affiliates, except as required to comply with Section 4(c) of
+//    the License and to reproduce the content of the NOTICE file.
+//
+// You may obtain a copy of the Apache License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the Apache License with the above modification is
+// distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the Apache License for the specific
+// language governing permissions and limitations under the Apache License.
+//
+
+#include <cstdio>
+
+extern "C"
+__declspec(dllexport)
+int testTfPyDllLinkImplementation() {
+    printf("Bad implementation - returning 0xBAD...\n");
+    return 0xBAD;
+}

pxr/base/tf/testenv/testTfPyDllLinkImplementationGood.cpp

@@ -0,0 +1,32 @@
+//
+// Copyright 2023 Pixar
+//
+// Licensed under the Apache License, Version 2.0 (the "Apache License")
+// with the following modification; you may not use this file except in
+// compliance with the Apache License and the following modification to it:
+// Section 6. Trademarks. is deleted and replaced with:
+//
+// 6. Trademarks. This License does not grant permission to use the trade
+//    names, trademarks, service marks, or product names of the Licensor
+//    and its affiliates, except as required to comply with Section 4(c) of
+//    the License and to reproduce the content of the NOTICE file.
+//
+// You may obtain a copy of the Apache License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the Apache License with the above modification is
+// distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the Apache License for the specific
+// language governing permissions and limitations under the Apache License.
+//
+
+#include <cstdio>
+
+extern "C"
+__declspec(dllexport)
+int testTfPyDllLinkImplementation() {
+    printf("Good implementation - returning zero...\n");
+    return 0;
+}