From c1c3808e1e94363747a0cfd6214d37f4ab745ebf Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Sat, 17 Oct 2026 12:41:45 +0200 Subject: [PATCH] fix: tell the installer user what blocks a write --- install.ps1 | 2 +- install.py | 61 ++++++++++++++++++++++++++++++++++++++----- install.sh | 2 +- tests/test_install.py | 48 ++++++++++++++++++++++++++++++++-- 4 files changed, 103 insertions(+), 10 deletions(-) diff --git a/install.ps1 b/install.ps1 index 51431bf0..cc260a6d 100644 --- a/install.ps1 +++ b/install.ps1 @@ -8,7 +8,7 @@ param( $ErrorActionPreference = "Stop" $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" -$defaultInstallSha256 = "953240dab33c25e52005155ecc8cd694269234697ca2c3f55939994fc6b61951" +$defaultInstallSha256 = "8c0e9c762d2fce4460e4ee097055dc0c51f068363ce20583652f7fe6b4a7ee40" $maximumInstallerBytes = 2MB $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null } $temporary = $null diff --git a/install.py b/install.py index 4b482033..71a0453f 100644 --- a/install.py +++ b/install.py @@ -16,6 +16,7 @@ from __future__ import annotations import argparse import concurrent.futures +import errno import hashlib import io import json @@ -1865,7 +1866,12 @@ def _download_one( dest = _destination_path(bios_path, f["dest"]) except ValueError: return str(f.get("dest", "?")), False, "unsafe destination" - dest.parent.mkdir(parents=True, exist_ok=True) + try: + dest.parent.mkdir(parents=True, exist_ok=True) + except OSError as exc: + if _write_refused(exc): + return f["dest"], False, NOT_WRITABLE + return f["dest"], False, exc.strerror or str(exc) if f.get("release_asset"): url = RELEASE_URL.format(asset=urllib.parse.quote(f["release_asset"], safe="")) @@ -1877,6 +1883,7 @@ def _download_one( # next run, asks only for what is missing. resumable = expected_size >= RESUME_FROM_SIZE reason = "download failed" + refused = False for attempt in range(1, MAX_RETRIES + 1): tmp_path: Path | None = None counted = 0 @@ -1976,10 +1983,15 @@ def _download_one( delay = _retry_delay(attempt, exc.headers.get("Retry-After") if exc.headers else None) except (urllib.error.URLError, OSError, ValueError) as exc: - reason = str(getattr(exc, "reason", "") or exc) + if (isinstance(exc, OSError) and not isinstance(exc, urllib.error.URLError) + and _write_refused(exc)): + # No attempt can change a refused write. + reason, refused = NOT_WRITABLE, True + else: + reason = str(getattr(exc, "reason", "") or exc) if verbose: print(f" Attempt {attempt} failed: {exc}", file=sys.stderr) - if tmp_path is not None and not resumable and tmp_path.exists(): + if not refused and tmp_path is not None and not resumable and tmp_path.exists(): tmp_path.unlink() delay = _retry_delay(attempt, None) @@ -1987,6 +1999,8 @@ def _download_one( progress.add(-counted) if resumed_count: progress.forget_resumed(resumed_count) + if refused: + return f["dest"], False, reason if attempt < MAX_RETRIES and delay and _stop.wait(delay): return f["dest"], False, "stopped" @@ -2031,6 +2045,30 @@ def _locations(paths: list[Path]) -> str: return ", ".join(str(p) for p in paths) if paths else "nowhere" +# A write refused by the file system: retrying cannot change it, and the +# summary names the two ways out. +NOT_WRITABLE = "folder not writable" + + +def _write_refused(exc: OSError) -> bool: + return isinstance(exc, PermissionError) or exc.errno == errno.EROFS + + +def _write_advice(path: Path, assumed: bool) -> str: + """What to do about a folder this run could not write. + + sudo fixes a root-owned BIOS folder on the console itself. On another + machine it would create that folder on the wrong disk: a platform named + but not found here is a card or share mounted elsewhere, named by --dest. + """ + if assumed: + return (f" {path} is where the platform keeps its BIOS on its own system, " + "and it was not found here. If this machine prepares its card or " + "share, run the same command with --dest pointing at that folder.") + return (" This user cannot write there: run the same command with sudo, " + "or choose another folder with --dest.") + + def _plural(count: int, word: str) -> str: """Count and noun, so a run of one file does not read as machine output.""" return f"{count} {word}" if count == 1 else f"{count} {word}s" @@ -2047,6 +2085,7 @@ def _report_outcome( transferred: int = 0, seconds: float = 0.0, declined: int = 0, + assumed: set[Path] | None = None, ) -> None: """Close the run on what happened, and leave with a status that says so. @@ -2070,9 +2109,14 @@ def _report_outcome( print("Run the same command again to carry on from here. Nothing is lost.") sys.exit(130) + refused = any(reason == NOT_WRITABLE for _dest, reason in failures or []) + advice = [_write_advice(path, path in (assumed or set())) for path in paths] if refused else [] + if errors and not downloaded: print(f"\nNothing was installed. {_plural(errors, 'file')} failed.") print(f"Location that was being written to: {_locations(paths)}") + for line in advice: + print(line) print("Run the same command again once that is resolved. " "Files already installed are kept.") sys.exit(2) @@ -2081,6 +2125,8 @@ def _report_outcome( print(f"\nFinished with errors. {_plural(downloaded, 'file')} installed, " f"{_plural(errors, 'file')} failed.") print(f"Location: {_locations(paths)}") + for line in advice: + print(line) print("Run the same command again to retry only those.") sys.exit(1) @@ -2726,7 +2772,9 @@ def _install(args: argparse.Namespace, real_stdout) -> None: print(f" {name}: {path}") return - # Platform detection or override + # Platform detection or override. A destination taken from the default + # table, the platform not being found here, is only a guess. + assumed_dests: set[Path] = set() if args.platform: args.platform = normalize_platform(args.platform) if args.platform and args.dest: @@ -2744,6 +2792,7 @@ def _install(args: argparse.Namespace, real_stdout) -> None: f"using default path: {default_dest}" ) platforms = [(args.platform, default_dest)] + assumed_dests.add(default_dest) elif args.dest: print(f" Using destination: {args.dest}") platforms = [("retroarch", args.dest)] @@ -2988,8 +3037,7 @@ def _install(args: argparse.Namespace, real_stdout) -> None: except OSError as exc: print(f"\nCannot create {bios_path}.", file=sys.stderr) print(f" {exc.strerror}.", file=sys.stderr) - print(" On Batocera, Recalbox and ROCKNIX the BIOS folder " - "belongs to root: run the same command with sudo.", + print(_write_advice(bios_path, bios_path in assumed_dests), file=sys.stderr) sys.exit(1) _check_free_space(bios_path, dl_size) @@ -3051,6 +3099,7 @@ def _install(args: argparse.Namespace, real_stdout) -> None: transferred=total_bytes, seconds=total_seconds, declined=total_declined, + assumed=assumed_dests, ) diff --git a/install.sh b/install.sh index 4b56be66..5fc64e0b 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ esac TEMP_INSTALLER="" TEMP_DIRECTORY="" DEFAULT_INSTALL_URL="https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" -DEFAULT_INSTALL_SHA256="953240dab33c25e52005155ecc8cd694269234697ca2c3f55939994fc6b61951" +DEFAULT_INSTALL_SHA256="8c0e9c762d2fce4460e4ee097055dc0c51f068363ce20583652f7fe6b4a7ee40" MAX_INSTALLER_BYTES=2097152 cleanup() { diff --git a/tests/test_install.py b/tests/test_install.py index f766052f..1173f194 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -377,6 +377,50 @@ class TestFreeSpaceGuard(unittest.TestCase): self.assertEqual(code, 1) +class AFolderThatRefusesWritesIsNamed(unittest.TestCase): + """A read-only BIOS folder ended the run on a traceback (a subfolder to + create), or on three retries per file and "once that is resolved"; a + platform named but not detected was told to use sudo, which on another + machine creates /userdata/bios on the wrong disk.""" + + ENTRY = {"dest": "mame2015/neogeo.zip", "repo_path": "bios/x", "size": 4} + + def test_a_subfolder_that_cannot_be_made_is_a_failure_not_a_crash(self): + with unittest.mock.patch.object(Path, "mkdir", side_effect=PermissionError(13, "denied")): + result = install._download_one(self.ENTRY, Path("/userdata/bios")) + self.assertEqual(result, ("mame2015/neogeo.zip", False, install.NOT_WRITABLE)) + + def test_a_refused_write_is_not_retried(self): + class _Body(io.BytesIO): + status, headers = 200, {"Content-Length": "4"} + + with tempfile.TemporaryDirectory() as tmp, \ + unittest.mock.patch.object(install.urllib.request, "urlopen", + return_value=_Body(b"data")) as opened, \ + unittest.mock.patch.object(install.tempfile, "NamedTemporaryFile", + side_effect=PermissionError(13, "denied")): + result = install._download_one(self.ENTRY, Path(tmp)) + self.assertEqual(result[1:], (False, install.NOT_WRITABLE)) + self.assertEqual(opened.call_count, 1) + + def _summary_for(self, assumed): + buf = io.StringIO() + with contextlib.redirect_stdout(buf), self.assertRaises(SystemExit): + install._report_outcome( + paths=[Path("/userdata/bios")], downloaded=0, up_to_date=0, errors=1, + omitted=0, interrupted=False, + failures=[("neogeo.zip", install.NOT_WRITABLE)], assumed=assumed, + ) + return buf.getvalue() + + def test_the_summary_says_sudo_on_the_console_and_dest_elsewhere(self): + here = self._summary_for(set()) + self.assertIn("sudo", here) + guessed = self._summary_for({Path("/userdata/bios")}) + self.assertIn("--dest", guessed) + self.assertNotIn("sudo", guessed) + + class TestStopFlag(unittest.TestCase): """Ctrl+C ends the run instead of letting the queue drain.""" @@ -1685,7 +1729,7 @@ class TargetListFailureIsNotAnAnswer(unittest.TestCase): def boom(*args, **kwargs): raise urllib.error.URLError("timed out") - with mock.patch.object(install.urllib.request, "urlopen", boom), \ + with unittest.mock.patch.object(install.urllib.request, "urlopen", boom), \ contextlib.redirect_stderr(io.StringIO()): self.assertIsNone(install.fetch_targets("retroarch")) @@ -1696,7 +1740,7 @@ class TargetListFailureIsNotAnAnswer(unittest.TestCase): def missing(url, *args, **kwargs): raise urllib.error.HTTPError(url, 404, "Not Found", {}, None) - with mock.patch.object(install.urllib.request, "urlopen", missing): + with unittest.mock.patch.object(install.urllib.request, "urlopen", missing): self.assertEqual(install.fetch_targets("retroarch"), {}) if __name__ == "__main__":