Set exec bit on Linux/macOS binaries after download

[waltsims]

Summary

After urlretrieve downloads the C++ binary, set the executable bit on Linux/macOS so the C++ backend can actually invoke it.

Why this was latent

Most users have hit _is_binary_present from a prior install where pip/wheel extraction set the bit, so the broken `urlretrieve` path wasn't exercised. Surfaced on a clean Colab install when validating v1.4.0:

```
$ kspaceFirstOrder-CUDA --version
bash: kspaceFirstOrder-CUDA: Permission denied
exit=126
```

`ls -l` showed `-rw-r--r--`.

Change

In `kwave/init.py` `download_binaries`, after `urlretrieve`, OR-in the user/group/other exec bits on non-Windows platforms.

Test plan

• Pre-commit clean (ruff + format + codespell)
• CI: clean install on Linux + macOS runners should now produce an executable binary
• Manual: on Colab, after `pip install` from this branch, `ls -l` of the downloaded binary shows the exec bit set

Closes #740

🤖 Generated with Claude Code

Greptile Summary

This PR fixes a latent bug where urlretrieve downloads binaries with 0644 permissions, causing Permission denied (exit 126) when the C++ backend tries to invoke them on Linux/macOS. A new _ensure_executable helper ORs in the user/group/other execute bits and is called both after fresh downloads and during cache-hit validation in _is_binary_present, so existing installations are self-healed at import time.

• _ensure_executable uses os.stat + os.chmod guarded by a broad except OSError that degrades to a warning rather than aborting import kwave.
• _is_binary_present now calls _ensure_executable before returning True, healing cached non-executable binaries on the next import kwave without requiring a re-download.
• Tests add two regression scenarios (test_download_sets_executable_bit, test_existing_non_executable_binary_is_healed) using a next(urls[0] for … if urls) pattern that safely skips the empty darwin["cuda"] list.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped, correctly heals both fresh-download and cached-binary paths, and degrades gracefully on read-only filesystems.

The _ensure_executable helper is called in both the download and cache-hit paths, ensuring all users are covered at the next import kwave. The except OSError catch is broad enough to handle read-only mounts and wrong-ownership scenarios without aborting the import. Tests cover both code paths with proper platform guards and the if urls filter that prevents an IndexError on macOS where the CUDA URL list is empty.

No files require special attention.

Important Files Changed

Filename	Overview
kwave/init.py	Adds _ensure_executable (OSError-safe chmod) and calls it in both download_binaries and _is_binary_present, correctly healing both fresh downloads and cached binaries at import time.
tests/test__init__.py	Adds two well-structured regression tests (download path and cache-hit healing) with if urls guard that safely handles the empty darwin["cuda"] list; also fixes existing test__init to restore module state after a reload-based exception test.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[import kwave] --> B{binaries_present?}
    B -- No --> C[install_binaries]
    C --> D[download_binaries]
    D --> E[urlretrieve]
    E --> F[_ensure_executable\nafter download]
    F --> G[_record_binary_metadata]

    B -- Yes --> H[_is_binary_present\nfor each binary]
    H --> I{file + hash\n+ URL valid?}
    I -- No --> J[return False\ntriggers re-download]
    I -- Yes --> K[_ensure_executable\nheals cached binary]
    K --> L[return True]

    F --> M{PLATFORM == windows?}
    K --> M
    M -- Yes --> N[return no-op]
    M -- No --> O[os.stat → compute\ndesired mode]
    O --> P{bits already set?}
    P -- Yes --> N
    P -- No --> Q[os.chmod]
    Q --> R{OSError?}
    R -- Yes --> S[log warning\nnever fatal]
    R -- No --> T[done ✓]

Loading

Comments Outside Diff (1)

1. kwave/__init__.py, line 81-112 (link)

   Existing broken installations won't be healed

   _is_binary_present returns True for a binary that is already on disk with a valid hash and matching URL — it never checks the executable bit. A user whose binary was downloaded before this fix (or whose pip install produced the binary without the exec bit) will have binaries_present() return True, so install_binaries is skipped and the chmod added in this PR never runs for them. Their Permission denied error persists across upgrades.

_{Reviews (9): Last reviewed commit: "Mark OSError defensive branch as no-cove..." | Re-trigger Greptile}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.63%. Comparing base (21f4521) to head (ba8eff1).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #741      +/-   ##
==========================================
+ Coverage   75.48%   75.63%   +0.14%     
==========================================
  Files          57       57              
  Lines        8168     8180      +12     
  Branches     1595     1597       +2     
==========================================
+ Hits         6166     6187      +21     
+ Misses       1382     1373       -9     
  Partials      620      620              

Flag	Coverage Δ
3.10	75.59% <91.66%> (+0.13%)	⬆️
3.11	75.59% <91.66%> (+0.13%)	⬆️
3.12	75.59% <91.66%> (+0.13%)	⬆️
3.13	75.59% <91.66%> (+0.13%)	⬆️
macos-latest	75.46% <83.33%> (+0.12%)	⬆️
ubuntu-latest	75.51% <83.33%> (+0.12%)	⬆️
windows-latest	75.30% <41.66%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[waltsims]

Addressed Greptile review (Confidence 3/5 → expecting 5/5 next pass).

Greptile P1 — "existing broken installations won't be healed": Valid. _is_binary_present short-circuits re-download for any cached binary with matching hash + URL, so the original chmod in download_binaries only ran for fresh installs. Users who already had a 0644 binary on disk from a pre-fix install would stay broken across upgrades.

Fix (commit f54800b): extracted the chmod logic into _ensure_executable() and now call it from both download_binaries (fresh download path) and _is_binary_present (cache-hit path). Idempotent — only chmods when the desired bits aren't already set, so it doesn't churn for healthy installs.

Greptile minor — PLATFORM vs system_os parameter mismatch: mooted by the refactor — _ensure_executable operates on the file path, not a system_os argument, so the inconsistency goes away.

Regression tests added (tests/test__init__.py):

• test_download_sets_executable_bit — mocks urlretrieve to drop a 0644 file, asserts the post-download mode has all three exec bits set
• test_existing_non_executable_binary_is_healed — seeds a cached 0644 binary + matching metadata, asserts _is_binary_present returns True AND the binary is now executable

Both tests are skipped on Windows where the exec bit is meaningless. They pass locally.

[waltsims]

Addressed latest Greptile feedback (Confidence 3/5, two findings):

1. PermissionError unwrapped on os.chmod — legitimate. If the binary is on a read-only filesystem or owned by another user, the chmod fails and the exception propagated out of _is_binary_present at module import time — turning "chmod failed" into "import kwave aborts entirely." Strictly worse than the original Permission-denied-on-execute bug. Commit 93ea62c wraps the chmod in try/except, logs a clear warning hint pointing at chmod +x or reinstall, and continues. backend='python' stays usable. Added test_ensure_executable_swallows_permission_error to lock it in.

2. Stale carry-over P1 ("existing broken installations won't be healed") — already fixed in commit f54800b; Greptile is still anchored on the round-1 SHA 2a327f3. The flowchart in Greptile's own summary on this round actually shows the cache-hit healing path (J --> _ensure_executable --> K). Re-trigger should resolve this one.

3. PLATFORM vs system_os mismatch — mooted by the refactor. _ensure_executable operates on a file path with no system_os argument, called from both download_binaries (where system_os is the param) and _is_binary_present (where there's no system_os at all, only module-level PLATFORM). Centralizing on PLATFORM is the consistent choice; and since install_binaries → download_binaries(PLATFORM, ...) is the only call path, system_os == PLATFORM always holds in practice anyway.

@greptileai review

[greptile-apps]

except PermissionError is too narrow to cover all filesystem-level chmod failures. On a truly read-only filesystem (e.g., a Docker image layer or a read-only bind-mount), os.chmod raises OSError with errno.EROFS (30), which is not a PermissionError — it's only a plain OSError. That unhandled exception would still propagate through _is_binary_present → binaries_present() → module-level if not binaries_present() and abort import kwave entirely, exactly the failure mode the try/except was meant to prevent. The fix is to widen the catch to OSError, which is the base class for all OS-level I/O errors including PermissionError.

Suggested change

	try:
	os.chmod(binary_filepath, desired_mode)
	except PermissionError:
	try:
	os.chmod(binary_filepath, desired_mode)
	except OSError:

[waltsims]

@greptileai review

Two changes since your last review:

1. d8150ab — broadened the guard in _ensure_executable from PermissionError → OSError so os.stat is covered too (addresses your 4/5 follow-up)
2. e158328 — trimmed the parameterized OSError test (guard kept; testing edge cases that can't realistically happen given kwave controls the binary path)