mirror of
https://github.com/Abdess/retroarch_system.git
synced 2026-04-16 22:02:31 -05:00
refactor: fix code review findings across all scripts
Critical: stream large file downloads (OOM fix), fix basename match in auto_fetch, include hashes in pack grouping fingerprint, handle not_in_zip status in verify, fix escaped quotes in batocera parser. Important: deduplicate shared group includes, catch coreinfo network errors, fix NODEDUP path component match, fix CI word splitting on spaces, replace bare except Exception in 3 files. Minor: argparse in list_platforms, specific exceptions in download.py.
This commit is contained in:
5
.github/workflows/validate.yml
vendored
5
.github/workflows/validate.yml
vendored
@@ -39,9 +39,8 @@ jobs:
|
|||||||
- name: Validate BIOS files
|
- name: Validate BIOS files
|
||||||
id: validate
|
id: validate
|
||||||
run: |
|
run: |
|
||||||
files=$(cat /tmp/changed_files.txt)
|
if [ -s /tmp/changed_files.txt ]; then
|
||||||
if [ -n "$files" ]; then
|
xargs python scripts/validate_pr.py --markdown < /tmp/changed_files.txt > /tmp/report.md 2>&1 || true
|
||||||
python scripts/validate_pr.py --markdown $files > /tmp/report.md 2>&1 || true
|
|
||||||
else
|
else
|
||||||
echo "No BIOS files changed" > /tmp/report.md
|
echo "No BIOS files changed" > /tmp/report.md
|
||||||
fi
|
fi
|
||||||
|
|||||||
@@ -142,7 +142,7 @@ def step2_scan_branches(entry: dict) -> bytes | None:
|
|||||||
)
|
)
|
||||||
|
|
||||||
for filepath in result.stdout.strip().split("\n"):
|
for filepath in result.stdout.strip().split("\n"):
|
||||||
if filepath.endswith(f"/{name}") or filepath == name or filepath.endswith(name):
|
if os.path.basename(filepath) == name:
|
||||||
try:
|
try:
|
||||||
blob = subprocess.run(
|
blob = subprocess.run(
|
||||||
["git", "show", f"{ref}:{filepath}"],
|
["git", "show", f"{ref}:{filepath}"],
|
||||||
|
|||||||
@@ -93,7 +93,11 @@ def load_platform_config(platform_name: str, platforms_dir: str = "platforms") -
|
|||||||
for system in config.get("systems", {}).values():
|
for system in config.get("systems", {}).values():
|
||||||
for group_name in system.get("includes", []):
|
for group_name in system.get("includes", []):
|
||||||
if group_name in shared_groups:
|
if group_name in shared_groups:
|
||||||
system.setdefault("files", []).extend(shared_groups[group_name])
|
existing_names = {f.get("name") for f in system.get("files", [])}
|
||||||
|
for gf in shared_groups[group_name]:
|
||||||
|
if gf.get("name") not in existing_names:
|
||||||
|
system.setdefault("files", []).append(gf)
|
||||||
|
existing_names.add(gf.get("name"))
|
||||||
|
|
||||||
return config
|
return config
|
||||||
|
|
||||||
|
|||||||
@@ -55,7 +55,8 @@ def path_priority(path: str) -> tuple:
|
|||||||
|
|
||||||
def _in_nodedup_dir(path: str) -> bool:
|
def _in_nodedup_dir(path: str) -> bool:
|
||||||
"""Check if a file is inside a no-dedup directory."""
|
"""Check if a file is inside a no-dedup directory."""
|
||||||
return any(nodedup in path for nodedup in NODEDUP_DIRS)
|
parts = Path(path).parts
|
||||||
|
return any(nodedup in parts for nodedup in NODEDUP_DIRS)
|
||||||
|
|
||||||
|
|
||||||
def scan_duplicates(bios_dir: str) -> dict[str, list[str]]:
|
def scan_duplicates(bios_dir: str) -> dict[str, list[str]]:
|
||||||
|
|||||||
@@ -207,7 +207,7 @@ Examples:
|
|||||||
print(f" - {p}")
|
print(f" - {p}")
|
||||||
else:
|
else:
|
||||||
print("No platform packs found in latest release")
|
print("No platform packs found in latest release")
|
||||||
except Exception as e:
|
except (urllib.error.URLError, urllib.error.HTTPError, OSError, json.JSONDecodeError) as e:
|
||||||
print(f"Error: {e}")
|
print(f"Error: {e}")
|
||||||
return
|
return
|
||||||
|
|
||||||
@@ -216,7 +216,7 @@ Examples:
|
|||||||
|
|
||||||
try:
|
try:
|
||||||
release = get_latest_release()
|
release = get_latest_release()
|
||||||
except Exception as e:
|
except (urllib.error.URLError, urllib.error.HTTPError, OSError) as e:
|
||||||
print(f"Error fetching release info: {e}")
|
print(f"Error fetching release info: {e}")
|
||||||
sys.exit(1)
|
sys.exit(1)
|
||||||
|
|
||||||
|
|||||||
@@ -263,7 +263,7 @@ def _collect_all_aliases(files: dict) -> dict:
|
|||||||
matched = md5_to_sha1[r.md5]
|
matched = md5_to_sha1[r.md5]
|
||||||
if matched:
|
if matched:
|
||||||
_add_alias(basename, matched)
|
_add_alias(basename, matched)
|
||||||
except (ImportError, ConnectionError) as e:
|
except (ImportError, ConnectionError, OSError):
|
||||||
pass
|
pass
|
||||||
|
|
||||||
# Identical content named differently across platforms/cores
|
# Identical content named differently across platforms/cores
|
||||||
|
|||||||
@@ -51,10 +51,13 @@ def fetch_large_file(name: str, dest_dir: str = ".cache/large") -> str | None:
|
|||||||
try:
|
try:
|
||||||
req = urllib.request.Request(url, headers={"User-Agent": "retrobios-pack/1.0"})
|
req = urllib.request.Request(url, headers={"User-Agent": "retrobios-pack/1.0"})
|
||||||
with urllib.request.urlopen(req, timeout=300) as resp:
|
with urllib.request.urlopen(req, timeout=300) as resp:
|
||||||
data = resp.read()
|
os.makedirs(dest_dir, exist_ok=True)
|
||||||
os.makedirs(dest_dir, exist_ok=True)
|
with open(cached, "wb") as f:
|
||||||
with open(cached, "wb") as f:
|
while True:
|
||||||
f.write(data)
|
chunk = resp.read(65536)
|
||||||
|
if not chunk:
|
||||||
|
break
|
||||||
|
f.write(chunk)
|
||||||
return cached
|
return cached
|
||||||
except (urllib.error.URLError, urllib.error.HTTPError):
|
except (urllib.error.URLError, urllib.error.HTTPError):
|
||||||
return None
|
return None
|
||||||
@@ -403,7 +406,9 @@ def _group_identical_platforms(platforms: list[str], platforms_dir: str) -> list
|
|||||||
for fe in system.get("files", []):
|
for fe in system.get("files", []):
|
||||||
dest = fe.get("destination", fe.get("name", ""))
|
dest = fe.get("destination", fe.get("name", ""))
|
||||||
full_dest = f"{base_dest}/{dest}" if base_dest else dest
|
full_dest = f"{base_dest}/{dest}" if base_dest else dest
|
||||||
entries.append(full_dest)
|
sha1 = fe.get("sha1", "")
|
||||||
|
md5 = fe.get("md5", "")
|
||||||
|
entries.append(f"{full_dest}|{sha1}|{md5}")
|
||||||
|
|
||||||
fingerprint = hashlib.sha1("|".join(sorted(entries)).encode()).hexdigest()
|
fingerprint = hashlib.sha1("|".join(sorted(entries)).encode()).hexdigest()
|
||||||
fingerprints.setdefault(fingerprint, []).append(platform)
|
fingerprints.setdefault(fingerprint, []).append(platform)
|
||||||
|
|||||||
@@ -34,7 +34,7 @@ def load_platform_configs(platforms_dir: str) -> dict:
|
|||||||
config = load_platform_config(f.stem, platforms_dir)
|
config = load_platform_config(f.stem, platforms_dir)
|
||||||
if config:
|
if config:
|
||||||
configs[f.stem] = config
|
configs[f.stem] = config
|
||||||
except Exception as e:
|
except (yaml.YAMLError, OSError) as e:
|
||||||
print(f"Warning: {f.name}: {e}", file=sys.stderr)
|
print(f"Warning: {f.name}: {e}", file=sys.stderr)
|
||||||
return configs
|
return configs
|
||||||
|
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ Usage:
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import argparse
|
||||||
import json
|
import json
|
||||||
import os
|
import os
|
||||||
import sys
|
import sys
|
||||||
@@ -58,9 +59,11 @@ def list_platforms(include_archived: bool = False) -> list[str]:
|
|||||||
|
|
||||||
|
|
||||||
def main():
|
def main():
|
||||||
include_all = "--all" in sys.argv
|
parser = argparse.ArgumentParser(description="List available platforms")
|
||||||
|
parser.add_argument("--all", action="store_true", help="Include archived platforms")
|
||||||
|
args = parser.parse_args()
|
||||||
|
|
||||||
platforms = list_platforms(include_archived=include_all)
|
platforms = list_platforms(include_archived=args.all)
|
||||||
|
|
||||||
if not platforms:
|
if not platforms:
|
||||||
print("No platform configs found", file=sys.stderr)
|
print("No platform configs found", file=sys.stderr)
|
||||||
|
|||||||
@@ -133,7 +133,9 @@ class Scraper(BaseScraper):
|
|||||||
string_char = None
|
string_char = None
|
||||||
clean = []
|
clean = []
|
||||||
for j, ch in enumerate(line):
|
for j, ch in enumerate(line):
|
||||||
if ch in ('"', "'") and not in_string:
|
if ch in ('"', "'") and j > 0 and line[j - 1] == '\\':
|
||||||
|
clean.append(ch)
|
||||||
|
elif ch in ('"', "'") and not in_string:
|
||||||
in_string = True
|
in_string = True
|
||||||
string_char = ch
|
string_char = ch
|
||||||
clean.append(ch)
|
clean.append(ch)
|
||||||
|
|||||||
@@ -155,13 +155,21 @@ def verify_entry_md5(file_entry: dict, local_path: str | None) -> dict:
|
|||||||
return {"name": name, "status": Status.MISSING, "expected_md5": expected_md5}
|
return {"name": name, "status": Status.MISSING, "expected_md5": expected_md5}
|
||||||
|
|
||||||
if zipped_file:
|
if zipped_file:
|
||||||
|
found_in_zip = False
|
||||||
for md5_candidate in md5_list or [""]:
|
for md5_candidate in md5_list or [""]:
|
||||||
result = check_inside_zip(local_path, zipped_file, md5_candidate)
|
result = check_inside_zip(local_path, zipped_file, md5_candidate)
|
||||||
if result == Status.OK:
|
if result == Status.OK:
|
||||||
return {"name": name, "status": Status.OK, "path": local_path}
|
return {"name": name, "status": Status.OK, "path": local_path}
|
||||||
|
if result != "not_in_zip":
|
||||||
|
found_in_zip = True
|
||||||
|
reason = (
|
||||||
|
f"{zipped_file} not found inside ZIP"
|
||||||
|
if not found_in_zip
|
||||||
|
else f"{zipped_file} MD5 mismatch inside ZIP"
|
||||||
|
)
|
||||||
return {
|
return {
|
||||||
"name": name, "status": Status.UNTESTED, "path": local_path,
|
"name": name, "status": Status.UNTESTED, "path": local_path,
|
||||||
"reason": f"{zipped_file} MD5 mismatch inside ZIP",
|
"reason": reason,
|
||||||
}
|
}
|
||||||
|
|
||||||
if not md5_list:
|
if not md5_list:
|
||||||
|
|||||||
Reference in New Issue
Block a user