fix(installer): address code review feedback on ledger installer#7849
fix(installer): address code review feedback on ledger installer#7849sirius-016 wants to merge 1 commit into
Conversation
- Replace deprecated wmic with Get-PnpDevice for USB detection - Fix Windows command execution logic - Fix macOS system_profiler parsing bugs - Improve cross-platform error handling
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a unified cross-platform Python installer for the Minotari Ledger Wallet, automating device detection and installation across macOS, Linux, and Windows. The reviewer identified bugs in macOS USB detection and Windows JSON parsing, along with opportunities to improve code quality by removing dead code and adopting more idiomatic Python patterns for dependency management and temporary file handling.
| if rc == 0 and out: | ||
| try: | ||
| data = json.loads(out) | ||
| for device in data.get("SPUSBDataType", []): |
There was a problem hiding this comment.
The system_profiler output for SPUSBDataType is a hierarchical tree structure where devices can be nested under hubs or buses within an _items list. Iterating only the top-level list will fail to detect Ledger devices if they are connected through a hub (which is common). A recursive search is required to correctly parse this data.
| for device in data.get("SPUSBDataType", []): | |
| def find_ledger(items): | |
| for device in items: | |
| name = device.get("_name", "") or device.get("name", "") | |
| if "Ledger" in name: | |
| vid = device.get("vendor_id", "") or device.get("Vendor ID", "") | |
| pid = device.get("product_id", "") or device.get("Product ID", "") | |
| vid = vid.replace("0x", "").lower().zfill(4) | |
| pid = pid.replace("0x", "").lower().zfill(4) | |
| if (vid, pid) in LEDGER_USB_IDS: | |
| return LEDGER_USB_IDS[(vid, pid)] | |
| if "_items" in device: | |
| res = find_ledger(device["_items"]) | |
| if res: return res | |
| return None | |
| model = find_ledger(data.get("SPUSBDataType", [])) | |
| if model: return model |
| def ensure_python_deps() -> bool: | ||
| """ | ||
| Check (not auto-install) required Python packages. | ||
| FIXED: Made dependency management less intrusive (check first, ask user). | ||
| Returns True if all deps are available. | ||
| """ | ||
| required = ["ledgerwallet", "ecdsa", "protobuf"] | ||
|
|
||
| # Check which packages are missing | ||
| missing = [] | ||
| for pkg in required: | ||
| try: | ||
| __import__(pkg.replace("-", "_")) | ||
| except ImportError: | ||
| missing.append(pkg) | ||
|
|
||
| if not missing: | ||
| return True # All deps already installed | ||
|
|
||
| print(f"[*] Missing Python packages: {', '.join(missing)}") | ||
| print(f"[*] Install them with: pip install {' '.join(missing)}") | ||
| print(f"[*] Or run with --auto-install to install automatically.") | ||
|
|
||
| # Check for --auto-install flag | ||
| if "--auto-install" in sys.argv: | ||
| print(f"[*] Auto-installing missing packages...") | ||
| pip_cmd = [sys.executable, "-m", "pip", "install", "--upgrade"] + missing | ||
| rc, out, err = run_cmd(pip_cmd) | ||
| if rc != 0: | ||
| print(f"[!] Failed to install Python deps: {err}") | ||
| return False | ||
| return True | ||
|
|
||
| print(f"[!] Please install missing packages manually, then re-run.") | ||
| return False |
There was a problem hiding this comment.
The ensure_python_deps function relies on checking sys.argv for the --auto-install flag, which is then manually appended in main. This is brittle and makes the function harder to test or reuse. It is better to pass auto_install as an explicit parameter to the function.
def ensure_python_deps(auto_install: bool = False) -> bool:
"""
Check (not auto-install) required Python packages.
Returns True if all deps are available.
"""
required = ["ledgerwallet", "ecdsa", "protobuf"]
# Check which packages are missing
missing = []
for pkg in required:
try:
__import__(pkg.replace("-", "_"))
except ImportError:
missing.append(pkg)
if not missing:
return True # All deps already installed
print(f"[*] Missing Python packages: {', '.join(missing)}")
print(f"[*] Install them with: pip install {' '.join(missing)}")
if auto_install:
print(f"[*] Auto-installing missing packages...")
pip_cmd = [sys.executable, "-m", "pip", "install", "--upgrade"] + missing
rc, out, err = run_cmd(pip_cmd)
if rc != 0:
print(f"[!] Failed to install Python deps: {err}")
return False
return True
print(f"[*] Or run with --auto-install to install automatically.")
print(f"[!] Please install missing packages manually, then re-run.")
return False| if rc == 0 and out: | ||
| try: | ||
| data = json.loads(out) |
There was a problem hiding this comment.
If Get-PnpDevice returns no results, the out variable will be empty. Calling json.loads(out) on an empty string will raise a json.decoder.JSONDecodeError. You should check if out is truthy before attempting to parse it.
| if rc == 0 and out: | |
| try: | |
| data = json.loads(out) | |
| rc, out, _ = run_cmd(["powershell", "-Command", ps_script]) | |
| if rc == 0 and out.strip(): | |
| try: | |
| data = json.loads(out) |
| tmp_dir = tempfile.mkdtemp(prefix="tari_ledger_") | ||
|
|
||
| # FIXED: Use try-finally to ensure cleanup | ||
| try: | ||
| zip_path = os.path.join(tmp_dir, f"minotari_{model}.zip") | ||
|
|
||
| print(f"[*] Downloading...") | ||
| try: | ||
| urllib.request.urlretrieve(asset_url, zip_path) | ||
| except Exception as e: | ||
| print(f"[!] Download failed: {e}") | ||
| return False | ||
|
|
||
| # Extract zip | ||
| print(f"[*] Extracting...") | ||
| app_json = None | ||
| try: | ||
| with zipfile.ZipFile(zip_path, "r") as z: | ||
| z.extractall(tmp_dir) | ||
| # Find the app JSON file | ||
| import re | ||
| pattern = ASSET_PATTERNS[model] | ||
| for asset in release.get("assets", []): | ||
| if re.search(pattern, asset["name"], re.IGNORECASE): | ||
| base = asset["name"].replace(".zip", "") | ||
| # The JSON inside has model-specific name | ||
| model_json = { | ||
| "nanos": "app.json", # Check actual filenames | ||
| "nanox": "app.json", | ||
| "nanosplus":"app.json", | ||
| "stax": "app.json", | ||
| "flex": "app.json", | ||
| } | ||
| # Find the app JSON | ||
| for fname in os.listdir(tmp_dir): | ||
| if fname.endswith(".json") and "app" in fname.lower(): | ||
| app_json = os.path.join(tmp_dir, fname) | ||
| break | ||
| break | ||
| except Exception as e: | ||
| print(f"[!] Extraction failed: {e}") | ||
| return False | ||
|
|
||
| if not app_json or not os.path.exists(app_json): | ||
| print(f"[!] app.json not found in extracted files") | ||
| return False | ||
|
|
||
| # Ensure ledgerctl is available | ||
| if not ensure_ledgerctl(): | ||
| return False | ||
|
|
||
| # Run ledgerctl install | ||
| model_display = { | ||
| "nanos": "Nano S", | ||
| "nanox": "Nano X", | ||
| "nanosplus":"Nano S Plus", | ||
| "stax": "Stax", | ||
| "flex": "Flex", | ||
| } | ||
| print(f"\n[*] Installing Minotari Wallet on Ledger {model_display.get(model, model)}...") | ||
| print(f"[*] Ensure:") | ||
| print(f" - Ledger {model_display.get(model, model)} connected via USB") | ||
| print(f" - Device unlocked") | ||
| print(f" - Developer Mode / Ledger Live ready") | ||
| print() | ||
|
|
||
| rc, stdout, stderr = run_cmd(["ledgerctl", "install", app_json]) | ||
| print(stdout) | ||
| if stderr: | ||
| print(stderr, file=sys.stderr) | ||
|
|
||
| if rc == 0: | ||
| print(f"\n[+] Minotari Ledger Wallet installed successfully!") | ||
| return True | ||
| else: | ||
| print(f"\n[!] Installation failed (exit code {rc})") | ||
| return False | ||
| finally: | ||
| # FIXED: Robust cleanup using try-finally | ||
| if os.path.exists(tmp_dir): | ||
| shutil.rmtree(tmp_dir, ignore_errors=True) |
There was a problem hiding this comment.
While the try...finally block ensures cleanup, using tempfile.TemporaryDirectory() as a context manager is the idiomatic Python approach for managing temporary directories. It automatically handles cleanup even if an exception occurs, making the code cleaner and less prone to resource leaks.
with tempfile.TemporaryDirectory(prefix="tari_ledger_") as tmp_dir:
zip_path = os.path.join(tmp_dir, f"minotari_{model}.zip")
print(f"[*] Downloading...")
try:
urllib.request.urlretrieve(asset_url, zip_path)
except Exception as e:
print(f"[!] Download failed: {e}")
return False
# ... rest of the logic inside the context manager ...| model_json = { | ||
| "nanos": "app.json", # Check actual filenames | ||
| "nanox": "app.json", | ||
| "nanosplus":"app.json", | ||
| "stax": "app.json", | ||
| "flex": "app.json", | ||
| } |
There was a problem hiding this comment.
The model_json dictionary is defined but never used in the subsequent logic. This appears to be dead code that should be removed to improve maintainability.
| if args.auto_install: | ||
| sys.argv.append("--auto-install") # Hack to pass flag to function |
There was a problem hiding this comment.
Appending to sys.argv to pass state to an internal function is a poor practice. It's better to pass the auto_install flag directly to ensure_python_deps.
| if args.auto_install: | |
| sys.argv.append("--auto-install") # Hack to pass flag to function | |
| # Ensure Python deps | |
| print("[*] Checking Python dependencies...") | |
| if not ensure_python_deps(args.auto_install): | |
| sys.exit(1) |
1 participant
Summary
Fixes code review feedback on PR #7795 (unified cross-platform ledger installer).
Changes
wmicwithGet-PnpDevicefor cross-platform USB detectionsystem_profilerparsing bugsAll changes are in a single file:
install_minotari_ledger.py