From 6f541e9b8c586424ebe0d4a34dc72ccba2f335bc Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:03:05 +0200 Subject: [PATCH] fix: tell an unreadable copy from a locked folder A copy that could not be read when it was copied was reported as a folder not writable. The source is now opened first and its errors are its own, and a download failing after it names both reasons. A run reading a pack falls back to the pack, never to the network. --- install.ps1 | 2 +- install.py | 71 ++++++++++++++++++++++++++++++++------ install.sh | 2 +- tests/test_install_from.py | 67 +++++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 13 deletions(-) diff --git a/install.ps1 b/install.ps1 index 85ad245b..ab2f66bd 100644 --- a/install.ps1 +++ b/install.ps1 @@ -11,7 +11,7 @@ $ErrorActionPreference = "Stop" $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" - $defaultInstallSha256 = "bbb64b0bf499e079e5c6166c2907834d65a437438aeb4b4511cebc76728b8bb0" + $defaultInstallSha256 = "baeaf6acc5a7813df508d997c72ce934fcf3010b953f278426ccb95fd9a4abf8" $maximumInstallerBytes = 2MB $installerExit = 0 $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null } diff --git a/install.py b/install.py index 4a83fb5a..b3a02e6d 100644 --- a/install.py +++ b/install.py @@ -4221,9 +4221,17 @@ def _download_one( if result[2] in ("stopped", NOT_WRITABLE, READ_ONLY) or result[2].startswith(NOT_KEPT): return result # The copy went or changed since it was read (a card pulled out, a - # file another run replaced): the file is fetched as if never found. + # file another run replaced): the file is fetched as if never found, + # and a fetch that fails too names both reasons. if verbose: print(f" {f['dest']}: {result[2]}, fetching it instead", file=sys.stderr) + fetched = _download_one(f, bios_path, verbose, progress) + why = fetched[2] + if (fetched[1] or why in ("stopped", NOT_WRITABLE, READ_ONLY) + or why.startswith(NOT_KEPT)): + return fetched + both = f"{result[2]}, and {why}" + return fetched[0], False, NetworkReason(both) if isinstance(why, NetworkReason) else both if LOCAL.pack is not None: return _extract_from_pack(f, bios_path, dest, progress) @@ -6529,7 +6537,7 @@ def _extract_from_pack( return f["dest"], False, NOT_IN_PACK archive, info = held return _write_verified(f, bios_path, dest, lambda: archive.open(info), progress, - "the pack") + "the pack", named=f"{info.filename} inside {archive.filename}") @contextlib.contextmanager @@ -6549,30 +6557,67 @@ def _copy_local( """Copy one file found on this machine to its place, checked like a download. Its bytes count as already here, not in the download speed.""" return _write_verified(f, bios_path, dest, lambda: _open_copy(copy), progress, - f"the copy in {copy.index.label}", local=True) + f"the copy in {copy.index.label}", local=True, + named=copy.named()) + + +class _SourceUnreadable(Exception): + """The bytes to write could not be read: the source's fault, never the + folder's, so it falls back like any copy that failed.""" + + +def unreadable_words(exc: BaseException) -> str: + """Why a file or a ZIP member could not be read, in words.""" + if isinstance(exc, RuntimeError) and "encrypted" in str(exc): + return "it is encrypted, a password is needed" + if isinstance(exc, NotImplementedError): + return "it is packed with a compression method this installer cannot read" + if isinstance(exc, (zipfile.BadZipFile, zlib.error, EOFError)): + return "it is damaged" + if isinstance(exc, OSError) and exc.strerror: + return exc.strerror + return str(exc) or type(exc).__name__ def _write_verified( f: dict, bios_path: Path, dest: Path, source: Callable[[], ContextManager[BinaryIO]], progress: Progress | None, - held_in: str, local: bool = False, + held_in: str, local: bool = False, named: str = "", ) -> tuple[str, bool, str]: """Write the bytes source opens at dest once they match the list's size - and hashes, the file in place kept aside first when it differs.""" + and hashes, the file in place kept aside first when it differs. + + The source is opened before the temporary file, and its read errors + are told apart from the write errors: a copy that could not be read + was reported as a folder not writable. named is the source in the + reason then, held_in when empty.""" expected = {name: f[name].lower() for name in ("sha256", "sha1") if f.get(name)} tmp_path: Path | None = None counted = 0 + + def unreadable(exc: BaseException) -> _SourceUnreadable: + return _SourceUnreadable(f"{named or held_in} could not be read: " + f"{unreadable_words(exc)}") + try: - with source() as src, tempfile.NamedTemporaryFile( - mode="wb", dir=dest.parent, prefix=f".{dest.name}.", suffix=".part", - delete=False, - ) as out: + with contextlib.ExitStack() as stack: + try: + src = stack.enter_context(source()) + except _MEMBER_ERRORS as exc: + raise unreadable(exc) from exc + out = stack.enter_context(tempfile.NamedTemporaryFile( + mode="wb", dir=dest.parent, prefix=f".{dest.name}.", suffix=".part", + delete=False, + )) tmp_path = Path(out.name) size = 0 while True: if _stop.is_set(): raise _Interrupted() - chunk = src.read(min(1024 * 1024, f["size"] + 1 - size)) + try: + chunk = src.read(min(1024 * 1024, f["size"] + 1 - size)) + except _MEMBER_ERRORS as exc: + raise unreadable(exc) from exc if not chunk: break size += len(chunk) @@ -6598,7 +6643,7 @@ def _write_verified( return f["dest"], False, "stopped" except _NotKept as exc: return f["dest"], False, f"{NOT_KEPT} ({exc})" - except (ValueError, *_MEMBER_ERRORS) as exc: + except (_SourceUnreadable, ValueError, *_MEMBER_ERRORS) as exc: if progress is not None and counted: progress.add(-counted, resumed=local) if isinstance(exc, OSError) and _write_refused(exc): @@ -6647,6 +6692,10 @@ class LocalCopy(NamedTuple): rel = self.index.relative(self.path) return f"{rel}, inside as {self.member}" if self.member else rel + def named(self) -> str: + """The copy by its full path, the member first when it is one.""" + return f"{self.member} inside {self.path}" if self.member else str(self.path) + class FolderIndex: """The files under a folder, by name and by size, hashed on demand. diff --git a/install.sh b/install.sh index 6d60d829..3d9f51d5 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="bbb64b0bf499e079e5c6166c2907834d65a437438aeb4b4511cebc76728b8bb0" +DEFAULT_INSTALL_SHA256="baeaf6acc5a7813df508d997c72ce934fcf3010b953f278426ccb95fd9a4abf8" MAX_INSTALLER_BYTES=2097152 cleanup() { diff --git a/tests/test_install_from.py b/tests/test_install_from.py index c9fc7490..4655e476 100644 --- a/tests/test_install_from.py +++ b/tests/test_install_from.py @@ -13,12 +13,15 @@ from __future__ import annotations import contextlib import copy +import errno import io import json +import os import subprocess import sys import time import unittest +import urllib.error import zipfile from pathlib import Path from unittest import mock @@ -481,6 +484,70 @@ class MisplacedInTheBiosFolder(Scratch): self.assertNotIn("copied from", proc.stdout) +def _unprivileged(test: unittest.TestCase) -> None: + if os.name != "posix" or os.geteuid() == 0: + test.skipTest("a file of mode 000 is read here anyway") + + +class ACopyThatFails(FromFolder): + """A copy found by the plan that cannot be read when it is copied: + it read as "folder not writable", and a download failing after it lost + the first reason.""" + + def setUp(self): + super().setUp() + self.entry = json.loads(self.list.read_text())["files"][0] + self.source = self.keep("old/scph5501.bin", US) + self.copy = install.FolderIndex(self.old).find(self.entry) + self.assertIsNotNone(self.copy) + self.addCleanup(install.LOCAL.reset) + self.dest.mkdir() + + def test_an_unreadable_source_falls_back_and_names_both_reasons(self): + _unprivileged(self) + self.source.chmod(0) + self.addCleanup(self.source.chmod, 0o644) + refused = urllib.error.URLError(ConnectionRefusedError(111, "Connection refused")) + with mock.patch.object(install, "_retry_delay", return_value=0), \ + mock.patch.object(install.urllib.request, "urlopen", side_effect=refused): + dest, ok, reason = install._download_one(self.entry, self.dest, copy=self.copy) + self.assertFalse(ok) + self.assertTrue(reason.startswith( + f"{self.source.resolve()} could not be read: Permission denied, and "), reason) + self.assertNotIn(install.NOT_WRITABLE, reason) + self.assertIsInstance(reason, install.NetworkReason) + + def test_a_read_error_is_the_source_and_not_the_folder(self): + class Failing(io.BytesIO): + def read(self, *_args): + raise OSError(errno.EIO, "Input/output error") + + for source, words in ( + (lambda: Failing(b""), "Input/output error"), + (mock.Mock(side_effect=PermissionError(errno.EACCES, "Permission denied")), + "Permission denied")): + with self.subTest(words=words): + result = install._write_verified( + self.entry, self.dest, self.dest / "scph5501.bin", source, None, + "the copy in E:\\", named="E:\\bios\\a.bin") + self.assertEqual(result, ("scph5501.bin", False, + f"E:\\bios\\a.bin could not be read: {words}")) + self.assertEqual(list(self.dest.iterdir()), [], "no temporary file is left") + + def test_a_run_reading_a_pack_falls_back_to_the_pack_only(self): + _unprivileged(self) + self.source.chmod(0) + self.addCleanup(self.source.chmod, 0o644) + install.LOCAL.pack = mock.Mock(find=mock.Mock(return_value=None)) + with mock.patch.object(install.urllib.request, "urlopen", + side_effect=AssertionError("network")): + _dest, ok, reason = install._download_one(self.entry, self.dest, copy=self.copy) + install.LOCAL.pack = None + self.assertFalse(ok) + self.assertEqual(reason, f"{self.source.resolve()} could not be read: " + f"Permission denied, and {install.NOT_IN_PACK}") + + class ManySameSizedFiles(FromFolder): """A card of 16 KB ROMs and a list of 16 KB entries: each entry sorted and walked every file of its size."""