From 91d5bde0fb9fb5c4333a3c4773d671a082a6ecae Mon Sep 17 00:00:00 2001 From: Carlos Villavicencio Date: Wed, 20 May 2026 17:59:14 -0500 Subject: [PATCH 1/6] Validate core swap version mismatch and provide logs --- python/tank/bootstrap/import_handler.py | 41 +++++++++++++++++-------- python/tank/pipelineconfig_utils.py | 1 + 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/python/tank/bootstrap/import_handler.py b/python/tank/bootstrap/import_handler.py index 715d61e19..7dbfb0bb0 100644 --- a/python/tank/bootstrap/import_handler.py +++ b/python/tank/bootstrap/import_handler.py @@ -16,6 +16,7 @@ import warnings from .. import LogManager +from .. import pipelineconfig_utils log = LogManager.get_logger(__name__) @@ -49,8 +50,16 @@ def swap_core(cls, core_path): """ # make sure handler is up handler = cls._initialize() + handler_core_version = pipelineconfig_utils.get_currently_running_api_version() - log.debug("%s: Begin swapping core to %s" % (handler, core_path)) + target_core_version = pipelineconfig_utils.get_core_api_version( + os.path.dirname(os.path.dirname(os.path.dirname(core_path))) + ) + + log.debug( + "%s (version %s): Begin swapping core to %s (version %s)" + % (handler, handler_core_version, core_path, target_core_version) + ) # swapping core means our logging singleton will be reset. # make sure that there are no log handlers registered @@ -91,6 +100,20 @@ def swap_core(cls, core_path): # Kick toolkit to re-import import tank + except: + # If anything happens here, log an error and continue. + # Check the core versions handler_core_version and target_core_version, + # There might be a breaking change between the two versions. + if pipelineconfig_utils.is_version_older( + target_core_version, handler_core_version + ): + log.exception( + f"Core version mismatch: {target_core_version} might be older than" + f" {handler_core_version}. Please check the release notes for breaking" + f" changes: https://github.com/shotgunsoftware/tk-core/releases" + ) + raise + finally: # Restore the list of warning filters. warnings.filters = original_filters @@ -289,20 +312,14 @@ def find_spec(self, module_fullname, package_path=None, target=None): # later raise FileNotFoundError instead of the expected ImportError # when the module doesn't exist on disk. if os.path.isdir(os.path.join(package_path[0], module_name)): - module_file = os.path.join( - package_path[0], module_name, "__init__.py" - ) + module_file = os.path.join(package_path[0], module_name, "__init__.py") else: - module_file = os.path.join( - package_path[0], module_name + ".py" - ) + module_file = os.path.join(package_path[0], module_name + ".py") if not os.path.isfile(module_file): return None - loader = importlib.machinery.SourceFileLoader( - module_fullname, module_file - ) + loader = importlib.machinery.SourceFileLoader(module_fullname, module_file) spec = importlib.util.spec_from_loader(loader.name, loader) self._module_info[module_fullname] = spec except ImportError: @@ -328,9 +345,7 @@ def load_module(self, module_fullname): try: spec.loader.exec_module(module) except FileNotFoundError as e: - raise ImportError( - f"No module named '{module_fullname}'" - ) from e + raise ImportError(f"No module named '{module_fullname}'") from e # the module needs to know the loader so that reload() works module.__loader__ = self diff --git a/python/tank/pipelineconfig_utils.py b/python/tank/pipelineconfig_utils.py index 7a5543dce..380021363 100644 --- a/python/tank/pipelineconfig_utils.py +++ b/python/tank/pipelineconfig_utils.py @@ -24,6 +24,7 @@ from .util import StorageRoots from .util import ShotgunPath from .util.shotgun import get_deferred_sg_connection +from .util.version import is_version_older # noqa: F401 from .errors import TankError From 09a8233f5bf2bf6cebef13b342d7dac1fd28207f Mon Sep 17 00:00:00 2001 From: Carlos Villavicencio Date: Wed, 20 May 2026 18:41:17 -0500 Subject: [PATCH 2/6] Add exception class --- python/tank/bootstrap/import_handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/tank/bootstrap/import_handler.py b/python/tank/bootstrap/import_handler.py index 7dbfb0bb0..98d3eb379 100644 --- a/python/tank/bootstrap/import_handler.py +++ b/python/tank/bootstrap/import_handler.py @@ -100,7 +100,7 @@ def swap_core(cls, core_path): # Kick toolkit to re-import import tank - except: + except Exception: # If anything happens here, log an error and continue. # Check the core versions handler_core_version and target_core_version, # There might be a breaking change between the two versions. From a7963beedde0818bff21699211e3a2dab1db6d25 Mon Sep 17 00:00:00 2001 From: Carlos Villavicencio Date: Wed, 20 May 2026 18:46:35 -0500 Subject: [PATCH 3/6] Add tests --- tests/bootstrap_tests/test_import_handler.py | 178 +++++++++++++++++++ 1 file changed, 178 insertions(+) diff --git a/tests/bootstrap_tests/test_import_handler.py b/tests/bootstrap_tests/test_import_handler.py index 2e90088d7..8629e8091 100644 --- a/tests/bootstrap_tests/test_import_handler.py +++ b/tests/bootstrap_tests/test_import_handler.py @@ -13,6 +13,7 @@ import types import tempfile import shutil +from unittest.mock import patch from tank_test.tank_test_base import setUpModule # noqa from tank_test.tank_test_base import ShotgunTestBase @@ -239,3 +240,180 @@ def test_nonexistent_submodule_raises_import_error_not_file_not_found(self): "find_spec should return None for non-existent module QtPrintSupport, " "not a spec pointing to a missing file", ) + + +class TestSwapCoreVersionMismatch(ShotgunTestBase): + """Tests that swap_core emits a version-mismatch diagnostic on import failure. + + These tests do not perform a real core swap. Instead they patch the internal + helpers and inject a meta-path blocker so that the post-swap ``import tank`` + statement always raises ImportError, letting us assert the diagnostic path in + isolation. + """ + + # ------------------------------------------------------------------ + # A meta-path finder that unconditionally raises ImportError for + # 'tank'. Inserting this at the front of sys.meta_path simulates a + # broken target core without touching the filesystem. + # ------------------------------------------------------------------ + class _TankImportBlocker: + def find_spec(self, fullname, path, target=None): + if fullname == "tank": + raise ImportError("Simulated broken target core (test blocker)") + return None + + def setUp(self): + super().setUp() + + # Build a minimal fake target core layout: + # + # / + # install/ + # core/ + # info.yml <- declares version v0.22.4 + # python/ <- empty; no real tank modules + self._target_root = tempfile.mkdtemp(prefix="tk_swap_mismatch_test_") + core_dir = os.path.join(self._target_root, "install", "core") + os.makedirs(core_dir) + with open(os.path.join(core_dir, "info.yml"), "w") as fh: + fh.write('version: "v0.22.4"\n') + self._target_python = os.path.join(core_dir, "python") + os.makedirs(self._target_python) + + # Snapshot sys.meta_path and the tank-related sys.modules entries so + # tearDown can restore them regardless of what the test does. + self._original_meta_path = sys.meta_path[:] + self._stashed_tank_modules = { + k: v + for k, v in sys.modules.items() + if k in ("tank", "sgtk", "tank_vendor") + or k.startswith(("tank.", "sgtk.", "tank_vendor.")) + } + + def tearDown(self): + sys.meta_path[:] = self._original_meta_path + sys.modules.update(self._stashed_tank_modules) + shutil.rmtree(self._target_root, ignore_errors=True) + super().tearDown() + + # ------------------------------------------------------------------ + # Helpers + # ------------------------------------------------------------------ + + def _make_fake_swap_core(self, handler): + """Return a drop-in for ``_swap_core`` that simulates the two side effects + that matter for these tests: + + 1. Redirects the handler's ``_core_path`` to the new (empty) target. + 2. Clears all tank-namespaced modules from ``sys.modules`` so that + the subsequent ``import tank`` actually goes through the import + machinery (and hits our blocker). + 3. Inserts the blocker at the front of ``sys.meta_path``. + """ + + def _fake(core_path): + handler._core_path = core_path + for key in list(sys.modules): + if key in ("tank", "sgtk", "tank_vendor") or key.startswith( + ("tank.", "sgtk.", "tank_vendor.") + ): + del sys.modules[key] + sys.meta_path.insert(0, TestSwapCoreVersionMismatch._TankImportBlocker()) + + return _fake + + def _run_swap(self, handler_version, target_version_on_disk=None): + """Run ``CoreImportHandler.swap_core`` with controlled versions. + + :param handler_version: The version reported by + ``get_currently_running_api_version`` (the active/site core). + :param target_version_on_disk: If given, overwrite the ``info.yml`` in + the fake target root so ``get_core_api_version`` returns this value. + :returns: The mock logger captured during the call. + :raises ImportError: The patched import always raises; the caller + decides whether to catch it. + """ + if target_version_on_disk is not None: + info_path = os.path.join( + self._target_root, "install", "core", "info.yml" + ) + with open(info_path, "w") as fh: + fh.write('version: "%s"\n' % target_version_on_disk) + + handler = CoreImportHandler(self._target_python) + + with patch( + "tank.pipelineconfig_utils.get_currently_running_api_version", + return_value=handler_version, + ), patch.object( + CoreImportHandler, "_initialize", return_value=handler + ), patch.object( + handler, + "_swap_core", + side_effect=self._make_fake_swap_core(handler), + ), patch( + "tank.bootstrap.import_handler.log" + ) as mock_log: + with self.assertRaises(ImportError): + CoreImportHandler.swap_core(self._target_python) + return mock_log + + # ------------------------------------------------------------------ + # Tests + # ------------------------------------------------------------------ + + def test_mismatch_message_logged_when_target_is_older(self): + """log.exception is called with version details when target < running core. + + Scenario: Desktop ships v0.23.8 (handler_version) but the project config + still pins v0.22.4 (target_version). The post-swap ``import tank`` fails + and the diagnostic must surface both version numbers and a link to the + release notes. + """ + mock_log = self._run_swap(handler_version="v0.23.8") + + logged = " ".join(str(c) for c in mock_log.exception.call_args_list) + self.assertTrue( + mock_log.exception.called, + "Expected log.exception to be called for a version-mismatch but it " + "was not. All log calls: %s" % mock_log.mock_calls, + ) + self.assertIn( + "v0.22.4", + logged, + "Target version v0.22.4 should appear in the mismatch message.", + ) + self.assertIn( + "v0.23.8", + logged, + "Running version v0.23.8 should appear in the mismatch message.", + ) + self.assertIn( + "tk-core/releases", + logged, + "Release notes URL should appear in the mismatch message.", + ) + + def test_no_mismatch_message_when_target_is_not_older(self): + """log.exception is NOT called when the target core is the same version or newer. + + Scenario: The project config has already been updated to v0.23.8 while the + running core is v0.22.4. Even though the import still fails (the fake + blocker is always active), the version-mismatch branch must NOT fire + because the target is not older than the handler. + """ + mock_log = self._run_swap( + handler_version="v0.22.4", + target_version_on_disk="v0.23.8", + ) + + mismatch_calls = [ + c + for c in mock_log.exception.call_args_list + if "mismatch" in str(c).lower() + ] + self.assertFalse( + mismatch_calls, + "log.exception should NOT be called with a mismatch message when " + "the target core is not older. Unexpected calls: %s" % mismatch_calls, + ) From c7f095664407f48e5db52e59bc8156aa48070b4f Mon Sep 17 00:00:00 2001 From: Carlos Villavicencio Date: Wed, 20 May 2026 19:08:03 -0500 Subject: [PATCH 4/6] Fix test --- tests/bootstrap_tests/test_import_handler.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/bootstrap_tests/test_import_handler.py b/tests/bootstrap_tests/test_import_handler.py index 8629e8091..964e5fca8 100644 --- a/tests/bootstrap_tests/test_import_handler.py +++ b/tests/bootstrap_tests/test_import_handler.py @@ -334,9 +334,7 @@ def _run_swap(self, handler_version, target_version_on_disk=None): decides whether to catch it. """ if target_version_on_disk is not None: - info_path = os.path.join( - self._target_root, "install", "core", "info.yml" - ) + info_path = os.path.join(self._target_root, "install", "core", "info.yml") with open(info_path, "w") as fh: fh.write('version: "%s"\n' % target_version_on_disk) @@ -353,7 +351,15 @@ def _run_swap(self, handler_version, target_version_on_disk=None): side_effect=self._make_fake_swap_core(handler), ), patch( "tank.bootstrap.import_handler.log" - ) as mock_log: + ) as mock_log, patch( + # swap_core calls LogManager().uninitialize_base_file_handler() on + # the process-wide singleton before the swap, and relies on the + # post-swap `import tank` to re-initialize it. Because our test + # intentionally makes that import fail, the handler is never + # restored, which leaks state into subsequent test classes. Mock + # the local import so the singleton is never touched. + "tank.log.LogManager" + ): with self.assertRaises(ImportError): CoreImportHandler.swap_core(self._target_python) return mock_log @@ -408,9 +414,7 @@ def test_no_mismatch_message_when_target_is_not_older(self): ) mismatch_calls = [ - c - for c in mock_log.exception.call_args_list - if "mismatch" in str(c).lower() + c for c in mock_log.exception.call_args_list if "mismatch" in str(c).lower() ] self.assertFalse( mismatch_calls, From 1117e6fd9cc59fdd6e86a3b97fe6ed99079e93f6 Mon Sep 17 00:00:00 2001 From: Carlos Villavicencio Date: Thu, 21 May 2026 11:09:06 -0500 Subject: [PATCH 5/6] Improve UI error display --- python/tank/bootstrap/import_handler.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/python/tank/bootstrap/import_handler.py b/python/tank/bootstrap/import_handler.py index 98d3eb379..d0d280c31 100644 --- a/python/tank/bootstrap/import_handler.py +++ b/python/tank/bootstrap/import_handler.py @@ -107,12 +107,13 @@ def swap_core(cls, core_path): if pipelineconfig_utils.is_version_older( target_core_version, handler_core_version ): - log.exception( + error_message = ( f"Core version mismatch: {target_core_version} might be older than" f" {handler_core_version}. Please check the release notes for breaking" f" changes: https://github.com/shotgunsoftware/tk-core/releases" ) - raise + log.error(error_message) + raise ImportError(error_message) finally: # Restore the list of warning filters. From 999312ea0ea1c438271312cea2ad2883b3a99f03 Mon Sep 17 00:00:00 2001 From: Carlos Villavicencio Date: Thu, 21 May 2026 11:35:15 -0500 Subject: [PATCH 6/6] Fix tests --- tests/bootstrap_tests/test_import_handler.py | 55 ++++++++++++++------ 1 file changed, 38 insertions(+), 17 deletions(-) diff --git a/tests/bootstrap_tests/test_import_handler.py b/tests/bootstrap_tests/test_import_handler.py index 964e5fca8..61fbca574 100644 --- a/tests/bootstrap_tests/test_import_handler.py +++ b/tests/bootstrap_tests/test_import_handler.py @@ -329,9 +329,9 @@ def _run_swap(self, handler_version, target_version_on_disk=None): ``get_currently_running_api_version`` (the active/site core). :param target_version_on_disk: If given, overwrite the ``info.yml`` in the fake target root so ``get_core_api_version`` returns this value. - :returns: The mock logger captured during the call. - :raises ImportError: The patched import always raises; the caller - decides whether to catch it. + :returns: ``(mock_log, raised_exception)`` - the mock logger captured + during the call and the exception raised by ``swap_core``, or + ``None`` if it returned normally. """ if target_version_on_disk is not None: info_path = os.path.join(self._target_root, "install", "core", "info.yml") @@ -359,29 +359,44 @@ def _run_swap(self, handler_version, target_version_on_disk=None): # restored, which leaks state into subsequent test classes. Mock # the local import so the singleton is never touched. "tank.log.LogManager" - ): - with self.assertRaises(ImportError): + ) as mock_lm: + # uninitialize_base_file_handler must return None (no active log + # file in tests) so that the `if prev_log_file:` guard in + # swap_core is False. Without this, swap_core tries to call + # tank.LogManager() after the import failed, hitting an + # UnboundLocalError because the `tank` assignment never completed. + mock_lm.return_value.uninitialize_base_file_handler.return_value = None + raised = None + try: CoreImportHandler.swap_core(self._target_python) - return mock_log + except Exception as exc: + raised = exc + return mock_log, raised # ------------------------------------------------------------------ # Tests # ------------------------------------------------------------------ def test_mismatch_message_logged_when_target_is_older(self): - """log.exception is called with version details when target < running core. + """log.error is called with version details when target < running core. Scenario: Desktop ships v0.23.8 (handler_version) but the project config still pins v0.22.4 (target_version). The post-swap ``import tank`` fails and the diagnostic must surface both version numbers and a link to the - release notes. + release notes. swap_core re-raises as ImportError. """ - mock_log = self._run_swap(handler_version="v0.23.8") + mock_log, raised = self._run_swap(handler_version="v0.23.8") - logged = " ".join(str(c) for c in mock_log.exception.call_args_list) + self.assertIsInstance( + raised, + ImportError, + "swap_core should re-raise as ImportError on a version mismatch. " + "Got: %s" % raised, + ) + logged = " ".join(str(c) for c in mock_log.error.call_args_list) self.assertTrue( - mock_log.exception.called, - "Expected log.exception to be called for a version-mismatch but it " + mock_log.error.called, + "Expected log.error to be called for a version-mismatch but it " "was not. All log calls: %s" % mock_log.mock_calls, ) self.assertIn( @@ -401,23 +416,29 @@ def test_mismatch_message_logged_when_target_is_older(self): ) def test_no_mismatch_message_when_target_is_not_older(self): - """log.exception is NOT called when the target core is the same version or newer. + """log.error is NOT called when the target core is the same version or newer. Scenario: The project config has already been updated to v0.23.8 while the running core is v0.22.4. Even though the import still fails (the fake blocker is always active), the version-mismatch branch must NOT fire - because the target is not older than the handler. + because the target is not older than the handler. swap_core swallows + the exception in this case (treated as a non-fatal import warning). """ - mock_log = self._run_swap( + mock_log, raised = self._run_swap( handler_version="v0.22.4", target_version_on_disk="v0.23.8", ) + self.assertIsNone( + raised, + "swap_core should NOT raise when the target core is not older. " + "Got: %s" % raised, + ) mismatch_calls = [ - c for c in mock_log.exception.call_args_list if "mismatch" in str(c).lower() + c for c in mock_log.error.call_args_list if "mismatch" in str(c).lower() ] self.assertFalse( mismatch_calls, - "log.exception should NOT be called with a mismatch message when " + "log.error should NOT be called with a mismatch message when " "the target core is not older. Unexpected calls: %s" % mismatch_calls, )