Skip to content

Commit 600d8a6

Browse files
fix(python): address final review (CI matrix, lazy-load docs, guards)
- Drop windows-latest from the new CI jobs (binding has no win32 support yet: no _build.py linker branch, run() uses POSIX signal.pause()). - Remove unused cibuildwheel from the wheel-build install step. - Restore the explicit LUOS_ENGINE_LIB_DIR-miss error instead of silently falling back to a .pio walk-up build. - Lock get_phy_handle's _PHY_HANDLES read-check-write. - Fix the stale load_phy docstring (phys load lazily, not at import). - Clean up the .tmp file if the atomic cache rename fails. - Refuse phy download on a +dev build with a clear message. - Tests for the LIB_DIR-miss error and the dev-build download guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 71d7233 commit 600d8a6

6 files changed

Lines changed: 72 additions & 12 deletions

File tree

.github/workflows/pio_release.yml

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,9 @@ jobs:
190190
runs-on: ${{ matrix.os }}
191191
strategy:
192192
matrix:
193-
os: [ubuntu-22.04, macos-latest, windows-latest]
193+
# Windows is not yet supported by the binding (no win32 linker
194+
# branch in _build.py; luos.run() uses POSIX signal.pause()).
195+
os: [ubuntu-22.04, macos-latest]
194196
steps:
195197
- uses: actions/checkout@v4
196198
- name: Set up Python
@@ -200,7 +202,7 @@ jobs:
200202
- name: Install build tooling
201203
run: |
202204
python -m pip install --upgrade pip
203-
pip install platformio cibuildwheel==2.* build
205+
pip install platformio build
204206
- name: Build native engine + phy dylibs
205207
run: pio run -e native_lib
206208
- name: Stage core engine into the package and pin version
@@ -223,7 +225,9 @@ jobs:
223225
runs-on: ${{ matrix.os }}
224226
strategy:
225227
matrix:
226-
os: [ubuntu-22.04, macos-latest, windows-latest]
228+
# Windows is not yet supported by the binding (no win32 linker
229+
# branch in _build.py; luos.run() uses POSIX signal.pause()).
230+
os: [ubuntu-22.04, macos-latest]
227231
steps:
228232
- uses: actions/checkout@v4
229233
- name: Set up Python
@@ -252,7 +256,9 @@ jobs:
252256
runs-on: ${{ matrix.os }}
253257
strategy:
254258
matrix:
255-
os: [ubuntu-22.04, macos-latest, windows-latest]
259+
# Windows is not yet supported by the binding (no win32 linker
260+
# branch in _build.py; luos.run() uses POSIX signal.pause()).
261+
os: [ubuntu-22.04, macos-latest]
256262
steps:
257263
- uses: actions/checkout@v4
258264
- uses: actions/setup-python@v5

bindings/python/luos_engine/_engine.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,8 @@ def is_detected() -> bool:
8484
def load_phy(descriptor, **kwargs) -> None:
8585
"""Load and initialise a network phy. Must be called before start().
8686
87-
The phy dylib is pre-loaded at package import time (see
88-
_ffi.load_dylib). This function validates kwargs via
87+
The phy dylib is resolved and loaded lazily on first use via
88+
get_phy_handle. This function validates kwargs via
8989
descriptor.configure, then calls the phy's init symbol (once per
9090
process — subsequent calls skip init but still register the phy
9191
for the loop thread to tick).

bindings/python/luos_engine/_ffi.py

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,17 @@ def resolve_lib_path() -> Path:
4242
)
4343
return p
4444

45+
# An explicitly-set LUOS_ENGINE_LIB_DIR must contain the engine; fail
46+
# loudly rather than silently falling back to a .pio walk-up build.
47+
env_dir = os.environ.get("LUOS_ENGINE_LIB_DIR")
48+
if env_dir:
49+
found = _find_in_dir(Path(env_dir))
50+
if found:
51+
return found
52+
raise LuosEngineNotFoundError(
53+
f"No libluos_engine.* in LUOS_ENGINE_LIB_DIR={env_dir}"
54+
)
55+
4556
for d in local_lib_dirs():
4657
found = _find_in_dir(d)
4758
if found:
@@ -56,6 +67,7 @@ def resolve_lib_path() -> Path:
5667

5768
_LIB_HANDLE = None
5869
_LIB_LOCK = threading.Lock()
70+
_PHY_LOCK = threading.Lock()
5971
_PHY_HANDLES: dict[str, ctypes.CDLL] = {}
6072

6173

@@ -76,8 +88,11 @@ def get_phy_handle(basename: str) -> ctypes.CDLL:
7688
and loading (and downloading if necessary) on first use."""
7789
handle = _PHY_HANDLES.get(basename)
7890
if handle is None:
79-
from ._phy_loader import ensure_phy_dylib
80-
path = ensure_phy_dylib(basename)
81-
handle = ctypes.CDLL(str(path), mode=ctypes.RTLD_GLOBAL)
82-
_PHY_HANDLES[basename] = handle
91+
with _PHY_LOCK:
92+
handle = _PHY_HANDLES.get(basename)
93+
if handle is None:
94+
from ._phy_loader import ensure_phy_dylib
95+
path = ensure_phy_dylib(basename)
96+
handle = ctypes.CDLL(str(path), mode=ctypes.RTLD_GLOBAL)
97+
_PHY_HANDLES[basename] = handle
8398
return handle

bindings/python/luos_engine/_phy_loader.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,11 @@ def _download(asset_name: str, dest: Path) -> Path:
5959
dest.parent.mkdir(parents=True, exist_ok=True)
6060
tmp = dest.with_name(dest.name + ".tmp")
6161
tmp.write_bytes(data)
62-
os.replace(tmp, dest) # atomic
62+
try:
63+
os.replace(tmp, dest) # atomic
64+
except OSError:
65+
tmp.unlink(missing_ok=True)
66+
raise
6367
return dest
6468

6569

@@ -86,5 +90,12 @@ def ensure_phy_dylib(basename: str) -> Path:
8690
f"variable to download from the {ENGINE_VERSION} release."
8791
)
8892

93+
if ENGINE_VERSION.endswith("+dev"):
94+
raise PhyDownloadError(
95+
f"{filename} not found locally and this is a dev build "
96+
f"(ENGINE_VERSION={ENGINE_VERSION!r}, no matching release). "
97+
f"Build it with `~/.platformio/penv/bin/pio run -e native_lib`."
98+
)
99+
89100
asset_name = f"{basename}-{platform_key()}{ext}"
90101
return _download(asset_name, cached)

bindings/python/tests/test_ffi_discovery.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,3 +25,14 @@ def test_missing_raises(monkeypatch, tmp_path):
2525
from luos_engine._ffi import resolve_lib_path, LuosEngineNotFoundError
2626
with pytest.raises(LuosEngineNotFoundError):
2727
resolve_lib_path()
28+
29+
30+
def test_explicit_lib_dir_miss_raises_specific(monkeypatch, tmp_path):
31+
"""An explicitly-set LUOS_ENGINE_LIB_DIR that lacks the engine must
32+
fail loudly, not silently fall back to a .pio walk-up build."""
33+
monkeypatch.delenv("LUOS_ENGINE_LIB", raising=False)
34+
monkeypatch.setenv("LUOS_ENGINE_LIB_DIR", str(tmp_path))
35+
from luos_engine._ffi import resolve_lib_path, LuosEngineNotFoundError
36+
with pytest.raises(LuosEngineNotFoundError) as exc:
37+
resolve_lib_path()
38+
assert "LUOS_ENGINE_LIB_DIR" in str(exc.value)

bindings/python/tests/test_phy_loader.py

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,13 @@
88
@pytest.fixture(autouse=True)
99
def _isolate_cache(tmp_path, monkeypatch):
1010
"""Force the cache into tmp and disable any local-build discovery so
11-
tests exercise the cache/download paths deterministically."""
11+
tests exercise the cache/download paths deterministically. Pin a
12+
non-dev engine version so the +dev download guard doesn't fire (the
13+
dev-build guard is covered by its own test)."""
1214
monkeypatch.setenv("LUOS_ENGINE_CACHE_DIR", str(tmp_path / "cache"))
1315
monkeypatch.delenv("LUOS_ENGINE_LIB_DIR", raising=False)
1416
monkeypatch.delenv("LUOS_ENGINE_NO_DOWNLOAD", raising=False)
17+
monkeypatch.setattr(_phy_loader, "ENGINE_VERSION", "9.9.9")
1518
# No local build dirs.
1619
monkeypatch.setattr(_phy_loader, "local_lib_dirs", lambda: iter(()))
1720

@@ -77,3 +80,17 @@ def test_no_download_env_raises_when_missing(monkeypatch):
7780
monkeypatch.setenv("LUOS_ENGINE_NO_DOWNLOAD", "1")
7881
with pytest.raises(LuosEngineNotFoundError):
7982
_phy_loader.ensure_phy_dylib("libws_network")
83+
84+
85+
def test_dev_build_refuses_download(monkeypatch):
86+
"""A +dev build has no matching release; fail with a clear message
87+
instead of 404-ing on a non-existent release asset."""
88+
monkeypatch.setattr(_phy_loader, "ENGINE_VERSION", "0.0.0+dev")
89+
90+
def _boom(*a, **k):
91+
raise AssertionError("must not attempt a download for a dev build")
92+
93+
monkeypatch.setattr(_phy_loader, "_fetch_json", _boom)
94+
with pytest.raises(_phy_loader.PhyDownloadError) as exc:
95+
_phy_loader.ensure_phy_dylib("libws_network")
96+
assert "dev build" in str(exc.value)

0 commit comments

Comments
 (0)