feat: add NSIS installer and classroom admin checklist#2045
Conversation
Closes Hmbown#1983 - Add scripts/installer/codewhale.nsi: NSIS installer that installs both codewhale.exe and codewhale-tui.exe to %LOCALAPPDATA%\Programs\CodeWhale\bin, adds to current-user PATH, and includes an uninstaller that cleans PATH - Add docs/CLASSROOM_INSTALL.md: step-by-step checklist for IT admins deploying CodeWhale in labs/classrooms, covering silent install, manual fallback, API key provisioning, imaging notes, and troubleshooting - Update docs/INSTALL.md: add Windows NSIS Installer section referencing the new installer and classroom checklist
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Windows NSIS installer for CodeWhale, along with comprehensive documentation for classroom and lab deployments. The review feedback highlights critical logic errors in the installer script, particularly within the uninstaller's PATH manipulation logic and the incorrect usage of string function macros, which could lead to environment variable corruption. Furthermore, an improvement was suggested for the documentation to dynamically fetch the latest release version rather than using a hardcoded value.
| !include "StrFunc.nsh" | ||
|
|
||
| ${StrStr} |
There was a problem hiding this comment.
To use string functions in both the installer and uninstaller, you must define both versions using the StrFunc macros. Additionally, Windows environment variables like PATH are case-insensitive; using a case-sensitive search (StrStr) can result in duplicate entries if the casing in the registry doesn't exactly match $INSTDIR.
!include "StrFunc.nsh"
; Define string functions for both installer and uninstaller
${StrStr}
${UnStrStr}
|
|
||
| ; Remove from current-user PATH | ||
| ReadRegStr $0 HKCU "Environment" "Path" | ||
| ${StrStr} $1 $0 "$INSTDIR\bin" |
There was a problem hiding this comment.
The uninstaller must call the uninstaller-specific version of the string function (e.g., ${un.StrStr}) to avoid compilation errors and ensure the code is reachable in the uninstaller context.
${un.StrStr} $1 $0 "$INSTDIR\bin"
| Function un.RemoveFromPath | ||
| Exch $R0 ; directory to remove | ||
| Exch | ||
| Exch $R1 ; original PATH | ||
| Push $R2 | ||
| Push $R3 | ||
| Push $R4 | ||
|
|
||
| StrCpy $R2 "" | ||
| StrCpy $R3 "" | ||
|
|
||
| loop: | ||
| ${StrStr} $R4 $R1 $R0 | ||
| StrCmp $R4 "" done | ||
| ; Found — get substring before match | ||
| StrLen $R4 $R4 | ||
| StrLen $R3 $R1 | ||
| IntOp $R3 $R3 - $R4 | ||
| StrCpy $R2 $R1 $R3 | ||
| ; Get substring after match + dir length | ||
| StrLen $R3 $R0 | ||
| IntOp $R4 $R4 - $R3 | ||
| StrCpy $R3 $R1 "" $R4 | ||
| ; Strip leading semicolon from remainder | ||
| StrCpy $R4 $R3 1 | ||
| StrCmp $R4 ";" 0 +2 | ||
| StrCpy $R3 $R3 "" 1 | ||
| ; Strip trailing semicolon from prefix | ||
| StrLen $R4 $R2 | ||
| IntOp $R4 $R4 - 1 | ||
| StrCpy $R4 $R2 1 $R4 | ||
| StrCmp $R4 ";" 0 +2 | ||
| StrCpy $R2 $R2 $R4 | ||
| ; Concatenate | ||
| StrCmp $R2 "" 0 +2 | ||
| StrCpy $R2 $R3 | ||
| Goto done | ||
| StrCmp $R3 "" 0 +2 | ||
| StrCpy $R1 $R2 | ||
| Goto done | ||
| StrCpy $R1 "$R2;$R3" | ||
| Goto done | ||
|
|
||
| done: | ||
| Pop $R4 | ||
| Pop $R3 | ||
| Pop $R2 | ||
| Pop $R0 | ||
| Exch $R1 | ||
| FunctionEnd |
There was a problem hiding this comment.
The un.RemoveFromPath function contains critical logic errors in string offset calculations and semicolon stripping. Specifically, it uses a string character as a length parameter at line 185 and incorrectly calculates the suffix offset at line 174. This will likely lead to PATH corruption during uninstallation.
Function un.RemoveFromPath
Exch $R0 ; directory to remove
Exch
Exch $R1 ; original PATH
Push $R2 ; prefix
Push $R3 ; suffix
Push $R4 ; match result
${un.StrStr} $R4 $R1 $R0
StrCmp $R4 "" done
; Calculate prefix
StrLen $R2 $R1
StrLen $R3 $R4
IntOp $R3 $R2 - $R3 ; Match offset
StrCpy $R2 $R1 $R3 ; Prefix string
; Calculate suffix
StrLen $R4 $R0
IntOp $R4 $R3 + $R4 ; Suffix offset = Match offset + Dir length
StrCpy $R3 $R1 "" $R4 ; Suffix string
; Clean up semicolons
StrCpy $R4 $R3 1
StrCmp $R4 ";" 0 +2
StrCpy $R3 $R3 "" 1 ; Strip leading semicolon from suffix
StrLen $R4 $R2
IntOp $R4 $R4 - 1
StrCpy $R0 $R2 1 $R4
StrCmp $R0 ";" 0 +2
StrCpy $R2 $R2 $R4 ; Strip trailing semicolon from prefix
; Concatenate
StrCmp $R2 "" 0 +3
StrCpy $R1 $R3
Goto done
StrCmp $R3 "" 0 +3
StrCpy $R1 $R2
Goto done
StrCpy $R1 "$R2;$R3"
done:
Pop $R4
Pop $R3
Pop $R2
Pop $R0
Exch $R1
FunctionEnd
| New-Item -ItemType Directory -Force -Path $binDir | ||
|
|
||
| # 2. Download binaries (adjust URL to your mirror or release tag) | ||
| $tag = "v0.9.0" # replace with desired version |
There was a problem hiding this comment.
Hardcoding the version string in a deployment checklist will lead to outdated documentation as new releases are published. It is better to provide a command that dynamically fetches the latest version tag from the GitHub API.
| $tag = "v0.9.0" # replace with desired version | |
| $tag = (Invoke-RestMethod -Uri "https://api.github.com/repos/Hmbown/CodeWhale/releases/latest").tag_name |
- Define UnStrStr macro for uninstaller string functions - Use un.StrStr instead of StrStr in uninstaller context - Rewrite un.RemoveFromPath with correct offset calculations and semicolon handling to prevent PATH corruption - Use dynamic version fetch from GitHub API in CLASSROOM_INSTALL.md
1 participant
Summary
Adds a Windows NSIS installer and a classroom deployment checklist, closing #1983.
What is included
scripts/installer/codewhale.nsi -- NSIS installer script that:
docs/CLASSROOM_INSTALL.md -- Classroom/lab admin checklist covering:
docs/INSTALL.md -- Updated with a new "Windows NSIS Installer" section under the Windows package managers area, cross-referencing the installer and classroom checklist.
Testing
Checklist