-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(nsis): terminate only processes running in installation folder #9069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: d82b90a The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
46904f1
to
370ff74
Compare
@beyondkmp would you mind taking a look at this when you have a chance? |
packages/app-builder-lib/templates/nsis/include/allowOnlyOneInstallerInstance.nsh
Outdated
Show resolved
Hide resolved
50e8b45
to
d937699
Compare
@panther7 following up here, what tests have you run so far? Wondering if we can create an install+update test somehow for this in GHA runner? Similar to how this works: https://github.com/electron-userland/electron-builder/pull/9113/files?diff=unified&w=1#diff-1d9441830c74b56db3ecc46ef9805c4519141c15e145fabb24159d86e49d526dR10-R31 |
Working on a full "blackbox" test suite for verifying installation and updater flows with native/non-mocked Updaters Will get that functional and then pull it into this PR for you so that we can add a black-box test for this |
523ac51
to
c73d47a
Compare
c73d47a
to
e34c1c8
Compare
e34c1c8
to
309b2e5
Compare
After hard testing, done. BUT, only works on powershell 7+, because powershell 5 have "bug" and sometimes are Anyway, |
0540a91
to
77b294b
Compare
I found solution for PowerShell 5+. It must be use Check fixup. |
Nice work @panther7 !! I need to do some hard testing on my side just to sanity check. Is there a way to force whether powershell or cmd approach is used for app termination? Just disable powershell via policy? |
!else | ||
StrCpy $CmdPath "$SYSDIR\cmd.exe" | ||
StrCpy $FindPath "$SYSDIR\find.exe" | ||
StrCpy $PowerShellPath "$SYSDIR\WindowsPowerShell\v1.0\powershell.exe" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmaietta You can edit this to StrCpy $PowerShellPath "blablablaexe"
if you want "disable" powershell.
Modified the process termination logic in NSIS scripts to only kill application processes that are running from the installation directory. This prevents accidental termination of processes with the same name running from other locations during installation or update.
6fb2de4
to
d82b90a
Compare
Modified the process termination logic in NSIS scripts to only kill application processes that are running from the installation directory. This prevents accidental termination of processes with the same name running from other locations during installation or update.
issue: #9009
EDIT:
Actually works only on PowerShell Core (7+), because (default) powershell 5.1 is broken, because#9069 (comment)Get-Process
returns emptyPath
for processes.