fix: tell the installer user what blocks a write

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

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,
         )
 
 

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() {

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__":