Studio: Appimage Improvements for Immutable Distros (Currently outdated due to mostly being superseded by #8695) - #8343
ThomasEricB wants to merge 35 commits into
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94ef687d18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Went through this one carefully. The torch 2.11 CPU bump holds up: I resolved the trio independently and got I pushed c535a4e with four fixes. 1. The gate in Separately, Nothing infers gfx1033 on its own (no Van Gogh entry in 2. The fixed
I dropped the meaningless uid, and AppRun now proves the link is one it controls (symlink, target owned by us, helpers present) and aborts with a readable message instead of continuing. Concurrent launches still work since either mount satisfies both checks. Added a behavioural test that runs the extracted guard against a good and a bad target rather than grepping for it. 3. 4 of 6 cases fail here. It calls 4. "Intel GPU detected" on AMD hosts.
Two things for you rather than code changes:
Branch is 32 commits behind main but merges clean. Everything green after my push: 5 shell suites, and 2894 python tests across |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c535a4e9e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…mp at py3.10 for PR unslothai#8343
|
Ran this through a full before/after simulation, since several of these changes touch paths that already work for people. Summary first: five of the seven changes are clean and I would take them as is. The AppImage change is not, and I have moved it off the release path with measurements below. I pushed e445a93 and 5f85707. Does it break already-supported pathsTorch constraints: 216 cells, 0 regressions. I lifted the real llama.cpp routing: 72 cells, 4 changed. Same approach against the real One behaviour change worth naming: a headless server whose only display device is an ATI/AMD BMC chip (ES1000 and friends are also PCI vendor 0x1002) now gets the Vulkan bundle instead of the CPU one. Your superset argument covers it, since that bundle carries the same One regression found in the torch bump, fixedThe 2.11 cpu wheels start at cp310. The installer defaults to 3.13 so this is normally moot, but So a 3.9 CPU install went from working to failing outright. e445a93 floors the cpu arm at 3.10 and keeps the default window below it, with an unreadable interpreter reading as newest so the floor is never silently dropped. The AppImage change reintroduces #7953@oobabooga flagged this and was right. I built the portable AppImage in CI exactly as the release does, then probed it on ubuntu-24.04, which is Linux Mint 22 Wilma's base. The bundle contains six of the eight libraries The first is #7953 character for character. Launching the app confirms the second: on 24.04 it comes up and then aborts the renderer with The diagnosis in your description is right as far as it goes, and bundling the whole closure genuinely does fix the GIO half of #7953. What it does not cover is that the bundle deliberately ships no glibc and no GL/EGL/DRM/GBM/X11/Wayland, so those load from the host and resolve their own dependencies out of the 22.04 bundle. There was also a guard gap. What I pushed:
I am not asking you to drop the portable bundle. The Steam Deck problem is real and your analysis of it is good. It just needs the host-load path solved before it can be the only AppImage, and now there is a CI signal for when it is. Everything else
One upgrade note, not a blocker: moving |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c8fd223a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c05d34d7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [[ -d "$injected_dir" ]]; then | ||
| mkdir -p "$webkit_exec/injected-bundle" | ||
| cp -a "$injected_dir/." "$webkit_exec/injected-bundle/" |
There was a problem hiding this comment.
When none of these candidate directories exists—for example on an unrecognized/non-FHS WebKit layout—the loop silently leaves WEBKIT_INJECTED_SRC empty and copies no injected bundle. The later path patch still succeeds through its generic compiled-string match, and assert_portable_appdir() checks only the two helper executables, so the build can ship successfully with $WEBKIT_LINK/injected-bundle missing; WebKit then starts but cannot load its injected bundle, leaving the application's JavaScript bridge unusable. Treat failure to copy this required directory as a build error and verify it in the packaged AppDir.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The control flow is as you describe, but I could not reach it. The candidates are $exec_prefix/lib/webkit2gtk-4.1/injected-bundle, $WEBKIT_HELPER_SRC/injected-bundle, the multiarch libdir, /usr/lib and /usr/lib64. Debian and Ubuntu, which is the CI and release host, hit the second and third; Arch hits the first and second; Fedora hits the last; nix hits the first through pkg-config. A host that misses all five could not have produced the 4.1-linked deb this script consumes.
| HOST_LIBS_RE='^(ld-linux.*|libc|libm|libdl|libpthread|librt|libresolv|libnss_.*|libutil|libanl)\.so' | ||
| HOST_LIBS_RE="$HOST_LIBS_RE"'|^(libGL|libGLX|libGLdispatch|libEGL|libOpenGL|libgbm|libdrm|libglapi)\.so' | ||
| HOST_LIBS_RE="$HOST_LIBS_RE"'|^(libX11|libX11-xcb|libxcb.*|libXext|libXrandr|libXi|libXcursor|libXfixes|libXrender|libXcomposite|libXdamage|libXinerama|libXau|libXdmcp|libxshmfence)\.so' | ||
| HOST_LIBS_RE="$HOST_LIBS_RE"'|^(libwayland-.*)\.so' | ||
| HOST_LIBS_RE="$HOST_LIBS_RE"'|^(libasound|libpulse.*)\.so' |
There was a problem hiding this comment.
On a newer target whose Mesa/LLVM requires newer GLIBCXX symbols than Ubuntu 22.04 provides, this boundary keeps GL/EGL/DRM on the host but still bundles libstdc++.so.6 through WebKit's dependency closure. WebKit loads that bundled SONAME before Mesa is dlopened, and the dynamic loader then reuses the already-loaded object for host LLVM regardless of LD_LIBRARY_PATH, so removing AppRun's export does not eliminate the documented GLIBCXX_3.4.32 failure. The noble probe cannot catch this because its tiny C process dlopens only the host library and never preloads bundled WebKit/libstdc++; validate the actual mixed load sequence and choose a boundary that does not make host graphics consume the older bundled C++ runtime.
Useful? React with 👍 / 👎.
|
The Noble probe is green. Building the portable AppImage on 22.04 and probing it inside an The Thanks for 500616b. Re-pointing the gate at For the record, the chain took four runs because each fault was hidden by the one above it:
Worth naming because it explains why the closure argument read as sound: Where I think this leaves things. The mechanism behind the Mint 22 failure is fixed and now has a CI gate on the exact platform that reported it. Two gaps remain before the portable bundle should take the plain
I have left Separately: the five installer changes are independently verified and I would take them now. Given how close this came to shipping a repeat of #7953 on the back of them, splitting the AppImage into its own PR would let the rest land while this finishes. |
|
Opened #8412 with the six installer changes split out, so they are not waiting on the AppImage work. Credited to you, with a That leaves this PR as the AppImage change alone, which I think is the right shape for it: the packaging question needs a Steam Deck and a real 24.04 desktop to close, and there is no reason the gfx1033 gate should wait on that. If you would rather keep everything together, say so and I will close #8412. One suggestion for the AppImage side, and it is a suggestion rather than something I have pushed. The PR frames it as thin or portable. Shipping both solves the problem you found without betting on the fat bundle behaving on every distro:
Deck users get a working download immediately, nobody on Mint can pick the experimental one by accident, and the release already builds both scripts. Two things still open here, both needing hardware rather than CI:
If both pass, I have no remaining objection to the portable bundle shipping. |
|
Ran the launch on real runners rather than containers, and with the thin bundle as a control. The result is clean and it is not good news for the portable bundle. The 2x2Both AppImages built from the SAME deb in the same job, so packaging is the only variable. Both launched by the same script under Xvfb on GitHub runners, which carry a working Mesa/llvmpipe.
The process stays alive ( The thin cell is the one that matters. It is the same application with the same WebKit on the same runner, so Noble under Xvfb can plainly render. That rules out the environment and leaves the packaging. What this is NOTIt is not #7953 returning. On an actual So the library mismatch is genuinely fixed, and the side by side shows what the old What I do not know yetThe cause. I had a loader trace that looked like the GPU stack was being bundled, and I was wrong: those were My capture was also truncated by a Reproduction
Happy to push that workflow here if you want it in the tree. It is the cheapest way to iterate on this, since it separates your packaging from the app in a single run. Where that leaves thingsThe plain My split still stands (#8412) so the six installer fixes are not waiting on this. If you would rather I keep digging on the EGL side than hand it over, say so. You know this packaging far better than I do, and the 2x2 gives you a one-run reproduction to work against. |
ThomasEricB
changed the title
The thin AppImage takes the desktop stack from the host. That is right on a distro that ships WebKitGTK 4.1, and its AppRun prints an apt command when the host does not. But the distros with the strongest reason to want an AppImage are the immutable ones, and those are exactly the hosts that cannot run `sudo apt install`. SteamOS 3 is the concrete case. It has libgtk-3, libsoup-3 and librsvg, but no libwebkit2gtk-4.1, no libjavascriptcoregtk-4.1 and no libayatana-appindicator3 -- verified across /usr, /lib, /lib64 and the flatpak runtimes. So on a Steam Deck the thin AppImage cannot start, and the remedy it prints cannot be applied: /usr is read-only and there is no apt. Electron apps ship on those machines without trouble because Electron carries its own browser engine; a Tauri app has to bundle WebKitGTK to reach parity. build-portable-appimage.sh produces a second bundle that does. It reuses the deb payload exactly as the thin build does, then walks the ldd closure of the executable plus the dlopened names (AppIndicator, the SVG pixbuf loader) and copies everything except a documented host boundary: glibc, because a bundled libc cannot load host libraries built against a newer one, and GL/EGL/DRM/GBM/X11/Wayland, because those must match the running driver -- which is what keeps hardware acceleration working. The thin script's comment names the real hazard in bundling: a PARTIAL closure mixes a bundled GLib with the host's newer GIO modules. That is an argument against bundling some of the stack, not all of it, so this script bundles the whole closure and then proves it: assert_portable_appdir resolves the executable and every bundled object with only the AppDir on the search path and fails on a single "not found". It also requires libwebkit2gtk-4.1 and the WebKitNetwork/Web Process helpers to be present, since a bundle missing those builds cleanly and then shows a blank window. WebKit helpers, GIO modules, gdk-pixbuf loaders and gsettings schemas travel too, with their caches regenerated bundle-relative and RUNPATHs rewritten to $ORIGIN, so nothing resolves back to the build host. AppRun sets LD_LIBRARY_PATH, which the thin build forbids. With a partial closure that is what causes mixing; with a verified-complete one the opposite holds -- the bundle has to win consistently or the loader satisfies half the graph from the host. tests/sh/test_portable_appimage_closure.sh builds real ELF fixtures and asserts the gate accepts a complete closure and rejects an unresolved dependency, a missing WebKit, missing helper processes and a missing library directory, plus the host/bundle boundary and the AppRun environment. 15/15. The thin bundle is unchanged and still built; this is an additional artifact. (cherry picked from commit 836ed40)
Building the bundle was not enough; each of these produced a clean build and a broken launch. 1. dlopened libraries were never copied. resolve_lib() found libayatana-appindicator3.so.1 and queued it, but the queue only drives the dependency walk -- so everything the tray library needs was bundled and the tray library itself was not. The app started and panicked in libappindicator-sys' dlopen. Copy the resolved file too. 2. patchelf silently did nothing. Copies inherit the source mode, and libraries on a read-only prefix arrive without write permission, so every `patchelf ... || true` failed and the bundle shipped with absolute build-host RUNPATHs. chmod u+w first, then verify: the build now fails if any object keeps a RUNPATH under /nix/store, /usr or /opt. 3. The executable kept the build host's ELF interpreter. A non-FHS toolchain stamps its own absolute loader path into PT_INTERP, so the AppImage could only ever start on the machine that built it -- and there it ran under a loader whose ld.so.cache did not know the target's library directories, so libwayland-client.so.0 came up missing on a host that has it in /usr/lib64. Normalise to /lib64/ld-linux-x86-64.so.2 and assert it. 4. WebKit's helper directory is compiled in. WebKitGTK dropped WEBKIT_EXEC_PATH, so exporting it did nothing and WebKitNetworkProcess / WebKitWebProcess were spawned from the build prefix. The AppImage mount point is random, so the path cannot be baked in; rewrite the string to a short fixed path and have AppRun link it to the bundle at launch. The replacement is NUL-padded to the original length, leaving every ELF offset intact. Also sources GIO modules and gdk-pixbuf loaders from the same prefix as the bundled GLib via pkg-config, rather than guessing at /usr/lib*/gio/modules -- guessing copied the HOST's modules next to our GLib, which is exactly the mix the thin build warns about, and left them linking against libraries absent from the bundle. Verified end to end on a Steam Deck: 142 libraries bundled, closure complete, RUNPATHs $ORIGIN-relative, interpreters host-standard, and the app launches and starts its backend on a host with no WebKitGTK. (cherry picked from commit 93f250c)
The bundle ran on the machine that built it and died on every other one. Three faults, none visible from the build host, found by launching it through the user systemd manager so the process escaped the build environment. Absolute DT_NEEDED entries. libsoup, libtinysparql and libwebkit2gtk each recorded sqlite as a full build-host path rather than a soname. The loader ignores RUNPATH for such an entry and opens the path verbatim, so the app exited with "cannot start because these libraries are missing" naming a directory that exists only where it was built. They are now rewritten to bare sonames, and the completeness gate rejects any DT_NEEDED containing a slash -- ldd resolves those happily on the build host, which is precisely why the closure check could not see the fault. The ldd parser was blind to them, which is the root cause of the above reaching a built bundle: ldd prints an ordinary dependency as `name => /path` but an absolute one as `/path (0x..)` with no arrow, and only the arrow form was matched. So sqlite was never copied in the first place. Both forms now go through one shared helper. WebKit's injected bundle. Beyond the helper directory already handled, WebKit dlopens the library it injects into every web process from a second compiled-in absolute path. Without it the window opens and every page logs "Error loading the injected bundle", leaving the JS bridge dead. It is bundled under the same fixed link as the helpers. The literal carries a trailing slash in some builds, which is why a first attempt redirected one of the two paths and silently left the other. Also stamps the Tauri bundle-type marker, which the thin build does and this script did not: the executable still identified as a deb, so the app logged "APPDIR ... but this application was not detected as an AppImage" and, per the thin script's comment, the updater would reject its own download as an invalid deb. And AppRun now points XKB_CONFIG_ROOT / XLOCALEDIR at the host's copies when unset, because libraries carry their data directories as absolute paths fixed at build time and, unlike RUNPATHs, those cannot be rewritten. tests/sh/test_portable_appimage_closure.sh is 22/22, extended with a fixture that builds a library without a SONAME so the linker records a genuinely absolute DT_NEEDED, plus assertions that both ldd output forms are parsed, both WebKit paths are redirected including the trailing-slash spelling, and the bundle-type marker is stamped. Verified on a Steam Deck through systemd-run --user: no /nix/store references, no loader errors, no injected-bundle error, process stays up and reaches its install preflight. (cherry picked from commit 048e7c2)
The portable bundle rewrites two absolute paths baked into libwebkit2gtk -- the
helper directory and the injected bundle -- so they point into the AppImage
rather than at the build host. The patterns matched those paths as they are laid
out here (nix): '/libexec/webkit2gtk-4.X' and '/lib/webkit2gtk-4.X/injected-bundle'.
Debian and Ubuntu put both under the multiarch libdir instead:
/usr/lib/x86_64-linux-gnu/webkit2gtk-4.1[/injected-bundle]
Neither pattern matches that, so on ubuntu-22.04 -- which is what
release-desktop.yml builds on -- the rewrite found nothing. It did not fail; it
quietly did nothing, and the build went on to produce an AppImage still carrying
/usr/lib/x86_64-linux-gnu paths. On any machine without WebKitGTK installed at
that exact path, which is the entire reason the portable bundle exists, the
helpers cannot spawn and the injected bundle cannot load: a blank window.
Verified against the real libwebkit2gtk-4.1.so.0 from
libwebkit2gtk-4.1-0_2.50.4-0ubuntu0.22.04.1_amd64.deb, where both old patterns
score zero hits.
Fixes:
* Patch the directories the helpers and bundle were actually COPIED FROM. That
string is by construction the one compiled into the library, so it needs no
assumption about the layout, and a layout-agnostic pattern backs it up.
* Treat "no path matched" as a build failure. The rewrite is the difference
between a bundle that runs anywhere and one that runs only on its build host,
so silence must not read as success -- that is what let this ship.
* Derive the injected bundle from the helper directory when they share a parent,
which is what makes the multiarch layout resolve.
* Shorten the redirect target to /tmp/.unsloth-wk-$UID. The rewrite is in-place
and NUL-padded so it can only shrink a string, and the old name was 2 bytes
too long to fit a plain /usr/lib/webkit2gtk-4.1/injected-bundle.
Tests run the real patcher against all four layouts and assert no build-host path
survives, plus the two ways it must fail loudly. The gcc/patchelf guard now gates
only the ELF fixtures rather than exiting the whole file, so these still run on a
runner without a compiler -- skipping everything silently is the same fault.
(cherry picked from commit e6522c5)
The release built two AppImages: a thin one that takes WebKitGTK from the host, and the portable one that carries it. Only the thin one was staged as a release asset under the plain -Linux.AppImage name, so the download users actually get was the one that cannot run without libwebkit2gtk-4.1 already installed. That is backwards. An AppImage exists to run where the distro will not cooperate, and the distros with the most reason to want one are the immutable ones that cannot install the dependency. SteamOS is the concrete case: libgtk-3 and libsoup-3 are present, libwebkit2gtk-4.1 is not, /usr is read-only and there is no apt, so the thin AppRun printed an install command the user could not run. The portable bundle now builds to the canonical output path. Asset naming, the signing step and the updater feed are untouched, because the name they key on is unchanged -- this swaps what is inside the file, not what it is called. build-thin-appimage.sh stays in the tree with its contract tests intact; nothing in the release path calls it. Guard step names lost the now-meaningless "thin" qualifier, including the digest-pinning guard that matches its step by name. The test that pins this contract compares against the step's commands rather than its full text: the step explains in a comment why the thin builder is no longer called, and a naive substring check matched that explanation. (cherry picked from commit af2fd1f)
… PR unslothai#8343 (AppImage portion; installer half moved to unslothai#8412)
The portable bundle is now the only AppImage the release ships, and nothing built it outside release-desktop.yml. That workflow's build job checks out `studio_version` -- the release tag -- rather than the dispatch ref, so it can only build code that has already been tagged. The first time the bundle was assembled from Debian-family libraries would therefore have been the release itself. That matters because this bundle is assembled from whatever the build host provides, so one that is correct on one distro can be silently wrong on another. Concretely: WebKitGTK's compiled-in helper and injected-bundle paths sit under $prefix/libexec on nix and Fedora but under the multiarch libdir on Debian and Ubuntu. A rewrite written against the first layout matched nothing on the second and did not fail, because "no path matched" was indistinguishable from "nothing needed patching" -- it shipped a bundle still pointing at the build host. The script treats that as fatal now; this job is where that assertion meets an actual Ubuntu library set. Same runner image, same pinned packager and digests, same script as the release. It builds the deb, runs the bundler, extracts the finished image and re-verifies the closure against the shipped squashfs rather than the AppDir the script already checked, then asserts no path points back at the build tree and that WebKit's compiled-in directories were redirected off the host. No signing, no release, no secrets, so it also runs on a fork -- which release-desktop.yml cannot, since it needs a tag and the signing key. Triggers are narrow: the closure depends on the bundling script, the crate being bundled and the packaging config, not on frontend or backend changes that studio-tauri-smoke.yml already covers. The artifact is ~150MB and Actions storage is the one resource that is not free on public repos, so PR runs prove the build and keep nothing; a manual dispatch is how you get a copy to launch. (cherry picked from commit c9657e2)
…mp at py3.10 for PR unslothai#8343 (AppImage portion; installer half moved to unslothai#8412)
(cherry picked from commit 5f85707)
for more information, see https://pre-commit.ci (cherry picked from commit b2ccd60)
…ctory depths (cherry picked from commit 3c05d34)
The RUNPATH sweep over the WebKit helper directory matched `-perm -u+x` only. A shared object does not need the execute bit, and Debian installs libwebkit2gtkinjectedbundle.so as 0644, so patchelf never ran on it and it kept an empty RUNPATH -- the bundle's injected library resolving through nothing. Invisible on this build host: nix marks its .so files executable, so the sweep matched there and the object came out patched. It took an Ubuntu build to expose it, and only after AppRun stopped exporting LD_LIBRARY_PATH, which until then was resolving the bundle regardless of what any RUNPATH said. The new "RUNPATH empty (patchelf did nothing)" assertion caught it on the first CI run. The injected bundle also sits one directory deeper than the helpers, so the fixed '$ORIGIN/../../lib/unsloth' the sweep applied would have pointed one level short of $libdir even if the object had been matched. Each object now computes its own way back with realpath --relative-to instead of assuming a depth. Tests build the real layout, with the injected bundle at 0644 as Debian ships it, lift the find expression out of the script so they cannot drift from it, and check that every swept object's RUNPATH actually resolves to $libdir rather than merely looking $ORIGIN-relative. (cherry picked from commit 1299ca3)
The assertion extracted the sweep by grepping for the exact `find "$webkit_exec"` line, which stopped matching the moment the three per-directory loops became one unified loop over $libdir and $webkit_exec. It failed on the rewrite even though the property it guards -- that .so files are selected by NAME and not by mode alone, so a 0644 injected bundle is not skipped -- still holds. Match the sweep by shape instead, so the test survives the next refactor of a line it only reads in order to check one thing about it. (cherry picked from commit 23fd3c1)
All three found by the Codex reviewer and reproduced here before changing anything.
1. The WebKit link guard rejected every launch.
`test -O` dereferences, and appimagetool normalises the payload to root:root, so on
a mounted AppImage the guard asked "is the bundle owned by me?" -- false for every
non-root user. Reproduced with the CI artifact on a Steam Deck:
Unsloth cannot start: /tmp/.unsloth-webkit is not a link this session controls.
exit 126
Confirmed root ownership by reading the squashfs directly (unsquashfs -lls:
drwxr-xr-x root/root usr/libexec/unsloth-webkit). The security property was always
about the LINK -- one we created in a world-writable directory is ours, one an
attacker pre-created is theirs -- so read the link with stat and not its target. The
-x test still follows, which is what proves the link reaches a real bundle.
The existing guard test could not see this: its fixture target is owned by the test
user. The new case uses a root-owned directory as the payload stand-in.
2. The post-package assertion demanded a line that was deliberately removed.
It required AppRun to SET LD_LIBRARY_PATH, from when that was how the bundle
resolved itself. Setting it is now the unslothai#7953 failure, so the generator stopped
writing it and the check could never pass -- every build died there after producing
a correct artifact. Inverted: it now fails if AppRun sets the variable, which is the
property actually worth guarding.
3. The Noble probe gated on a configuration the runtime no longer produces.
It forced LD_LIBRARY_PATH to the bundle and failed the build when a host library
would not load that way. That is exactly the state the export was removed to avoid,
and it fails on 24.04 by construction, so the probe failed every build. It now gates
on the default search path -- what AppRun actually gives users -- and reports the
bundle-first result as evidence rather than a verdict.
Also fixes the build-path leak check, which failed OPEN: `grep -rlI | head -5 |
grep -q .` under `set -o pipefail` reports SIGPIPE from the killed grep as the
pipeline status, so the guard went quiet precisely when a path leaked into many
objects. No pipeline now.
(cherry picked from commit 500616b)
Three faults found in review of the portable-bundle work, all confirmed against the built artifact rather than by reading. Release validation rejected its own new asset. The packaging step builds the portable AppImage, appends it to artifactPaths and stages it as Unsloth-Desktop-<version>-Linux-portable.AppImage, but expected_names never listed it -- and that comparison is exact set equality. Every desktop release would have failed at "Validate release asset set", reporting the portable bundle as unexpected, before a single asset was uploaded. Simulating both blocks from the workflow file: with the entry the sets match exactly and nothing duplicates; without it, unexpected=['-Linux-portable.AppImage']. AppRun died silently when XDG_RUNTIME_DIR pointed somewhere unusable. mktemp fails there, and under set -e the failed command substitution ended the script before the fallback that uses the bundled loader cache could run -- with stderr discarded, so nothing was printed. Measured with XDG_RUNTIME_DIR set to a missing directory: exit 1, no output at all; with `|| true`, the app starts. A stale session, a container or su to another user all reach this. The portable bundle took in-app updates meant for the thin one. APPIMAGE is set by the AppImage runtime for both Linux bundles and both carry the same Tauri bundle-type marker, so desktop_update_mode() classified the portable build InApp exactly like the thin one; the frontend checks five seconds after startup and calls downloadAndInstall, and latest.json points at the signed THIN AppImage. A user running the portable bundle *because* their host has no WebKitGTK would have been updated into the one build that cannot start there. Keeping the portable artifact out of the updater feed does not prevent this: being unsigned stops it being an update payload, not an update consumer. Its AppRun now exports UNSLOTH_PORTABLE_APPIMAGE and the policy returns the existing ManualLinuxPackage, which opens the release page instead of swapping the binary underneath the user -- the same treatment a deb install already gets. Rebuilt from this commit and re-run across ubuntu 22.04/24.04, Mint 22, Debian 12, Arch with no host WebKitGTK, and a Debian host carrying only what HOST_LIBS_RE requires, with and without libGLESv2: all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24e5a40c21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
I'll send some follow-ups. |
|
I pushed four commits to this branch and reacted to every open review item.
Verification: Five items were anchored to The red checks were not from this PR: the same workflows were failing on The EGL abort on 24.04 is untouched, and still the reason the plainly-named download stays thin. |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bba0fa1e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # A build-time `id -u` would bake the CI runner's uid, which says nothing about who runs | ||
| # the AppImage, so the path is a plain constant and AppRun proves at launch that the link | ||
| # is one it owns (see there). Must stay no longer than the paths it replaces. | ||
| WEBKIT_LINK_PATH="/tmp/.unsloth-webkit" |
There was a problem hiding this comment.
On multi-user systems, this fixed /tmp path makes the portable AppImage usable by only the first UID that launches it: the symlink remains owned by that user even after its AppImage unmounts, /tmp's sticky bit prevents another user from removing it, and _webkit_link_ok consequently exits 126 for every subsequent user. Use a per-user runtime location or otherwise provide each login with an isolated compiled-in helper path.
Useful? React with 👍 / 👎.
| else | ||
| export GDK_PIXBUF_MODULE_FILE="$libdir/gdk-pixbuf/loaders.cache" | ||
| fi |
There was a problem hiding this comment.
When XDG_RUNTIME_DIR is set but missing or unwritable, the new || true avoids terminating AppRun, but this fallback points GDK at the packaged cache whose module paths still contain literal @APPDIR@ tokens. Gdk-pixbuf cannot resolve those entries, so image loaders and GTK icons remain broken in the stale-session/container scenario this fallback is intended to support; retry mktemp under /tmp or otherwise provide an expanded cache.
Useful? React with 👍 / 👎.
| printf '%-22s after bundled WebKit: ' "$lib" | ||
| out=$(/tmp/probe "$p" "$webkit"); echo "$out" | ||
| case "$out" in |
There was a problem hiding this comment.
In the Ubuntu 24.04 compatibility step, a probe that terminates by signal or otherwise exits nonzero before printing a FAILED prefix leaves out empty, which this case treats as success and counts as a completed preload. Thus a loader or library-constructor crash can let the compatibility job pass despite reproducing a runtime failure; capture and gate on /tmp/probe's exit status in addition to its diagnostic text.
Useful? React with 👍 / 👎.
| _gles_ldd=$(ldd "$_gles" 2>&1) && case "$_gles_ldd" in | ||
| *"not found"*|*"not a dynamic"*) ;; | ||
| *) _gles_ok=1 ;; | ||
| esac |
There was a problem hiding this comment.
When the packaged libGLESv2.so.2 has an unresolved relocation or an ABI-symbol mismatch against the host's libGLdispatch, plain ldd can list every dependency and emit neither not found nor not a dynamic, so this branch marks the library usable even though the later dlopen fails and WebKit aborts. This is also reflected in ldd --help, which reserves -r for “process data and function relocations”; use an actual dlopen probe or a relocation-aware check rather than dependency enumeration.
Useful? React with 👍 / 👎.
| # WebKit's two compiled-in paths must have been redirected off the host. | ||
| for lib in "$appdir"/usr/lib/unsloth/libwebkit2gtk-*.so*; do | ||
| [ -e "$lib" ] || continue | ||
| if strings -a "$lib" | grep -qE '^/usr/(lib|libexec)[^ ]*/webkit2gtk-4\.[01]'; then |
There was a problem hiding this comment.
In the Verify the shipped AppImage is self-contained step, set -o pipefail makes this assertion fail open when a forbidden path is found before strings finishes producing its large output: grep -q exits on the match, strings receives SIGPIPE, and the resulting nonzero pipeline status skips the if body. Although the earlier recursive leak pipeline was removed, this remaining WebKit-specific pipeline preserves the same failure mode; capture the strings output first or use a consumer that reads the full stream.
Useful? React with 👍 / 👎.
ff435a0 to
2a623e1
Compare
|
Don't you hate when @claude pushes some made up solution that you didn't ask for? I had to force push to fix Claude's completely wrong solution LOL. Didn't even allow me to review the commit. |
…ayland Two independent faults produced the same symptom -- a window that opens, a backend that serves, and nothing ever painted. Both were invisible to every check in this script, because the closure resolves, the process lives, and only the child that renders is gone. The packaged libGLESv2 renders on exactly one host: the one it was built on. glvnd's libGLESv2 is a dispatch shim that binds a vendor only when the host glvnd is the one it was built against. Measured across a container matrix, artifact built in an ubuntu:22.04 builder, window contents sampled with xwd rather than "the process is alive at 45s" -- a blank window passes that check, which is how it went unnoticed: host packaged shim not packaged ubuntu2204 RENDERS (1141) RENDERS (1141) <- the BUILD host ubuntu2404 BLANK (2) RENDERS (1141) mint22 BLANK (2) RENDERS (1141) debian12 BLANK (2) RENDERS (1141) arch BLANK (2) RENDERS (1142) minimal BLANK (2) RENDERS (1141) minimal-nogles BLANK (2) RENDERS (1124) (distinct colours in a 1280x800 root capture; an empty X server is 1, a single xclock about 204.) "No provider of glViewport found" appears verbatim in every BLANK run and never in a RENDERS run. It fails even on the host with no libGLESv2 to shadow -- the one case it was packaged for -- so packaging buys nothing the fallback does not. So it leaves DLOPEN_LIBS, and the probe asks whether the HOST can serve GLES rather than whether a packaged copy loads. That alone still left a Steam Deck blank, because of the second fault. With the shim gone and the host's own libGLESv2 in use, WebKit's DMA-BUF renderer does not render on Wayland. Measured there: DMA-BUF on web process dead, "No provider of glViewport" WEBKIT_DMABUF_RENDERER_FORCE_SHM=1 web process alive, epoxy still errors once WEBKIT_DISABLE_DMABUF_RENDERER=1 web process alive, no error, paints The app already knew: main.rs logs "Wayland detected; set WEBKIT_DMABUF_RENDERER_FORCE_SHM=1 for WebKitGTK compatibility" at every startup and never acted on it. This does not reproduce under Xvfb/llvmpipe, so a container matrix cannot see it, which is why it survived the work above. Verified by frontend API traffic rather than by process liveness -- a blank window never requests /api/models/list. Before: 0 frontend calls, no desktop-login. After: 4 frontend calls and desktop-login 200, on a Steam Deck, with nothing set by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ice says The Wayland condition is wider than the hardware that needs it. Measured on KDE Plasma Wayland (NVIDIA, host libGLESv2, DMA-BUF forced back on) with the same signals the Deck was judged by: WebKitWebProcess alive: 2, "No provider of glViewport": 0 18 frontend API calls, desktop-login 200 So the renderer works on that desktop, and every Wayland session currently gives up the accelerated path for a fault measured on one. Widening was still the right call -- the failing signal is not knowable at launch, and a Deck that never paints is worse than a desktop that paints slowly -- but the export made the trade compulsory: it overwrote WEBKIT_DISABLE_DMABUF_RENDERER unconditionally, so setting it yourself looked like it should work and was silently discarded. It is now left alone when already set. That escape hatch only means anything because WebKit reads the variable by VALUE, not by presence. Verified against the built bundle, counting distinct colours in a 1280x800 root capture (renderer on and off differ consistently): unset -> 1141 =0 -> 1141 =1 -> 1124 so =0 genuinely keeps it on rather than merely being present and disabling it. Two things the notice got wrong. The branches shared one "..., so WebKit's DMA-BUF renderer is being disabled." tail, which made the GLES branches read "this system has no libGLESv2.so.2, so it is being disabled" -- naming libGLESv2 as the thing turned off -- and made the Wayland branch say "WebKit's DMA-BUF renderer" twice in one sentence. Each branch now supplies a whole sentence. And the opt-out hint was printed on every branch, including the one reached because the host has no usable libGLESv2. Acting on it there restores the SIGABRT this block exists to prevent, so it is advice that breaks the machine it is printed on. It is now scoped to the Wayland branch, the only one where GLES is known to work. Rebuilt and re-run: all seven container hosts render; Wayland disables and prints the hint, no-GLES disables and does not; an explicit =0 is honoured on both a container and that KDE Wayland desktop. tests/sh/test_portable_appimage_closure.sh 69/69. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e529c13ca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| grep -q 'LoaderDir\|"[a-z]' "$pixbuf_dir/loaders.cache" 2>/dev/null \ | ||
| || die "the pixbuf loader cache came out empty; the bundled loaders would be invisible" |
There was a problem hiding this comment.
When the copied pixbuf loader directory is wrong or empty, gdk-pixbuf-query-loaders can still emit a comment-only cache containing the LoaderDir header; because this check treats that header as success, the build can ship a cache with zero module entries and AppRun will point GDK at no PNG/SVG loaders, breaking GTK icons/images at runtime. The tool is documented to scan GDK_PIXBUF_MODULEDIR and write the cache to stdout (https://man.archlinux.org/man/gdk-pixbuf-query-loaders.1.en), so gate on at least one quoted module path such as the tokenized @APPDIR@/.../*.so line rather than the header.
Useful? React with 👍 / 👎.
| # the recursive grep dies of SIGPIPE, and pipefail reports that nonzero status | ||
| # for the whole pipeline -- so the `if` is false precisely when a build path | ||
| # leaked into MANY objects, which is the case that matters most. | ||
| if grep -rlI --exclude-dir=proc "$RUNNER_TEMP" "$appdir" \ |
There was a problem hiding this comment.
The extracted AppImage mostly contains ELF/shared-object binaries, but this guard uses grep -I; checked GNU grep --help, and -I is equivalent to --binary-files=without-match, so a $RUNNER_TEMP string embedded in an ELF is treated as no match and the leak check passes. Keep the SIGPIPE-safe file output, but search as text (for example grep -rla) or run strings so binary build-host path leaks are not ignored.
Useful? React with 👍 / 👎.
| export PATH="$appdir/usr/bin${PATH:+:$PATH}" | ||
| # Deliberately NO LD_LIBRARY_PATH. |
There was a problem hiding this comment.
When the portable AppImage is launched from a shell or desktop session that already has LD_LIBRARY_PATH set (common with Conda/CUDA tooling), the dynamic loader still consults that inherited path before the $ORIGIN RUNPATHs this AppRun relies on, so the bundle can resolve GTK/WebKit or C++/nghttp2 pieces from the user's environment and reintroduce the mixed-library failures this change is trying to avoid. Unset or sanitize LD_LIBRARY_PATH before probing and execing the binary so the runtime is actually RUNPATH-only.
Useful? React with 👍 / 👎.
@danielhanchen @oobabooga Ready for a new review! |
|
Since the work looks like has been (accidentally) superseded by merged PR #8695, I have no choice but to rework this entire PR... |
ThomasEricB
changed the title
|
Closed in favor of #8813 |
Replace the global AppDir LD_LIBRARY_PATH launcher with an ORIGIN-only closure, keep host graphics/display/network ABI families host-owned, and verify the boundary after all linuxdeploy plugins. Extend clean-host prerequisites without weakening the no-GLES lane. Adapted from the mechanism-level AppImage design and measured host-boundary work in unslothai#8343. Co-authored-by: Thomas Eric <thombelcar@gmail.com>
…m, gfx1033 gate, and three install fixes (#8412) * Studio installer: torch 2.11 on Linux CPU, Vulkan for AMD without ROCm, gfx1033 gate, and three install fixes Split out of #8343, which also rewrites the Linux AppImage packaging. These six installer changes are independent of that work and verified on their own, so they land here while the AppImage side finishes. Original work by Thomas Eric (@ThomasEricB), who found all six on a Steam Deck. Co-authored-by: Thomas Eric <thombelcar@gmail.com> * Find the venv creation call by what it runs, not by its label #8479 moved venv creation behind _run_uv_venv, so the label this test grepped for no longer exists and the ordering assertion read as a too-late cache rather than a stale grep. Also drop a .studio-test-root artifact that a local test run left behind in the commit. * Three install.sh fixes: gfx1033 token match, override handoff, uv cache fallback Match gfx1033 as a TOKEN in the multiline ROCm probe. _probe_amd_gfx_arch keeps every `grep -oE` hit, and rocminfo names each GPU agent twice: once as the agent's own "Name: gfx1033" and once in its ISA Info block as "amdgcn-amd-amdhsa--gfx1033". A single-GPU Steam Deck therefore already probes as "gfx1033\ngfx1033", which the exact-string case did not match, so the host fell through to the version-keyed ROCm index and installed the very wheels the gate exists to avoid. Flatten to one space-delimited, lowercased, suffix-stripped line; the surrounding spaces keep gfx10330 out. Stop forwarding a rejected gfx1033 override to llama.cpp. Clearing the local _linux_inferred_gfx is not enough: setup.sh copies UNSLOTH_ROCM_GFX_ARCH straight into --rocm-gfx, and _apply_host_overrides reads any forwarded gfx as proof of ROCm (has_rocm = True). On a Van Gogh host with no ROCm that skips the AMD-without-ROCm Vulkan branch this release adds and asks for a ROCm prebuilt or a HIP source build the host cannot run. Unset the rejected override so setup.sh re-probes; a host that really does have ROCm still finds gfx1033 through its own rocminfo call, and a UNSLOTH_TORCH_INDEX_URL pin returns long before this block. Fall back to uv's default cache when the colocated one cannot be created. uv aborts with "Failed to initialize cache at ..." on a cache path it cannot create, so ignoring the mkdir status while keeping the export turned a disk optimisation into a hard install failure on a host where uv's own default would have worked. Measured: with STUDIO_HOME writable and its "cache" entry a file, `uv venv` exits 2 with the export and 0 without it. Tests: the gate test fed the gate a hand-built one-token probe, which is not what the probe produces, so it passed throughout. Drive the real get_torch_index_url against a real rocminfo shape instead, and assert first that the probe really is multi-line so a future single-hit probe cannot turn that into a vacuous pass. * Demote an already-installed ROCm torch on a miscomputing AMD arch Returning None from _amd_arch_index_url only stops a gfx1033 host from being GIVEN ROCm wheels. A venv that already holds them -- installed before the gate existed, and carried through the legacy-venv migration untouched because its forward-only validation passes on this arch -- was never demoted: install.sh resolves UNSLOTH_TORCH_BACKEND=cpu, which returns _ensure_rocm_torch at its first line; _ensure_cpu_torch fired only for an EXPLICIT pin; and the base update does not reinstall an already-satisfied torch. Upgrading therefore left in place exactly the build the gate exists to remove, on the machines that most need it removed. Treat the arch itself as CPU authority so _ensure_cpu_torch does the demotion. Scoped tightly: every AMD arch on the host must be a measured-bad one (a healthy dGPU beside the APU keeps ROCm), an explicit UNSLOTH_TORCH_INDEX_URL / _FAMILY still wins, and the disk label is read before any probe so hosts with no ROCm torch pay nothing. The arch probe ignores HSA_OVERRIDE_GFX_VERSION, which is the usual Van Gogh workaround and would otherwise make rocminfo answer gfx1030 and hide the arch being judged. * Honour an AMD visibility mask before auto-selecting Vulkan on Linux ROCR_VISIBLE_DEVICES filters the HSA runtime's agent list, so it removes GPUs from rocminfo's output too. A masked-out AMD device on a ROCm host therefore leaves the ROCm probe empty and reads as a driver-only box, and the new DRM sysfs pass set has_amd_gpu_without_rocm anyway. Vulkan honours none of the HIP masks -- it selects through GGML_VK_VISIBLE_DEVICES -- so the install would hand llama.cpp the exact GPU the caller hid, which _should_auto_vulkan_for_amd_windows already refuses on Windows and the has_physical_nvidia gate already refuses for CUDA_VISIBLE_DEVICES. Conjoin the mask check with a ROCm probe actually being installed rather than reusing _hip_visible_device_mask_set alone: the driver-only AMD host this branch exists for ships no rocminfo or amd-smi, and measuring it showed a bare exported CUDA_VISIBLE_DEVICES would otherwise cost a Steam Deck the Vulkan bundle. Intel is not addressed by HIP masks and keeps its plain detection. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Judge the whole AMD inventory, and prove the uv cache is writable * Restore the read-only rc warning main's tests assert * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Decline the ROCm route on a miscomputing target, and honour ROCR isolation on its own * Read the physical arch through an HSA spoof and a visibility mask * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Say why ARM64 keeps the CPU archive * Gate the CPU torch 2.11 floor on glibc 2.28 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Judge the gfx1033 gate on the visible AMD targets * Set the colocated uv cache on the standalone update path too * Resolve HIP-layer ordinals before classifying the visible gfx set * Resolve both visibility masks in the gate, not in the probe environment * Create the uv cache write probe with mktemp * Decline ROCm when a visibility mask or declared arch cannot be trusted * Gate gfx1033 on presence, not on which device the runtime picks * Ask the kernel before falling back to a spoofable probe * Disqualify the shared wheel family when gfx1033 is in the inventory * Carry the physical gfx1033 veto into the reroute and refuse an unverifiable spoof * Refuse the ROCm route before the inferred install, not after it * Demote ROCm torch when the selected target is a miscomputing arch The gfx1033 gate declined to INSTALL ROCm when the selected runtime target is a miscomputing arch, but it did not remove a ROCm build the venv already held. On a mixed host, a gfx1033 APU beside a healthy dGPU, _rocm_miscomputing_host() is False by design (it requires every physical AMD arch to be miscomputing), so _ensure_cpu_torch() had no authority to demote either. A device mask selecting the APU therefore printed "keeping CPU torch" while ROCm torch stayed installed, and the selected APU kept producing incorrect gradients under it. The message was wrong at the same time as the state. The two gates answer different questions and only the first could demote: _rocm_miscomputing_host() reads the inventory, while this branch reads the selected target, which is the one that decides what actually runs. Recorded the selected-target verdict and taught _ensure_cpu_torch to treat it as CPU authority, which is the path the inventory gate already used. Scoped to the case: with no such verdict a mixed host is untouched, so a machine whose selected target is the healthy dGPU keeps ROCm. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Revert the selected-target ROCm demotion I accepted a review item asking for this and was wrong: it contradicts a deliberate invariant this file already documents and tests. test_a_mask_cannot_shrink_the_host_to_its_bad_gpu states it directly. "Every arch is bad" is a question about the HOST, so it is asked of the whole host: ROCR_VISIBLE_DEVICES hiding a healthy dGPU would otherwise leave gfx1033 as the only arch and demote a working ROCm install to CPU. The demotion path calls _detect_amd_gfx_codes(ignore_hsa_override = True, ignore_visible_masks = True) for that reason, and install.sh's _probe_amd_gfx_arch unsets both masks too. Keying demotion off the selected runtime target is exactly the bypass that guards against, one door over from the HSA_OVERRIDE spoof the same tests cover. It went in green only because the demotion tests call _ensure_cpu_torch() directly, so the flag set inside _ensure_rocm_torch() was never exercised. What remains true is narrower and not worth this risk: on a masked mixed host the branch prints "keeping CPU torch" while a ROCm build stays installed, so the wording is inaccurate even though keeping it is the intended outcome. * Say what the selected-target branch actually does It printed "keeping CPU torch", which it cannot promise: whether an already-installed ROCm build is removed is _ensure_cpu_torch's call, and that asks about the whole host with the visible-device masks stripped, so a mixed host with a healthy dGPU keeps ROCm on purpose. The branch only declines to install ROCm for this target, so it now says that. The whole-host message above is unchanged, since there the demotion really does follow. * Tighten the comments added by this PR Compress the explanatory blocks to their load-bearing content: 584 added comment lines down to 313. No behaviour change; every test suite this PR touches still passes. * Make the arch-gate scenarios about the simulated host, not the test machine * Shorten the comments added by this PR Same content, fewer lines. Comments and docstrings only; no code changed. --------- Co-authored-by: Thomas Eric <thombelcar@gmail.com> Co-authored-by: danielhanchen <unslothshared@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: oobabooga <112222186+oobabooga@users.noreply.github.com>
3 participants
Side show of PR #6982 and #8412 . (Still needs to be merged!)
Superseded by #8695 and #8813