diff --git a/install.py b/install.py index 01a1b03c..4b1b1ba8 100644 --- a/install.py +++ b/install.py @@ -7198,6 +7198,10 @@ class FolderIndex: # was opened again, its directory parsed again, for each member. self._zips: dict[Path, zipfile.ZipFile] = {} self._zip_lock = threading.Lock() + # The places this run writes, resolved. An archive among them is + # never kept open: Windows refuses to move aside or replace a file + # a process holds, and a --from folder can hold the BIOS folder. + self._written: set[Path] = set() # (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]] = {} @@ -7275,7 +7279,7 @@ class FolderIndex: A few stay open, the least recently read closed first: a card of MAME sets would otherwise hold a descriptor per archive. Closing one leaves a member being read open until it is closed itself. - Only archives the list does not install are read this way. + An archive this run writes is never read this way (see writes). """ archive = self._zips.pop(path, None) if archive is None: @@ -7285,15 +7289,38 @@ class FolderIndex: self._zips[path] = archive return archive + def writes(self, paths: Collection[Path]) -> None: + """Note the places this run writes, resolved, and close the archives + among them: the next read of one opens it and closes it at once.""" + with self._zip_lock: + self._written.update(paths) + for held in [held for held in self._zips if self._is_written(held)]: + self._zips.pop(held).close() + + def _is_written(self, path: Path) -> bool: + return bool(self._written) and ( + path in self._written or Path(os.path.realpath(path)) in self._written) + def infolist(self, path: Path) -> list[zipfile.ZipInfo]: with self._zip_lock: + if self._is_written(path): + with zipfile.ZipFile(path) as archive: + return archive.infolist() return self._archive(path).infolist() @contextlib.contextmanager def open_member(self, path: Path, member: str) -> Iterator[BinaryIO]: """A member of an archive under the folder, read from any thread.""" with self._zip_lock: - handle = self._archive(path).open(member) + if self._is_written(path): + archive = zipfile.ZipFile(path) + try: + handle = archive.open(member) + finally: + # The member holds the file until it is closed itself. + archive.close() + else: + handle = self._archive(path).open(member) try: yield handle finally: @@ -7913,6 +7940,8 @@ def plan_local_copies( written.add(Path(os.path.realpath(_entry_path(bios_path, entry)))) except ValueError: continue + for index in ([bios_index] if bios_index is not None else []) + folders: + index.writes(written) copies: dict[int, LocalCopy] = {} for entry in entries: copy = None diff --git a/tests/test_install_from.py b/tests/test_install_from.py index 2f5f2880..ef445ca5 100644 --- a/tests/test_install_from.py +++ b/tests/test_install_from.py @@ -854,6 +854,67 @@ class AFolderAroundTheBiosFolder(FromFolder): ["old card/copy.zip, inside as inner.bin"]) + @unittest.skipUnless(Path("/proc/self/fd").is_dir(), "reads the open files from /proc") + def test_an_archive_the_list_replaces_is_not_held_open(self): + """Windows refuses to rename, replace or delete a file a process + holds open. The --from index kept the list's own wrong archive open + once its members had been listed, and the run failed to move it + aside, every run.""" + good = io.BytesIO() + with zipfile.ZipFile(good, "w") as archive: + archive.writestr("a.bin", b"A" * 300) + good = good.getvalue() + card = self.root / "card" + bios = card / "bios" + _zip(bios / "neogeo.zip", {"x.bin": b"X" * 77}) + (card / "backup").mkdir() + (card / "backup" / "neogeo.zip").write_bytes(good) + _zip(card / "backup" / "pack.zip", {"renamed.bin": GBA}) + self.list.write_text(json.dumps({ + "manifest_version": 2, "platform": "retroarch", + "files": [_entry("neogeo.zip", good, ["snk-neogeo"]), + _entry("gba_bios.bin", GBA, ["nintendo-gba"])]})) + + def held() -> set[str]: + out = set() + for fd in os.listdir("/proc/self/fd"): + with contextlib.suppress(OSError): + out.add(os.path.realpath(os.readlink(f"/proc/self/fd/{fd}"))) + return out + + real_replace, real_rename, real_unlink = os.replace, os.rename, os.unlink + + def refusing(real): + def call(*paths, **kwargs): + files = paths[:2] if real is not real_unlink else paths[:1] + if any(os.path.realpath(path) in held() for path in files): + raise PermissionError(13, "The process cannot access the file", + str(files[0])) + return real(*paths, **kwargs) + return call + + argv = ["install.py", "--platform", "retroarch", "--dest", str(bios), + "--manifest", str(self.list), "--from", str(card), "--no-input", "--yes"] + out = io.StringIO() + with mock.patch.object(sys, "argv", argv), \ + mock.patch.dict(os.environ, {"RETROBIOS_INSTALL_OFFLINE": "1"}), \ + mock.patch.object(os, "replace", refusing(real_replace)), \ + mock.patch.object(os, "rename", refusing(real_rename)), \ + mock.patch.object(os, "unlink", refusing(real_unlink)), \ + mock.patch.object(os, "remove", refusing(real_unlink)), \ + contextlib.redirect_stdout(out), contextlib.redirect_stderr(out): + try: + install.main() + code = 0 + except SystemExit as exc: + code = exc.code + finally: + install.LOCAL.reset() + self.assertEqual(code, 0, out.getvalue()) + self.assertEqual((bios / "neogeo.zip").read_bytes(), good) + self.assertEqual((bios / "gba_bios.bin").read_bytes(), GBA) + + 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."""