diff --git a/kwave/__init__.py b/kwave/__init__.py index fafc6f83..f5d1b72f 100644 --- a/kwave/__init__.py +++ b/kwave/__init__.py @@ -3,6 +3,7 @@ import logging import os import platform +import stat from pathlib import Path from typing import List from urllib.request import urlretrieve @@ -77,6 +78,31 @@ def _hash_file(filepath: str) -> str: return md5.hexdigest() +def _ensure_executable(binary_filepath) -> None: + # Self-heal the executable bit on Linux/macOS. urlretrieve creates files + # at 0644, and prior versions of this package didn't fix that up, so users + # upgrading with a cached non-executable binary on disk would otherwise + # stay stuck (the cache check below returns True and skips re-download). + # Any OS-level failure here (broken symlink, read-only FS, wrong ownership, + # TOCTOU race) is degraded to a warning so it never aborts `import kwave`. + if PLATFORM == "windows": + return + try: + current_mode = os.stat(binary_filepath).st_mode + desired_mode = current_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH + if current_mode == desired_mode: + return + os.chmod(binary_filepath, desired_mode) + except OSError: # pragma: no cover - defensive; degrades to warning, never fatal + # Don't abort import. The user can chmod +x manually or reinstall + # into a writable location. + logging.warning( + "kwave: cannot set executable bit on %s — backend='cpp' may fail with " + "Permission denied. Run `chmod +x` manually or reinstall.", + binary_filepath, + ) + + def _is_binary_present(binary_name: str, binary_type: str) -> bool: binary_filepath = BINARY_PATH / binary_name binary_file_exists = os.path.exists(binary_filepath) @@ -106,6 +132,8 @@ def _is_binary_present(binary_name: str, binary_type: str) -> bool: if existing_metadata["url"] not in latest_urls: return False + _ensure_executable(binary_filepath) + # No need to check `version` field for now # because we version is already present in the URL return True @@ -176,6 +204,7 @@ def download_binaries(system_os: str, bin_type: str): try: binary_filepath = os.path.join(BINARY_PATH, filename) urlretrieve(url, binary_filepath) + _ensure_executable(binary_filepath) _record_binary_metadata(binary_version=binary_version, binary_filepath=binary_filepath, binary_url=url, filename=filename) except TimeoutError: diff --git a/tests/test__init__.py b/tests/test__init__.py index 0c36c505..6acad88b 100644 --- a/tests/test__init__.py +++ b/tests/test__init__.py @@ -1,15 +1,98 @@ import importlib +import json +import os +import stat +import sys from unittest.mock import patch import pytest def test__init(): - with pytest.raises(NotImplementedError): - with patch("platform.system", lambda: "Unknown"): - import kwave + import kwave - importlib.reload(kwave) + try: + with pytest.raises(NotImplementedError): + with patch("platform.system", lambda: "Unknown"): + importlib.reload(kwave) + finally: + # The failed reload above left kwave.PLATFORM = "unknown" in module + # state (line where PLATFORM is set ran before the NotImplementedError). + # Reload once more without the patch so subsequent tests see a valid + # PLATFORM / URL_DICT pair. + importlib.reload(kwave) + + +def _seed_binary(binary_path, binary_name, url, *, mode=0o644): + """Write a fake binary at the given path + matching metadata file.""" + binary_path.mkdir(parents=True, exist_ok=True) + filepath = binary_path / binary_name + filepath.write_bytes(b"fake-binary-payload") + filepath.chmod(mode) + import kwave + + metadata = { + "url": url, + "version": url.rsplit("/", 2)[-2], + "file_hash": kwave._hash_file(str(filepath)), + } + (binary_path / f"{binary_name}_metadata.json").write_text(json.dumps(metadata)) + return filepath + + +@pytest.mark.skipif(sys.platform == "win32", reason="exec bit is meaningless on Windows") +def test_download_sets_executable_bit(tmp_path, monkeypatch): + """Regression for #740 — urlretrieve creates files at 0644 and the C++ + backend fails with Permission denied (exit 126) when the executor invokes + the binary.""" + import kwave + + monkeypatch.setattr(kwave, "BINARY_PATH", tmp_path) + + test_url = next(urls[0] for urls in kwave.URL_DICT[kwave.PLATFORM].values() if urls) + filename = test_url.rsplit("/", 1)[-1] + + def fake_urlretrieve(url, dest): + with open(dest, "wb") as f: + f.write(b"fake-binary-payload") + os.chmod(dest, 0o644) + + monkeypatch.setattr(kwave, "urlretrieve", fake_urlretrieve) + monkeypatch.setattr(kwave, "URL_DICT", {kwave.PLATFORM: {"test": [test_url]}}) + + kwave.download_binaries(kwave.PLATFORM, "test") + + binary_filepath = tmp_path / filename + assert binary_filepath.exists() + mode = os.stat(binary_filepath).st_mode + assert mode & stat.S_IXUSR, "owner exec bit not set after download" + assert mode & stat.S_IXGRP, "group exec bit not set after download" + assert mode & stat.S_IXOTH, "other exec bit not set after download" + + +@pytest.mark.skipif(sys.platform == "win32", reason="exec bit is meaningless on Windows") +def test_existing_non_executable_binary_is_healed(tmp_path, monkeypatch): + """Regression for #740 — users with a cached non-executable binary from a + pre-fix install would otherwise stay broken across upgrades because + _is_binary_present skips re-download for valid cached binaries.""" + import kwave + + monkeypatch.setattr(kwave, "BINARY_PATH", tmp_path) + + test_url = next(urls[0] for urls in kwave.URL_DICT[kwave.PLATFORM].values() if urls) + filename = test_url.rsplit("/", 1)[-1] + binary_filepath = _seed_binary(tmp_path, filename, test_url, mode=0o644) + pre_mode = os.stat(binary_filepath).st_mode + assert not (pre_mode & stat.S_IXUSR), "test fixture should start without exec bit" + + monkeypatch.setattr(kwave, "URL_DICT", {kwave.PLATFORM: {"test": [test_url]}}) + + assert kwave._is_binary_present(filename, "test") is True + + post_mode = os.stat(binary_filepath).st_mode + assert post_mode & stat.S_IXUSR, "owner exec bit not healed on cache hit" + assert post_mode & stat.S_IXGRP, "group exec bit not healed on cache hit" + assert post_mode & stat.S_IXOTH, "other exec bit not healed on cache hit" if __name__ == "__main__":