diff --git a/pxr/base/tf/CMakeLists.txt b/pxr/base/tf/CMakeLists.txt index c0e89f5373..dda8f3af94 100644 --- a/pxr/base/tf/CMakeLists.txt +++ b/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 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