Merge pull request #223 from plotly/andrew/ldd-check

Andrew/ldd check

Author: ayjayt

.github/workflows/publish_testpypi.yml

@@ -50,7 +50,7 @@ jobs:
         timeout-minutes: 8
 
       - name: Test (Debug)
-        if: runner.debug
+        if: ${{ runner.debug && matrix.os != 'ubuntu-latest' }}
         run: uv run --no-sync poe debug-test
         timeout-minutes: 20
 

.github/workflows/test.yml

@@ -22,7 +22,6 @@ jobs:
         if: ${{ matrix.os == 'ubuntu-latest' }}
         run: sudo apt-get update && sudo apt-get install xvfb
         timeout-minutes: 1
-
       - name: Install choreographer
         run:  uv sync --no-sources --all-extras
       - name: Install google-chrome-for-testing
@@ -42,7 +41,7 @@ jobs:
         timeout-minutes: 7
 
       - name: Test (Debug)
-        if: runner.debug
+        if: ${{ runner.debug && matrix.os != 'ubuntu-latest' }}
         run: uv run --no-sources poe debug-test
         timeout-minutes: 20
 

CHANGELOG.txt

@@ -1,3 +1,8 @@
+v1.0.6
+- Package in chromium deps and use them if ldd shows they're needed
+- Add env var LDD_FAIL and flag --ldd_fail to always fail if deps needed
+- Add env var FORCED_PACKAGED_DEPS and flag --forced-packaged-deps to do as read
+- Fix some API bugs in choreo_diagnose
 v1.0.5
 - Add Browser.is_isolated() returning if /tmp is sandboxed
 v1.0.4

choreographer/browser_async.py

@@ -101,14 +101,6 @@ def __init__(
         self._channel = channel_cls()
         self._broker = Broker(self, self._channel)
         self._browser_impl = browser_cls(self._channel, path, **kwargs)
-        if hasattr(browser_cls, "logger_parser"):
-            parser = browser_cls.logger_parser
-        else:
-            parser = None
-        self._logger_pipe, _ = logistro.getPipeLogger(
-            "browser_proc",
-            parser=parser,
-        )
 
     def is_isolated(self) -> bool:
         """Return if process is isolated."""
@@ -119,13 +111,23 @@ async def open(self) -> None:
         _logger.info("Opening browser.")
         if await self._is_open():
             raise RuntimeError("Can't re-open the browser")
-        cli = self._browser_impl.get_cli()
-        stderr = self._logger_pipe
-        env = self._browser_impl.get_env()
-        args = self._browser_impl.get_popen_args()
 
         # asyncio's equiv doesn't work in all situations
+        if hasattr(self._browser_impl, "logger_parser"):
+            parser = self._browser_impl.logger_parser
+        else:
+            parser = None
+        self._logger_pipe, _ = logistro.getPipeLogger(
+            "browser_proc",
+            parser=parser,
+        )
+
         def run() -> subprocess.Popen[bytes]:
+            self._browser_impl.pre_open()
+            cli = self._browser_impl.get_cli()
+            stderr = self._logger_pipe
+            env = self._browser_impl.get_env()
+            args = self._browser_impl.get_popen_args()
             return subprocess.Popen(  # noqa: S603
                 cli,
                 stderr=stderr,
@@ -151,8 +153,8 @@ def run() -> subprocess.Popen[bytes]:
             raise BrowserFailedError(
                 "The browser seemed to close immediately after starting.",
                 "You can set the `logging.Logger` level lower to see more output.",
-                "You may try installed a known working copy of chrome from ",
-                "`$ choreo_get_chome`. It may be your copy auto-updated.",
+                "You may try installing a known working copy of Chrome by running ",
+                "`$ choreo_get_chrome`. It may be your copy auto-updated.",
             ) from e
 
     async def __aenter__(self) -> Self:

choreographer/browsers/_interface_type.py

@@ -21,6 +21,7 @@ def __init__(
         path: Path | str | None = None,
         **kwargs: Any,
     ) -> None: ...
+    def pre_open(self) -> None: ...
     def get_popen_args(self) -> Mapping[str, Any]: ...
     def get_cli(self) -> Sequence[str]: ...
     def get_env(self) -> MutableMapping[str, str]: ...

choreographer/browsers/chromium.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import argparse
 import os
 import platform
 import re
@@ -30,8 +31,30 @@
     Path(__file__).resolve().parent / "_unix_pipe_chromium_wrapper.py"
 )
 
+_packaged_chromium_libs = Path(__file__).resolve().parent / "packaged_chromium_libs"
+
 _logger = logistro.getLogger(__name__)
 
+_parser = argparse.ArgumentParser(add_help=False)
+_g = _parser.add_mutually_exclusive_group()
+_g.add_argument(
+    "--ldd-fail",
+    action="store_true",
+    dest="ldd_fail",
+    default="LDD_FAIL" in os.environ,
+    help="Will cause to fail if not right deps.",
+)
+
+_g.add_argument(
+    "--force-packaged-deps",
+    action="store_true",
+    dest="force_deps",
+    default="FORCE_PACKAGED_DEPS" in os.environ,
+    help="Will force us to try local deps.",
+)
+
+_args, _ = _parser.parse_known_args()
+
 
 def _is_exe(path: str | Path) -> bool:
     try:
@@ -85,6 +108,61 @@ def logger_parser(
         # we just eliminate their stamp, we dont' extract it
         return True
 
+    def _need_libs(self) -> bool:  # noqa: C901 complexity
+        if self.skip_local:
+            _logger.debug(
+                "If we HAVE to skip local.",
+            )
+            if _args.force_deps:
+                _logger.warning(
+                    "We can NOT force deps in these security conditions, "
+                    "we must use locals.",
+                )
+            return False
+        _logger.debug("Checking for libs needed.")
+        if platform.system() != "Linux":
+            _logger.debug("We're not in linux, so no need for check.")
+            if _args.ldd_fail:
+                _logger.warning("You asked for ldd-fail but we're not on linux.")
+            if _args.force_deps:
+                _logger.warning("You asked for packages deps but we're not on linux.")
+            return False
+        if _args.force_deps:
+            _logger.debug("Force using packaged deps.")
+            return True
+        p = None
+        try:
+            _logger.debug(f"Trying ldd {self.path}")
+            p = subprocess.run(  # noqa: S603, validating run with variables
+                [  # noqa: S607 path is all we have
+                    "ldd",
+                    str(self.path),
+                ],
+                capture_output=True,
+                timeout=5,
+                check=True,
+            )
+        except BaseException as e:
+            msg = "ldd failed."
+            if _args.ldd_fail:
+                _logger.exception(msg)
+                raise
+            else:
+                stderr = p.stderr.decode() if p and p.stderr else None
+                _logger.warning(
+                    msg  # noqa: G003 + in log
+                    + f" e: {e}, stderr: {stderr}",
+                )
+                return True
+        if b"not found" in p.stdout:
+            msg = "Found deps missing in chrome"
+            if _args.ldd_fail:
+                raise RuntimeError(msg + f" {p.stdout.decode()}")
+            _logger.debug(msg + f" {p.stdout.decode()}")  # noqa: G003 + in log
+            return True
+        _logger.debug("No problems found with dependencies")
+        return False
+
     def __init__(
         self,
         channel: ChannelInterface,
@@ -144,12 +222,15 @@ def __init__(
                 "please see documentation.",
             )
         _logger.info(f"Found chromium path: {self.path}")
+
         self._channel = channel
         if not isinstance(channel, Pipe):
             raise NotImplementedError("Websocket style channels not implemented yet.")
 
         self._is_isolated = "snap" in str(self.path)
 
+    def pre_open(self) -> None:
+        """Prepare browser for opening."""
         self.tmp_dir = TmpDirectory(
             path=self._tmp_dir_path,
             sneak=self._is_isolated,
@@ -247,8 +328,14 @@ def get_cli(self) -> Sequence[str]:
 
     def get_env(self) -> MutableMapping[str, str]:
         """Return the env needed for chromium."""
-        _logger.debug("Returning env: same env, no modification.")
-        return os.environ.copy()
+        env = os.environ.copy()
+        if self._need_libs():
+            original = env.get("LD_LIBRARY_PATH", "")
+            env["LD_LIBRARY_PATH"] = f"{_packaged_chromium_libs!s}:{original}"
+            _logger.debug(
+                f"Added LD_LIBRARY_PATH={env['LD_LIBRARY_PATH']!s} to env vars.",
+            )
+        return env
 
     def clean(self) -> None:
         """Clean up any leftovers form browser, like tmp files."""