NXDRIVE-3133: End the running Drive task before attempting to install#6281
Open
S-Ghosh-Dev2 wants to merge 2 commits intomasterfrom
Open
NXDRIVE-3133: End the running Drive task before attempting to install#6281S-Ghosh-Dev2 wants to merge 2 commits intomasterfrom
S-Ghosh-Dev2 wants to merge 2 commits intomasterfrom
Conversation
Contributor
Reviewer's GuideUpdates the Windows Inno Setup installer to proactively terminate the running Drive process before installation and to fully clean the install directory, plus documents the change in the 6.1.0 release notes. Sequence diagram for Windows installer killing running Drive task before installationsequenceDiagram
actor User
participant WindowsInstaller
participant PrepareToInstall
participant DriveProcess
User->>WindowsInstaller: Start_installation
WindowsInstaller->>PrepareToInstall: PrepareToInstall(NeedsRestart)
PrepareToInstall->>PrepareToInstall: IsProcessRunning(MyAppExeName)?
alt Drive_not_running
PrepareToInstall-->>WindowsInstaller: ""
WindowsInstaller->>WindowsInstaller: Proceed_with_installation
else Drive_running
PrepareToInstall->>DriveProcess: taskkill /F /IM MyAppExeName
PrepareToInstall->>PrepareToInstall: Sleep(2000)
PrepareToInstall->>PrepareToInstall: IsProcessRunning(MyAppExeName)?
alt Kill_successful
PrepareToInstall-->>WindowsInstaller: ""
WindowsInstaller->>WindowsInstaller: Proceed_with_installation
else Kill_failed
PrepareToInstall-->>WindowsInstaller: Error_message_requesting_manual_close
WindowsInstaller-->>User: Show_error_and_abort_installation
end
end
WindowsInstaller->>WindowsInstaller: Delete_existing_files_in_app_dir
WindowsInstaller->>WindowsInstaller: Install_new_files
Flow diagram for PrepareToInstall logic in the Windows installerflowchart TD
Start["PrepareToInstall called"] --> Init["Set Result to empty string"]
Init --> CheckRunning["IsProcessRunning(MyAppExeName)?"]
CheckRunning -->|No| ExitEarly["Exit function (no restart needed)"]
ExitEarly --> End
CheckRunning -->|Yes| KillProc["Exec taskkill /F /IM MyAppExeName"]
KillProc --> Wait["Sleep 2000 ms"]
Wait --> Recheck["IsProcessRunning(MyAppExeName)?"]
Recheck -->|No| Success["Leave Result empty and continue installation"]
Success --> End
Recheck -->|Yes| Fail["Set Result to error message asking user to close app"]
Fail --> End
End["Return Result to installer"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
PrepareToInstall, the comment// Force-kill the processon the earlyExitis misleading (that path actually does nothing), which may confuse future maintainers; either remove or update the comment and consider explicitly documenting the intended control flow when the process is not running. - The
IsProcessRunning/taskkillcommands build shell arguments by concatenating the process name without quoting, which will break if{#MyAppExeName}ever contains spaces or special characters; it would be safer to wrap the process name in quotes and avoid relying on shell parsing where possible. - Changing
[InstallDelete]toName: "{app}\*"will delete everything under the install directory before upgrade; double‑check this path contains only application binaries and not user-specific or shared data, and if needed narrow the pattern to just the known runtime artifacts to minimize accidental deletion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PrepareToInstall`, the comment `// Force-kill the process` on the early `Exit` is misleading (that path actually does nothing), which may confuse future maintainers; either remove or update the comment and consider explicitly documenting the intended control flow when the process is not running.
- The `IsProcessRunning`/`taskkill` commands build shell arguments by concatenating the process name without quoting, which will break if `{#MyAppExeName}` ever contains spaces or special characters; it would be safer to wrap the process name in quotes and avoid relying on shell parsing where possible.
- Changing `[InstallDelete]` to `Name: "{app}\*"` will delete everything under the install directory before upgrade; double‑check this path contains only application binaries and not user-specific or shared data, and if needed narrow the pattern to just the known runtime artifacts to minimize accidental deletion.
## Individual Comments
### Comment 1
<location> `tools/windows/setup-common.iss:53` </location>
<code_context>
+; Delete everything inside the installation directory before installing new files.
+; This ensures no orphaned files from previous versions cause conflicts (e.g.,
+; "DLL load failed while importing QtCore" errors due to stale DLLs).
+Type: filesandordirs; Name: "{app}\*"
</code_context>
<issue_to_address>
**question (bug_risk):** Deleting everything under {app} on install can remove user-modified or runtime-generated files that were previously preserved.
Changing from targeted DLL cleanup to `Type: filesandordirs; Name: "{app}\*"` will now wipe everything under `{app}` on upgrade, including any user data, logs, or config placed there. Please either (a) confirm that `{app}` never holds user-touched data (and that it all lives elsewhere, e.g. `%APPDATA%`), or (b) explicitly exclude any user-data subdirectories/files from this cleanup to avoid accidental data loss.
</issue_to_address>
### Comment 2
<location> `docs/changes/6.1.0.md:28` </location>
<code_context>
- [NXDRIVE-3108](https://hyland.atlassian.net/browse/NXDRIVE-3108): Fix MacOS release version checking on Github
- [NXDRIVE-3110](https://hyland.atlassian.net/browse/NXDRIVE-3110): Update timeout on macOS
- [NXDRIVE-3122](https://hyland.atlassian.net/browse/NXDRIVE-3122): Fix upgrade issue from 5.4.0 to latest
</code_context>
<issue_to_address>
**suggestion (typo):** Consider correcting "MacOS" to "macOS" for consistency and accuracy.
Here, "MacOS" should match the "macOS" spelling used below to stay consistent with Apple’s official naming.
```suggestion
- [NXDRIVE-3108](https://hyland.atlassian.net/browse/NXDRIVE-3108): Fix macOS release version checking on Github
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR addresses NXDRIVE-3133 by ensuring that the running Nuxeo Drive process is terminated before installation proceeds. The changes replace Inno Setup's built-in application closure mechanism with a custom implementation that forcefully kills ndrive.exe if it's running.
Changes:
- Disabled Inno Setup's automatic "close applications" prompt and replaced it with custom process termination logic
- Simplified the InstallDelete section to remove all files in the installation directory (replacing specific file patterns)
- Added two new functions:
IsProcessRunningto check if ndrive.exe is active, andPrepareToInstallto terminate the process before installation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/windows/setup-common.iss | Implements custom process termination logic and simplified cleanup, disables built-in close applications prompt |
| docs/changes/6.1.0.md | Adds changelog entry for NXDRIVE-3133 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+143
to
+145
| Exit; // Force-kill the process | ||
|
|
||
| Exec('taskkill', '/F /IM {#MyAppExeName}', '', SW_HIDE, ewWaitUntilTerminated, ResultCode); |
There was a problem hiding this comment.
The logic in this function is inverted. The comment says "Force-kill the process" but it actually exits early when the process is NOT running. This means when the process IS running (line 142 evaluates to false), the code continues to lines 145-150 to kill it. However, the comment placement makes it confusing and suggests incorrect logic. The early exit should occur when the process is already stopped (not running), which is the correct behavior but the comment is misleading.
Suggested change
| Exit; // Force-kill the process | |
| Exec('taskkill', '/F /IM {#MyAppExeName}', '', SW_HIDE, ewWaitUntilTerminated, ResultCode); | |
| Exit; // Process is not running; nothing to do | |
| Exec('taskkill', '/F /IM {#MyAppExeName}', '', SW_HIDE, ewWaitUntilTerminated, ResultCode); // Force-kill the process |
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
1 participant
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 by Sourcery
Ensure the Windows installer safely stops any running Nuxeo Drive process before upgrading and simplifies cleanup of previous installations.
Bug Fixes:
Enhancements:
Documentation: