SG-43269 Improve core swap diagnostics with version mismatch detection

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,21 @@ def swap_core(cls, core_path):
             # Kick toolkit to re-import
             import tank
 
+        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.
+            if pipelineconfig_utils.is_version_older(
+                target_core_version, handler_core_version
+            ):
+                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"
+                )
+                log.error(error_message)
+                raise ImportError(error_message)
+
         finally:
             # Restore the list of warning filters.
             warnings.filters = original_filters
@@ -289,20 +313,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 +346,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

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
 

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,205 @@ 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:
+        #
+        #   <tmp_root>/
+        #     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: ``(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")
+            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, 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"
+        ) 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)
+            except Exception as exc:
+                raised = exc
+            return mock_log, raised
+
+    # ------------------------------------------------------------------
+    # Tests
+    # ------------------------------------------------------------------
+
+    def test_mismatch_message_logged_when_target_is_older(self):
+        """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. swap_core re-raises as ImportError.
+        """
+        mock_log, raised = self._run_swap(handler_version="v0.23.8")
+
+        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.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(
+            "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.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.  swap_core swallows
+        the exception in this case (treated as a non-fatal import warning).
+        """
+        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.error.call_args_list if "mismatch" in str(c).lower()
+        ]
+        self.assertFalse(
+            mismatch_calls,
+            "log.error should NOT be called with a mismatch message when "
+            "the target core is not older. Unexpected calls: %s" % mismatch_calls,
+        )