diff --git a/.github/workflows/deploy-pages.yml b/.github/workflows/deploy-pages.yml index 71f11572..8db35041 100644 --- a/.github/workflows/deploy-pages.yml +++ b/.github/workflows/deploy-pages.yml @@ -16,7 +16,8 @@ concurrency: jobs: build: - runs-on: ubuntu-latest + runs-on: + group: Default timeout-minutes: 20 steps: - name: Checkout @@ -48,7 +49,8 @@ jobs: environment: name: github-pages url: ${{ steps.deployment.outputs.page_url }} - runs-on: ubuntu-latest + runs-on: + group: Default needs: build timeout-minutes: 20 steps: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c5d2ec54..efddf092 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -19,7 +19,8 @@ concurrency: jobs: release-state: - runs-on: ubuntu-latest + runs-on: + group: Default timeout-minutes: 20 steps: - uses: actions/checkout@v6 diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index be9ab1dc..32b19d18 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -19,7 +19,8 @@ concurrency: jobs: precheck: - runs-on: ubuntu-latest + runs-on: + group: Default timeout-minutes: 20 steps: @@ -30,6 +31,11 @@ jobs: with: python-version: '3.12' + - name: Setup Node + uses: actions/setup-node@v6 + with: + node-version: '22' + - name: Install uv uses: astral-sh/setup-uv@v8.0.0 with: @@ -49,7 +55,8 @@ jobs: discover-standard-tests: needs: precheck - runs-on: ubuntu-latest + runs-on: + group: Default timeout-minutes: 20 outputs: test-files: ${{ steps.set-matrix.outputs.test-files }} @@ -78,7 +85,7 @@ jobs: echo "$json_array" build: - name: ${{ matrix.target.os }} py${{ matrix.target.python_version }} ${{ matrix.test.name }} + name: ${{ matrix.target.os_name }} py${{ matrix.target.python_version }} ${{ matrix.test.name }} needs: [precheck, discover-standard-tests] runs-on: ${{ matrix.target.os }} timeout-minutes: 20 @@ -88,11 +95,16 @@ jobs: max-parallel: 20 matrix: target: - - os: ubuntu-latest + - os: + group: Default + os_name: linux python_version: '3.11' - - os: ubuntu-latest + - os: + group: Default + os_name: linux python_version: '3.14' - os: macOS-latest + os_name: macOS python_version: '3.13' test: ${{ fromJson(needs.discover-standard-tests.outputs.test-files) }} @@ -219,7 +231,8 @@ jobs: discover-live-tests: needs: precheck - runs-on: ubuntu-latest + runs-on: + group: Default timeout-minutes: 20 outputs: live-tests: ${{ steps.set-matrix.outputs.live-tests }} @@ -239,16 +252,20 @@ jobs: needs_docker=true fi - os_targets="ubuntu-latest macOS-latest" + os_targets="linux macOS" if grep -q "@pytest.mark.docker_required" "$test_file" || grep -q "@pytest.mark.root_required" "$test_file" || grep -q 'skipif("darwin"' "$test_file"; then - os_targets="ubuntu-latest" + os_targets="linux" elif grep -q 'require_tool("brew")' "$test_file" && ! grep -q 'require_tool("apt-get")' "$test_file" && ! grep -q "operations.apt.packages" "$test_file" && ! grep -q "ansible.builtin.apt" "$test_file"; then - os_targets="macOS-latest" + os_targets="macOS" fi for os_target in $os_targets; do - os_name=$(printf '%s' "$os_target" | tr '[:upper:]' '[:lower:]' | sed 's/-latest//') - entry="{\"name\":\"${test_name}-${os_name}\",\"path\":\"$test_file\",\"os\":\"$os_target\",\"needs_docker\":$needs_docker}" + if [ "$os_target" = "linux" ]; then + os_json='{"group":"Default"}' + else + os_json='"macOS-latest"' + fi + entry="{\"name\":\"${test_name}-${os_target}\",\"path\":\"$test_file\",\"os\":${os_json},\"os_name\":\"${os_target}\",\"needs_docker\":$needs_docker}" if [ "$first" = true ]; then first=false; else json_array+=","; fi json_array+="$entry" done diff --git a/README.md b/README.md index 3d076342..f20fe451 100644 --- a/README.md +++ b/README.md @@ -1077,7 +1077,8 @@ install_root = $ABXPKG_PUPPETEER_ROOT or $ABXPKG_LIB_DIR/puppeteer bin_dir = /bin ``` -- Install root: set `install_root` for the root dir and `bin_dir` for symlinked executables. Downloaded browser artifacts live under `/cache` when an install root is pinned. Leave it unset for ambient/global mode, where cache ownership stays with the host and `INSTALLER_BINARY` must already be resolvable from the ambient provider set. +- Install root: set `install_root` for the root dir and `bin_dir` for symlinked executables. Leave it unset for ambient/global mode, where cache ownership stays with the host and `INSTALLER_BINARY` must already be resolvable from the ambient provider set. +- Browser cache: when `install_root` is pinned, abxpkg manages `/cache` end-to-end — it's exported as `PUPPETEER_CACHE_DIR` to every subprocess, used for `--path=` on `puppeteer-browsers install` / `list`, and `uninstall()` resolves the real browser directory via `load()` then rmtrees it. When `install_root` is unset the provider is in pure passthrough mode: the caller's ambient `$PUPPETEER_CACHE_DIR` (or the CLI's `~/.cache/puppeteer` default) flows through to subprocesses unchanged, `load()` trusts whatever path `puppeteer-browsers list` reports, and `uninstall()` still rmtrees the real browser directory returned by `load()` — leaving any unrelated browsers in the shared cache alone. - Auto-switching: bootstraps `@puppeteer/browsers` through `NpmProvider` and then uses that CLI for browser installs. - `dry_run`: shared behavior. - Security: `min_release_age` is unsupported for browser installs and is ignored with a warning if explicitly requested. `postinstall_scripts=False` is supported for the underlying npm bootstrap path, and `ABXPKG_POSTINSTALL_SCRIPTS` hydrates the provider default here. @@ -1094,18 +1095,19 @@ Source: [`abxpkg/binprovider_playwright.py`](./abxpkg/binprovider_playwright.py) ```python INSTALLER_BIN = "playwright" PATH = "" -install_root = None # when set, doubles as PLAYWRIGHT_BROWSERS_PATH +install_root = None # abxpkg-managed root dir for bin_dir / nested npm prefix bin_dir = /bin # symlink dir for resolved browsers euid = 0 # routes exec() through sudo-first-then-fallback ``` -- Install root: set `install_root` to pin both the abxpkg root dir AND `PLAYWRIGHT_BROWSERS_PATH` to the same directory. Leave it unset to let playwright use its own OS-default browsers path (`~/.cache/ms-playwright` on Linux etc.) — in that case abxpkg maintains no symlink dir or npm prefix at all, the `playwright` npm CLI bootstraps against the host's npm default, and `load()` returns the resolved `executablePath()` directly. `bin_dir` overrides the symlink directory when `install_root` is pinned. +- Install root: set `install_root` to pin the abxpkg-managed root dir (where `bin_dir` symlinks and the nested npm prefix live). Leave it unset to let playwright use its own OS-default browsers path (`~/.cache/ms-playwright` on Linux etc.) — in that case abxpkg maintains no symlink dir or npm prefix at all, the `playwright` npm CLI bootstraps against the host's npm default, and `load()` returns the resolved `executablePath()` directly. `bin_dir` overrides the symlink directory when `install_root` is pinned. +- Browser cache: when `install_root` is pinned, abxpkg manages `/cache` end-to-end — exported as `PLAYWRIGHT_BROWSERS_PATH` to every subprocess (including the `env KEY=VAL -- ...` wrapper used when we go through sudo), used to scope `executablePath()` hits on `load()`, and `uninstall()` resolves the real browser directory via `load()` then rmtrees it. When `install_root` is unset the provider is in pure passthrough mode: the caller's ambient `$PLAYWRIGHT_BROWSERS_PATH` (or playwright's `~/.cache/ms-playwright` default on Linux) flows through to subprocesses unchanged, `load()` trusts whatever path `executablePath()` reports, and `uninstall()` still rmtrees the real browser directory returned by `load()`. - Auto-switching: bootstraps the `playwright` npm package through `NpmProvider`, then runs `playwright install --with-deps ` against it. Resolves each installed browser's real executable via the `playwright-core` Node.js API (`chromium.executablePath()` etc.) and writes a symlink into `bin_dir` when one is configured. - `dry_run`: shared behavior — the install handler short-circuits to a placeholder without touching the host. - Privilege handling: `--with-deps` installs system packages and requires root on Linux. ``euid`` defaults to ``0``, which routes every ``exec()`` call through the base ``BinProvider.exec`` sudo-first-then-fallback path — it tries ``sudo -n -- playwright install --with-deps ...`` first on non-root hosts, falls back to running the command directly if sudo fails or isn't available, and merges both stderr outputs into the final error if both attempts fail. - Security: `min_release_age` and `postinstall_scripts=False` are unsupported for browser installs and are ignored with a warning if explicitly requested. - Overrides: `install_args` are appended onto `playwright install` after `playwright_install_args` (defaults to `["--with-deps"]`) and passed through verbatim — use whatever browser names / flags the `playwright install` CLI accepts (`chromium`, `firefox`, `webkit`, `--no-shell`, `--only-shell`, `--force`, etc.). -- Notes: `update()` bumps the `playwright` npm package in `install_root` first (via `NpmProvider.update`) so its pinned browser versions refresh, then re-runs `playwright install --force ` to pull any new browser builds. `uninstall()` removes the relevant `-*/` directories from `install_root` alongside the bin-dir symlink, since `playwright uninstall` only drops *unused* browsers on its own. Both `update()` and `uninstall()` leave playwright's OS-default cache untouched when `install_root` is unset. +- Notes: `update()` bumps the `playwright` npm package in `install_root` first (via `NpmProvider.update`) so its pinned browser versions refresh, then re-runs `playwright install --force ` to pull any new browser builds. `uninstall()` resolves the browser's real install directory via `playwright-core`'s `executablePath()`, walks up to the containing `-/` dir, and rmtrees that dir — in both managed and passthrough modes — because `playwright uninstall` itself has no per-browser argument and only drops *unused* browsers wholesale. diff --git a/abxpkg/binprovider_apt.py b/abxpkg/binprovider_apt.py index 3bfdc146..521abdd7 100755 --- a/abxpkg/binprovider_apt.py +++ b/abxpkg/binprovider_apt.py @@ -28,8 +28,14 @@ class AptProvider(BinProvider): def setup_PATH(self, no_cache: bool = False) -> None: """Populate PATH on first use from dpkg-discovered package runtime bin dirs, not from apt-get itself.""" - if no_cache or ( - self._INSTALLER_BINARY is None + # Rebuild PATH on first use, when the caller forces no_cache, or when + # PATH is still empty — the last case covers the "INSTALLER_BINARY was + # resolved out-of-band (hook preflight etc.), so _INSTALLER_BINARY is + # non-None but self.PATH was never populated" race. + if ( + no_cache + or not self.PATH + or self._INSTALLER_BINARY is None or self._INSTALLER_BINARY.loaded_abspath is None ): dpkg_binary = EnvProvider().load("dpkg") diff --git a/abxpkg/binprovider_brew.py b/abxpkg/binprovider_brew.py index 69a88dac..aa84f422 100755 --- a/abxpkg/binprovider_brew.py +++ b/abxpkg/binprovider_brew.py @@ -183,6 +183,15 @@ def _refresh_bin_link( except Exception: break walk_path = walk_path.parent + # Idempotent refresh: skip when shim already points at target. + # Rewriting on every load() bumps mtime and churns the inode, + # which invalidates fingerprint caches unnecessarily. + if link_path.is_symlink(): + try: + if link_path.readlink() == Path(target): + return TypeAdapter(HostBinPath).validate_python(link_path) + except OSError: + pass if link_path.exists() or link_path.is_symlink(): link_path.unlink(missing_ok=True) link_path.symlink_to(target) @@ -190,8 +199,13 @@ def _refresh_bin_link( def setup_PATH(self, no_cache: bool = False) -> None: """Populate PATH on first use from the resolved brew prefix and known runtime brew bin dirs.""" - if no_cache or ( - self._INSTALLER_BINARY is None + # Rebuild PATH on first use, when the caller forces no_cache, or when + # PATH is still empty — the last case covers provider copies that + # inherited a resolved ``_INSTALLER_BINARY`` but an unset ``PATH``. + if ( + no_cache + or not self.PATH + or self._INSTALLER_BINARY is None or self._INSTALLER_BINARY.loaded_abspath is None ): install_root = self.install_root @@ -391,12 +405,12 @@ def default_abspath_handler( if not self.PATH: return None + # Authoritative lookup: search brew's own Cellar / opt / PATH + # entries for the real formula binary. The managed ``bin_dir`` + # shim is a convenience side-effect of install — never a source + # of truth — so we always consult brew's paths first and only + # refresh the shim to match the freshly-resolved target. linked_bin = self._linked_bin_path(bin_name) - if linked_bin is not None: - linked_abspath = bin_abspath(bin_name, PATH=str(self.bin_dir)) - if linked_abspath: - return linked_abspath - search_paths = self._brew_search_paths(bin_name, no_cache=no_cache) abspath = bin_abspath(bin_name, PATH=search_paths) if abspath: diff --git a/abxpkg/binprovider_goget.py b/abxpkg/binprovider_goget.py index 138689a9..5e0f1181 100755 --- a/abxpkg/binprovider_goget.py +++ b/abxpkg/binprovider_goget.py @@ -294,6 +294,15 @@ def default_abspath_handler( assert bin_dir is not None link_path = bin_dir / str(bin_name) link_path.parent.mkdir(parents=True, exist_ok=True) + # Idempotent refresh: skip when shim already points at target. + # Rewriting on every load() bumps mtime and churns the inode, + # which invalidates fingerprint caches unnecessarily. + if link_path.is_symlink(): + try: + if link_path.readlink() == Path(direct_abspath): + return TypeAdapter(HostBinPath).validate_python(link_path) + except OSError: + pass if link_path.exists() or link_path.is_symlink(): link_path.unlink(missing_ok=True) link_path.symlink_to(direct_abspath) diff --git a/abxpkg/binprovider_npm.py b/abxpkg/binprovider_npm.py index 75ccdf25..4c7ae341 100755 --- a/abxpkg/binprovider_npm.py +++ b/abxpkg/binprovider_npm.py @@ -377,6 +377,15 @@ def _refresh_bin_link( link_path = self._linked_bin_path(bin_name) assert link_path is not None, "_refresh_bin_link requires bin_dir to be set" link_path.parent.mkdir(parents=True, exist_ok=True) + # Idempotent refresh: skip when shim already points at target. + # Rewriting on every load() bumps mtime and churns the inode, + # which invalidates fingerprint caches unnecessarily. + if link_path.is_symlink(): + try: + if link_path.readlink() == Path(target): + return TypeAdapter(HostBinPath).validate_python(link_path) + except OSError: + pass if link_path.exists() or link_path.is_symlink(): link_path.unlink(missing_ok=True) link_path.symlink_to(target) diff --git a/abxpkg/binprovider_playwright.py b/abxpkg/binprovider_playwright.py index fe3daf43..496eea42 100755 --- a/abxpkg/binprovider_playwright.py +++ b/abxpkg/binprovider_playwright.py @@ -28,20 +28,27 @@ logger = get_logger(__name__) +CLAUDE_SANDBOX_NO_PROXY = ( + "localhost,127.0.0.1,169.254.169.254,metadata.google.internal," + ".svc.cluster.local,.local" +) + class PlaywrightProvider(BinProvider): """Playwright browser installer provider. Drives ``playwright install --with-deps `` against the - ``playwright`` npm package. When ``playwright_root`` is set it - doubles as the abxpkg install root AND ``PLAYWRIGHT_BROWSERS_PATH``: - browsers land inside it (``chromium-/`` etc.), a dedicated - npm prefix is nested under it, and each requested browser is - surfaced from ``bin_dir`` so ``load(bin_name)`` finds it directly. + ``playwright`` npm package. When ``playwright_root`` is set it acts + as the abxpkg install root: a dedicated npm prefix is nested under + it, ``bin_dir`` surfaces each requested browser so ``load(bin_name)`` + finds it directly, and ``PLAYWRIGHT_BROWSERS_PATH`` is pinned to + ``/cache`` for every subprocess the provider runs. When ``playwright_root`` is left unset, playwright picks its own - default browsers path, the npm CLI bootstraps against the host's - npm default, and ``load()`` returns the resolved ``executablePath()`` - directly without creating any install_root/bin_dir symlinks. + default browsers path (``$PLAYWRIGHT_BROWSERS_PATH`` from the + ambient env, otherwise ``~/.cache/ms-playwright`` on Linux), the + npm CLI bootstraps against the host's npm default, and ``load()`` + returns the resolved ``executablePath()`` directly without + creating any install_root/bin_dir symlinks. ``--with-deps`` installs system packages and requires root on Linux, so ``euid`` defaults to ``0``: the base ``BinProvider.exec`` @@ -59,9 +66,8 @@ class PlaywrightProvider(BinProvider): postinstall_scripts: bool | None = Field(default=None, repr=False) min_release_age: float | None = Field(default=None, repr=False) - # ``playwright_root`` is both the abxpkg install root and the - # ``PLAYWRIGHT_BROWSERS_PATH`` we export to the CLI. Leave unset to - # let playwright use its own OS-default browsers path. + # ``playwright_root`` is the abxpkg-managed provider root dir. Leave + # unset to let playwright use its own OS-default browsers path. # Default: ABXPKG_PLAYWRIGHT_ROOT > ABXPKG_LIB_DIR/playwright > None. install_root: Path | None = Field( default_factory=lambda: abxpkg_install_root_default("playwright"), @@ -79,9 +85,30 @@ class PlaywrightProvider(BinProvider): @computed_field @property def ENV(self) -> "dict[str, str]": - if not self.install_root: - return {} - return {"PLAYWRIGHT_BROWSERS_PATH": str(self.install_root)} + # In managed mode we pin ``PLAYWRIGHT_BROWSERS_PATH`` to + # ``/cache``. In unmanaged mode we leave the + # ambient env (or playwright's own ``~/.cache/ms-playwright`` + # default) untouched. + env: dict[str, str] = {} + if self.install_root is not None: + env["PLAYWRIGHT_BROWSERS_PATH"] = str(self.install_root / "cache") + # ``playwright install`` downloads browsers from + # ``cdn.playwright.dev`` (Azure blob storage) and + # ``storage.googleapis.com`` depending on the build. In + # sandboxed environments the egress proxy's NO_PROXY often + # includes ``.googleapis.com`` / ``.google.com``, which forces + # the direct connection — which then fails DNS resolution or + # times out. Mirror the PuppeteerProvider fix: override + # NO_PROXY / no_proxy to a safe sandbox allowlist so the + # download goes through the proxy instead. + ambient_no_proxy = os.environ.get("NO_PROXY") or os.environ.get("no_proxy") + if not ambient_no_proxy or ( + ".googleapis.com" in ambient_no_proxy.lower() + or ".google.com" in ambient_no_proxy.lower() + ): + env["NO_PROXY"] = CLAUDE_SANDBOX_NO_PROXY + env["no_proxy"] = CLAUDE_SANDBOX_NO_PROXY + return env def supports_min_release_age(self, action, no_cache: bool = False) -> bool: return False @@ -291,10 +318,20 @@ def exec( env = self.build_exec_env(base_env=(kwargs.pop("env", None) or os.environ)) env_assignments: list[str] = [] if self.install_root is not None: - env["PLAYWRIGHT_BROWSERS_PATH"] = str(self.install_root) + cache_dir = self.install_root / "cache" + env["PLAYWRIGHT_BROWSERS_PATH"] = str(cache_dir) env_assignments.append( - f"PLAYWRIGHT_BROWSERS_PATH={self.install_root}", + f"PLAYWRIGHT_BROWSERS_PATH={cache_dir}", ) + # ``NO_PROXY`` / ``no_proxy`` are set by ``ENV`` to rescue + # browser downloads in sandboxes whose egress proxy NO_PROXY + # blocks ``cdn.playwright.dev`` / ``storage.googleapis.com``. + # They must also survive sudo's ``env_reset``, so forward them + # through the ``/usr/bin/env KEY=VAL -- ...`` wrapper below. + for proxy_key in ("NO_PROXY", "no_proxy"): + proxy_value = env.get(proxy_key) + if proxy_value: + env_assignments.append(f"{proxy_key}={proxy_value}") needs_sudo_env_wrapper = os.geteuid() != 0 and self.EUID != os.geteuid() if env_assignments and needs_sudo_env_wrapper: resolved_bin = bin_name @@ -524,16 +561,36 @@ def _refresh_symlink(self, bin_name: str, target: Path) -> Path: ) link = self.bin_dir / bin_name link.parent.mkdir(parents=True, exist_ok=True) - if link.exists() or link.is_symlink(): - link.unlink(missing_ok=True) # On macOS the executable is buried inside a ``.app`` bundle, so # write a tiny shell shim instead of a symlink (same pattern as # PuppeteerProvider). - if os.name == "posix" and ".app/Contents/MacOS/" in str(target): - link.write_text( - f'#!/bin/sh\nexec {shlex.quote(str(target))} "$@"\n', - encoding="utf-8", - ) + use_shell_shim = os.name == "posix" and ".app/Contents/MacOS/" in str(target) + desired_script = ( + f'#!/bin/sh\nexec {shlex.quote(str(target))} "$@"\n' + if use_shell_shim + else None + ) + # Idempotent refresh: leave the shim untouched when it already + # points at ``target``. Rewriting on every ``load()`` would bump + # the shim's mtime (shell-script case), which breaks callers + # that stat the shim to validate a freshly-installed binary. + if link.is_symlink(): + try: + if os.readlink(link) == str(target): + return link + except OSError: + pass + elif use_shell_shim and link.is_file(): + try: + if link.read_text(encoding="utf-8") == desired_script: + return link + except OSError: + pass + if link.exists() or link.is_symlink(): + link.unlink(missing_ok=True) + if use_shell_shim: + assert desired_script is not None + link.write_text(desired_script, encoding="utf-8") link.chmod(0o755) return link link.symlink_to(target) @@ -558,24 +615,25 @@ def default_abspath_handler( except Exception: return None return None - if self.bin_dir is not None: - link = self.bin_dir / str(bin_name) - if link.exists() and os.access(link, os.X_OK): - return link + # Authoritative lookup: ask ``playwright-core`` where the browser + # actually lives via its ``executablePath()`` API — never trust + # the managed ``bin_dir`` shim as a source of truth, it may + # point at a browser that was removed out-of-band. When + # playwright-core reports nothing, we report nothing. resolved = self._playwright_browser_path( str(bin_name), no_cache=no_cache, ) if not resolved: return None - # When ``playwright_root`` is pinned, a hit from - # ``executablePath()`` that points outside that install_root tree - # (e.g. an ambient system install) should not satisfy - # ``load()`` — otherwise an unrelated host-wide playwright - # install would silently hijack resolution. + # When ``install_root`` is pinned, an ``executablePath()`` hit + # that points outside our managed cache tree (e.g. an ambient + # system install) should not satisfy ``load()`` — otherwise an + # unrelated host-wide playwright install would silently hijack + # resolution. if self.install_root is not None: - root_real = self.install_root.resolve(strict=False) - if root_real not in resolved.resolve(strict=False).parents: + cache_real = (self.install_root / "cache").resolve(strict=False) + if cache_real not in resolved.resolve(strict=False).parents: return None if self.bin_dir is None: return resolved @@ -719,21 +777,29 @@ def default_uninstall_handler( install_args: InstallArgs | None = None, **context, ) -> bool: - # Drop the symlink first (if we're managing one) so ``load()`` - # stops seeing the tool even if browser dir removal partially - # fails. + # Resolve the real browser directory via playwright-core's + # ``executablePath()`` directly (``_playwright_browser_path`` + # shells out to the node API) so we don't round-trip through + # ``load()`` → ``default_abspath_handler`` and have it refresh + # the managed shim right as we're about to delete it. Honours + # managed ``install_root``, ambient ``PLAYWRIGHT_BROWSERS_PATH``, + # and playwright's own default (``~/.cache/ms-playwright``) + # uniformly. + resolved = self._playwright_browser_path(str(bin_name), no_cache=True) + if resolved is not None: + for parent in Path(resolved).resolve().parents: + if parent.name.startswith(f"{bin_name}-"): + logger.info("$ %s", format_command(["rm", "-rf", str(parent)])) + shutil.rmtree(parent, ignore_errors=True) + break + + # Finally, drop the convenience shim under ``bin_dir``. Doing + # this last avoids the "unlink → load() refresh → rmtree → + # dangling shim" ordering bug (playwright's own CLI has no + # per-browser uninstall argument, so this rmtree dance is + # still the only way to remove a specific browser). if self.bin_dir is not None: (self.bin_dir / bin_name).unlink(missing_ok=True) - - # ``playwright uninstall`` only removes *unused* browsers from - # the entire host, so drop the matching directories ourselves. - # Only touch ``playwright_root`` if the caller pinned one — we - # don't delete from playwright's own OS-default cache. - if self.install_root is not None and self.install_root.is_dir(): - for entry in self.install_root.iterdir(): - if entry.is_dir() and entry.name.startswith(f"{bin_name}-"): - logger.info("$ %s", format_command(["rm", "-rf", str(entry)])) - shutil.rmtree(entry, ignore_errors=True) return True diff --git a/abxpkg/binprovider_pnpm.py b/abxpkg/binprovider_pnpm.py index 634f73d9..87f1e150 100755 --- a/abxpkg/binprovider_pnpm.py +++ b/abxpkg/binprovider_pnpm.py @@ -274,6 +274,15 @@ def _refresh_bin_link( link_path = self._linked_bin_path(bin_name) assert link_path is not None, "_refresh_bin_link requires bin_dir to be set" link_path.parent.mkdir(parents=True, exist_ok=True) + # Idempotent refresh: skip when shim already points at target. + # Rewriting on every load() bumps mtime and churns the inode, + # which invalidates fingerprint caches unnecessarily. + if link_path.is_symlink(): + try: + if link_path.readlink() == Path(target): + return TypeAdapter(HostBinPath).validate_python(link_path) + except OSError: + pass if link_path.exists() or link_path.is_symlink(): link_path.unlink(missing_ok=True) link_path.symlink_to(target) diff --git a/abxpkg/binprovider_puppeteer.py b/abxpkg/binprovider_puppeteer.py index 3cd06ec1..00faf4dc 100755 --- a/abxpkg/binprovider_puppeteer.py +++ b/abxpkg/binprovider_puppeteer.py @@ -66,33 +66,37 @@ class PuppeteerProvider(BinProvider): # Only set in managed mode: setup()/default_abspath_handler() use it to expose stable # browser launch shims under ``/bin``; global mode leaves it unset. bin_dir: Path | None = None - # Explicit override for the directory browsers get downloaded into. When - # unset, cache_dir defaults to ``/cache``; when set, it wins - # so callers can point ``PUPPETEER_CACHE_DIR`` at an arbitrary path. - browser_cache_dir: Path | None = None @computed_field @property def ENV(self) -> "dict[str, str]": - if not self.cache_dir: - return {} - return {"PUPPETEER_CACHE_DIR": str(self.cache_dir)} + # In managed mode we pin ``PUPPETEER_CACHE_DIR`` to + # ``/cache``. In unmanaged mode we leave the + # ambient env (or puppeteer-browsers' own ``~/.cache/puppeteer`` + # default) untouched. + env: dict[str, str] = {} + if self.install_root is not None: + env["PUPPETEER_CACHE_DIR"] = str(self.install_root / "cache") + # @puppeteer/browsers downloads browsers from + # storage.googleapis.com. In sandboxed environments the egress + # proxy's NO_PROXY often includes ``.googleapis.com`` / ``.google.com``, + # which forces the direct connection — which then fails DNS + # resolution or times out. Override NO_PROXY / no_proxy to a + # safe sandbox allowlist so the download goes through the proxy + # instead. Callers that need their own NO_PROXY can still set it + # via the CLI override flags; our value only fills in the default. + ambient_no_proxy = os.environ.get("NO_PROXY") or os.environ.get("no_proxy") + if not ambient_no_proxy or ( + ".googleapis.com" in ambient_no_proxy.lower() + or ".google.com" in ambient_no_proxy.lower() + ): + env["NO_PROXY"] = CLAUDE_SANDBOX_NO_PROXY + env["no_proxy"] = CLAUDE_SANDBOX_NO_PROXY + return env def supports_postinstall_disable(self, action, no_cache: bool = False) -> bool: return action in ("install", "update") - @computed_field - @property - def cache_dir(self) -> Path | None: - # Explicit override wins so PUPPETEER_CACHE_DIR can be any path. - if self.browser_cache_dir is not None: - return self.browser_cache_dir - # Otherwise browser downloads live under ``install_root/cache`` when we - # manage an install root; global mode leaves cache ownership to the host. - if self.install_root is not None: - return self.install_root / "cache" - return None - @model_validator(mode="after") def detect_euid_to_use(self) -> Self: if self.bin_dir is None and self.install_root is not None: @@ -122,6 +126,70 @@ def setup_PATH(self, no_cache: bool = False) -> None: def INSTALLER_BINARY(self, no_cache: bool = False): from . import DEFAULT_PROVIDER_NAMES, PROVIDER_CLASS_BY_NAME + # Prefer the puppeteer-browsers bootstrapped by an earlier install + # under ``/npm/node_modules/.bin``. Without this, a + # fresh provider copy (e.g. the one Binary.load() builds via + # get_provider_with_overrides) can't locate puppeteer-browsers and + # ``_list_installed_browsers()`` silently returns empty. + lib_dir = os.environ.get("ABXPKG_LIB_DIR") + if ( + self.install_root is not None + and lib_dir + and str(self.install_root).startswith(lib_dir.rstrip("/") + "/") + ): + local_cli = ( + Path(lib_dir) / "npm" / "node_modules" / ".bin" / self.INSTALLER_BIN + ) + elif self.install_root is not None: + local_cli = ( + self.install_root / "npm" / "node_modules" / ".bin" / self.INSTALLER_BIN + ) + else: + local_cli = None + + if ( + local_cli is not None + and local_cli.is_file() + and os.access(local_cli, os.X_OK) + ): + if ( + not no_cache + and self._INSTALLER_BINARY + and self._INSTALLER_BINARY.loaded_abspath == local_cli + and self._INSTALLER_BINARY.is_valid + ): + return self._INSTALLER_BINARY + if not no_cache: + cached = self.load_cached_binary(self.INSTALLER_BIN, local_cli) + if cached and cached.loaded_abspath: + self._INSTALLER_BINARY = cached + return cached + env_provider = EnvProvider( + PATH=str(local_cli.parent), + install_root=None, + bin_dir=None, + ) + loaded_local = env_provider.load( + bin_name=self.INSTALLER_BIN, + no_cache=no_cache, + ) + if loaded_local and loaded_local.loaded_abspath: + if loaded_local.loaded_version and loaded_local.loaded_sha256: + self.write_cached_binary( + self.INSTALLER_BIN, + loaded_local.loaded_abspath, + loaded_local.loaded_version, + loaded_local.loaded_sha256, + resolved_provider_name=( + loaded_local.loaded_binprovider.name + if loaded_local.loaded_binprovider is not None + else self.name + ), + cache_kind="dependency", + ) + self._INSTALLER_BINARY = loaded_local + return self._INSTALLER_BINARY + loaded = super().INSTALLER_BINARY(no_cache=no_cache) raw_provider_names = os.environ.get("ABXPKG_BINPROVIDERS") selected_provider_names = ( @@ -211,7 +279,9 @@ def setup( owner_paths=( self.install_root, self.bin_dir, - self.cache_dir, + self.install_root / "cache" + if self.install_root is not None + else None, self.install_root / "npm" if self.install_root is not None else None, @@ -247,10 +317,9 @@ def setup( if self.install_root is not None: self.install_root.mkdir(parents=True, exist_ok=True) + (self.install_root / "cache").mkdir(parents=True, exist_ok=True) if self.bin_dir is not None: self.bin_dir.mkdir(parents=True, exist_ok=True) - if self.cache_dir is not None: - self.cache_dir.mkdir(parents=True, exist_ok=True) cli_binary = self._cli_binary( postinstall_scripts=postinstall_scripts, @@ -301,8 +370,8 @@ def _normalize_install_args(self, install_args: Iterable[str]) -> list[str]: if arg_str.startswith("--path="): continue normalized.append(arg_str) - if self.cache_dir is not None: - normalized.append(f"--path={self.cache_dir}") + if self.install_root is not None: + normalized.append(f"--path={self.install_root / 'cache'}") return normalized def _list_installed_browsers( @@ -316,8 +385,8 @@ def _list_installed_browsers( if not installer_bin: return [] cmd = ["list"] - if self.cache_dir is not None: - cmd.append(f"--path={self.cache_dir}") + if self.install_root is not None: + cmd.append(f"--path={self.install_root / 'cache'}") proc = self.exec( bin_name=installer_bin, cmd=cmd, @@ -379,7 +448,14 @@ def _resolve_installed_browser_path( install_args: Iterable[str] | None = None, no_cache: bool = False, ) -> Path | None: - browser_name = self._browser_name(bin_name, install_args or [bin_name]) + # Pick up the caller's configured install_args so + # ``bin_name=chrome`` + ``install_args=["chromium@latest"]`` + # resolves to ``browser_name="chromium"`` (matching what + # ``puppeteer-browsers list`` reports), instead of falling + # back to ``[bin_name]`` which would look for the alias name. + if install_args is None: + install_args = self.get_install_args(bin_name, quiet=True) or [bin_name] + browser_name = self._browser_name(bin_name, install_args) candidates = [ (version, path) for candidate_browser, version, path in self._list_installed_browsers( @@ -394,22 +470,54 @@ def _resolve_installed_browser_path( ] if parsed_candidates: return max(parsed_candidates, key=lambda item: item[0])[1] + if not candidates: + return None if len(candidates) == 1: return candidates[0][1] - return None + # Multiple cached builds but none parse as ``SemVer`` (e.g. chromium's + # integer build IDs like ``1618539``). Fall back to the newest one + # by file mtime so post-install lookups land on the freshly- + # downloaded version rather than ``None``. + candidates.sort( + key=lambda item: ( + item[1].stat().st_mtime if item[1].exists() else 0, + item[0], + ), + ) + return candidates[-1][1] def _refresh_symlink(self, bin_name: str, target: Path) -> Path: bin_dir = self.bin_dir assert bin_dir is not None link_path = bin_dir / bin_name link_path.parent.mkdir(parents=True, exist_ok=True) + use_shell_shim = os.name == "posix" and ".app/Contents/MacOS/" in str(target) + desired_script = ( + f'#!/bin/sh\nexec {shlex.quote(str(target))} "$@"\n' + if use_shell_shim + else None + ) + # Idempotent refresh: leave the shim untouched when it already + # points at ``target``. Rewriting on every ``load()`` would bump + # the shim's mtime (shell-script case), which breaks callers + # that stat the shim to validate a freshly-installed binary. + if link_path.is_symlink(): + try: + if os.readlink(link_path) == str(target): + return link_path + except OSError: + pass + elif use_shell_shim and link_path.is_file(): + try: + if link_path.read_text(encoding="utf-8") == desired_script: + return link_path + except OSError: + pass if link_path.exists() or link_path.is_symlink(): link_path.unlink(missing_ok=True) - if os.name == "posix" and ".app/Contents/MacOS/" in str(target): - link_path.write_text( - f'#!/bin/sh\nexec {shlex.quote(str(target))} "$@"\n', - encoding="utf-8", - ) + if use_shell_shim: + assert desired_script is not None + link_path.write_text(desired_script, encoding="utf-8") link_path.chmod(0o755) return link_path link_path.symlink_to(target) @@ -430,15 +538,23 @@ def default_abspath_handler( except Exception: return None return None - bin_dir = self.bin_dir - assert bin_dir is not None - link_path = bin_dir / str(bin_name) - if link_path.exists() and os.access(link_path, os.X_OK): - return link_path + # Authoritative lookup: ask puppeteer-browsers where the browser + # actually lives — never trust the managed ``bin_dir`` shim as a + # source of truth, it may point at a browser that was removed + # out-of-band. When the CLI reports nothing, we report nothing. resolved = self._resolve_installed_browser_path(str(bin_name)) if not resolved or not resolved.exists(): return None + + # Refresh the convenience shim under ``bin_dir`` so ``PATH`` users + # get a stable entry pointing at the freshly-resolved executable. + # In global/unmanaged mode (``install_root=None``) we have no + # managed shim dir, so just return the resolved path directly. + # When the shim refresh fails (read-only FS etc.) we also fall + # back to the resolved path. + if self.bin_dir is None: + return resolved try: return self._refresh_symlink(str(bin_name), resolved) except OSError: @@ -449,10 +565,11 @@ def _cleanup_partial_browser_cache( install_output: str, browser_name: str, ) -> bool: - if self.cache_dir is None: + if self.install_root is None: return False + cache_dir = self.install_root / "cache" targets: set[Path] = set() - browser_cache_dir = self.cache_dir / browser_name + browser_cache_dir = cache_dir / browser_name missing_dir_match = re.search( r"browser folder \(([^)]+)\) exists but the executable", @@ -474,7 +591,7 @@ def _cleanup_partial_browser_cache( targets.update(browser_cache_dir.glob(f"*{build_id}*")) removed_any = False - resolved_cache = self.cache_dir.resolve(strict=False) + resolved_cache = cache_dir.resolve(strict=False) for target in targets: resolved_target = target.resolve(strict=False) if not ( @@ -552,27 +669,25 @@ def _run_install_with_sudo( cwd=self.install_root or ".", timeout=self.install_timeout, ) - if ( - proc.returncode == 0 - and self.cache_dir is not None - and self.cache_dir.exists() - ): - uid = os.getuid() - gid = os.getgid() - chown_proc = self.exec( - bin_name=sudo_binary.loaded_abspath, - cmd=["chown", "-R", f"{uid}:{gid}", str(self.cache_dir)], - cwd=self.install_root or ".", - timeout=30, - quiet=True, - ) - if chown_proc.returncode != 0: - log_subprocess_output( - logger, - f"{self.__class__.__name__} sudo chown", - chown_proc.stdout, - chown_proc.stderr, + if proc.returncode == 0 and self.install_root is not None: + cache_dir = self.install_root / "cache" + if cache_dir.exists(): + uid = os.getuid() + gid = os.getgid() + chown_proc = self.exec( + bin_name=sudo_binary.loaded_abspath, + cmd=["chown", "-R", f"{uid}:{gid}", str(cache_dir)], + cwd=self.install_root or ".", + timeout=30, + quiet=True, ) + if chown_proc.returncode != 0: + log_subprocess_output( + logger, + f"{self.__class__.__name__} sudo chown", + chown_proc.stdout, + chown_proc.stderr, + ) return proc @remap_kwargs({"packages": "install_args"}) @@ -700,16 +815,38 @@ def default_uninstall_handler( install_args: InstallArgs | None = None, **context, ) -> bool: + # Resolve the real browser directory via the CLI directly + # (``_resolve_installed_browser_path`` shells out to + # ``puppeteer-browsers list``) so we don't round-trip through + # ``load()`` → ``default_abspath_handler`` and have it refresh + # the managed shim right as we're about to delete it. Honours + # managed ``install_root``, ambient ``PUPPETEER_CACHE_DIR``, + # and puppeteer-browsers' own default uniformly. install_args = list(install_args or self.get_install_args(bin_name)) browser_name = self._browser_name(bin_name, install_args) + # Forward install_args so ``_resolve_installed_browser_path`` uses + # the same ``browser_name`` we just derived; otherwise it re-runs + # ``get_install_args`` and can pick up a different alias (e.g. + # caller-passed ``chromium@latest`` vs provider default ``chrome``), + # which would leave the parent.name match below unsatisfied and + # silently skip the rmtree. + resolved = self._resolve_installed_browser_path( + str(bin_name), + install_args=install_args, + ) + if resolved is not None: + for parent in Path(resolved).resolve().parents: + if parent.name == browser_name: + logger.info("$ %s", format_command(["rm", "-rf", str(parent)])) + shutil.rmtree(parent, ignore_errors=True) + break + + # Finally, drop the convenience shim under ``bin_dir``. Doing + # this last avoids the "unlink → load() refresh → rmtree → + # dangling shim" ordering bug. if self.bin_dir is not None: bin_path = self.bin_dir / bin_name if bin_path.exists() or bin_path.is_symlink(): logger.info("$ %s", format_command(["rm", "-f", str(bin_path)])) bin_path.unlink(missing_ok=True) - if self.cache_dir is not None: - browser_dir = self.cache_dir / browser_name - if browser_dir.exists(): - logger.info("$ %s", format_command(["rm", "-rf", str(browser_dir)])) - shutil.rmtree(browser_dir, ignore_errors=True) return True diff --git a/tests/test_playwrightprovider.py b/tests/test_playwrightprovider.py index 45deda7b..5831ad53 100644 --- a/tests/test_playwrightprovider.py +++ b/tests/test_playwrightprovider.py @@ -1,4 +1,5 @@ import os +import re import shutil import tempfile from pathlib import Path @@ -8,6 +9,31 @@ from abxpkg import Binary, PlaywrightProvider +def _resolve_shim_target(shim: Path) -> Path: + """Resolve a managed bin_dir shim to its real browser target. + + On Linux the shim is a symlink, so ``.resolve()`` naturally follows + it. On macOS the shim is a shell script that ``exec``s the binary + inside a ``.app`` bundle (a direct symlink breaks dyld's + ``@executable_path``-relative Framework loading), so ``.resolve()`` + just returns the script path itself. Parse the ``exec `` line + to recover the target in that case. We key off ``is_symlink()`` + rather than comparing ``shim == shim.resolve()`` because macOS + ``$TMPDIR`` lives under ``/var/folders/...`` → ``/private/var/...``, + so ``resolve()`` always differs from the input even for plain files. + """ + if shim.is_symlink(): + return shim.resolve() + try: + script = shim.read_text(encoding="utf-8") + except OSError: + return shim.resolve() + match = re.search(r"exec '([^']+)'", script) + if not match: + return shim.resolve() + return Path(match.group(1)).resolve() + + @pytest.fixture(scope="module") def seeded_playwright_root(): with tempfile.TemporaryDirectory() as temp_dir: @@ -35,15 +61,37 @@ def copy_seeded_playwright_root( copied_bin_dir = install_root / "bin" if not copied_bin_dir.is_dir(): return + seeded_resolved = seeded_playwright_root.resolve() for link_path in copied_bin_dir.iterdir(): - if not link_path.is_symlink(): + if link_path.is_symlink(): + link_target = link_path.resolve(strict=False) + if seeded_resolved not in link_target.parents: + continue + relative_target = link_target.relative_to(seeded_resolved) + link_path.unlink() + link_path.symlink_to(install_root / relative_target) + continue + # macOS chrome/chromium shims are shell scripts that hardcode + # the seeded install_root path; rewrite them so they exec the + # copy under this test's install_root instead. + if not link_path.is_file(): continue - link_target = link_path.resolve(strict=False) - if seeded_playwright_root.resolve() not in link_target.parents: + try: + script = link_path.read_text(encoding="utf-8") + except OSError: continue - relative_target = link_target.relative_to(seeded_playwright_root.resolve()) - link_path.unlink() - link_path.symlink_to(install_root / relative_target) + match = re.search(r"exec '([^']+)'", script) + if not match: + continue + target_path = Path(match.group(1)) + if seeded_resolved not in target_path.resolve().parents: + continue + relative_target = target_path.resolve().relative_to(seeded_resolved) + new_target = install_root / relative_target + link_path.write_text( + script.replace(str(target_path), str(new_target)), + encoding="utf-8", + ) def test_chromium_install_puts_real_browser_into_managed_bin_dir( self, @@ -70,14 +118,14 @@ def test_chromium_install_puts_real_browser_into_managed_bin_dir( assert provider.bin_dir is not None assert installed.loaded_abspath.parent == provider.bin_dir assert installed.loaded_abspath == provider.bin_dir / "chromium" - # The symlink resolves into playwright_root (which is also - # PLAYWRIGHT_BROWSERS_PATH for this provider). - real_target = installed.loaded_abspath.resolve() - assert playwright_root.resolve() in real_target.parents + # The shim resolves into ``playwright_root/cache`` (the + # managed ``PLAYWRIGHT_BROWSERS_PATH`` for this provider). + real_target = _resolve_shim_target(installed.loaded_abspath) + assert (playwright_root / "cache").resolve() in real_target.parents # Playwright lays out chromium builds as chromium-/. assert any( child.name.startswith("chromium-") - for child in playwright_root.iterdir() + for child in (playwright_root / "cache").iterdir() if child.is_dir() ) @@ -134,7 +182,7 @@ def test_install_root_alias_without_explicit_bin_dir_uses_root_bin( # the effective PLAYWRIGHT_BROWSERS_PATH for this provider). assert any( child.name.startswith("chromium-") - for child in install_root.iterdir() + for child in (install_root / "cache").iterdir() if child.is_dir() ) @@ -171,7 +219,7 @@ def test_install_root_and_bin_dir_aliases_install_into_the_requested_paths( # Browser tree still landed in install_root, not bin_dir. assert any( child.name.startswith("chromium-") - for child in install_root.iterdir() + for child in (install_root / "cache").iterdir() if child.is_dir() ) @@ -244,9 +292,10 @@ def test_provider_install_args_are_passed_through_to_playwright_install( assert installed is not None assert installed.loaded_abspath is not None assert installed.loaded_abspath.exists() + cache_dir = playwright_root / "cache" chromium_dirs = [ child - for child in playwright_root.iterdir() + for child in cache_dir.iterdir() if child.is_dir() and child.name.startswith("chromium-") and not child.name.startswith("chromium_headless_shell") @@ -255,7 +304,7 @@ def test_provider_install_args_are_passed_through_to_playwright_install( # ``--no-shell`` should have skipped the headless shell download. headless_shell_dirs = [ child - for child in playwright_root.iterdir() + for child in cache_dir.iterdir() if child.is_dir() and child.name.startswith("chromium_headless_shell") ] assert not headless_shell_dirs, ( @@ -373,17 +422,17 @@ def test_update_refreshes_chromium_in_place( ) assert updated is not None assert updated.loaded_abspath is not None - # The symlink resolves to a chromium build that actually + # The shim resolves to a chromium build that actually # exists on disk after update (whether the build-id moved # depends on the current playwright release, but the # resolved target must always exist and still live inside # ``playwright_root``). - updated_target = updated.loaded_abspath.resolve() + updated_target = _resolve_shim_target(updated.loaded_abspath) assert updated_target.exists() - assert playwright_root.resolve() in updated_target.parents + assert (playwright_root / "cache").resolve() in updated_target.parents assert any( child.name.startswith("chromium-") - for child in playwright_root.iterdir() + for child in (playwright_root / "cache").iterdir() if child.is_dir() ) @@ -397,14 +446,15 @@ def test_provider_dry_run_does_not_install_chromium(self, test_machine): test_machine.exercise_provider_dry_run(provider, bin_name="chromium") # dry_run must not have actually downloaded any browsers. + cache_dir = playwright_root / "cache" browser_dirs = ( [ p - for p in playwright_root.iterdir() + for p in cache_dir.iterdir() if p.is_dir() and p.name.startswith(("chromium-", "firefox-", "webkit-")) ] - if playwright_root.is_dir() + if cache_dir.is_dir() else [] ) assert not browser_dirs, ( diff --git a/tests/test_puppeteerprovider.py b/tests/test_puppeteerprovider.py index 88d4ceec..659544c3 100644 --- a/tests/test_puppeteerprovider.py +++ b/tests/test_puppeteerprovider.py @@ -32,9 +32,9 @@ def test_chrome_alias_installs_real_browser_binary(self, test_machine): assert "@latest" not in installed.name assert "@" not in installed.loaded_abspath.name bin_dir = provider.bin_dir - cache_dir = provider.cache_dir assert bin_dir is not None - assert cache_dir is not None + assert provider.install_root is not None + cache_dir = provider.install_root / "cache" assert installed.loaded_abspath.parent == bin_dir assert installed.loaded_abspath == bin_dir / "chrome" assert (cache_dir / "chromium").exists()