Merge pull request #2595 from pmolodo/pr/python-dll-directory-path-or…

…der-fix

[tf] fix for python dll loading order

(Internal change: 2303782)
(Internal change: 2304093)

pxr/base/tf/CMakeLists.txt

@@ -8,6 +8,103 @@ 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)
+    # Skip this this for static builds (BUILD_SHARED_LIBS set to False), as
+    # python modules/plugins require a shared library.
+    if (NOT (PXR_ENABLE_PYTHON_SUPPORT AND PXR_BUILD_TESTS AND WIN32 AND BUILD_SHARED_LIBS))
+        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 +388,8 @@ if(PXR_ENABLE_PYTHON_SUPPORT)
         CPPFILES
             testenv/testTfPyResultConversions.cpp
     )
+
+    add_py_dll_link_test()
 endif()
 
 pxr_build_test(testTfSIGFPE
@@ -392,6 +491,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;
+}