diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 08d74051..e2fe693a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -61,8 +61,21 @@ jobs: # Browser tests for static/js (#339). node --check above is a syntax parse; # this drives the real UI against the real backend, including the Tauri code # path, which is where #335 hid because it was invisible in a browser. + # Split three ways, each shard on its own runner (#686). The suite runs + # serially inside a shard -- workers: 1 in playwright.config -- because it + # shares one backend and one jobs directory, and several specs change server + # state. A shard is a separate machine with its own backend, so splitting + # here adds parallelism without adding sharing. Files stay whole, since + # fullyParallel is off, so tests that rely on order within a file still get + # it. Free on a public repository; each shard pays its own setup. frontend-e2e: runs-on: ubuntu-latest + strategy: + # One failing shard must not cancel the others: the report is only + # useful whole. + fail-fast: false + matrix: + shard: [1, 2, 3] steps: - uses: actions/checkout@v7.0.1 - uses: actions/setup-node@v7.0.0 @@ -74,11 +87,12 @@ jobs: - run: uv sync --frozen --all-extras - run: npm ci - run: npx playwright install --with-deps chromium - - run: npx playwright test + - run: npx playwright test --shard=${{ matrix.shard }}/3 - if: failure() uses: actions/upload-artifact@v7.0.1 with: - name: playwright-report + # Per shard: a second upload under the same name fails the job. + name: playwright-report-${{ matrix.shard }} path: | playwright-report/ test-results/ diff --git a/app/core/config.py b/app/core/config.py index fe9f86ea..0a9f0bc9 100644 --- a/app/core/config.py +++ b/app/core/config.py @@ -1,10 +1,13 @@ import json +import logging import os import re import shutil import sys from pathlib import Path +logger = logging.getLogger("stemdeck.config") + def _env_int(name: str, default: int) -> int: raw = os.environ.get(name, "").strip() @@ -456,17 +459,74 @@ def ffprobe_executable() -> str: return str(FFPROBE_BIN) if FFPROBE_BIN.is_file() else "ffprobe" +def ffmpeg_dir() -> Path | None: + """The directory holding the FFmpeg this process actually runs, or None. + + PATH and yt-dlp both need a directory, and they used to be given + FFMPEG_DIR: in a desktop install always data/ffmpeg, whatever setup had + concluded about what lives there. Setup can reject that copy and settle on + another -- an Intel pair on Apple Silicon (#637), a build missing an + encoder -- and tell the backend through STEMDECK_FFMPEG/STEMDECK_FFPROBE, + which ffmpeg_executable() honours. PATH and yt-dlp never got the message, + so separation used the verified binary while every YouTube import ran the + rejected one (#651). + + So the directory is derived from the binary, which makes it one answer + everywhere. It also covers data/ffmpeg/bin/, the nested layout setup + accepts (#248): yt-dlp was handed data/ffmpeg there, which holds no binary + at all, and yt-dlp does not fall back to PATH from a directory it was + given. + + None when there is no single directory to hand over: no binaries (Docker, + or a source checkout with FFmpeg on PATH, where PATH is already right), or + ffmpeg and ffprobe in different places. yt-dlp takes one directory and + looks for both in it, so a split pair is left to PATH, where the desktop + shell has already put the verified directory first. + + A hand-set STEMDECK_FFMPEG_DIR is still honoured: FFMPEG_BIN defaults to a + file inside it. + + Evaluated per call, not bound at import, so nothing has to agree with a + value captured before the environment was final. + """ + if not (FFMPEG_BIN.is_file() and FFPROBE_BIN.is_file()): + return None + directory = FFMPEG_BIN.resolve().parent + if FFPROBE_BIN.resolve().parent != directory: + return None + return directory + + def configure_portable_environment() -> None: """Keep generated caches inside the portable data folder when requested. This is intentionally best-effort. It only sets variables that are still unset, so explicit caller/env choices win. """ - if FFMPEG_DIR.is_dir(): + # FFmpeg first on PATH for anything that runs a bare `ffmpeg`, Demucs + # decoding a compressed source among them. See ffmpeg_dir() for why this is + # the verified binary's directory and not FFMPEG_DIR. + directory = ffmpeg_dir() + if directory is not None: path = os.environ.get("PATH", "") - ffmpeg_path = str(FFMPEG_DIR) + ffmpeg_path = str(directory) if ffmpeg_path not in path.split(os.pathsep): os.environ["PATH"] = ffmpeg_path + (os.pathsep + path if path else "") + logger.info("FFmpeg directory: %s", directory) + else: + logger.info("FFmpeg directory: none of our own, resolving from PATH") + # Said out loud because the failure it prevents was silent: a data-directory + # FFmpeg that is not the one in use is the copy setup rejected, and the + # error a user sees if it is ever run names that binary (#651). Not said + # for data/ffmpeg/bin/, which is the same install in its nested layout. + if FFMPEG_DIR.is_dir() and ( + directory is None or not directory.is_relative_to(FFMPEG_DIR.resolve()) + ): + logger.info( + "Not using %s: the FFmpeg in use is %s", + FFMPEG_DIR, + directory or "whatever PATH resolves", + ) if PORTABLE_DATA_DIR_ENABLED: os.environ.setdefault("XDG_CACHE_HOME", str(CACHE_DIR)) diff --git a/app/pipeline/download.py b/app/pipeline/download.py index 12afb24f..440a3d34 100644 --- a/app/pipeline/download.py +++ b/app/pipeline/download.py @@ -8,7 +8,7 @@ from yt_dlp import YoutubeDL -from app.core.config import FFMPEG_DIR, bundled_js_runtime, js_solver_available +from app.core.config import bundled_js_runtime, ffmpeg_dir, js_solver_available from app.core.models import Job, JobCancelled, _set from app.core.settings import get_cookies_file, get_max_duration_sec, get_video_max_height @@ -212,9 +212,11 @@ def _base_ydl_opts(extractors: list[str], *, use_cookies: bool = False) -> dict: "socket_timeout": _SOCKET_TIMEOUT_SEC, } # Portable builds have no ffmpeg on PATH; needed wherever a DASH stream - # might be remuxed. Inert for the metadata-only calls. - if FFMPEG_DIR.is_dir(): - opts["ffmpeg_location"] = str(FFMPEG_DIR) + # might be remuxed. Inert for the metadata-only calls. The directory of the + # FFmpeg the backend verified, not FFMPEG_DIR, which can be the copy setup + # rejected (#651); yt-dlp runs whatever it finds there and never falls back. + if (directory := ffmpeg_dir()) is not None: + opts["ffmpeg_location"] = str(directory) # YouTube's n-challenge solver needs a JS runtime. Absent outside portable # builds, where yt-dlp resolves its own from PATH instead (#432). if (runtime := bundled_js_runtime()) is not None: diff --git a/tests/test_ffmpeg_dir.py b/tests/test_ffmpeg_dir.py new file mode 100644 index 00000000..2741b807 --- /dev/null +++ b/tests/test_ffmpeg_dir.py @@ -0,0 +1,208 @@ +"""Which FFmpeg directory the backend hands to PATH and to yt-dlp (#651). + +Setup can reject the FFmpeg in the data directory and settle on another -- an +Intel pair on Apple Silicon (#637), a build missing an encoder -- and it tells +the backend through STEMDECK_FFMPEG / STEMDECK_FFPROBE. Separation honoured +that. PATH and yt-dlp were given FFMPEG_DIR instead, which in a desktop install +is always data/ffmpeg, so every YouTube import ran the binary setup had thrown +out, and the error named a binary the app had supposedly stopped using. + +These build real files in a temporary data directory and reload the config +module under the environment the desktop shell would give it, because what is +under test is exactly what the backend concludes from that environment. +""" + +from __future__ import annotations + +import importlib +import logging +import os +import sys +from pathlib import Path + +import pytest + +EXE = ".exe" if sys.platform.startswith("win") else "" +ENV = ("STEMDECK_DATA_DIR", "STEMDECK_FFMPEG", "STEMDECK_FFPROBE", "STEMDECK_FFMPEG_DIR") + + +def _pair(directory: Path) -> tuple[Path, Path]: + """An ffmpeg and an ffprobe, side by side, as a real install has them.""" + directory.mkdir(parents=True, exist_ok=True) + ffmpeg = directory / f"ffmpeg{EXE}" + ffprobe = directory / f"ffprobe{EXE}" + ffmpeg.write_bytes(b"") + ffprobe.write_bytes(b"") + return ffmpeg, ffprobe + + +@pytest.fixture +def load(monkeypatch): + """Reload config as the backend would start under the given environment.""" + import app.core.config as config + + def _load(data_dir: Path, **env: Path | None): + monkeypatch.setenv("STEMDECK_DATA_DIR", str(data_dir)) + for name in ENV[1:]: + value = env.get(name.removeprefix("STEMDECK_").lower()) + if value is None: + monkeypatch.delenv(name, raising=False) + else: + monkeypatch.setenv(name, str(value)) + return importlib.reload(config) + + yield _load + monkeypatch.undo() + importlib.reload(config) + + +@pytest.fixture +def config_log(): + """What config logged. Attached to its own logger rather than through + caplog, because the app's logging setup may stop propagation upward.""" + records: list[logging.LogRecord] = [] + + class Keep(logging.Handler): + def emit(self, record): + records.append(record) + + logger = logging.getLogger("stemdeck.config") + handler = Keep(level=logging.INFO) + previous = logger.level + logger.addHandler(handler) + logger.setLevel(logging.INFO) + yield records + logger.removeHandler(handler) + logger.setLevel(previous) + + +def _ydl_location(config) -> str | None: + # Imported here: download.py holds the function it imported from config, + # and a reload of config re-executes the same module dict, so it sees the + # reloaded values. + from app.pipeline.download import _base_ydl_opts + + return _base_ydl_opts(["youtube"]).get("ffmpeg_location") + + +# ── the bug ────────────────────────────────────────────────────────────────── + + +def test_a_rejected_data_dir_ffmpeg_is_not_the_directory_used(load, tmp_path): + data = tmp_path / "data" + _pair(data / "ffmpeg") # the pair setup rejected + ffmpeg, ffprobe = _pair(tmp_path / "system") # the one it verified + + config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe) + + assert config.ffmpeg_dir() == (tmp_path / "system").resolve() + + +def test_path_gets_the_verified_directory_and_never_the_rejected_one(load, monkeypatch, tmp_path): + data = tmp_path / "data" + _pair(data / "ffmpeg") + ffmpeg, ffprobe = _pair(tmp_path / "system") + monkeypatch.setenv("PATH", str(tmp_path / "elsewhere")) + + config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe) + config.configure_portable_environment() + + entries = [Path(p) for p in os.environ["PATH"].split(os.pathsep)] + assert entries[0] == (tmp_path / "system").resolve() + assert (data / "ffmpeg").resolve() not in [p.resolve() for p in entries] + + +def test_yt_dlp_is_given_the_verified_directory(load, tmp_path): + data = tmp_path / "data" + _pair(data / "ffmpeg") + ffmpeg, ffprobe = _pair(tmp_path / "system") + + config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe) + + assert _ydl_location(config) == str((tmp_path / "system").resolve()) + + +def test_skipping_the_data_dir_copy_is_logged(load, config_log, monkeypatch, tmp_path): + # The failure was silent; the next report should find this in the log. + data = tmp_path / "data" + _pair(data / "ffmpeg") + ffmpeg, ffprobe = _pair(tmp_path / "system") + monkeypatch.setenv("PATH", "") + + config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe) + config.configure_portable_environment() + + messages = [r.getMessage() for r in config_log] + assert any(m.startswith("Not using") and "ffmpeg" in m for m in messages), messages + + +# ── the nested layout (#248) ───────────────────────────────────────────────── + + +def test_the_nested_bin_layout_hands_over_bin(load, config_log, monkeypatch, tmp_path): + # yt-dlp was given data/ffmpeg here, which holds no binary, and it runs + # dir/ffmpeg literally without falling back to PATH. + data = tmp_path / "data" + ffmpeg, ffprobe = _pair(data / "ffmpeg" / "bin") + monkeypatch.setenv("PATH", "") + + config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe) + config.configure_portable_environment() + + assert config.ffmpeg_dir() == (data / "ffmpeg" / "bin").resolve() + assert _ydl_location(config) == str((data / "ffmpeg" / "bin").resolve()) + # The same install, nested: nothing is being skipped. + assert not any(r.getMessage().startswith("Not using") for r in config_log) + + +# ── what has to keep working ───────────────────────────────────────────────── + + +def test_a_verified_data_dir_ffmpeg_is_still_used(load, tmp_path): + data = tmp_path / "data" + ffmpeg, ffprobe = _pair(data / "ffmpeg") + + config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe) + + assert config.ffmpeg_dir() == (data / "ffmpeg").resolve() + assert _ydl_location(config) == str((data / "ffmpeg").resolve()) + + +def test_docker_and_source_runs_leave_it_to_path(load, monkeypatch, tmp_path): + # No FFmpeg of our own and no override: PATH is already right there. + monkeypatch.setenv("PATH", str(tmp_path / "usr-bin")) + + config = load(tmp_path / "data") + config.configure_portable_environment() + + assert config.ffmpeg_dir() is None + assert os.environ["PATH"] == str(tmp_path / "usr-bin") + assert _ydl_location(config) is None + + +def test_a_hand_set_ffmpeg_dir_is_still_honoured(load, tmp_path): + # Source runs point STEMDECK_FFMPEG_DIR at a bundled build when FFmpeg is + # not on PATH, with no STEMDECK_FFMPEG. The binary defaults into it. + bundled = tmp_path / "bundled-ffmpeg" + _pair(bundled) + + config = load(tmp_path / "data", ffmpeg_dir=bundled) + + assert config.ffmpeg_dir() == bundled.resolve() + assert _ydl_location(config) == str(bundled.resolve()) + + +def test_a_split_pair_is_left_to_path(load, monkeypatch, tmp_path): + # yt-dlp takes one directory and looks for both binaries in it, so there + # is no directory to give it. PATH decides, where the desktop shell has + # already put the verified FFmpeg's directory first. + ffmpeg, _ = _pair(tmp_path / "a") + _, ffprobe = _pair(tmp_path / "b") + monkeypatch.setenv("PATH", str(tmp_path / "shell-path")) + + config = load(tmp_path / "data", ffmpeg=ffmpeg, ffprobe=ffprobe) + config.configure_portable_environment() + + assert config.ffmpeg_dir() is None + assert os.environ["PATH"] == str(tmp_path / "shell-path") + assert _ydl_location(config) is None