diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 602da920..be1d0bb5 100644 --- a/CHANGELOG.txt +++ b/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 diff --git a/ROADMAP.md b/ROADMAP.md index 565720d9..2b68f0c7 100644 --- a/ROADMAP.md +++ b/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 diff --git a/src/choreographer/browsers/chromium.py b/src/choreographer/browsers/chromium.py index d3ff25e6..15eceb9b 100644 --- a/src/choreographer/browsers/chromium.py +++ b/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: diff --git a/src/choreographer/cli/_cli_utils.py b/src/choreographer/cli/_cli_utils.py index 4c4cb1d8..6a3bdd6d 100644 --- a/src/choreographer/cli/_cli_utils.py +++ b/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] diff --git a/src/choreographer/cli/_cli_utils_no_qa.py b/src/choreographer/cli/_cli_utils_no_qa.py index c8058d07..93e0aaff 100644 --- a/src/choreographer/cli/_cli_utils_no_qa.py +++ b/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) diff --git a/src/choreographer/cli/defaults.py b/src/choreographer/cli/defaults.py index a186db19..ac19e86a 100644 --- a/src/choreographer/cli/defaults.py +++ b/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" diff --git a/src/choreographer/utils/_which.py b/src/choreographer/utils/_which.py index 7d45b80a..ce64c1d7 100644 --- a/src/choreographer/utils/_which.py +++ b/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 (