fix(desktop): resolve a Hermes-capable interpreter for the .desktop Exec - #92122
autumn8-builds wants to merge 2 commits into
Conversation
Test notes:
Minor items:
|
addb4a8 to
90a8baf
Compare
|
@jackulau — thank you, this is a genuinely better fix and you're right that You're also right about Re: landing — I'm happy for this PR to be the base and for you to port |
90a8baf to
fe4ac25
Compare
|
@Enough1122 (and maintainers) — both minor review notes were good calls, applied in the latest force-push:
No behaviour change to the resolution order, Exec output, or (Noted triage's duplicate label was already withdrawn — #92122 remains the active resubmission; #92088 is the closed predecessor.) |
|
Thanks for the cross-post on #92090, and for folding in the So, one thing I think is a real defect, in wrapper = shutil.which("hermes")
if wrapper:
argv = [str(Path(wrapper).resolve()), "desktop"]That rung is only reached when
The net effect is narrow but bad: on exactly the machines where Two ways out, and I do not have a strong preference: either gate rung 2 on Two smaller notes, both non-blocking:
On base choice: yours is the larger and more general fix and I am happy for it to |
fe4ac25 to
d52ffb3
Compare
|
@jackulau — that rung-2 trace is exactly right and it's a genuine defect; thank you for working through it rather than letting it slide. Applied in the latest force-push ( On your two smaller notes — I left them as-is deliberately:
Glad to have #92122 as the base. Once you're satisfied rung 2 is settled, close #92090 whenever — this is the one you recommended and I'm happy for it to land. (Maintainers: this PR now combines the symlink-preserving |
d52ffb3 to
cadc3aa
Compare
|
Real hardware confirmation from the machine that produced #88709: Zorin 18.1, Python 3.11.16, uv-managed venv.
There are two testable gaps worth fixing before merge. First, test_exec_leaves_venv_shebang_scripts_alone writes its shebang with Path(sys.executable).resolve(), which produces the base interpreter path. That does not test a real venv shebang. _needs_interpreter() also compares against Path(sys.executable).resolve().parent, so a real shebang containing .venv/bin/python is classified as foreign. The prefixed command still works, but the claimed no-prefix behavior is not proven. Smallest fix: build that test shebang from os.path.abspath(sys.executable), then compare _needs_interpreter() against the non-dereferenced running-interpreter directory. Second, _can_import_hermes_cli() probes only import hermes_cli and inherits the current working directory. With the base interpreter, that probe succeeds from the repository root because the source tree is importable, while import hermes_cli.main fails immediately because its dependencies are absent. From home or /tmp, even the package import fails. That can falsely certify the exact interpreter this change is meant to reject. Probing import hermes_cli.main from a neutral working directory would pin the real capability. Ruff check is clean. Ruff format --check reports that both touched files need formatting. The raw checkout case itself is fixed on this machine. |
|
I pushed the two follow-up corrections to Cherry-pick: git cherry-pick 4150501f641829a961ae7e0deef538e46f1a395cThis commit is based directly on Verification:
I did not modify or force-push your branch. |
e865a6a to
63a1d75
Compare
63a1d75 to
1952271
Compare
7e217e0 to
1bec735
Compare
Correction to the earlier triage note: #92088 has since been closed, so this PR is the active fix for #92086 / #101097 (not a duplicate). It competes with #94051, which takes the alternate approach of preserving the venv interpreter symlink in |
1057e12 to
1458b99
Compare
|
exact-head 8de6a65 — void if moved KEEP: Linux PICK-ONE vs open spray (same file; all CONFLICTING narrower leaves):
Recommendation: land this PR; close or rebase the CONFLICTING spray onto it. SOFT (different design — do not auto-close): #98381 points |
Upstream PR NousResearch#92090 / Nous's own merge (268e105) fixed interpreter path resolution but left StartupNotify=true, which causes a perpetual launcher spinner (gtk-launch hangs) when the desktop process exits before StartupId acknowledgment. Set false to prevent the hang. On top of NousResearch#92090.
|
Ping @jackulau — branch rebased onto main (which now includes the Nous interpreter fix at Diff is just the one-line |
…h (exit 1002) When Electron's GPU process terminates abnormally on Linux, the desktop process exits with code 1002, leaving the user with a frozen spinner. Detect this specific exit code, set HERMES_DESKTOP_DISABLE_GPU=1, and retry the launch once — no manual config change needed. Complements the configurable desktop.disable_gpu option by adding the automatic fallback the launcher path was missing.
Summary
Fixes the Linux
.desktoplauncher failing silently when Hermes is launched via an interpreter that cannot importhermes_cli(e.g. a uv-managed shim, or a venvbin/pythonwhose symlink is followed out of the venv). Closes #92086.resolve_exec_command()previously picked the interpreter fromsys.executable(viaPath(...).resolve(), which follows the venv symlink onto the base interpreter that lacks Hermes' deps) and blindly prefixed it. The generated entry then dies onimport hermes_cli— invisibly, becauseTerminal=false. Clicking the pinned launcher does nothing.Fix
The prefix interpreter is now chosen by capability, not by
resolve():sys.executable— but only if it canimport hermes_cli~/.local/bin/hermes), gated on it being genuinely runnable (a native binary or bash launcher, not a foreignpython3shebang script that would reproduce the broken form)sys.executable -m hermes_cli.mainA venv's
bin/pythonis a symlink to the base interpreter on POSIX (python -m venvanduv venvboth do this), so the capable-interpreter probe usesos.path.abspath(sys.executable)rather thanPath(...).resolve()—abspathnormalises without dereferencing the symlink, keeping the selection inside the venv. The_needs_interpretercomparison is case-folded so a venv path with an uppercase segment isn't misclassified as foreign.The capability probe runs in isolated mode (
-I -c "import hermes_cli.main",cwd=/) so an inheritedPYTHONPATHcannot fake the import gate (hardening from the #88709 reporter, @nosliwhtes).StartupNotify=false (separate change, called out per review)
render_desktop_entry()setsStartupNotify=false. The Electron startup token does not propagate through the wrapper, sotrueleaves a pinned launcher spinning forever. Window association still works viaStartupWMClass=Hermes.Test plan
tests/hermes_cli/test_linux_desktop_entry.py: 21 passed, 2 platform-skipped.hermes_cli; falls back tosys.executable -m hermes_cli.mainwhen no wrapper exists; venv python with an uppercase path segment recognised as in-venv;_needs_interpreterpreserves the venv symlink; capability probe rejects aPYTHONPATH-injected checkout false positive.Notes
.desktopis regenerated on everyhermes desktoplaunch, so manual edits are overwritten; the fix is in the source that renders it.abspath/_needs_interpreterhardening reviewed by @jackulau on fix(desktop-entry): keep the venv interpreter in Exec= instead of its symlink target #92090, the rung-2 wrapper gate, and @nosliwhtes' isolated-mode probe hardening. Closes Linux .desktop launcher fails silently when sys.executable lacks hermes_cli (e.g. uv-managed install) #92086.