From b439371579846fe5c3a011977acf3ec7334f311f Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Tue, 27 Oct 2026 12:39:28 +0100 Subject: [PATCH 1/2] fix: take a backup folder on one-owner storage --- install.ps1 | 2 +- install.py | 21 ++++++++++++++----- install.sh | 2 +- install/launchers/index.json | 16 +++++++-------- tests/test_install.py | 35 +++++++++++++++++++++++++++----- tests/test_install_experience.py | 15 ++++++++++++++ 6 files changed, 71 insertions(+), 20 deletions(-) diff --git a/install.ps1 b/install.ps1 index 5ea6fd3b7..c85d9c23b 100644 --- a/install.ps1 +++ b/install.ps1 @@ -11,7 +11,7 @@ $ErrorActionPreference = "Stop" $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" - $defaultInstallSha256 = "b3363820bb97bec559287f72d9712281e22307387fa752b663f6f7af023a0e33" + $defaultInstallSha256 = "cc940a96dea46fb43d3940a13fddb4c9f1c02b8d81d1c810bcbddd5120143ce4" $maximumInstallerBytes = 2MB $installerExit = 0 $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null } diff --git a/install.py b/install.py index 7acc9cbb4..ae27b2dbf 100644 --- a/install.py +++ b/install.py @@ -4468,15 +4468,19 @@ def _check_holder(holder: Path) -> os.stat_result: A link would send them wherever it points. On a system with user ids, a folder another user owns was not made by this user's runs, and its owner - could read or swap what is moved in. Returns what was checked, so the - caller can confirm the same folder is still there once it has written - into it. + could read or swap what is moved in. A storage that keeps no owner per + file gives every file one owner: Android's shared storage shows a folder + Termux has just made as another user's, as does a FAT card. A holder + owned like the folder it sits in is that storage's owner, not someone + else's folder. Returns what was checked, so the caller can confirm the + same folder is still there once it has written into it. """ info = os.lstat(holder) if not stat.S_ISDIR(info.st_mode): raise OSError(errno.ELOOP, f"{holder} is a link or not a folder") getuid = getattr(os, "getuid", None) - if getuid is not None and info.st_uid != getuid(): + if (getuid is not None and info.st_uid != getuid() + and info.st_uid != os.lstat(holder.parent).st_uid): raise OSError(errno.EPERM, f"{holder} belongs to another user") return info @@ -5497,6 +5501,12 @@ def _report_outcome( "(or delete them), then run again.") if any(isinstance(reason, NetworkReason) for _dest, reason in failures or []): advice.append(NETWORK_FAILURE_ADVICE) + if any(str(reason).startswith(NOT_KEPT) for _dest, reason in failures or []): + # The same run fails the same way: "retry only those" sent a phone + # user round the loop with nothing changed. + advice.append(" The files in place were left as they were, as no backup folder could " + "be made for them. Move them out of the folder, then run again to " + "install the new ones.") paths = paths + [folder for folder in elsewhere if folder not in paths] if offline: @@ -5554,7 +5564,8 @@ def _report_outcome( print(f"Location: {_locations(paths)}") for line in kept + advice: print(line) - print("Run the same command again to retry only those.") + if not all(str(reason).startswith(NOT_KEPT) for _dest, reason in failures or []): + print("Run the same command again to retry only those.") sys.exit(1) print(f"\nDone. {_plural(downloaded, 'file')} installed, " diff --git a/install.sh b/install.sh index 27bcceaf4..2bc9b6703 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="b3363820bb97bec559287f72d9712281e22307387fa752b663f6f7af023a0e33" +DEFAULT_INSTALL_SHA256="cc940a96dea46fb43d3940a13fddb4c9f1c02b8d81d1c810bcbddd5120143ce4" MAX_INSTALLER_BYTES=2097152 cleanup() { diff --git a/install/launchers/index.json b/install/launchers/index.json index c943862b0..8735664d5 100644 --- a/install/launchers/index.json +++ b/install/launchers/index.json @@ -27,8 +27,8 @@ { "dest": "roms/ports/retrobios/install.py", "repo_path": "install.py", - "size": 734772, - "sha256": "b3363820bb97bec559287f72d9712281e22307387fa752b663f6f7af023a0e33", + "size": 735645, + "sha256": "cc940a96dea46fb43d3940a13fddb4c9f1c02b8d81d1c810bcbddd5120143ce4", "executable": false }, { @@ -71,8 +71,8 @@ "muos": { "archive": { "dest": "ARCHIVE/RetroBIOS.muxapp", - "size": 787321, - "sha256": "45366e0a225d8143f7b359fd04cd6b341a21f43057b7d5a41ad8f734f765f142" + "size": 788194, + "sha256": "3a9bc441d4178cc15833dc8a3f28877370b4075a05131d209208186247756cc7" }, "members": [ { @@ -92,8 +92,8 @@ { "name": "RetroBIOS/install.py", "repo_path": "install.py", - "size": 734772, - "sha256": "b3363820bb97bec559287f72d9712281e22307387fa752b663f6f7af023a0e33", + "size": 735645, + "sha256": "cc940a96dea46fb43d3940a13fddb4c9f1c02b8d81d1c810bcbddd5120143ce4", "executable": false }, { @@ -166,8 +166,8 @@ { "dest": ".config/modules/retrobios/install.py", "repo_path": "install.py", - "size": 734772, - "sha256": "b3363820bb97bec559287f72d9712281e22307387fa752b663f6f7af023a0e33", + "size": 735645, + "sha256": "cc940a96dea46fb43d3940a13fddb4c9f1c02b8d81d1c810bcbddd5120143ce4", "executable": false }, { diff --git a/tests/test_install.py b/tests/test_install.py index 55151f917..72c85866b 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -9,6 +9,7 @@ import importlib.util import io import json import os +import stat import shlex import re import shutil @@ -5737,11 +5738,35 @@ class AFileYouHadIsKept(unittest.TestCase): @unittest.skipUnless(hasattr(os, "getuid"), "user ids are POSIX") def test_a_holder_another_user_owns_is_refused(self): - (self.home / install.BACKUP_DIR).mkdir() - with self.assertRaises(OSError): - with unittest.mock.patch.object(install.os, "getuid", return_value=os.getuid() + 1): - install._check_holder(self.home / install.BACKUP_DIR) - install._check_holder(self.home / install.BACKUP_DIR) + holder = self.home / install.BACKUP_DIR + holder.mkdir() + real = os.lstat + + def planted(path, *args, **kwargs): + info = real(path, *args, **kwargs) + if Path(path) != holder: + return info + fields = list(info) + fields[stat.ST_UID] = info.st_uid + 2 + return os.stat_result(fields) + + with self.assertRaises(OSError), \ + unittest.mock.patch.object(install.os, "lstat", side_effect=planted): + install._check_holder(holder) + install._check_holder(holder) + + @unittest.skipUnless(hasattr(os, "getuid"), "user ids are POSIX") + def test_a_storage_with_one_owner_for_every_file_takes_the_holder(self): + # Android's shared storage, seen from Termux on Android 17: a folder + # the run has just made is shown as another user's, like every other + # file there. Refusing it left 25 files unreplaced on a real phone. + holder = self.home / install.BACKUP_DIR + holder.mkdir() + with unittest.mock.patch.object(install.os, "getuid", return_value=os.getuid() + 1): + install._check_holder(holder) + (self.bios / "x.bin").write_bytes(self.OLD) + self.assertTrue(self._download("x.bin")[1]) + self.assertEqual((self._run_folder() / "system" / "x.bin").read_bytes(), self.OLD) def test_restore_refuses_a_folder_the_installer_did_not_write(self): planted = self.home / install.BACKUP_DIR / "20260101-000000" diff --git a/tests/test_install_experience.py b/tests/test_install_experience.py index b74cfb7d9..34d5a9af8 100644 --- a/tests/test_install_experience.py +++ b/tests/test_install_experience.py @@ -337,6 +337,21 @@ class ProgressOutput(unittest.TestCase): self.assertIn("failed: x.bin (HTTP 404)", out) self.assertIn("2.0 KB downloaded in 0:02", out) + def test_files_not_set_aside_get_a_remedy_not_a_retry(self): + # A real Android run: 25 files whose backup folder was refused, told + # to run again, which refuses them the same way. + reason = f"{install.NOT_KEPT} (/sdcard/RetroArch/retrobios-backup belongs to another user)" + buf = io.StringIO() + with contextlib.redirect_stdout(buf), self.assertRaises(SystemExit) as end: + install._report_outcome( + [Path("/b")], 3, 0, 2, 0, False, + failures=[("a.zip", reason), ("b.zip", reason)], + ) + out = buf.getvalue() + self.assertEqual(end.exception.code, 1) + self.assertIn("The files in place were left as they were", out) + self.assertNotIn("retry only those", out) + class CommandLine(unittest.TestCase): def test_a_dry_run_writes_nothing(self): From c138270edcb953787856d43f3a908ef310acb7f4 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Tue, 27 Oct 2026 12:41:06 +0100 Subject: [PATCH 2/2] refactor: keep commit rules in a tracked module The rules lived only in the local commit-msg hook, which CI and other clones do not have, so autorun could only check a subject's shape there. scripts/commit_rules.py now holds them, the hook calls it, and autorun checks every job subject with it when the job list loads. --- scripts/autorun.py | 32 +++-------------------- scripts/commit_rules.py | 54 +++++++++++++++++++++++++++++++++++++++ tests/test_autorun.py | 42 +++++++++++++++--------------- tests/test_prose_style.py | 3 ++- 4 files changed, 82 insertions(+), 49 deletions(-) create mode 100644 scripts/commit_rules.py diff --git a/scripts/autorun.py b/scripts/autorun.py index 9632f4f61..8711ee883 100644 --- a/scripts/autorun.py +++ b/scripts/autorun.py @@ -43,7 +43,6 @@ import shlex import shutil import subprocess import sys -import tempfile import time from collections.abc import Callable, Iterator, Sequence from dataclasses import dataclass, field @@ -73,7 +72,6 @@ SEGFAULT_CODES = (139, -11) HOST_MISREAD_MARKERS = ("SystemError", "unknown opcode") VOLATILE_KEYS = frozenset({"generated_at", "generated", "imported_at"}) -SUBJECT_TYPES = re.compile(r"^(feat|refactor|chore|docs|fix): [a-z0-9]") CI_RED = frozenset({"failure", "timed_out", "startup_failure"}) CI_GREEN = frozenset({"success", "skipped", "neutral"}) @@ -186,33 +184,11 @@ class Context: def check_subject(subject: str, body: str = "") -> list[str]: - """What the repository's commit-msg hook would refuse in this message. + """What the repository's commit rules refuse in this message, checked + when the job list loads instead of two hours into the job.""" + from commit_rules import problems # noqa: PLC0415 - The hook holds the vocabulary rules, so they live in one place: a job - list is checked against it when it loads, not two hours into the job. - Without a hook (another clone) only the shape is checked. - """ - problems = [] - if len(subject) > 50: - problems.append(f"subject is {len(subject)} characters, 50 at most") - if not SUBJECT_TYPES.match(subject): - problems.append("subject must read 'type: description'") - if re.search(r"^co-authored-by", body, re.IGNORECASE | re.MULTILINE): - problems.append("no Co-Authored-By trailer") - hook = subprocess.run(["git", "rev-parse", "--git-path", "hooks/commit-msg"], - capture_output=True, text=True, cwd=REPO_ROOT, check=False).stdout.strip() - hook_path = (REPO_ROOT / hook) if hook else None - if hook_path and hook_path.is_file() and os.access(hook_path, os.X_OK): - with tempfile.NamedTemporaryFile("w", suffix=".msg", delete=False) as message: - message.write(f"{subject}\n\n{body}\n" if body else f"{subject}\n") - try: - refused = subprocess.run([str(hook_path), message.name], capture_output=True, - text=True, check=False) - finally: - os.unlink(message.name) - if refused.returncode: - problems.append((refused.stderr or refused.stdout).strip() or "refused by commit-msg") - return problems + return problems(f"{subject}\n\n{body}" if body else subject) def _step(raw: object, where: str) -> Step: diff --git a/scripts/commit_rules.py b/scripts/commit_rules.py new file mode 100644 index 000000000..1dc2bf6fa --- /dev/null +++ b/scripts/commit_rules.py @@ -0,0 +1,54 @@ +"""Commit message rules of this repository, in one place. + +The local commit-msg hook calls this file, and autorun checks every job's +subject with it when the job list loads. Kept in the tree, the rules hold in +any clone and in CI: a copy in the hook alone left the runner checking only +the shape there. + +Usage as a hook: python scripts/commit_rules.py MESSAGE_FILE +""" + +from __future__ import annotations + +import re +import sys +from pathlib import Path + +MAX_SUBJECT = 50 +SUBJECT_TYPES = re.compile(r"^(feat|refactor|chore|docs|fix): [a-z0-9]") +REFUSED_WORDS = re.compile( + r"\b(bug|regression|revert|dead code|broken|wrong|hack|workaround|" + r"comprehensive|implement)", + re.IGNORECASE, +) +CO_AUTHOR = re.compile(r"^co-authored-by", re.IGNORECASE | re.MULTILINE) + + +def problems(message: str) -> list[str]: + """What this repository refuses in a commit message, first line first.""" + found = [] + subject = message.split("\n", 1)[0] + if len(subject) > MAX_SUBJECT: + found.append(f"subject is {len(subject)} characters, {MAX_SUBJECT} at most: {subject}") + if not SUBJECT_TYPES.match(subject): + found.append("subject must read 'type: description' (feat, refactor, chore, docs, fix)") + text = "\n".join(line for line in message.splitlines() if not line.startswith("#")) + if REFUSED_WORDS.search(text): + found.append("the message uses a word this repository's history does not carry") + if CO_AUTHOR.search(message): + found.append("no Co-Authored-By trailer here") + return found + + +def main(argv: list[str]) -> int: + if len(argv) != 2: + print("usage: commit_rules.py MESSAGE_FILE", file=sys.stderr) + return 2 + found = problems(Path(argv[1]).read_text(encoding="utf-8", errors="replace")) + for problem in found: + print(f"commit-msg: {problem}", file=sys.stderr) + return 1 if found else 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/tests/test_autorun.py b/tests/test_autorun.py index 0edaf2388..32e5d930a 100644 --- a/tests/test_autorun.py +++ b/tests/test_autorun.py @@ -835,28 +835,30 @@ class BiosIntegrity(Scratch): return str(self.tmp / "database.json") -class SubjectsFollowTheRepositoryHook(unittest.TestCase): - """The vocabulary rules live in the commit-msg hook alone: a copy in the - runner put the very words the history refuses into a tracked file.""" +class SubjectsFollowTheRepositoryRules(unittest.TestCase): + """The commit rules live in scripts/commit_rules.py, which the local hook + calls: a copy in the hook alone let CI check only the shape.""" - def test_a_refusal_comes_from_the_hook(self): + def test_the_runner_reads_the_tracked_rules(self): + import commit_rules # noqa: PLC0415 + + for subject in ("chore: fix the broken manifests", "update things", "chore: " + "a" * 60): + with self.subTest(subject=subject): + self.assertEqual(ar.check_subject(subject), commit_rules.problems(subject)) + self.assertTrue(ar.check_subject(subject)) + self.assertEqual(ar.check_subject("chore: tidy the cache"), []) + + def test_the_rules_run_as_a_hook(self): with tempfile.TemporaryDirectory() as tmp: - hook = Path(tmp) / "commit-msg" - hook.write_text('#!/bin/sh\ngrep -q forbiddenword "$1" && echo refused && exit 1\nexit 0\n') - hook.chmod(0o755) - real = subprocess.run - def fake(argv, *args, **kwargs): - if argv[:3] == ["git", "rev-parse", "--git-path"]: - return subprocess.CompletedProcess(argv, 0, stdout=str(hook) + "\n", stderr="") - return real(argv, *args, **kwargs) - with unittest.mock.patch.object(ar.subprocess, "run", side_effect=fake): - self.assertEqual(ar.check_subject("chore: tidy the cache"), []) - self.assertEqual(ar.check_subject("chore: add forbiddenword"), ["refused"]) - - def test_the_shape_is_checked_without_a_hook(self): - problems = ar.check_subject("not a subject that follows the type rule at all, too long") - self.assertTrue(any("50 at most" in p for p in problems)) - self.assertTrue(any("type: description" in p for p in problems)) + message = Path(tmp) / "MSG" + script = REPO_ROOT / "scripts" / "commit_rules.py" + for text, code in (("fix: keep the cache\n", 0), ("fix: a broken cache\n", 1), + ("fix: keep it\n\nCo-Authored-By: x\n", 1)): + message.write_text(text) + done = subprocess.run([sys.executable, str(script), str(message)], + capture_output=True, text=True, check=False) + with self.subTest(text=text): + self.assertEqual(done.returncode, code, done.stderr) if __name__ == "__main__": diff --git a/tests/test_prose_style.py b/tests/test_prose_style.py index df24a41f0..d7fe7bf24 100644 --- a/tests/test_prose_style.py +++ b/tests/test_prose_style.py @@ -168,7 +168,8 @@ class NoMarketingVocabulary(unittest.TestCase): def test_no_tracked_text_uses_them(self): offenders = [] for path in _tracked(): - if path.name == "test_prose_style.py": + # This file and the commit rules have to name the words they refuse. + if path.name in {"test_prose_style.py", "commit_rules.py"}: continue text = path.read_text(encoding="utf-8", errors="replace") for number, line in enumerate(text.splitlines(), 1):