fix: name --from copies that could not be read

A file, ZIP member or folder below a --from folder that could not be
read was taken as no match. When its size is that of a file the run
still needs, or unknown, --check, --dry-run and an install now name it
with the reason, and an unreadable event carries it.

install.ps1

@@ -11,7 +11,7 @@
 
     $ErrorActionPreference = "Stop"
     $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py"
-    $defaultInstallSha256 = "baeaf6acc5a7813df508d997c72ce934fcf3010b953f278426ccb95fd9a4abf8"
+    $defaultInstallSha256 = "4b658dc29528e94460995eaf844b1fc1cca9040ffa10cd71ac6656b4db5f1b34"
     $maximumInstallerBytes = 2MB
     $installerExit = 0
     $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null }

install.py

@@ -510,6 +510,10 @@ EVENT_KEYS: dict[str, tuple[str, ...]] = {
     # a folder named with --from (reason empty). member names the ZIP member
     # when path is an archive, else "". Its copy is a downloaded event.
     "found": ("platform", "dest", "folder", "path", "member", "reason"),
+    # What a folder named with --from holds that could not be read, of the
+    # size of a file no copy was found for, or of no known size (a folder,
+    # a ZIP that would not open). member as in found, reason in words.
+    "unreadable": ("platform", "folder", "path", "member", "reason"),
     "download": ("platform", "files", "bytes"),
     "progress": ("files_done", "files_total", "bytes_done", "bytes_total"),
     "downloaded": ("dest", "ok", "reason", "size", "files_done", "files_total",
@@ -6732,12 +6736,20 @@ class FolderIndex:
         self._loose_named: dict[tuple[int, str], list[Path]] | None = None
         self._member_named: dict[int, dict[str, list[tuple[Path, str]]]] = {}
         self._scans: dict[tuple[str, int, str], _HashScan] = {}
+        self._member_size: dict[tuple[Path, str], int] = {}
+        # (path, member or "") -> (its size or None when unknown, why) of each
+        # file, member or folder below root that could not be read.
+        self.unreadable: dict[tuple[Path, str], tuple[int | None, str]] = {}
         if root.is_file():
             if root.name.casefold().endswith(".zip"):
                 self.archives.append(root)
             return
         seen = 0
-        for folder, dirs, names in os.walk(root):
+
+        def unlisted(exc: OSError) -> None:
+            self._unread(Path(exc.filename or root), "", None, exc)
+
+        for folder, dirs, names in os.walk(root, onerror=unlisted):
             dirs[:] = sorted(d for d in dirs
                              if d != BACKUP_DIR and d.casefold() not in SYSTEM_FOLDERS)
             for name in sorted(names):
@@ -6753,7 +6765,8 @@ class FolderIndex:
                     if not path.is_file():
                         continue
                     size = path.stat().st_size
-                except OSError:
+                except OSError as exc:
+                    self._unread(path, "", None, exc)
                     continue
                 self.by_name.setdefault(name.casefold(), []).append(path)
                 self.by_size.setdefault(size, []).append(path)
@@ -6769,11 +6782,17 @@ class FolderIndex:
         """A SHA-1 already known, so the file is not read again."""
         self._sha1[path] = sha1
 
+    def _unread(self, path: Path, member: str, size: int | None, exc: BaseException) -> None:
+        """Note a file, a member or a folder that could not be read: a copy
+        the run needs may be the one, and it is named instead of dropped."""
+        self.unreadable.setdefault((path, member), (size, unreadable_words(exc)))
+
     def sha1(self, path: Path) -> str:
         if path not in self._sha1:
             try:
                 self._sha1[path] = _sha1_file(path)
-            except OSError:
+            except OSError as exc:
+                self._unread(path, "", self.size.get(path), exc)
                 self._sha1[path] = ""
         return self._sha1[path]
 
@@ -6789,11 +6808,21 @@ class FolderIndex:
                         value = _digest_handle(handle, (algorithm,))[algorithm]
                 else:
                     value = _digest_file(path, (algorithm,))[algorithm]
-            except _MEMBER_ERRORS:
+            except _MEMBER_ERRORS as exc:
+                size = self._member_size.get((path, member)) if member else self.size.get(path)
+                self._unread(path, member, size, exc)
                 value = ""
             self._digests[key] = value
         return self._digests[key]
 
+    def unreadable_for(self, sizes: Collection[int]) -> list[tuple[Path, str, str]]:
+        """(path, member, why) of what could not be read and may hold a file
+        of one of sizes: its size is one of them, or unknown (a folder, an
+        archive whose list of members could not be read)."""
+        return sorted((path, member, why)
+                      for (path, member), (size, why) in self.unreadable.items()
+                      if size is None or size in sizes)
+
     def _scan(self, kind: str, size: int, algorithm: str) -> _HashScan:
         """The files (kind "loose") or ZIP members ("member") of one size,
         hashed in the order the folder was read, each read once."""
@@ -6839,12 +6868,18 @@ class FolderIndex:
                 try:
                     with zipfile.ZipFile(path) as archive:
                         infos = archive.infolist()
-                except (OSError, zipfile.BadZipFile):
+                except OSError as exc:
+                    # A ZIP that will not open may hold anything: named. A
+                    # damaged one is a download cut short, not a copy.
+                    self._unread(path, "", None, exc)
+                    continue
+                except zipfile.BadZipFile:
                     continue
                 for info in infos:
                     if not info.is_dir():
                         self._member_sizes.setdefault(info.file_size, []).append(
                             (path, info.filename))
+                        self._member_size[(path, info.filename)] = info.file_size
         return self._member_sizes.get(size, [])
 
     def find(self, entry: dict, skip: Collection[Path] = ()) -> LocalCopy | None:
@@ -7461,6 +7496,47 @@ def emit_copies(
                     path=str(copy.path), member=copy.member, reason=reason)
 
 
+class Unreadable(NamedTuple):
+    """A file, ZIP member or folder below a --from folder that could not be
+    read, and may be a file the run still needs."""
+    index: FolderIndex
+    path: Path
+    member: str
+    why: str
+
+    def line(self) -> str:
+        where = f"{self.member} inside {self.path}" if self.member else str(self.path)
+        return f"{where} could not be read: {self.why}"
+
+
+def unreadable_copies(
+    entries: list[dict], copies: dict[int, LocalCopy], folders: list[FolderIndex],
+) -> list[Unreadable]:
+    """What the --from folders hold that could not be read, of the size of
+    a file no copy was found for, or of no known size: dropped silently,
+    a file a user had read as one to download."""
+    sizes = {e["size"] for e in entries
+             if id(e) not in copies and isinstance(e.get("size"), int)}
+    if not sizes:
+        return []
+    return [Unreadable(index, *held) for index in folders
+            for held in index.unreadable_for(sizes)]
+
+
+def unreadable_lines(found: list[Unreadable], every: bool) -> list[str]:
+    lines = [f"  {item.line()}" for item in (found if every else found[:10])]
+    if len(found) > len(lines):
+        lines.append(f"  ... and {len(found) - len(lines)} more could not be read "
+                     "(--verbose lists them all)")
+    return lines
+
+
+def emit_unreadable(plat: str, found: list[Unreadable]) -> None:
+    for item in found:
+        EVENTS.emit("unreadable", platform=plat, folder=item.index.label,
+                    path=str(item.path), member=item.member, reason=item.why)
+
+
 def sources_line(
     folders: list[tuple[str, int]], fixed: int, downloaded: int, missing: int,
     fetched: str = "downloaded", shared: int = 0,
@@ -8553,6 +8629,7 @@ def _install(args: argparse.Namespace, real_stdout: TextIO) -> None:
         if bios_index is not None:
             bios_indexes.append(bios_index)
         copies = plan_local_copies(to_download, bios_path, bios_index, LOCAL.folders)
+        unread = unreadable_copies(to_download, copies, LOCAL.folders)
         hint = [] if LOCAL.pack is not None else bulk_hint(
             to_download, manifest,
             systems_chosen=bool(selection and selection[0]),
@@ -8577,6 +8654,7 @@ def _install(args: argparse.Namespace, real_stdout: TextIO) -> None:
                 chosen=selection[0] if selection else None,
             )
             emit_copies(plat_name, to_download, copies, bios_index, bios_path, files)
+            emit_unreadable(plat_name, unread)
         total_verified += len(up_to_date)
 
         if args.check:
@@ -8595,6 +8673,8 @@ def _install(args: argparse.Namespace, real_stdout: TextIO) -> None:
                 for line in check_lines(missing, mismatched, files, manifest, bios_path,
                                         args.verbose, up_to_date, bios_index, copies):
                     print(line)
+                for line in unreadable_lines(unread, args.verbose):
+                    print(line)
                 print("  Run the same command without --check to fetch them; "
                       "a file with a wrong hash is replaced, and the one in place "
                       f"kept in {BACKUP_DIR} beside the BIOS folder.")
@@ -8628,6 +8708,8 @@ def _install(args: argparse.Namespace, real_stdout: TextIO) -> None:
             _print_plan([f for f in to_download if id(f) not in copies], bios_path, term,
                         trees, kept, copy_plan_lines(to_download, copies, bios_index,
                                                      bios_path, files, args.verbose, True))
+            for line in unreadable_lines(unread, args.verbose):
+                print(line)
             for line in hint:
                 print(line)
             continue
@@ -8653,6 +8735,10 @@ def _install(args: argparse.Namespace, real_stdout: TextIO) -> None:
                 files_pending += len(to_download)
                 continue
             EVENTS.emit("download", platform=plat_name, files=len(to_download), bytes=dl_size)
+            if unread:
+                print()
+                for line in unreadable_lines(unread, args.verbose):
+                    print(line)
             if copies:
                 print()
                 for line in copy_plan_lines(to_download, copies, bios_index, bios_path,

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="baeaf6acc5a7813df508d997c72ce934fcf3010b953f278426ccb95fd9a4abf8"
+DEFAULT_INSTALL_SHA256="4b658dc29528e94460995eaf844b1fc1cca9040ffa10cd71ac6656b4db5f1b34"
 MAX_INSTALLER_BYTES=2097152
 
 cleanup() {

tests/test_install_from.py

@@ -548,6 +548,108 @@ class ACopyThatFails(FromFolder):
                                  f"Permission denied, and {install.NOT_IN_PACK}")
 
 
+def _patched_zip(path: Path, name: str, data: bytes, flags: int = 0,
+                 method: int | None = None) -> Path:
+    """A stored ZIP of one member, its headers then given other flags or
+    another compression method: zipfile writes neither encryption nor
+    methods it cannot read."""
+    _zip(path, {})
+    with zipfile.ZipFile(path, "w", zipfile.ZIP_STORED) as archive:
+        archive.writestr(name, data)
+    raw = bytearray(path.read_bytes())
+    for signature, at in ((b"PK\x03\x04", 6), (b"PK\x01\x02", 8)):
+        start = raw.find(signature)
+        raw[start + at] |= flags
+        if method is not None:
+            raw[start + at + 2:start + at + 4] = method.to_bytes(2, "little")
+    path.write_bytes(bytes(raw))
+    return path
+
+
+class UnreadableCopies(FromFolder):
+    """A copy that could not be read was cached as no match and dropped:
+    the user saw a file to download that the old card held."""
+
+    def setUp(self):
+        super().setUp()
+        self.keep("scph5500.bin", JP)
+        self.keep("pcsx/extra.bin", EXTRA)
+        self.keep("gba_bios.bin", GBA)
+
+    def locked(self, relative: str, data: bytes) -> Path:
+        _unprivileged(self)
+        path = self.keep(relative, data)
+        path.chmod(0)
+        self.addCleanup(path.chmod, 0o644)
+        return path
+
+    def test_a_file_of_mode_000_is_named_in_every_mode(self):
+        path = self.locked("psx/scph5501.bin", US).resolve()
+        self.locked("notes.txt", b"not the size of any file")
+        line = f"{path} could not be read: Permission denied"
+        for options in (("--check",), ("--dry-run",), ()):
+            with self.subTest(options=options):
+                proc = self.install(*options)
+                self.assertEqual(proc.returncode, 1 if options != ("--dry-run",) else 0,
+                                 proc.stdout + proc.stderr)
+                self.assertIn(line, proc.stdout)
+                self.assertNotIn("notes.txt", proc.stdout)
+
+    def test_an_unreadable_copy_is_an_event(self):
+        path = self.locked("psx/scph5501.bin", US).resolve()
+        proc = self.install("--check", "--events")
+        events = _events(proc)
+        for event in events:
+            self.assertEqual(list(event), ["event", *install.EVENT_KEYS[event["event"]]])
+        self.assertEqual([e for e in events if e["event"] == "unreadable"], [{
+            "event": "unreadable", "platform": "retroarch", "folder": str(self.old),
+            "path": str(path), "member": "", "reason": "Permission denied"}])
+        kinds = [e["event"] for e in events]
+        self.assertLess(kinds.index("found"), kinds.index("unreadable"))
+
+    def test_a_copy_found_elsewhere_leaves_the_unreadable_one_unnamed(self):
+        self.locked("psx/scph5501.bin", US)
+        self.keep("psx/copy.bin", US)
+        proc = self.install("--check")
+        self.assertNotIn("could not be read", proc.stdout)
+
+    def test_a_folder_of_mode_000_is_named(self):
+        _unprivileged(self)
+        folder = self.old / "psx"
+        folder.mkdir()
+        folder.chmod(0)
+        self.addCleanup(folder.chmod, 0o755)
+        proc = self.install("--check")
+        self.assertIn(f"{folder.resolve()} could not be read: Permission denied", proc.stdout)
+
+    def test_members_that_cannot_be_read_say_why(self):
+        for flags, method, words in (
+                (0x1, None, "it is encrypted, a password is needed"),
+                (0, 6, "it is packed with a compression method this installer "
+                       "cannot read")):
+            with self.subTest(words=words):
+                archive = _patched_zip(self.old / "psx.zip", "scph5501.bin", US,
+                                       flags, method).resolve()
+                proc = self.install("--check")
+                self.assertIn(f"scph5501.bin inside {archive} could not be read: {words}",
+                              proc.stdout)
+
+    def test_an_input_output_error_is_named(self):
+        self.keep("psx/scph5501.bin", US)
+        index = install.FolderIndex(self.old)
+        entry = json.loads(self.list.read_text())["files"][0]
+        failing = OSError(errno.EIO, "Input/output error")
+        with mock.patch.object(install, "_sha1_file", side_effect=failing):
+            self.assertIsNone(index.find(entry))
+        found = install.unreadable_copies([entry], {}, [index])
+        # scph5500.bin holds another region of the same size: read too.
+        self.assertEqual([item.line() for item in found], [
+            f"{(self.old / 'psx' / 'scph5501.bin').resolve()} could not be read: "
+            "Input/output error",
+            f"{(self.old / 'scph5500.bin').resolve()} could not be read: "
+            "Input/output error"])
+
+
 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."""

wiki/installer.md

@@ -1191,6 +1191,7 @@ kind, and each kind has exactly the keys below, in this order. The schema is
 | `system` | `platform`, `system`, `name`, `files`, `present`, `missing`, `wrong_hash`, `bytes_needed` | Each system, sorted by id, and with `--system` only the systems named. Files with no system count under `other`. `name` is the platform's own name for it |
 | `check` | `platform`, `name`, `bios_dir`, `files`, `present`, `missing`, `wrong_hash`, `bytes_needed`, `data_missing`, `not_installable` | The platform's totals. `data_missing` counts data-tree files to fetch, `not_installable` the files the collection cannot supply |
 | `found` | `platform`, `dest`, `folder`, `path`, `member`, `reason` | Each file to install that this machine already holds, after `check` and before anything is written, in `--check` and `--dry-run` too. `folder` is the `--from` folder as given, or the `bios_dir` of the check when the BIOS folder holds the file at another place, and then `reason` says where in words (`nested folder: ...` for a misplaced file, `the same file is at ...` for one the list installs there too). `path` is the file, and `member` the ZIP member holding the bytes when `path` is an archive, an empty string otherwise. Its copy is reported by a `downloaded` event |
+| `unreadable` | `platform`, `folder`, `path`, `member`, `reason` | Each file in a `--from` folder that could not be read and may be one the run still needs, right after the `found` events: its size is that of a file no copy was found for, or unknown (a folder that could not be listed, a ZIP that would not open). `member` names the ZIP member, or is empty. `reason` says why in words (`Permission denied`, `it is encrypted, a password is needed`) |
 | `download` | `platform`, `files`, `bytes` | Before the downloads start. Counts the files copied from this machine too |
 | `progress` | `files_done`, `files_total`, `bytes_done`, `bytes_total` | At most every half second while bytes arrive |
 | `downloaded` | `dest`, `ok`, `reason`, `size`, `files_done`, `files_total`, `bytes_done`, `bytes_total` | Each finished file, downloaded or copied from where a `found` event named it. `reason` says why one failed, and is empty otherwise |