From 6e87bade2504d009c9130b78d46708f0f5cf2166 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Thu, 16 Apr 2026 12:01:01 +0000 Subject: [PATCH] Trim verbose comments in PATH helpers Reduce inline comments from ~160 lines to ~25 across both files. Keep one-line summaries of the "why"; drop multi-paragraph rationale blocks that repeated information already captured in commit messages and PR discussion. --- install.ps1 | 114 ++++++++--------------------------------------- studio/setup.ps1 | 93 +++++++------------------------------- 2 files changed, 34 insertions(+), 173 deletions(-) diff --git a/install.ps1 b/install.ps1 index 26ba14c5e5..dfb54b1e87 100644 --- a/install.ps1 +++ b/install.ps1 @@ -100,15 +100,8 @@ function Install-UnslothStudio { Write-Host "" # ── Helper: refresh PATH from registry (deduplicating entries) ── - # Merge order: - # 1. Activated venv Scripts dir (only if $env:VIRTUAL_ENV is set) so an - # explicitly-activated venv keeps precedence. - # 2. Machine, then User PATH freshly read from registry so a tool we - # just installed wins over any stale shim still in $env:Path. - # 3. Current $env:Path as fallback so process-only entries that nothing - # else covers are not lost. - # Dedup compares both raw and expanded forms so %VAR% references don't - # survive twice (once as %VAR%\foo and once as the expanded literal). + # Merge order: venv Scripts (if active) > Machine > User > current $env:Path. + # Dedup compares both raw and expanded forms (%VAR% vs literal). function Refresh-SessionPath { $machine = [System.Environment]::GetEnvironmentVariable("Path", "Machine") $user = [System.Environment]::GetEnvironmentVariable("Path", "User") @@ -132,16 +125,8 @@ function Install-UnslothStudio { } # ── Helper: safely add a directory to the persistent User PATH ── - # Uses direct registry access to preserve REG_EXPAND_SZ type - # (avoids .NET SetEnvironmentVariable bug that converts to REG_SZ). - # - # Position: 'Append' (default) adds $Directory to the END of the persisted - # User PATH so existing user tools (e.g. system python, pip) keep taking - # precedence in new shells. This matches rustup/cargo/nvm/pyenv/uv behavior - # and avoids silently hijacking resolution of common executables. Pass - # 'Prepend' only when a caller truly needs the new entry to win over - # existing ones at registry scope. In-session precedence should be handled - # by an inline $env:Path = "$Dir;$env:Path" prepend instead. + # Direct registry access preserves REG_EXPAND_SZ (avoids dotnet/runtime#1442). + # Append (default) keeps existing tools first; Prepend for must-win entries. function Add-ToUserPath { param( [Parameter(Mandatory = $true)][string]$Directory, @@ -152,18 +137,9 @@ function Install-UnslothStudio { $regKey = [Microsoft.Win32.Registry]::CurrentUser.CreateSubKey('Environment') try { $rawPath = $regKey.GetValue('Path', '', [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames) - # Explicit string[] cast: a single-entry split otherwise collapses - # to a scalar string, which then gets char-indexed and breaks the - # partition loop below. - [string[]]$entries = if ($rawPath) { $rawPath -split ';' } else { @() } - # Normalize both the raw and expanded forms of the new directory - # so dedup catches mirror-image cases: PATH holding %USERPROFILE%\foo - # vs Directory passed as C:\Users\me\foo, and vice versa. + [string[]]$entries = if ($rawPath) { $rawPath -split ';' } else { @() } # string[] prevents scalar collapse $normalDir = $Directory.Trim().Trim('"').TrimEnd('\').ToLowerInvariant() $expNormalDir = [Environment]::ExpandEnvironmentVariables($Directory).Trim().Trim('"').TrimEnd('\').ToLowerInvariant() - # Partition existing entries into "kept" (not our dir) and "dropped" - # (matches our dir). Track match indices so we can distinguish - # "already at position 0" from "present but at a late position". $kept = New-Object System.Collections.Generic.List[string] $matchIndices = New-Object System.Collections.Generic.List[int] for ($i = 0; $i -lt $entries.Count; $i++) { @@ -179,21 +155,14 @@ function Install-UnslothStudio { $kept.Add($entries[$i]) } $alreadyPresent = $matchIndices.Count -gt 0 - # Append semantics: if the entry is already anywhere in PATH we - # leave it untouched (idempotent, never reorder user-curated order). - if ($alreadyPresent -and $Position -eq 'Append') { + if ($alreadyPresent -and $Position -eq 'Append') { # Append: idempotent no-op return $false } - # Prepend semantics: if the entry is already at position 0 with - # exactly one copy, preserve the user's existing casing/form and - # no-op. Only rebuild when a reorder or dedup is actually needed. - if ($alreadyPresent -and $Position -eq 'Prepend' -and + if ($alreadyPresent -and $Position -eq 'Prepend' -and # Prepend: no-op if already at front $matchIndices.Count -eq 1 -and $matchIndices[0] -eq 0) { return $false } - # One-time backup of the pristine User PATH before our first - # mutation. Stored under HKCU\Software\Unsloth so a wiped/clobbered - # PATH can be recovered. Idempotent: existing backup is preserved. + # One-time backup under HKCU\Software\Unsloth\PathBackup if ($rawPath) { try { $backupKey = [Microsoft.Win32.Registry]::CurrentUser.CreateSubKey('Software\Unsloth') @@ -219,21 +188,12 @@ function Install-UnslothStudio { } else { $Directory } - # Prepend idempotency: if the new directory was already at - # position 0 (and no duplicates existed elsewhere) the composed - # string matches rawPath byte-for-byte. Skip the registry write - # so we do not broadcast an unnecessary WM_SETTINGCHANGE. - if ($newPath -ceq $rawPath) { + if ($newPath -ceq $rawPath) { # no actual change return $false } $regKey.SetValue('Path', $newPath, [Microsoft.Win32.RegistryValueKind]::ExpandString) - # Broadcast WM_SETTINGCHANGE so other processes pick up the change. - # Use [NullString]::Value (not $null) for the delete call so the - # sentinel crosses into .NET as a real null reference -- on - # PowerShell 7.5+ / .NET 9, a bare $null here can be coerced to - # an empty string, which sets the dummy variable to "" instead - # of deleting it and leaves UnslothPathRefresh_XXXXXXXX in - # HKCU\Environment permanently. + # Broadcast WM_SETTINGCHANGE via dummy env-var roundtrip. + # [NullString]::Value avoids PS 7.5+/.NET 9 $null-to-"" coercion. try { $d = "UnslothPathRefresh_$([guid]::NewGuid().ToString('N').Substring(0,8))" [Environment]::SetEnvironmentVariable($d, '1', 'User') @@ -1078,46 +1038,21 @@ shell.Run cmd, 0, False New-StudioShortcuts -UnslothExePath $UnslothExe - # ── Expose the `unsloth` command via a single-purpose shim directory ── - # The venv's Scripts dir holds python.exe and pip.exe alongside unsloth.exe, - # so adding that dir to PATH (at either position) has unwanted side effects: - # Prepend hijacks the user's system python / pip in every future shell; - # Append makes the installer's newly-built unsloth.exe lose to any older - # unsloth.exe the user already had earlier on PATH. Both are bad. - # - # Instead we create a small directory that contains only the unsloth - # launcher (hardlinked or copied from the venv's Scripts\unsloth.exe), - # and Prepend just that directory. Benefits over a .cmd wrapper: - # - no batch %...% expansion of user arguments (e.g. prompts with `%`) - # - works in Git Bash / MSYS2 / POSIX-style shells on Windows that do - # not resolve .cmd by bare name - # - no source encoding concerns on non-ASCII profile paths - # - programmatic callers (subprocess.run, child_process.execFile) hit - # the native executable directly instead of shelling into cmd.exe - # We try a hardlink first so pip upgrades inside the venv propagate - # automatically (same inode). If the filesystem or volume rejects the - # hardlink we fall back to a plain copy, which the next install run - # will refresh. + # ── Expose `unsloth` via a shim dir containing only unsloth.exe ── + # We do NOT add the venv Scripts dir to PATH (it also holds python.exe + # and pip.exe, which would hijack the user's system interpreter). + # Hardlink preferred; falls back to copy if cross-volume or non-NTFS. $ShimDir = Join-Path $StudioHome "bin" New-Item -ItemType Directory -Force -Path $ShimDir | Out-Null $ShimExe = Join-Path $ShimDir "unsloth.exe" - # Wrap the whole remove/link/copy sequence in a try/catch so a locked - # launcher does not crash the installer. The common case is a re-run - # while the user still has `unsloth studio` open: the existing shim is - # held open by the running process, Remove-Item refuses (and under the - # script's $ErrorActionPreference this would otherwise be fatal). When - # that happens the existing shim is perfectly usable, so we log and - # keep going instead of aborting the install. + # try/catch: if unsloth.exe is locked (Studio running), keep the old shim. $shimUpdated = $false try { if (Test-Path $ShimExe) { Remove-Item $ShimExe -Force -ErrorAction Stop } try { New-Item -ItemType HardLink -Path $ShimExe -Target $UnslothExe -ErrorAction Stop | Out-Null } catch { - # Hardlink unavailable (cross-volume, non-NTFS, permissions). Copy - # is self-contained; future pip upgrades inside the venv will not - # update the copy until the user re-runs the installer. - Copy-Item -Path $UnslothExe -Destination $ShimExe -Force -ErrorAction Stop + Copy-Item -Path $UnslothExe -Destination $ShimExe -Force -ErrorAction Stop # fallback: copy } $shimUpdated = $true } catch { @@ -1132,14 +1067,7 @@ shell.Run cmd, 0, False Write-Host " Launch unsloth studio directly via '$UnslothExe' until the next successful install." -ForegroundColor Yellow } } - # Only add the shim directory to PATH when the launcher actually exists - # in it. Otherwise a total shim-creation failure on a fresh install (e.g. - # antivirus blocks unsloth.exe, disk full, restrictive FS permissions) - # would prepend an empty directory to User PATH and leave the user with - # an install that reports success but cannot resolve `unsloth` in a new - # shell. Also gate the "added to PATH" step message on both a successful - # shim (re)create AND a fresh PATH insertion, so idempotent re-runs stay - # quiet. + # Only add to PATH when the launcher actually exists on disk. $pathAdded = $false if (Test-Path $ShimExe) { $pathAdded = Add-ToUserPath -Directory $ShimDir -Position 'Prepend' @@ -1147,11 +1075,7 @@ shell.Run cmd, 0, False if ($shimUpdated -and $pathAdded) { step "path" "added unsloth launcher to PATH" } - # Sync the current session unconditionally so re-runs in stale terminals - # see the shim, and so PATH entries that the studio/setup.ps1 subprocess - # persisted (cmake, nvcc, Python Scripts) are visible in this parent - # process before it returns control to the user's shell. - Refresh-SessionPath + Refresh-SessionPath # sync current session with registry # Launch studio automatically in interactive terminals; # in non-interactive environments (CI, Docker) just print instructions. diff --git a/studio/setup.ps1 b/studio/setup.ps1 index 53a559e6e0..218be7e045 100644 --- a/studio/setup.ps1 +++ b/studio/setup.ps1 @@ -73,15 +73,7 @@ function Refresh-Environment { } $machinePath = [System.Environment]::GetEnvironmentVariable('Path', 'Machine') $userPath = [System.Environment]::GetEnvironmentVariable('Path', 'User') - # Merge order: - # 1. Activated venv Scripts dir (only if $env:VIRTUAL_ENV is set) so an - # explicitly-activated venv keeps precedence. - # 2. Machine, then User PATH freshly read from registry so a tool we - # just installed wins over any stale shim still in $env:Path. - # 3. Current $env:Path as fallback so process-only entries that nothing - # else covers are not lost. - # Dedup compares both raw and expanded forms so %VAR% references don't - # survive twice (once as %VAR%\foo and once as the expanded literal). + # Merge: venv Scripts (if active) > Machine > User > current $env:Path. Dedup raw+expanded. $venvScripts = if ($env:VIRTUAL_ENV) { Join-Path $env:VIRTUAL_ENV 'Scripts' } else { $null } $sources = @() if ($venvScripts) { $sources += $venvScripts } @@ -102,16 +94,8 @@ function Refresh-Environment { } # ── Helper: safely add a directory to the persistent User PATH ── -# Uses direct registry access to preserve REG_EXPAND_SZ type -# (avoids .NET SetEnvironmentVariable bug that converts to REG_SZ). -# -# Position: 'Append' (default) adds $Directory to the END of the persisted -# User PATH so existing user tools (e.g. system python, pip) keep taking -# precedence in new shells. This matches rustup/cargo/nvm/pyenv/uv behavior -# and avoids silently hijacking resolution of common executables. Pass -# 'Prepend' only when a caller truly needs the new entry to win over -# existing ones at registry scope. In-session precedence should be handled -# by an inline $env:Path = "$Dir;$env:Path" prepend instead. +# Direct registry access preserves REG_EXPAND_SZ (avoids dotnet/runtime#1442). +# Append (default) keeps existing tools first; Prepend for must-win entries. function Add-ToUserPath { param( [Parameter(Mandatory = $true)][string]$Directory, @@ -122,18 +106,9 @@ function Add-ToUserPath { $regKey = [Microsoft.Win32.Registry]::CurrentUser.CreateSubKey('Environment') try { $rawPath = $regKey.GetValue('Path', '', [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames) - # Explicit string[] cast: a single-entry split otherwise collapses - # to a scalar string, which then gets char-indexed and breaks the - # partition loop below. - [string[]]$entries = if ($rawPath) { $rawPath -split ';' } else { @() } - # Normalize both the raw and expanded forms of the new directory - # so dedup catches mirror-image cases: PATH holding %USERPROFILE%\foo - # vs Directory passed as C:\Users\me\foo, and vice versa. + [string[]]$entries = if ($rawPath) { $rawPath -split ';' } else { @() } # string[] prevents scalar collapse $normalDir = $Directory.Trim().Trim('"').TrimEnd('\').ToLowerInvariant() $expNormalDir = [Environment]::ExpandEnvironmentVariables($Directory).Trim().Trim('"').TrimEnd('\').ToLowerInvariant() - # Partition existing entries into "kept" (not our dir) and "dropped" - # (matches our dir). Track match indices so we can distinguish - # "already at position 0" from "present but at a late position". $kept = New-Object System.Collections.Generic.List[string] $matchIndices = New-Object System.Collections.Generic.List[int] for ($i = 0; $i -lt $entries.Count; $i++) { @@ -149,23 +124,14 @@ function Add-ToUserPath { $kept.Add($entries[$i]) } $alreadyPresent = $matchIndices.Count -gt 0 - # Append semantics: if the entry is already anywhere in PATH we - # leave it untouched (idempotent, never reorder user-curated order). - if ($alreadyPresent -and $Position -eq 'Append') { + if ($alreadyPresent -and $Position -eq 'Append') { # Append: idempotent no-op return $false } - # Prepend semantics: if the entry is already at position 0 with - # exactly one copy, preserve the user's existing casing/form and - # no-op. Only rebuild when a reorder or dedup is actually needed. - if ($alreadyPresent -and $Position -eq 'Prepend' -and + if ($alreadyPresent -and $Position -eq 'Prepend' -and # Prepend: no-op if already at front $matchIndices.Count -eq 1 -and $matchIndices[0] -eq 0) { return $false } - # One-time backup of the pristine User PATH before our first - # mutation. Stored under HKCU\Software\Unsloth so a wiped/clobbered - # PATH can be recovered. Idempotent: existing backup is preserved. - # The script-top backup at line ~547 covers the studio entry point; - # this in-helper backup also covers callers that bypass that block. + # One-time backup under HKCU\Software\Unsloth\PathBackup if ($rawPath) { try { $backupKey = [Microsoft.Win32.Registry]::CurrentUser.CreateSubKey('Software\Unsloth') @@ -191,21 +157,12 @@ function Add-ToUserPath { } else { $Directory } - # Prepend idempotency: if the new directory was already at - # position 0 (and no duplicates existed elsewhere) the composed - # string matches rawPath byte-for-byte. Skip the registry write - # so we do not broadcast an unnecessary WM_SETTINGCHANGE. - if ($newPath -ceq $rawPath) { + if ($newPath -ceq $rawPath) { # no actual change return $false } $regKey.SetValue('Path', $newPath, [Microsoft.Win32.RegistryValueKind]::ExpandString) - # Broadcast WM_SETTINGCHANGE so other processes pick up the change. - # Use [NullString]::Value (not $null) for the delete call so the - # sentinel crosses into .NET as a real null reference -- on - # PowerShell 7.5+ / .NET 9, a bare $null here can be coerced to - # an empty string, which sets the dummy variable to "" instead - # of deleting it and leaves UnslothPathRefresh_XXXXXXXX in - # HKCU\Environment permanently. + # Broadcast WM_SETTINGCHANGE via dummy env-var roundtrip. + # [NullString]::Value avoids PS 7.5+/.NET 9 $null-to-"" coercion. try { $d = "UnslothPathRefresh_$([guid]::NewGuid().ToString('N').Substring(0,8))" [Environment]::SetEnvironmentVariable($d, '1', 'User') @@ -638,9 +595,7 @@ if ($script:StudioVtOk -and -not $env:NO_COLOR) { Write-Host " $Rule" -ForegroundColor DarkGray } -# Back up User PATH before any modifications for recovery. -# Stored under HKCU\Software\Unsloth (not HKCU\Environment) to avoid -# polluting the process environment block with a multi-KB variable. +# Back up User PATH under HKCU\Software\Unsloth before any modifications. try { $envKey = [Microsoft.Win32.Registry]::CurrentUser.OpenSubKey('Environment', $false) if ($envKey) { @@ -798,10 +753,7 @@ if (-not $HasCmake) { foreach ($d in $cmakeDefaults) { if (Test-Path (Join-Path $d "cmake.exe")) { $env:Path = "$d;$env:Path" - # Persist to user PATH so Refresh-Environment does not drop it later. - # Prepend so the newly-selected cmake wins over any older cmake - # entry already in the user PATH (this dir has only cmake.exe, no - # python.exe, so prepending does not hijack the user's interpreter). + # Persist to user PATH (Prepend so this cmake wins over older ones). Add-ToUserPath -Directory $d -Position 'Prepend' | Out-Null $HasCmake = $null -ne (Get-Command cmake -ErrorAction SilentlyContinue) if ($HasCmake) { @@ -1068,11 +1020,7 @@ $nvccBinDir = Split-Path $NvccPath -Parent if ($env:PATH -notlike "*$nvccBinDir*") { [Environment]::SetEnvironmentVariable('PATH', "$nvccBinDir;$env:PATH", 'Process') } -# Persist nvcc bin dir to User PATH so it works in new terminals. -# Prepend so the toolkit we just selected (driver-compatible) wins over any -# older CUDA bin dir already on the user PATH. Critical for llama.cpp builds: -# a later Refresh-Environment could otherwise reorder the selected nvcc behind -# a stale one. No hijack risk since this dir has only CUDA tools, no python. +# Persist nvcc bin dir (Prepend so the driver-compatible toolkit wins). if (Add-ToUserPath -Directory $nvccBinDir -Position 'Prepend') { substep "Persisted CUDA bin dir to user PATH" } @@ -1231,21 +1179,10 @@ if ($HasPython) { $PythonOk = $true } -# Ensure the user-scheme Python Scripts dir is on PATH so any pip-installed -# console scripts (including `unsloth` if installed via `pip install --user`) -# are discoverable in new terminals. Stick strictly to the 'nt_user' scheme: -# we do NOT fall back to sysconfig.get_path('scripts') because that returns -# the venv's Scripts dir when this setup.ps1 is invoked inside an activated -# venv, which would re-introduce the python / pip hijack that the dedicated -# shim directory (install.ps1) was designed to avoid. +# Add user-scheme Python Scripts dir to PATH (nt_user only, no venv fallback). $ScriptsDir = python -c "import os, sysconfig; p = sysconfig.get_path('scripts', 'nt_user'); print(p if os.path.exists(p) else '')" if ($LASTEXITCODE -eq 0 -and $ScriptsDir -and (Test-Path $ScriptsDir)) { - # Use Append semantics here: this dir holds ALL user-installed pip - # console scripts (pip, pytest, huggingface-cli, etc.), and reordering - # it to the front of PATH would silently change resolution precedence - # for every one of those tools. Install.ps1 already guarantees the new - # `unsloth` wins via a dedicated shim dir at PATH position 0, so we - # only need to make sure this directory is present, not at the front. + # Append (not Prepend) -- this dir has other pip scripts; shim handles unsloth. if (Add-ToUserPath -Directory $ScriptsDir) { # Also add to current process so it's available immediately $ProcessPathEntries = $env:PATH.Split(';')