feat: unified cross-platform Minotari Ledger installer with auto-detection#7805
feat: unified cross-platform Minotari Ledger installer with auto-detection#7805Sp1kyss wants to merge 3 commits into
Conversation
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
There was a problem hiding this comment.
Code Review
This pull request introduces a unified installer for the Minotari Ledger wallet, including a core Python script and platform-specific launchers for Windows, macOS, and Linux. The review feedback identifies several critical issues, such as a naming mismatch for the Nano S Plus model that would prevent asset downloads and logic errors in the device detection fallback. Additionally, the reviewer suggests refining the Python dependency list, improving subprocess reliability, and adopting more idiomatic practices for temporary directory management and system-level dependency installation.
| print(" \u2022 Ledger is connected via USB") | ||
| print(" \u2022 Device is unlocked") | ||
| print(" \u2022 Developer Mode is enabled\n") | ||
| run(["ledgerctl", "install", app_json]) |
There was a problem hiding this comment.
Invoking ledgerctl directly relies on the system PATH being correctly updated after venv activation, which can be unreliable in subprocesses. It is safer to use sys.executable -m ledgerwallet to ensure the command is executed using the specific version installed in the virtual environment.
| run(["ledgerctl", "install", app_json]) | |
| run([sys.executable, "-m", "ledgerwallet", "install", app_json]) |
| tmp_dir = tempfile.mkdtemp(prefix="minotari_ledger_") | ||
| try: | ||
| download_and_extract(url, filename, tmp_dir) | ||
| app_json = find_app_json(model, tmp_dir) | ||
| install_app(app_json) | ||
| except RuntimeError as exc: | ||
| error(str(exc)) | ||
| sys.exit(1) | ||
| finally: | ||
| shutil.rmtree(tmp_dir, ignore_errors=True) |
There was a problem hiding this comment.
Using tempfile.TemporaryDirectory as a context manager is more idiomatic and robust than mkdtemp with manual cleanup, as it ensures the directory is removed even if an exception occurs or the script is interrupted.
with tempfile.TemporaryDirectory(prefix="minotari_ledger_") as tmp_dir:
try:
download_and_extract(url, filename, tmp_dir)
app_json = find_app_json(model, tmp_dir)
install_app(app_json)
except RuntimeError as exc:
error(str(exc))
sys.exit(1)| sudo apt-get install -y libhidapi-dev libusb-1.0-0-dev 2>/dev/null || true | ||
| elif command -v dnf &>/dev/null; then | ||
| sudo dnf install -y hidapi-devel libusb-devel 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
Using sudo inside a script can be disruptive and may fail in non-interactive environments. It is better to check if the required libraries are already present before attempting installation. Additionally, using || true masks potential failures that could lead to cryptic errors during the Python execution phase.
SWvheerden
left a comment
There was a problem hiding this comment.
please check the review commands, and we dont support the nano, only the nanosplus
Apply the changes requested by @SWvheerden and gemini-code-assist on PR tari-project#7805. Per @SWvheerden ("we don't support the nano, only the nanosplus"): * Remove Nano S product IDs from LEDGER_PRODUCT_IDS. * Drop "nanos" from the ledgerctl-info detection table. * Update README to call out Nano S as explicitly unsupported. Per gemini-code-assist's review comments: * Slug `nanosplus` -> `nanos+` everywhere, matching `ledger_app.toml` and the published asset names (`minotari_ledger_wallet-nanos+...zip`, `app_nanos+.json`). Without this, asset/manifest lookups silently 404. * Drop `ledgerctl` and `hid` from the pip packages list. `ledgerctl` is not a standalone PyPI package (the command ships with `ledgerwallet`), and the `hid` PyPI package is a different project from the `hid` module that `ledgerwallet`/`hidapi` provides; installing both broke import order on some platforms. * Replace bare `ledgerctl` invocations with `sys.executable -m ledgerwallet ...` so the active venv's interpreter is used directly, instead of relying on PATH being correctly inherited by subprocess.run. * Rewrite the ledgerctl-info fallback to match full model names ("Nano S Plus", "Nano X", ...) rather than slug substrings. The old code had a substring-match bug where "nanos" was a prefix of "Nano S Plus" output, mis-classifying a Plus device as a Nano S. * Use `tempfile.TemporaryDirectory` as a context manager instead of `mkdtemp` + manual `shutil.rmtree`, so the temp dir is cleaned up even on unexpected exceptions or interrupts. * Stop calling `sudo apt-get install` / `sudo dnf install` from the .sh launcher. Sudo prompts mid-flow and `|| true` masks real failures; instead, detect missing libusb/hidapi and print a clear "please run X to install" message so the user does it explicitly.
Closes tari-project#7795 Replaces the per-model/per-OS install scripts with a single installer that auto-detects the connected Ledger hardware wallet model (Nano S, Nano S Plus, Nano X, Stax, Flex) and installs the correct Minotari app. Files added: - applications/minotari_ledger_wallet/wallet/install_scripts/install_minotari_ledger.py Cross-platform Python installer (macOS, Windows, Linux) - applications/minotari_ledger_wallet/wallet/install_scripts/install_minotari_ledger.sh Shell launcher for macOS/Linux - applications/minotari_ledger_wallet/wallet/install_scripts/install_minotari_ledger.ps1 PowerShell launcher for Windows - applications/minotari_ledger_wallet/wallet/install_scripts/README.md Documentation How it works: 1. Install Python deps (hid, ledgerctl) into an isolated venv 2. Enumerate USB HID devices to detect Ledger VID (0x2c97) + PID 3. Map PID to model slug (flex, stax, nanosplus, nanox, nanos) 4. Fetch latest GitHub release, filter assets by model name 5. Download + extract zip, install via `ledgerctl install app_<model>.json` 6. Falls back to `ledgerctl info` if hid enumeration is unavailable"
Apply the changes requested by @SWvheerden and gemini-code-assist on PR tari-project#7805. Per @SWvheerden ("we don't support the nano, only the nanosplus"): * Remove Nano S product IDs from LEDGER_PRODUCT_IDS. * Drop "nanos" from the ledgerctl-info detection table. * Update README to call out Nano S as explicitly unsupported. Per gemini-code-assist's review comments: * Slug `nanosplus` -> `nanos+` everywhere, matching `ledger_app.toml` and the published asset names (`minotari_ledger_wallet-nanos+...zip`, `app_nanos+.json`). Without this, asset/manifest lookups silently 404. * Drop `ledgerctl` and `hid` from the pip packages list. `ledgerctl` is not a standalone PyPI package (the command ships with `ledgerwallet`), and the `hid` PyPI package is a different project from the `hid` module that `ledgerwallet`/`hidapi` provides; installing both broke import order on some platforms. * Replace bare `ledgerctl` invocations with `sys.executable -m ledgerwallet ...` so the active venv's interpreter is used directly, instead of relying on PATH being correctly inherited by subprocess.run. * Rewrite the ledgerctl-info fallback to match full model names ("Nano S Plus", "Nano X", ...) rather than slug substrings. The old code had a substring-match bug where "nanos" was a prefix of "Nano S Plus" output, mis-classifying a Plus device as a Nano S. * Use `tempfile.TemporaryDirectory` as a context manager instead of `mkdtemp` + manual `shutil.rmtree`, so the temp dir is cleaned up even on unexpected exceptions or interrupts. * Stop calling `sudo apt-get install` / `sudo dnf install` from the .sh launcher. Sudo prompts mid-flow and `|| true` masks real failures; instead, detect missing libusb/hidapi and print a clear "please run X to install" message so the user does it explicitly.
9319094 to
21be81d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified cross-platform installer for the Minotari Ledger wallet, supporting Nano S Plus, Nano X, Stax, and Flex models. The implementation consists of a core Python script for automated device detection and firmware installation, complemented by Bash and PowerShell launchers that handle virtual environment setup and system dependency checks. Review feedback recommends replacing the legacy urllib.request.urlretrieve function with urllib.request.urlopen to improve the robustness of the download logic.
| pct = min(100, count * block_size * 100 // total_size) | ||
| print(f"\r {pct}%", end="", flush=True) | ||
|
|
||
| urllib.request.urlretrieve(url, zip_path, reporthook=progress) |
There was a problem hiding this comment.
The urllib.request.urlretrieve function is considered a legacy interface and is generally discouraged in favor of urllib.request.urlopen(). Using urlopen provides better control over the request and is more robust for modern Python applications.
| urllib.request.urlretrieve(url, zip_path, reporthook=progress) | |
| with urllib.request.urlopen(url) as response, open(zip_path, "wb") as out_file: | |
| total_size = int(response.getheader("Content-Length", 0)) | |
| block_size = 8192 | |
| count = 0 | |
| while True: | |
| buffer = response.read(block_size) | |
| if not buffer: | |
| break | |
| count += 1 | |
| out_file.write(buffer) | |
| progress(count, block_size, total_size) |
Address gemini-code-assist review comment from the /gemini review run on 2026-05-11. urllib.request.urlretrieve is in the legacy interface section of the urllib.request docs and is discouraged. Switch to a streaming urlopen loop that writes to disk in 64 KiB chunks and renders a percentage indicator from Content-Length without loading the whole archive into memory.
|
this looks fine, I just want to test this on a clean install machine |
Closes #7795
What this does
Replaces the per-model/per-OS scripts with a single installer that works on macOS, Windows, and Linux and auto-detects which Ledger is connected — no user input needed.
How to run
macOS / Linux:
Windows (PowerShell):
How detection works
0x2C97)flex,stax,nanosplus,nanox,nanos)ledgerctl infooutput parsing if thehidlibrary isn't availableAll known PID variants per model are covered (including alternate transport PIDs like
0x5000for Nano X).Error handling
Files changed
install_scripts/install_minotari_ledger.pyinstall_scripts/install_minotari_ledger.shinstall_scripts/install_minotari_ledger.ps1install_scripts/README.mdThe existing per-model scripts are untouched — this adds the unified option alongside them.