Merge remote-tracking branch 'origin/main' into fix/1933-port-holder
# Conflicts: # CHANGELOG.md
This commit is contained in:
@@ -11,6 +11,7 @@ the frozen-backend fallback mirror it for their toolchains.
|
||||
**Highlights**
|
||||
- A pronunciation entry that is stored but not applied yet says so, instead of looking like it did not match (#1949)
|
||||
- A bare 500 report now names the backend error class, so two unrelated faults stop filing the same issue (#1773)
|
||||
- A rejected dubbing source language now names the code it rejected (#1960)
|
||||
- The first-run install log is kept on disk instead of vanishing with the setup screen (#1847)
|
||||
- `bun run desktop` reclaims port 3900 from a backend the app itself left running, instead of refusing to start (#1974)
|
||||
- A dictation shortcut another app already owns now says so, instead of silently doing nothing (#1858)
|
||||
@@ -91,6 +92,7 @@ the frozen-backend fallback mirror it for their toolchains.
|
||||
|
||||
### Fixed
|
||||
|
||||
- Windows contributors can run the test suite without Developer Mode: tests that create a symlink now skip instead of failing with `WinError 1314` (#1990)
|
||||
- A port-3900 conflict now names who is actually holding it, and gives the command that ends an orphaned backend, instead of telling you to quit an app that has no window (#1933) — thanks @Chang-Jin-Lee!
|
||||
|
||||
- Windows desktop launches no longer freeze at "Loading ML runtime (PyTorch)": the parent-liveness watchdog polls the stdin pipe instead of leaving a read pending, which deadlocked numpy's OpenBLAS initializer (#1952)
|
||||
|
||||
@@ -540,12 +540,30 @@ _DUB_SOURCE_LANG_CODES = frozenset({
|
||||
|
||||
|
||||
def _source_lang_override(value: str | None) -> str | None:
|
||||
"""Normalize a user-selected source language; auto/und means detect."""
|
||||
"""Normalize a user-selected source language; auto/und means detect.
|
||||
|
||||
A rejection NAMES the code it rejected. "Invalid source language code" on
|
||||
its own cannot be acted on or reported usefully: it does not say which of
|
||||
the ninety-odd codes was wrong, so neither the user nor a maintainer
|
||||
reading the auto-filed issue can tell whether the picker offered something
|
||||
the backend does not accept, or a stale preference from an older build is
|
||||
still being sent (#1960).
|
||||
|
||||
The value is a language code the user chose from a menu — not private
|
||||
data — and the neighbouring engine validator already echoes its input the
|
||||
same way.
|
||||
"""
|
||||
code = (value or "").strip().lower()
|
||||
if code in {"", "auto", "und"}:
|
||||
return None
|
||||
if code not in _DUB_SOURCE_LANG_CODES:
|
||||
raise HTTPException(status_code=400, detail="Invalid source language code")
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail=(
|
||||
f"Invalid source language code: {code!r}. Pick a language from "
|
||||
"the Dubbing source-language menu, or leave it on auto-detect."
|
||||
),
|
||||
)
|
||||
return code
|
||||
|
||||
|
||||
|
||||
+18
-5
@@ -258,17 +258,30 @@ docker compose -f deploy/docker-compose.yml --profile rocm up -d
|
||||
|
||||
> **ARM64 hosts:** Compose has no per-command `--platform` flag, so the
|
||||
> override that works for `docker pull` and `docker run` does not reach it.
|
||||
> Export `DOCKER_DEFAULT_PLATFORM=linux/amd64` for the shell you run Compose
|
||||
> from, or the image resolves to the ARM64 manifest that does not exist and
|
||||
> fails with `no matching manifest for linux/arm64/v8`:
|
||||
> Set `DOCKER_DEFAULT_PLATFORM=linux/amd64` in the shell you run Compose from,
|
||||
> or the image resolves to the ARM64 manifest that does not exist and fails
|
||||
> with `no matching manifest for linux/arm64/v8`. Only the CPU profile makes
|
||||
> sense under emulation — it is not a GPU workaround.
|
||||
>
|
||||
> ```bash
|
||||
> export DOCKER_DEFAULT_PLATFORM=linux/amd64
|
||||
> docker compose -f deploy/docker-compose.yml --profile cpu pull
|
||||
> docker compose -f deploy/docker-compose.yml --profile cpu up -d
|
||||
> ```
|
||||
>
|
||||
> Same caveat as above — this is emulation, not native ARM64 support, and only
|
||||
> the CPU profile makes sense under it.
|
||||
> In PowerShell, set both the administrator key and the platform for the
|
||||
> session before running the same two commands:
|
||||
>
|
||||
> ```powershell
|
||||
> $env:OMNIVOICE_API_KEY = python -c "import secrets; print(secrets.token_urlsafe(32))"
|
||||
> $env:DOCKER_DEFAULT_PLATFORM = 'linux/amd64'
|
||||
> docker compose -f deploy/docker-compose.yml --profile cpu pull
|
||||
> docker compose -f deploy/docker-compose.yml --profile cpu up -d
|
||||
> ```
|
||||
>
|
||||
> Either way the setting lives only in that shell and the processes it starts.
|
||||
> The [architecture limits](#architecture) still apply: this is emulated CPU
|
||||
> inference, not native ARM64 support.
|
||||
|
||||
The `docker-compose.yml` shipped in `deploy/` defaults to `127.0.0.1:3900`
|
||||
on the host. The backend inside the container binds to `0.0.0.0` so the
|
||||
|
||||
@@ -510,12 +510,14 @@ def test_package_filename_default_and_override(monkeypatch, app_modules):
|
||||
assert bootstrap.package_filename() == "breeze-tts-2-bf16.gguf"
|
||||
|
||||
|
||||
def test_materialize_hf_symlink_keeps_gguf_suffix_without_copy(tmp_path, app_modules):
|
||||
def test_materialize_hf_symlink_keeps_gguf_suffix_without_copy(
|
||||
tmp_path, app_modules, symlink_or_skip,
|
||||
):
|
||||
bootstrap = app_modules.bootstrap
|
||||
blob = tmp_path / "content-addressed-blob"
|
||||
blob.write_bytes(b"GGUF test payload")
|
||||
snapshot = tmp_path / "breeze-tts-2-q8_0.gguf"
|
||||
snapshot.symlink_to(blob)
|
||||
symlink_or_skip(snapshot, blob)
|
||||
|
||||
materialized = bootstrap._materialize_gguf_cache_path(snapshot)
|
||||
|
||||
@@ -533,16 +535,18 @@ def test_materialize_rejects_extensionless_model(tmp_path, app_modules):
|
||||
bootstrap._materialize_gguf_cache_path(model)
|
||||
|
||||
|
||||
def test_materialize_replaces_preexisting_symlink_alias(tmp_path, app_modules):
|
||||
def test_materialize_replaces_preexisting_symlink_alias(
|
||||
tmp_path, app_modules, symlink_or_skip,
|
||||
):
|
||||
bootstrap = app_modules.bootstrap
|
||||
blob = tmp_path / "content-addressed-blob"
|
||||
blob.write_bytes(b"GGUF test payload")
|
||||
snapshot = tmp_path / "breeze-tts-2-q8_0.gguf"
|
||||
snapshot.symlink_to(blob)
|
||||
symlink_or_skip(snapshot, blob)
|
||||
alias = snapshot.with_name(
|
||||
f".{snapshot.stem}-{bootstrap.HF_MODEL_REVISION[:12]}.audiocpp.gguf"
|
||||
)
|
||||
alias.symlink_to(blob)
|
||||
symlink_or_skip(alias, blob)
|
||||
|
||||
materialized = bootstrap._materialize_gguf_cache_path(snapshot)
|
||||
|
||||
@@ -552,7 +556,7 @@ def test_materialize_replaces_preexisting_symlink_alias(tmp_path, app_modules):
|
||||
|
||||
|
||||
def test_materialize_cross_filesystem_symlink_links_beside_target(
|
||||
tmp_path, monkeypatch, app_modules,
|
||||
tmp_path, monkeypatch, app_modules, symlink_or_skip,
|
||||
):
|
||||
bootstrap = app_modules.bootstrap
|
||||
source_dir = tmp_path / "source"
|
||||
@@ -562,7 +566,7 @@ def test_materialize_cross_filesystem_symlink_links_beside_target(
|
||||
link_dir = tmp_path / "link"
|
||||
link_dir.mkdir()
|
||||
snapshot = link_dir / "custom.gguf"
|
||||
snapshot.symlink_to(blob)
|
||||
symlink_or_skip(snapshot, blob)
|
||||
real_link = os.link
|
||||
calls = 0
|
||||
|
||||
@@ -584,13 +588,13 @@ def test_materialize_cross_filesystem_symlink_links_beside_target(
|
||||
|
||||
|
||||
def test_file_override_materializes_hf_style_symlink(
|
||||
tmp_path, monkeypatch, app_modules,
|
||||
tmp_path, monkeypatch, app_modules, symlink_or_skip,
|
||||
):
|
||||
bootstrap = app_modules.bootstrap
|
||||
blob = tmp_path / "blob"
|
||||
blob.write_bytes(b"GGUF test payload")
|
||||
model = tmp_path / "custom.gguf"
|
||||
model.symlink_to(blob)
|
||||
symlink_or_skip(model, blob)
|
||||
monkeypatch.setenv("OMNIVOICE_AUDIOCPP_MODEL", str(model))
|
||||
|
||||
resolved = bootstrap.resolve_model_file()
|
||||
@@ -601,7 +605,7 @@ def test_file_override_materializes_hf_style_symlink(
|
||||
|
||||
|
||||
def test_directory_override_materializes_hf_style_symlink(
|
||||
tmp_path, monkeypatch, app_modules,
|
||||
tmp_path, monkeypatch, app_modules, symlink_or_skip,
|
||||
):
|
||||
bootstrap = app_modules.bootstrap
|
||||
blob = tmp_path / "blob"
|
||||
@@ -609,7 +613,7 @@ def test_directory_override_materializes_hf_style_symlink(
|
||||
model_dir = tmp_path / "models"
|
||||
model_dir.mkdir()
|
||||
model = model_dir / bootstrap.DEFAULT_PACKAGE
|
||||
model.symlink_to(blob)
|
||||
symlink_or_skip(model, blob)
|
||||
monkeypatch.setenv("OMNIVOICE_AUDIOCPP_MODEL", str(model_dir))
|
||||
|
||||
resolved = bootstrap.resolve_model_file()
|
||||
|
||||
@@ -570,3 +570,23 @@ def _restore_config_paths_after_reload():
|
||||
for const, value in before.items():
|
||||
if isinstance(getattr(mod, const, None), str) and getattr(mod, const) != value:
|
||||
setattr(mod, const, value)
|
||||
|
||||
|
||||
# ── Symlinks on a stock Windows checkout ───────────────────────────────────
|
||||
# Creating a symlink on Windows needs SeCreateSymbolicLinkPrivilege, which a
|
||||
# normal user account does not hold unless Developer Mode is on. Hosted CI
|
||||
# runs elevated, so unguarded `Path.symlink_to` / `os.symlink` calls pass
|
||||
# there and hand a Windows contributor a suite that fails on their machine
|
||||
# for reasons that have nothing to do with their change (WinError 1314). Tests
|
||||
# that need a real symlink take this fixture, so the environment that cannot
|
||||
# make one skips instead of erroring. Coverage still holds: the full pytest
|
||||
# job runs on Linux.
|
||||
@pytest.fixture
|
||||
def symlink_or_skip():
|
||||
def _make(link, target, *, target_is_directory: bool = False):
|
||||
try:
|
||||
os.symlink(target, link, target_is_directory=target_is_directory)
|
||||
except (OSError, NotImplementedError) as exc: # pragma: no cover - Windows-only
|
||||
pytest.skip(f"symlinks unavailable in this environment: {exc}")
|
||||
|
||||
return _make
|
||||
|
||||
@@ -320,8 +320,15 @@ class TestAudioOnlyDubbing:
|
||||
|
||||
assert form.status_code == 400
|
||||
assert json_response.status_code == 400
|
||||
assert form.json()["detail"] == "Invalid source language code"
|
||||
assert json_response.json()["detail"] == "Invalid source language code"
|
||||
# #1960: the message now NAMES the rejected code, because the bare
|
||||
# sentence could not be triaged from an auto-filed report. Assert the
|
||||
# substance rather than the exact wording, so improving the guidance
|
||||
# again does not fail this test for the wrong reason.
|
||||
# Each request sent a DIFFERENT bad code; pair each response with its
|
||||
# own, or the assertion passes on whichever happens to match.
|
||||
for body, sent in ((form.json()["detail"], "english"), (json_response.json()["detail"], "x-123")):
|
||||
assert "Invalid source language code" in body
|
||||
assert sent in body
|
||||
|
||||
def test_upload_accepts_a_registered_source_language(self, app_client, monkeypatch):
|
||||
client, dc, _dx, _tmp = app_client
|
||||
|
||||
@@ -0,0 +1,68 @@
|
||||
"""#1960 — a rejected source language must say WHICH code it rejected.
|
||||
|
||||
The report was "400 Bad Request: Invalid source language code" and nothing
|
||||
else. That cannot be acted on or triaged: it does not say which of the ninety
|
||||
or so codes was wrong, so neither the user nor a maintainer reading the
|
||||
auto-filed issue can tell whether the picker offered something the backend
|
||||
does not accept, or a stale preference from an older build is still being
|
||||
sent. I could not determine the cause from the report — which is the point.
|
||||
|
||||
The value is a language code chosen from a menu, not private data, and the
|
||||
neighbouring engine validator already echoes its input the same way.
|
||||
|
||||
The last test is the durable one: it reads the picker's own list and asserts
|
||||
the backend accepts all of it, so a code added to the menu cannot silently
|
||||
become a 400.
|
||||
"""
|
||||
import pathlib
|
||||
import re
|
||||
|
||||
import pytest
|
||||
from fastapi import HTTPException
|
||||
|
||||
from api.routers.dub_core import _DUB_SOURCE_LANG_CODES, _source_lang_override
|
||||
|
||||
_REPO = pathlib.Path(__file__).resolve().parents[1]
|
||||
_LANGUAGES_JS = _REPO / "frontend" / "src" / "utils" / "languages.js"
|
||||
|
||||
|
||||
def test_the_rejection_names_the_code():
|
||||
with pytest.raises(HTTPException) as caught:
|
||||
_source_lang_override("zz-XX")
|
||||
detail = caught.value.detail
|
||||
assert "zz-xx" in detail
|
||||
assert caught.value.status_code == 400
|
||||
|
||||
|
||||
def test_the_rejection_says_what_to_do():
|
||||
with pytest.raises(HTTPException) as caught:
|
||||
_source_lang_override("nope")
|
||||
assert "auto-detect" in caught.value.detail
|
||||
|
||||
|
||||
@pytest.mark.parametrize("code", ["", " ", "auto", "und", None])
|
||||
def test_detect_is_still_detect(code):
|
||||
assert _source_lang_override(code) is None
|
||||
|
||||
|
||||
def test_accepted_codes_are_unchanged():
|
||||
# This only improved a message; it must not start accepting or rejecting
|
||||
# anything different.
|
||||
for code in sorted(_DUB_SOURCE_LANG_CODES):
|
||||
assert _source_lang_override(code) == code
|
||||
|
||||
|
||||
def test_every_code_the_picker_offers_is_accepted():
|
||||
src = _LANGUAGES_JS.read_text(encoding="utf-8")
|
||||
codes = re.findall(r"code:\s*['\"]([A-Za-z-]+)['\"]", src)
|
||||
assert len(codes) > 50, "LANG_CODES did not parse; update this extraction"
|
||||
rejected = []
|
||||
for code in codes:
|
||||
try:
|
||||
_source_lang_override(code)
|
||||
except HTTPException:
|
||||
rejected.append(code)
|
||||
assert rejected == [], (
|
||||
"the Dubbing source-language menu offers codes the backend rejects, so "
|
||||
f"picking them returns 400: {rejected}"
|
||||
)
|
||||
@@ -209,13 +209,13 @@ def test_export_404_when_source_gone(client, outputs_dir, tmp_path, authorize_de
|
||||
|
||||
|
||||
def test_export_symlink_inside_outputs_pointing_outside_is_rejected(
|
||||
client, outputs_dir, tmp_path, authorize_destination
|
||||
client, outputs_dir, tmp_path, authorize_destination, symlink_or_skip
|
||||
):
|
||||
# A symlink planted in OUTPUTS_DIR must not let /export read arbitrary
|
||||
# files: realpath resolves it outside the root, failing containment.
|
||||
secret = tmp_path / "secret.txt"
|
||||
secret.write_bytes(b"credentials")
|
||||
(outputs_dir / "innocent.wav").symlink_to(secret)
|
||||
symlink_or_skip(outputs_dir / "innocent.wav", secret)
|
||||
|
||||
r = client.post("/export", json={
|
||||
"source_filename": "innocent.wav",
|
||||
|
||||
@@ -334,12 +334,12 @@ def test_clear_temp_removes_only_app_owned_entries(tmp_path):
|
||||
assert (tmp / "keep.txt").exists()
|
||||
|
||||
|
||||
def test_clear_temp_unlinks_symlinks_without_following(tmp_path):
|
||||
def test_clear_temp_unlinks_symlinks_without_following(tmp_path, symlink_or_skip):
|
||||
tmp = tmp_path / "tmp"
|
||||
target = tmp_path / "precious"
|
||||
_write(str(target / "data.bin"), 50)
|
||||
os.makedirs(tmp, exist_ok=True)
|
||||
os.symlink(str(target), str(tmp / "omnivoice_link"))
|
||||
symlink_or_skip(str(tmp / "omnivoice_link"), str(target), target_is_directory=True)
|
||||
|
||||
res = storage_report.clear_temp(str(tmp))
|
||||
|
||||
|
||||
@@ -0,0 +1,114 @@
|
||||
"""Every symlink a test creates must be able to skip instead of erroring.
|
||||
|
||||
Creating a symlink on Windows requires SeCreateSymbolicLinkPrivilege, which a
|
||||
normal account does not hold without Developer Mode. Hosted CI runs elevated,
|
||||
so an unguarded `Path.symlink_to` / `os.symlink` passes there and fails only
|
||||
on a contributor's Windows checkout, with `WinError 1314` and no connection to
|
||||
their change. Five tests in `test_audiocpp_backend.py` plus one each in
|
||||
`test_exports_api.py` and `test_storage_report.py` shipped exactly that.
|
||||
|
||||
`tests/conftest.py` provides the `symlink_or_skip` fixture. This test keeps new
|
||||
call sites on it — a mechanical rule, so it lives in CI rather than in reviewer
|
||||
attention (CLAUDE.md token economy).
|
||||
|
||||
Guarded means one of: the `symlink_or_skip` fixture, a call inside a `try`
|
||||
(the module already handles the failure), or a helper that skips on its own.
|
||||
"""
|
||||
|
||||
import ast
|
||||
from pathlib import Path
|
||||
|
||||
TESTS = Path(__file__).resolve().parent
|
||||
|
||||
# Helpers that already skip on failure themselves; calls inside them are the
|
||||
# guard, not a violation.
|
||||
GUARD_FUNCTIONS = {"_symlink_or_skip", "_make", "symlink_or_skip"}
|
||||
|
||||
|
||||
def _creates_symlink(node: ast.AST) -> bool:
|
||||
if not isinstance(node, ast.Call):
|
||||
return False
|
||||
func = node.func
|
||||
if isinstance(func, ast.Attribute):
|
||||
if func.attr == "symlink_to":
|
||||
return True
|
||||
if func.attr == "symlink" and isinstance(func.value, ast.Name) and func.value.id == "os":
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _has_skipif(decorators) -> bool:
|
||||
for decorator in decorators:
|
||||
for node in ast.walk(decorator):
|
||||
if isinstance(node, ast.Attribute) and node.attr in ("skipif", "skip"):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _module_is_skippable(tree: ast.Module) -> bool:
|
||||
"""A module-level `pytestmark = pytest.mark.skipif(...)` guards every test
|
||||
in the file, so a symlink call inside one is already conditional."""
|
||||
for node in tree.body:
|
||||
if not isinstance(node, ast.Assign):
|
||||
continue
|
||||
names = {t.id for t in node.targets if isinstance(t, ast.Name)}
|
||||
if "pytestmark" in names and _has_skipif([node.value]):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _calls_a_guard(function: ast.AST) -> bool:
|
||||
"""The test already ran a skipping helper, so reaching a later raw call
|
||||
means the environment demonstrably supports symlinks."""
|
||||
for node in ast.walk(function):
|
||||
if isinstance(node, ast.Call) and isinstance(node.func, ast.Name):
|
||||
if node.func.id in GUARD_FUNCTIONS:
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _unguarded(path: Path) -> list:
|
||||
tree = ast.parse(path.read_text(encoding="utf-8"))
|
||||
if _module_is_skippable(tree):
|
||||
return []
|
||||
parents = {}
|
||||
for parent in ast.walk(tree):
|
||||
for child in ast.iter_child_nodes(parent):
|
||||
parents[child] = parent
|
||||
|
||||
bad = []
|
||||
for node in ast.walk(tree):
|
||||
if not _creates_symlink(node):
|
||||
continue
|
||||
cursor = node
|
||||
guarded = False
|
||||
while cursor in parents:
|
||||
cursor = parents[cursor]
|
||||
if isinstance(cursor, ast.Try):
|
||||
guarded = True
|
||||
break
|
||||
if isinstance(cursor, (ast.FunctionDef, ast.AsyncFunctionDef)):
|
||||
if cursor.name in GUARD_FUNCTIONS or _has_skipif(cursor.decorator_list):
|
||||
guarded = True
|
||||
break
|
||||
if cursor.name.startswith("test_") and _calls_a_guard(cursor):
|
||||
guarded = True
|
||||
break
|
||||
if not guarded:
|
||||
bad.append(node.lineno)
|
||||
return bad
|
||||
|
||||
|
||||
def test_no_unguarded_symlink_creation_in_tests():
|
||||
offenders = {}
|
||||
for path in sorted(TESTS.rglob("test_*.py")):
|
||||
if path.name == Path(__file__).name:
|
||||
continue
|
||||
lines = _unguarded(path)
|
||||
if lines:
|
||||
offenders[str(path.relative_to(TESTS.parent))] = lines
|
||||
assert not offenders, (
|
||||
"These tests create a symlink without a way to skip, so they fail with "
|
||||
"WinError 1314 on a stock Windows checkout. Take the `symlink_or_skip` "
|
||||
f"fixture from tests/conftest.py instead: {offenders}"
|
||||
)
|
||||
Reference in New Issue
Block a user