Conversation
57fdbe5 to
8cb027a
Compare
|
CI caught something my Windows run structurally could not, and it is the same blind spot this PR is about.
So this file had two tests written against the bug, not one. I called out Fixed by writing the expectation from what the Squashed into the one commit and rebased onto current main ( Separately, #92095 came in as a duplicate report from a different host and derived the same |
|
Please consider merging this PR as a necessary complement to #90492 which was done by @teknium1 I originally tested and supported #90492 because it correctly fixes the first problem: the generated desktop entry must not run the repository’s However, after applying #90492 on my affected Ubuntu 26.04 installation, I found that its use of: Path(sys.executable).resolve()still produces a broken Resolving that symlink writes the base uv interpreter into I verified the difference directly:
So #90492 fixes the incorrect repo-script/shebang behavior, while #92090 completes that fix by preserving the venv interpreter path and correcting the related This is not a damaged or unusual local environment: symlinked Python executables are normal for POSIX virtual environments and uv installs. Please merge #92090 so the #90492 fix also works reliably on these installations. Tested on Ubuntu 26.04 with Python 3.11.16 and a uv 0.12.5-managed Hermes venv. |
… symlink target `resolve_exec_command()` wrote `Path(sys.executable).resolve()` into the generated `hermes.desktop`. On POSIX a venv's `bin/python` is a symlink to the base interpreter - the default for both `python -m venv` and `uv venv` - so resolving it leaves the venv and names an interpreter with none of the venv's site-packages. The desktop environment then launches an entry that dies on `import hermes_cli`, and because the entry sets `Terminal=false` the launcher simply does nothing: no window, no error (NousResearch#92086). The same call broke `_needs_interpreter()` one layer up. It compared a script's shebang against `Path(sys.executable).resolve().parent`, so a console script carrying the venv's own `bin/python` - the correct interpreter - failed the comparison and was classified as needing a prefix. The two compose: the code decides to prefix, then prefixes the interpreter that cannot import Hermes. `_running_interpreter()` returns `os.path.abspath(sys.executable)`: absolute, as the entry requires, without leaving the environment that can actually run Hermes. `_needs_interpreter()` now accepts a shebang naming either spelling, so a script installed against the base interpreter is still recognised and NousResearch#90292's foreign-shebang prefix still fires. Also case-folds that comparison. The shebang was already lowercased on read while the interpreter path was not, so any install path containing an uppercase character never matched and every wrapper was classified as foreign. Two existing tests had built their expectations from the same `Path(sys.executable).resolve()` the fix removes, so they agreed with the bug. `test_exec_prefixes_interpreter_for_env_shebang_python_script` now writes its expectation from what the Exec line is for - an absolute path to an interpreter that can import `hermes_cli` - rather than by calling the helper back, so it still fails if the helper starts resolving again. `test_exec_leaves_venv_shebang_scripts_alone` keeps its behaviour, since a script installed against the base interpreter is a real case, but says so: its name promised a venv shebang while it wrote the base interpreter's path, which is how it passed against the bug it looks like it covers. Neither reproduces on Windows, where venvs copy the interpreter and `abspath` and `resolve` agree - the same reason the underlying bug never shows up on this platform. Fixes NousResearch#92086
8cb027a to
f08fef4
Compare
|
@vampyren thank you, and specifically thank you for the numbers rather than a I am on Windows. Windows venvs copy the interpreter into
That is the causal chain the PR asserts, observed end to end on hardware I do On the framing: I agree with you that this completes #90492 rather than Two things worth stating plainly for whoever reviews this:
It is not an unusual environment, as you say. Symlinked Local run on the rebased head: Ready for review whenever a maintainer has time. |
Excellent root-cause work on a nasty silent failure. The core insight — POSIX venvs symlink Notes:
|
|
Tested this exact PR head ( Environment:
Validation: I generated the launcher from this PR in an isolated Exec=<install>/venv/bin/python <install>/hermes desktopFor comparison: Then: The Electron process remained alive, and its backend used the correct venv path:
This is a successful real-host Linux/uv regression and end-to-end validation of the PR. |
|
@uncrayon this is the second independent real-host confirmation on this PR and the more complete of the two, because you ran the launcher rather than only the suite. Thank you. The line that matters most is the one nobody could have produced from my machine: Two different interpreters, and only the first can import Hermes. On Windows those are the same string, because Windows venvs copy the interpreter into On coordinating with #92088, which I think is the important open question here@Enough1122's first note asks that #92088 and this PR say which host topology each covers. Having read theirs properly, I do not think this is two fixes for one bug so much as two halves of one, and the seam between them is sharp enough to name. #92088's theory: This PR's theory: The two compose badly right now, and specifically at one expression. #92088's ladder is: if _can_import_hermes_cli(Path(sys.executable).resolve()):
return [str(Path(sys.executable).resolve()), str(script), "desktop"]
wrapper = shutil.which("hermes")
if wrapper:
return [str(Path(wrapper).resolve()), "desktop"]
return [str(Path(sys.executable).resolve()), "-m", "hermes_cli.main", "desktop"]Every branch names
So on the default POSIX layout, #92088 either routes around the venv or terminates in a branch that is dead by construction, and it does so because of the The composition is one substitution, not a merge conflict. If #92088 probes and emits I have no stake in which PR carries it. If a maintainer prefers #92088 as the base, this PR's The other review note@Enough1122's second point, about the 7 pre-existing Windows-quoting failures in |
|
Thanks! I was just annoyed of running all the time |
|
That is the useful half of the report, actually. "I was annoyed of running Thanks for filing it with enough detail to reproduce on the first try. |
hehe exactly, but still we have that issue since the first time after update it ask for password so its still not perfect. I haven't figured out why.... |
|
@jackulau — cross-posting from #92122: I've incorporated your |
|
@autumn8-builds Appreciated, and agreed on not racing. I have reviewed #92122 and left the detail there rather than here. Short version: Maintainers: treat this as my recommendation to prefer #92122 and close this For the record on what is not duplicated: the symlink point is now in both |
monerostar
left a comment
There was a problem hiding this comment.
Ubuntu 26.04 on a local 5800X box (kernel 7.0.0-30-generic). Hermes v0.20.3, CPython 3.11.15 from the managed runtime.
This install is the uv-style venv symlink case:
sys.executable=/home/hermes/.hermes/hermes-agent/venv/bin/python(symlink)Path.resolve()=.../.hermes-runtime/python/generation-1785724843-18242-a8c8abe2/cpython-3.11.15-linux-x86_64-gnu/bin/python3.11
Live import check:
- venv python:
import yamlok - resolved base:
ModuleNotFoundError: No module named 'yaml'
Forced #!/usr/bin/env python3 launcher (the prefix path):
- main
resolve_exec_command()still writes the resolved base intoExec= - this PR writes the venv symlink path instead
pytest tests/hermes_cli/test_linux_desktop_entry.py -o addopts=
- main: 15 passed, 2 skipped
- this PR: 21 passed, 2 skipped
Siblings #92516, #92278, #92285 hit the same Exec prefix idea. Prefer this one for the fuller matrix (venv shebang, resolved shebang, env python3, module fallback).
Looks good.
|
@monerostar — thank you for the independent real-host confirmation. Especially useful that your box is on the exact same kernel ( For visibility: #92122 (the base @jackulau recommended) already folds in #92090's |
|
Confirmed the underlying failure mode on an affected Arch Linux installation (CPython 3.11.16 / uv-managed venv)… preserving the symlink spelling for desktop-entry Exec= is necessary on this platform too |
|
Ran the end-to-end verification you asked for in step 3, on an affected host. Everything holds. Details below so this is reproducible. Host: Arch-based Linux (Omarchy, Hyprland/Wayland), Hermes venv created by The venv topology is exactly the shape this PR targets — with a detail worth noting: $ ls -l venv/bin/python
venv/bin/python -> ~/.local/share/uv/python/cpython-3.11-linux-x86_64-gnu/bin/python3.11
$ readlink -f venv/bin/python
~/.local/share/uv/python/cpython-3.11.16-linux-x86_64-gnu/bin/python3.11The symlink itself names the minor-versioned store dir, but 1. Test suite
2.
|
| argv[0] scenario | merge-base emitted | this branch emits |
|---|---|---|
venv console script (…/venv/bin/hermes, shebang #!…/venv/bin/python3) |
UV …/venv/bin/hermes desktop |
…/venv/bin/hermes desktop |
bash wrapper (~/.local/bin/hermes) |
~/.local/bin/hermes desktop |
~/.local/bin/hermes desktop (unchanged, as claimed) |
repo script (…/hermes, shebang #!/usr/bin/env python3) |
UV …/hermes desktop |
…/venv/bin/python …/hermes desktop |
| module fallback (no launcher on PATH) | UV -m hermes_cli.main desktop |
…/venv/bin/python -m hermes_cli.main desktop |
Both defects visible on baseline: the needless prefix on a correct venv shebang, and every prefix naming the interpreter that can't import Hermes. This branch fixes both, and the wrapper path is untouched.
3. Launching the emitted commands under env -i
(No PATH, no venv activation — what the DE actually does.)
# merge-base's emitted line:
$ env -i HOME=$HOME <UV> …/venv/bin/hermes --version
ModuleNotFoundError: No module named 'hermes_cli'
# this branch's lines:
$ env -i HOME=$HOME …/venv/bin/hermes --version # ✅ runs, prints version
$ env -i HOME=$HOME …/venv/bin/python …/hermes --version # ✅ runs, prints versionOne honesty note: I substituted --version for desktop as the trailing arg because Hermes is currently running from this same venv and I didn't want to spawn a second GUI instance mid-session. The bug's failure point — the base interpreter never seeing the venv's site-packages — is upstream of argument handling, and the baseline line dies exactly there while this branch's lines get through the full import chain and execute. (The baseline dies on hermes_cli rather than yaml for the console script because venv/bin is the script dir; the repo-script variant dies on yaml. Same class.)
4. Full install_desktop_entry() into a clean XDG_DATA_HOME
$ grep '^Exec=' <tmp-xdg>/applications/hermes.desktop
Exec=/home/<u>/.hermes/hermes-agent/venv/bin/hermes desktop
$ desktop-file-validate <tmp-xdg>/applications/hermes.desktop # ✅ passesNo uv store path, no patch version anywhere in the entry.
Disclosure: I'm the author of #96396, one of the duplicates of this PR — closing it in favor of this one, since this is the earlier canonical fix and it now has the Linux end-to-end run it was missing. #96777 carries a comparison table of all six open PRs for this bug class, for whoever consolidates.
What does this PR do?
hermes desktopregenerates~/.local/share/applications/hermes.desktoponevery launch. Its
Exec=line was built fromPath(sys.executable).resolve().On POSIX a venv's
bin/pythonis a symlink to the base interpreter - thedefault for both
python -m venvanduv venv- so.resolve()follows it outof the venv and names an interpreter that has none of the venv's site-packages:
The desktop environment then launches an entry that dies on
import hermes_cli. Because the entry setsTerminal=false, the traceback goesnowhere: clicking the pinned launcher does nothing at all - no window, no error,
no spinner. #92086 reports exactly that, and its "uv-managed python3.11 shim" is
not a stray interpreter that happened to launch Hermes; it is the reporter's own
venv python with the symlink followed.
The same call broke the guard above it
_needs_interpreter()decides whether the launcher script needs an explicitinterpreter prefix, by checking whether its shebang points inside the running
interpreter's environment:
exe_diris the base interpreter's directory, so a console script carrying#!/.../venv/bin/python- the correct interpreter - fails the comparison and isclassified as needing a prefix. The two failures compose: the code decides to
prefix, and then prefixes the interpreter that cannot import Hermes. Without the
.resolve(), that wrapper is recognised as already correct and the entry isleft as a plain
Exec=<wrapper> desktop, which works.And a third defect the tests surfaced
shebangis lowercased when it is read; the interpreter path it was comparedagainst was not. So any install path containing an uppercase character
(
/home/User/...,~/Projects/..., anything on Windows) never matched, andevery wrapper on such a host was classified as foreign. The comparison is now
case-folded on both sides.
Why not the fix suggested in the issue
The issue proposes preferring
shutil.which("hermes")overresolve_hermes_bin().That works on the reporter's host, but it resolves the launcher through
PATHrather than through "which Hermes is running", so on a machine with more than
one install the entry can end up pointing at a different install than the one
that wrote it.
resolve_hermes_bin()preferssys.argv[0]precisely for thatreason. This fixes the corrupted interpreter path instead of reordering the
preference around it.
StartupNotify=false, also suggested there, is deliberately not included:it is a separate UX question about token propagation that I cannot observe, and
it should not ride along on a correctness fix.
Related Issue
Fixes #92086
Type of Change
Changes Made
hermes_cli/linux_desktop_entry.py_running_interpreter():os.path.abspath(sys.executable)- absolute,as the entry requires, without following the venv symlink. Both
Exec=branches (the interpreter prefix and the
-m hermes_cli.mainfallback) nowuse it.
_needs_interpreter()accepts a shebang naming either spelling of theinterpreter, so a script installed against the base interpreter is still
recognised, and case-folds the comparison.
tests/hermes_cli/test_linux_desktop_entry.py: 6 new tests.How to Test
pytest tests/hermes_cli/test_linux_desktop_entry.py -qPath(sys.executable).resolve()in_running_interpreter→4 of the new tests fail, including
test_running_interpreter_keeps_the_venv_pythonand bothExec=tests.test_needs_interpreter_accepts_*tests fail.uv venvorpython -m venv(symlinkedbin/python):desktopshould be the venv's ownbin/python(or thewrapper, unprefixed), not the interpreter
readlink -freports for it.gtk-launch hermes.desktopshould open the app.On coverage and what I could not run
The new tests build a real base-interpreter-plus-symlinked-venv tree in
tmp_pathand driveresolve_exec_command()/_needs_interpreter()directly,rather than going through
install_desktop_entry(), so they assert on thedecision instead of on a rendered
Execline with platform-specific quoting.They skip if the platform cannot create symlinks.
The venv path in the fixture deliberately contains an uppercase segment, so the
case-fold guard binds on Linux CI and not only on Windows.
I have not run this on Linux. I am on Windows 11, where venvs copy the
interpreter instead of symlinking it, which is exactly why this bug has never
shown up on this platform. Everything above is source reasoning plus tests that
reproduce the symlink topology; the end-to-end
gtk-launchcheck in step 3 isfor someone with the affected host. @the reporter of #92086 - if you run step 3
against this branch I will fold in whatever it shows.
Worth noting the existing
test_exec_leaves_venv_shebang_scripts_alonedid notcatch this: it builds the script's shebang from
Path(sys.executable).resolve(),the same corrupted value the code used, so the test agreed with the bug.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — see "Test run honesty" belowDocumentation & Housekeeping
docs/, docstrings) — docstrings only; no user-facing doc describes this internalis_supported()(Linux/BSD). macOS venvs symlink too, so the same defect existed there for anyone reaching this code; Windows venvs copy the interpreter, soabspathandresolveagree and nothing changesTest run honesty
pytest tests/hermes_cli/test_linux_desktop_entry.py -qwas run with andwithout this change on the same machine:
The same 7 fail on both sides. They are the pre-existing Windows-quoting
mismatches in this file (
Execescapes backslashes, so an assertion comparingagainst a raw
WindowsPathcannot match) and are unrelated to this change.