Merge pull request #286 from plotly/andrew/roadmap

andrew/roadmap

Author: ayjayt

CHANGELOG.txt

@@ -1,3 +1,5 @@
+v1.3.0rc1
+- We now look for old download path as well as new download path
 v1.3.0rc0
 - Change to process group for better killing of multi-process chrome
 - Add argument to Session/Target `send_command(..., *, with_perf: bool)` to

ROADMAP.md

@@ -2,7 +2,7 @@
 
 - [ ] What happens to the underlying task when we cancel a future in a
   `protocol.devtools_async_helpers` timeout situation?
-- [ ] Extract local download check to chromium implementation class
+- [x] Extract local download check to chromium implementation class
 - [x] Fix up browser deps error (eliminate in-package analysis)
 - [x] Switch to process group and kill that to catch all chromium children
 - [x] Add helpers for running javascript

src/choreographer/browsers/chromium.py

@@ -16,6 +16,10 @@
     import msvcrt
 
 from choreographer.channels import Pipe
+from choreographer.cli._cli_utils import (
+    get_chrome_download_path,
+    get_old_chrome_download_path,
+)
 from choreographer.utils import TmpDirectory, get_browser_path
 
 from ._chrome_constants import chromium_based_browsers
@@ -75,13 +79,23 @@ def find_browser(
         *,
         skip_local: bool,
         skip_typical: bool = False,
+        verify_local: bool = False,
     ) -> str | None:
         """Find a chromium based browser."""
+        if not skip_local:
+            if ((p := get_chrome_download_path(mkdir=False)) and p.exists()) or (
+                (p := get_old_chrome_download_path()) and p.exists()
+            ):
+                return str(p)
+            elif verify_local:
+                raise RuntimeError("verify_local set to True, local not found.")
+        elif verify_local:
+            raise ValueError("Cannot set both skip_local and verify_local.")
+
         for name, browser_data in chromium_based_browsers.items():
             _logger.debug(f"Looking for a {name} browser.")
             path = get_browser_path(
                 executable_names=browser_data.exe_names,
-                skip_local=skip_local,
                 ms_prog_id=browser_data.ms_prog_id,
             )
             if not path and not skip_typical:

src/choreographer/cli/_cli_utils.py

@@ -14,7 +14,7 @@
 
 import logistro
 
-from choreographer.cli.defaults import default_download_path
+from choreographer.cli.defaults import default_download_path, old_download_path
 
 _logger = logistro.getLogger(__name__)
 
@@ -42,36 +42,44 @@ def get_google_supported_platform_string() -> str | None:
     return platform_string
 
 
-def get_chrome_download_path() -> Path | None:
+def get_chrome_download_path(
+    root=default_download_path,
+    *,
+    mkdir=True,
+) -> Path | None:
     _chrome_platform_detected = get_google_supported_platform_string()
 
     if not _chrome_platform_detected:
         return None
 
-    _default_exe_path = default_download_path
-    _default_exe_path.mkdir(parents=True, exist_ok=True)
+    _default_exe_path = root
+    if mkdir:
+        _default_exe_path.mkdir(parents=True, exist_ok=True)
 
     if platform.system().startswith("Linux"):
-        _default_exe_path = (
-            default_download_path / f"chrome-{_chrome_platform_detected}" / "chrome"
-        )
+        _default_exe_path /= f"chrome-{_chrome_platform_detected}/chrome"
     elif platform.system().startswith("Darwin"):
-        _default_exe_path = (
-            default_download_path
-            / f"chrome-{_chrome_platform_detected}"
-            / "Google Chrome for Testing.app"
-            / "Contents"
-            / "MacOS"
-            / "Google Chrome for Testing"
+        _default_exe_path /= (
+            f"chrome-{_chrome_platform_detected}"
+            "/Google Chrome for Testing.app"
+            "/Contents"
+            "/MacOS"
+            "/Google Chrome for Testing"
         )
     elif platform.system().startswith("Win"):
-        _default_exe_path = (
-            default_download_path / f"chrome-{_chrome_platform_detected}" / "chrome.exe"
-        )
+        _default_exe_path /= f"chrome-{_chrome_platform_detected}/chrome.exe"
 
     return _default_exe_path
 
 
+# can remove this and it's default after some time - 2025/11/29
+def get_old_chrome_download_path():
+    return get_chrome_download_path(
+        root=old_download_path,
+        mkdir=False,
+    )
+
+
 # https://stackoverflow.com/questions/39296101/python-zipfile-removes-execute-permissions-from-binaries
 class _ZipFilePermissions(zipfile.ZipFile):
     def _extract_member(self, member, targetpath, pwd):  # type: ignore [no-untyped-def]

src/choreographer/cli/_cli_utils_no_qa.py

@@ -55,11 +55,11 @@ def diagnose() -> None:
     print("*".center(50, "*"))
     print("BROWSER:".center(50, "*"))
     try:
-        local_path = browser_which([], verify_local=True)
-        if local_path and not Path(local_path).exists():
+        local_path = Chromium.find_browser(skip_local=False, verify_local=True)
+        if not local_path or Path(local_path).exists():
             print(f"Local doesn't exist at {local_path}")
         else:
-            print(f"Found local: {browser_which([], verify_local=True)}")
+            print(f"Found local: {local_path}")
     except RuntimeError:
         print("Didn't find local.")
     browser_path = Chromium.find_browser(skip_local=True)

src/choreographer/cli/defaults.py

@@ -11,3 +11,5 @@
     / "deps"
 )
 """The path where we download chrome if no path argument is supplied."""
+
+old_download_path = Path(__file__).resolve().parent / "browser_exe"

src/choreographer/utils/_which.py

@@ -8,8 +8,6 @@
 
 import logistro
 
-from choreographer.cli._cli_utils import get_chrome_download_path
-
 _logger = logistro.getLogger()
 
 if TYPE_CHECKING:
@@ -47,41 +45,19 @@ def _which_from_windows_reg(key: str) -> str | None:
 def browser_which(
     executable_names: Sequence[str],
     *,
-    skip_local: bool = False,
     ms_prog_id: str | None = None,
-    verify_local: bool = False,
 ) -> str | None:
     """
     Look for and return first name found in PATH.
 
     Args:
         executable_names: the list of names to look for
-        skip_local: (default False) don't look for a choreo download of anything.
         ms_prog_id: A windows registry ID string to lookup program paths
-        verify_local: (default False) force using local install.
 
     """
-    _logger.debug(f"Looking for browser, skipping local? {skip_local}")
-    path = None
-
     if isinstance(executable_names, str):
         executable_names = [executable_names]
 
-    if skip_local:
-        _logger.debug("Skipping searching for local download of chrome.")
-        if verify_local:
-            raise ValueError("Cannot set both skip_local and verify_local.")
-    else:
-        local_chrome = get_chrome_download_path()
-        _logger.debug(f"Looking for at local chrome download path: {local_chrome}")
-        if local_chrome is not None and local_chrome.exists():
-            _logger.debug("Returning local chrome.")
-            return str(local_chrome)
-        else:
-            _logger.debug(f"Local chrome not found at path: {local_chrome}.")
-            if verify_local:
-                raise RuntimeError("verify_local set to True, local not found.")
-
     if platform.system() == "Windows":
         os.environ["NoDefaultCurrentDirectoryInExePath"] = "0"  # noqa: SIM112 var name set by windows
         if (