Installer: suppress macOS uv developer tools dialog - #8479
Merged
danielhanchen merged 7 commits intoAug 11, 2026
Merged
danielhanchen merged 7 commits into
danielhanchen merged 7 commits into
Conversation
`loopback_http::client` is the client that posts `.desktop_secret` to /api/auth/desktop-login, and it was built without a redirect policy. reqwest follows up to 10 redirects by default, and its cross-host protection strips headers rather than bodies, so a responder answering 307 (which preserves the method and the body) would carry the secret to whatever the Location header names, after the loopback URL had already been checked. Its sibling `streaming_client` already refuses redirects for exactly this reason: "Redirects are refused so a loopback URL cannot be bounced off-host after the check." Give `client` the same policy. No behaviour change for any real backend, which never redirects these routes.
Collaborator
Author
|
@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: 24389585f0
ℹ️ 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".
On a Mac without the Command Line Tools, /usr/bin/git, lipo, install_name_tool and friends are libxcselect shims. Executing one resolves no developer dir and posts to com.apple.dt.CommandLineTools.installondemand, which draws the 'requires the command line developer tools' dialog naming the tool. Resolving the path does not; only execution does. Two call sites execute a shim on the consumer path: _has_working_git ran 'git --version' to decide whether git works, so on a clean Mac the probe raised the dialog it exists to detect. It now answers from the resolved path when that path is exactly /usr/bin/git and no toolchain is selected. Deliberately narrow: a Homebrew, MacPorts or Xcode.app git earlier on PATH is a real binary and is still probed by executing it, so a Mac with a working git but no CLT selected behaves exactly as before. An earlier version of this gated on 'no CLT implies no working git' and broke that case, which the existing test caught. xcode-select -p only asks which toolchain is selected and never prompts. The venv arch probe called lipo first and fell back to file -L. lipo is a shim; 2>/dev/null hides its stderr but not a GUI dialog. file is base system and always answers, so the order is swapped. Both spellings feed the same case below, against 'Mach-O 64-bit executable arm64' or 'universal binary ... [x86_64] [arm64]' rather than lipo's 'arm64' / 'x86_64 arm64', so the branch taken is unchanged. clean-machine-assert.sh already made this same swap for its own use. The cctools binaries were missing from the clean machine CI tool list, so none of this was visible: trace mode generated no wrapper and the absent list never checked them. install_name_tool, lipo, otool, objdump, vtool, strip and nm are added, which is what makes these fixes regression testable. test_macos_clt_gate.sh gains two cases pinning the contract: with a shim git and no toolchain selected the probe answers no WITHOUT executing it, proven by a stub that records execution into a marker file, and with a real git elsewhere on PATH the stub IS executed. The first assertion passed vacuously when written (wrong temp path meant the marker could never be created) and was fixed by making its pair fail first. 18 to 23 passing.
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8407e950a3
ℹ️ 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".
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2705e7016
ℹ️ 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".
Comment on lines
+2124
to
+2126
| if [ "${OS:-}" = "macos" ] && | ||
| [ "${MAC_INTEL:-false}" != true ] && | ||
| [ "$(command -v git)" = "${_CLT_GIT_SHIM:-/usr/bin/git}" ] && |
There was a problem hiding this comment.
On an Apple Silicon Mac running an x86_64 shell under Rosetta, the architecture block still sets MAC_INTEL=true even after sysctl identifies the hardware as arm64. For a --local install without CLT, this exception therefore bypasses the non-executing /usr/bin/git check and runs git --version, reopening the developer-tools dialog this change is intended to suppress. Track the physical Apple Silicon result separately so only genuine Intel Macs receive this exception.
Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment.
Correct, and fixed in e913d13. install.sh:1840 set MAC_INTEL=true for any x86_64 macOS shell, Rosetta included, so a --local install on Apple Silicon in a Rosetta shell fell through to git --version against the CLT shim. The architecture block now records the hardware answer separately as _MAC_ROSETTA (set in the same sysctl hw.optional.arm64 branch that already prints the Rosetta warning), and _has_working_git takes the non-executing path when it is true. MAC_INTEL itself is unchanged, since the Python version and SKIP_TORCH really do follow the x86_64 shell. tests/sh/test_macos_clt_gate.sh gains the Rosetta case (shim git answered without execution) plus an assertion that the architecture block still sets the variable, so a rename cannot silently disable the guard: 28 passed. Full tests/sh is 47 suites green.
Member
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
danielhanchen
added a commit
that referenced
this pull request
Aug 13, 2026
#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.
danielhanchen
added a commit
that referenced
this pull request
Sep 7, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
install_name_toolcall from opening Apple's Command Line Tools installer on consumer Macs without developer tools/usr/bin/gitCLT shim during dependency detection and prefer base-systemfileover theliposhim for architecture detectionThe
git,lipo, and expanded cctools changes are integrated from #8478 as commit8407e950a, retaining Daniel's authorship.Context
uv currently invokes
install_name_tool -id <libpython.dylib> <libpython.dylib>after downloading managed CPython. On macOS without CLT, Apple's/usr/binshim opens a GUI installation prompt even though uv treats the patch failure as a warning. This scopes a non-success shim touv venvonly, so uv retains its warning path and no unpatched dylib is reported as successfully modified.Upstream issue: astral-sh/uv#14893
Verification
tests/sh/test_macos_uv_install_name_tool_guard.sh: 95 passedtests/sh/test_clean_machine_install_name_tool_assert.sh: 23 passedtests/sh/test_macos_clt_gate.sh: 23 passeddashunavailable and the known localtest_linux_deps_gate.shtimeout)sh -n install.sh,bash -n install.sh, workflow YAML parsing, sentinel integration simulation, andgit diff --checkpassed