Conversation
There was a problem hiding this comment.
Pull request overview
Migrates the Windows installer pipeline from the legacy custom SFX/setup projects to an Inno Setup–based installer, updating staging, CI/CD, and documentation accordingly.
Changes:
- Add an Inno Setup installer script and update CI to compile it (with build-number versioning).
- Update the PowerShell staging script to prepare an
Installer_Stagingdirectory for Inno Setup. - Remove the legacy setup/remove Visual Studio projects, sources, and the custom SFX builder.
Reviewed changes
Copilot reviewed 47 out of 54 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/prepare_installer.ps1 | Stages build outputs/resources into Installer_Staging for Inno Setup compilation. |
| tools/Create-Sfx.ps1 | Removes legacy custom SFX builder script. |
| src/vcxproj/setup/x86.props | Removes legacy setup project props. |
| src/vcxproj/setup/x64.props | Removes legacy setup project props. |
| src/vcxproj/setup/setup_release.props | Removes legacy setup project props. |
| src/vcxproj/setup/setup_debug.props | Removes legacy setup project props. |
| src/vcxproj/setup/setup_base.props | Removes legacy setup project props. |
| src/vcxproj/setup/setup.vcxproj.filters | Removes legacy setup project filters. |
| src/vcxproj/setup/setup.vcxproj | Removes legacy setup project file. |
| src/vcxproj/setup/setup.sln | Removes legacy setup solution. |
| src/vcxproj/setup/remove_release.props | Removes legacy uninstaller project props. |
| src/vcxproj/setup/remove_debug.props | Removes legacy uninstaller project props. |
| src/vcxproj/setup/remove_base.props | Removes legacy uninstaller project props. |
| src/vcxproj/setup/remove.vcxproj.filters | Removes legacy uninstaller project filters. |
| src/vcxproj/setup/remove.vcxproj | Removes legacy uninstaller project file. |
| src/vcxproj/salamand.sln | Removes setup/remove projects from the main solution. |
| src/setup/setup.rc | Removes legacy setup resources (custom installer UI). |
| src/setup/resource.h | Removes legacy setup resource header. |
| src/setup/remove/utils.h | Removes legacy uninstaller implementation. |
| src/setup/remove/utils.c | Removes legacy uninstaller implementation. |
| src/setup/remove/resource.h | Removes legacy uninstaller resources. |
| src/setup/remove/remove_en.rc2 | Removes legacy uninstaller localized strings. |
| src/setup/remove/remove_de.rc2 | Removes legacy uninstaller localized strings. |
| src/setup/remove/remove_cz.rc2 | Removes legacy uninstaller localized strings. |
| src/setup/remove/remove.rc2 | Removes legacy uninstaller rc2 resources. |
| src/setup/remove/remove.rc | Removes legacy uninstaller resource script. |
| src/setup/remove/remove.c | Removes legacy uninstaller implementation. |
| src/setup/remove/process.h | Removes legacy uninstaller process enumeration. |
| src/setup/remove/process.c | Removes legacy uninstaller process enumeration. |
| src/setup/remove/precomp.h | Removes legacy uninstaller PCH header. |
| src/setup/remove/precomp.c | Removes legacy uninstaller PCH source. |
| src/setup/remove/manifest.xml | Removes legacy uninstaller manifest. |
| src/setup/remove/language.h | Removes legacy uninstaller language config. |
| src/setup/remove/ctxmenu.h | Removes legacy uninstaller shell context menu helper. |
| src/setup/remove/ctxmenu.c | Removes legacy uninstaller shell context menu helper. |
| src/setup/precomp.h | Removes legacy setup PCH header. |
| src/setup/precomp.c | Removes legacy setup PCH source. |
| src/setup/manifest.xml | Removes legacy setup manifest. |
| src/setup/language.h | Removes legacy setup language config. |
| src/setup/infinst.h | Removes legacy setup INF-driven installer interface. |
| src/setup/infinst.c | Removes legacy setup INF-driven installer implementation. |
| README.md | Updates installer documentation to Inno Setup flow + CI notes. |
| Installer/x64 | Removes legacy marker file used by prior installer flow. |
| Installer/setup.iss | Adds Inno Setup installer definition and custom code hooks. |
| Installer/setup.inf | Removes legacy setup INF used by the old custom installer. |
| .github/workflows/build-installer.yml | Updates CI to stage files, install Inno Setup, compile .iss, and attach the output to a release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check for old custom uninstaller (remove.exe) | ||
| if FileExists(ExpandConstant('{app}\remove.exe')) then | ||
| begin | ||
| // Run the old uninstaller silently | ||
| if MsgBox('A previous version of Open Salamander was detected. Would you like to uninstall it first?', | ||
| mbConfirmation, MB_YESNO) = IDYES then | ||
| begin | ||
| Exec(ExpandConstant('{app}\remove.exe'), '/S', '', SW_SHOW, ewWaitUntilTerminated, ResultCode); | ||
| end; |
There was a problem hiding this comment.
CurStepChanged checks for a legacy uninstaller at '{app}\remove.exe', but DefaultDirName is now '{autopf}\Open Salamander'. If the legacy installer used a different default directory, this detection will miss most upgrades unless the user manually selects the old install folder. Consider detecting/removing the legacy install via its uninstall registry key or by probing the old default path before proceeding.
| // Check for old custom uninstaller (remove.exe) | |
| if FileExists(ExpandConstant('{app}\remove.exe')) then | |
| begin | |
| // Run the old uninstaller silently | |
| if MsgBox('A previous version of Open Salamander was detected. Would you like to uninstall it first?', | |
| mbConfirmation, MB_YESNO) = IDYES then | |
| begin | |
| Exec(ExpandConstant('{app}\remove.exe'), '/S', '', SW_SHOW, ewWaitUntilTerminated, ResultCode); | |
| end; | |
| // First try to locate old installation via its uninstall registry key | |
| if RegQueryStringValue(HKLM, | |
| 'Software\Microsoft\Windows\CurrentVersion\Uninstall\Open Salamander', | |
| 'UninstallString', OldUninstallString) then | |
| begin | |
| // Run the old uninstaller using the registered uninstall command | |
| if MsgBox('A previous version of Open Salamander was detected. Would you like to uninstall it first?', | |
| mbConfirmation, MB_YESNO) = IDYES then | |
| begin | |
| Exec(OldUninstallString, '', '', SW_SHOW, ewWaitUntilTerminated, ResultCode); | |
| end; | |
| end | |
| else | |
| begin | |
| // Fall back to checking for old custom uninstaller executable on disk | |
| if FileExists(ExpandConstant('{pf}\Open Salamander\remove.exe')) then | |
| begin | |
| // Run the old uninstaller from the legacy default path | |
| if MsgBox('A previous version of Open Salamander was detected. Would you like to uninstall it first?', | |
| mbConfirmation, MB_YESNO) = IDYES then | |
| begin | |
| Exec(ExpandConstant('{pf}\Open Salamander\remove.exe'), '/S', '', SW_SHOW, ewWaitUntilTerminated, ResultCode); | |
| end; | |
| end | |
| else if FileExists(ExpandConstant('{app}\remove.exe')) then | |
| begin | |
| // Run the old uninstaller from the current application directory | |
| if MsgBox('A previous version of Open Salamander was detected. Would you like to uninstall it first?', | |
| mbConfirmation, MB_YESNO) = IDYES then | |
| begin | |
| Exec(ExpandConstant('{app}\remove.exe'), '/S', '', SW_SHOW, ewWaitUntilTerminated, ResultCode); | |
| end; | |
| end; |
README.md
Outdated
| - Includes the latest SVG icons from `src\res\toolbars`. | ||
| - Modifies `setup.inf` (internally in the package) if necessary to ensure icons are installed. | ||
| - Produces a single `OpenSalamander_Setup.exe`. | ||
| The workflow is triggered on pushes to `master` and produces versioned installers named `OpenSalamander_5.0.{build_number}.exe`. |
There was a problem hiding this comment.
README says the build-installer workflow is triggered on pushes to master, but .github/workflows/build-installer.yml is configured for pushes to main. Update the documentation (or the workflow trigger) so they match.
| The workflow is triggered on pushes to `master` and produces versioned installers named `OpenSalamander_5.0.{build_number}.exe`. | |
| The workflow is triggered on pushes to `main` and produces versioned installers named `OpenSalamander_5.0.{build_number}.exe`. |
| param( | ||
| [string]$BuildDir = "build_stage", | ||
| [string]$StagingDir = "Installer_Staging", | ||
| [string]$OutputPath = "OpenSalamander_v5.exe" | ||
| [string]$BuildNumber = "0" |
There was a problem hiding this comment.
The script now accepts a BuildNumber parameter, but it is never referenced. Either remove the parameter (and stop passing it from CI) or use it (e.g., to stamp a version file / log it / validate staging corresponds to the intended build) so the interface stays meaningful.
Installer/setup.iss
Outdated
| // Check if salamand.exe is running | ||
| if CheckForMutexes('OpenSalamanderMutex') then | ||
| begin |
There was a problem hiding this comment.
CheckForMutexes is looking for 'OpenSalamanderMutex', but that mutex name does not exist in the codebase, so this running-instance check will never trigger. Use one of the actual mutex names created by salamand.exe (e.g., the process list mutex 'TaskscapeLtdSalamander3bProcessListMutex' created in src/tasklist.cpp) or introduce a dedicated named mutex for installer detection.
Installer/setup.iss
Outdated
|
|
||
| // Check if salamand.exe is running | ||
| if CheckForMutexes('OpenSalamanderMutex') then | ||
| begin | ||
| if MsgBox('Open Salamander is currently running. Please close it before continuing installation.' + #13#10 + #13#10 + | ||
| 'Click OK to retry or Cancel to exit setup.', mbError, MB_OKCANCEL) = IDCANCEL then | ||
| begin | ||
| Result := False; | ||
| end | ||
| else | ||
| begin | ||
| // Retry the check | ||
| Result := InitializeSetup(); |
There was a problem hiding this comment.
InitializeSetup retries by recursively calling itself. Repeated retries can grow the call stack unnecessarily; prefer a loop (repeat/until) that re-checks the mutex and exits cleanly on Cancel.
| // Check if salamand.exe is running | |
| if CheckForMutexes('OpenSalamanderMutex') then | |
| begin | |
| if MsgBox('Open Salamander is currently running. Please close it before continuing installation.' + #13#10 + #13#10 + | |
| 'Click OK to retry or Cancel to exit setup.', mbError, MB_OKCANCEL) = IDCANCEL then | |
| begin | |
| Result := False; | |
| end | |
| else | |
| begin | |
| // Retry the check | |
| Result := InitializeSetup(); | |
| // Repeatedly check if salamand.exe is running until it is closed or user cancels | |
| while True do | |
| begin | |
| // If the application is not running, continue with setup | |
| if not CheckForMutexes('OpenSalamanderMutex') then | |
| break; | |
| // Application is running; prompt user to close or cancel setup | |
| if MsgBox('Open Salamander is currently running. Please close it before continuing installation.' + #13#10 + #13#10 + | |
| 'Click OK to retry or Cancel to exit setup.', mbError, MB_OKCANCEL) = IDCANCEL then | |
| begin | |
| Result := False; | |
| break; |
No description provided.