From 17c6186989eef9a9c5a7fe7d1508e08c9f3860da Mon Sep 17 00:00:00 2001 From: Paul Molodowitch Date: Thu, 10 Aug 2023 18:06:14 -0700 Subject: [PATCH] [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 | 98 +++++++++++++++++++ pxr/base/tf/__init__.py | 16 ++- .../testTfPyDllLinkModule/__init__.py | 26 +++++ pxr/base/tf/testenv/testTfPyDllLink.py | 97 ++++++++++++++++++ .../testTfPyDllLinkImplementationBad.cpp | 32 ++++++ .../testTfPyDllLinkImplementationGood.cpp | 32 ++++++ pxr/base/tf/testenv/testTfPyDllLinkModule.c | 69 +++++++++++++ 7 files changed, 369 insertions(+), 1 deletion(-) create mode 100644 pxr/base/tf/testenv/TfPyDllLinkTestEnv/testTfPyDllLinkModule/__init__.py create mode 100644 pxr/base/tf/testenv/testTfPyDllLink.py create mode 100644 pxr/base/tf/testenv/testTfPyDllLinkImplementationBad.cpp create mode 100644 pxr/base/tf/testenv/testTfPyDllLinkImplementationGood.cpp create mode 100644 pxr/base/tf/testenv/testTfPyDllLinkModule.c diff --git a/pxr/base/tf/CMakeLists.txt b/pxr/base/tf/CMakeLists.txt index c0e89f5373..ca96cf3e4b 100644 --- a/pxr/base/tf/CMakeLists.txt +++ b/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 diff --git a/pxr/base/tf/__init__.py b/pxr/base/tf/__init__.py index 30d236226e..dc2d122516 100644 --- a/pxr/base/tf/__init__.py +++ b/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 != '.': diff --git a/pxr/base/tf/testenv/TfPyDllLinkTestEnv/testTfPyDllLinkModule/__init__.py b/pxr/base/tf/testenv/TfPyDllLinkTestEnv/testTfPyDllLinkModule/__init__.py new file mode 100644 index 0000000000..7b302587dd --- /dev/null +++ b/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 diff --git a/pxr/base/tf/testenv/testTfPyDllLink.py b/pxr/base/tf/testenv/testTfPyDllLink.py new file mode 100644 index 0000000000..1d03536ae6 --- /dev/null +++ b/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() diff --git a/pxr/base/tf/testenv/testTfPyDllLinkImplementationBad.cpp b/pxr/base/tf/testenv/testTfPyDllLinkImplementationBad.cpp new file mode 100644 index 0000000000..3fed50d34d --- /dev/null +++ b/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 + +extern "C" +__declspec(dllexport) +int testTfPyDllLinkImplementation() { + printf("Bad implementation - returning 0xBAD...\n"); + return 0xBAD; +} \ No newline at end of file diff --git a/pxr/base/tf/testenv/testTfPyDllLinkImplementationGood.cpp b/pxr/base/tf/testenv/testTfPyDllLinkImplementationGood.cpp new file mode 100644 index 0000000000..65524bc4aa --- /dev/null +++ b/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 + +extern "C" +__declspec(dllexport) +int testTfPyDllLinkImplementation() { + printf("Good implementation - returning zero...\n"); + return 0; +} \ No newline at end of file diff --git a/pxr/base/tf/testenv/testTfPyDllLinkModule.c b/pxr/base/tf/testenv/testTfPyDllLinkModule.c new file mode 100644 index 0000000000..3a3e947cc4 --- /dev/null +++ b/pxr/base/tf/testenv/testTfPyDllLinkModule.c @@ -0,0 +1,69 @@ +/* + * 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. + */ + + +#define PY_SSIZE_T_CLEAN /* Make "s#" use Py_ssize_t rather than int. */ + +// On windows, Python.h will try to force linking against pythonX.YY_d.lib if +// _DEBUG is defined... we only want to do that if PXR_USE_DEBUG_PYTHON is on +#if (defined(_WIN32) || defined(_WIN64)) && defined(_DEBUG) && !defined(PXR_USE_DEBUG_PYTHON) + #undef _DEBUG + #include + #define _DEBUG +#else + #include +#endif + +/* Will be defined in testTfPyDllLinkImplementation.dll */ +int testTfPyDllLinkImplementation(); + +static PyObject * +testTfPyDllLinkModule_callImplementation(PyObject *self, PyObject *args) +{ + return PyLong_FromLong(testTfPyDllLinkImplementation()); +} + +static PyMethodDef testTfPyDllLinkModule_methods[] = { + /* The cast of the function is necessary since PyCFunction values + * only take two PyObject* parameters, and keywdarg_parrot() takes + * three. + */ + {"call_implementation", testTfPyDllLinkModule_callImplementation, METH_NOARGS, + "Make a call to a function implemented in another .dll."}, + {NULL, NULL, 0, NULL} /* sentinel */ +}; + +static struct PyModuleDef testTfPyDllLinkModule = { + PyModuleDef_HEAD_INIT, + "_testTfPyDllLink", + NULL, + -1, + testTfPyDllLinkModule_methods +}; + +PyMODINIT_FUNC +PyInit__testTfPyDllLinkModule(void) +{ + return PyModule_Create(&testTfPyDllLinkModule); +} \ No newline at end of file