Skip to content
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.

Sign up for GitHub

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

fix(nsis): use atomic rmdir on update #6551

Merged
merged 2 commits into from
Jan 16, 2022
Merged

fix(nsis): use atomic rmdir on update #6551

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd8770f
fix(nsis): use revertible rmdir on update
indutny-signal Jan 15, 2022
f8fe5ff
Retry uninstall
indutny-signal Jan 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/chilly-trains-study.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"app-builder-lib": patch
---

fix(nsis): use revertible rmdir on update
28 changes: 12 additions & 16 deletions packages/app-builder-lib/templates/nsis/include/extractAppPackage.nsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,28 +104,24 @@
LoopExtract7za:
IntOp $R1 $R1 + 1

# Attempt to copy files in atomic way
CopyFiles /SILENT "$PLUGINSDIR\7z-out\*" $OUTDIR
IfErrors 0 DoneExtract7za

${if} $R1 > 1
DetailPrint `Can't modify "${PRODUCT_NAME}"'s files.`
${if} $R1 < 5
# Try copying a few times before giving up
Goto LoopExtract7za
${else}
MessageBox MB_RETRYCANCEL|MB_ICONEXCLAMATION "$(appCannotBeClosed)" /SD IDCANCEL IDRETRY RetryExtract7za
${endIf}

# CopyFiles will remove all overwritten files when it encounters an
# issue and make app non-launchable. Extract over from the archive
# ignoring the failures so at least we will partially update and the
# app would start.
Nsis7z::Extract "${FILE}"
Quit
DetailPrint `Can't modify "${PRODUCT_NAME}"'s files.`
${if} $R1 < 5
# Try copying a few times before asking for a user action.
Goto RetryExtract7za
${else}
Goto LoopExtract7za
MessageBox MB_RETRYCANCEL|MB_ICONEXCLAMATION "$(appCannotBeClosed)" /SD IDCANCEL IDRETRY RetryExtract7za
${endIf}

# As an absolutely last resort after a few automatic attempts and user
# intervention - we will just overwrite everything with `Nsis7z::Extract`
# even though it is not atomic and will ignore errors.
Nsis7z::Extract "${FILE}"
Quit

RetryExtract7za:
Sleep 1000
Goto LoopExtract7za
Expand Down
3 changes: 3 additions & 0 deletions packages/app-builder-lib/templates/nsis/include/installUtil.nsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,10 @@ Function handleUninstallResult
Return

${if} $R0 != 0
MessageBox MB_OK|MB_ICONEXCLAMATION "$(uninstallFailed): $R0"
DetailPrint `Uninstall was not successful. Uninstaller error code: $R0.`
SetErrorLevel 2
Quit
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally we should retry running uninstaller (with user's confirmation) in such case instead of failing whole installation immediately. Let me know if you want me to add this to the current patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and btw. Abort doesn't quite work here for some reason (the installer just hangs), so I had to resort to Quit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny-signal This sounds like a great idea. Is it complex to add?

we should retry running uninstaller (with user's confirmation) in such case instead of failing whole installation immediately

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually already pushed it 😉

${endif}
FunctionEnd

Expand Down
2 changes: 2 additions & 0 deletions packages/app-builder-lib/templates/nsis/messages.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,5 @@ areYouSureToUninstall:
da: Er du sikker på, at du vil afinstallere ${PRODUCT_NAME}?
decompressionFailed:
en: Failed to decompress files. Please try running the installer again.
uninstallFailed:
en: Failed to uninstall old application files. Please try running the installer again.
138 changes: 131 additions & 7 deletions packages/app-builder-lib/templates/nsis/uninstaller.nsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,109 @@ Function un.onInit
!endif
FunctionEnd

Function un.atomicRMDir
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely proud of this, but it implements RMDir /r that preserves backups of all files and directories that it removed in $PLUGINSDIR\old-install.

Exch $R0
Push $R1
Push $R2
Push $R3

StrCpy $R3 "$INSTDIR$R0\*.*"
FindFirst $R1 $R2 $R3

loop:
StrCmp $R2 "" break

StrCmp $R2 "." continue
StrCmp $R2 ".." continue

IfFileExists "$INSTDIR$R0\$R2\*.*" isDir isNotDir

isDir:
CreateDirectory "$PLUGINSDIR\old-install$R0\$R2"

Push "$R0\$R2"
Call un.atomicRMDir
Pop $R3

${if} $R3 != 0
Goto done
${endIf}

Goto continue

isNotDir:
ClearErrors
Rename "$INSTDIR$R0\$R2" "$PLUGINSDIR\old-install$R0\$R2"

# Ignore errors when renaming ourselves.
StrCmp "$R0\$R2" "${UNINSTALL_FILENAME}" 0 +2
ClearErrors

IfErrors 0 +3
StrCpy $R3 "$INSTDIR$R0\$R2"
Goto done

continue:
FindNext $R1 $R2
Goto loop

break:
StrCpy $R3 0

done:
FindClose $R1

StrCpy $R0 $R3

Pop $R3
Pop $R2
Pop $R1
Exch $R0
FunctionEnd

Function un.restoreFiles
Exch $R0
Push $R1
Push $R2
Push $R3

StrCpy $R3 "$PLUGINSDIR\old-install$R0\*.*"
FindFirst $R1 $R2 $R3

loop:
StrCmp $R2 "" break

StrCmp $R2 "." continue
StrCmp $R2 ".." continue

IfFileExists "$INSTDIR$R0\$R2\*.*" isDir isNotDir

isDir:
CreateDirectory "$INSTDIR$R0\$R2"

Push "$R0\$R2"
Call un.restoreFiles
Pop $R3

Goto continue

isNotDir:
Rename $PLUGINSDIR\old-install$R0\$R2" "$INSTDIR$R0\$R2"

continue:
FindNext $R1 $R2
Goto loop

break:
StrCpy $R0 0
FindClose $R1

Pop $R3
Pop $R2
Pop $R1
Exch $R0
FunctionEnd

Section "un.install"
# for assisted installer we check it here to show progress
!ifndef ONE_CLICK
Expand All @@ -38,6 +141,34 @@ Section "un.install"

!insertmacro setLinkVars

# delete the installed files
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of operations changed as stated in the commit message. We try to keep as much state as possible in case we will need to restore the files on failure. The users would be really frustrated to see their shortcuts and file associations go if uninstall.exe --updated fails while restoring all app's files.

Copy link
Contributor Author

@indutny-signal indutny-signal Jan 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went ahead and did it. Makes for a much better user experience!

!ifmacrodef customRemoveFiles
!insertmacro customRemoveFiles
!else
${if} ${isUpdated}
CreateDirectory "$PLUGINSDIR\old-install"

Push ""
Call un.atomicRMDir
Pop $R0

${if} $R0 != 0
DetailPrint "File is busy, aborting: $R0"

# Attempt to restore previous directory
Push ""
Call un.restoreFiles
Pop $R0

Abort `Can't rename "$INSTDIR" to "$PLUGINSDIR\old-install".`
${endif}

${endif}

# Remove all files (or remaining shallow directories from the block above)
RMDir /r $INSTDIR
!endif

${ifNot} ${isKeepShortcuts}
WinShell::UninstAppUserModelId "${APP_ID}"

Expand All @@ -64,13 +195,6 @@ Section "un.install"
!insertmacro unregisterFileAssociations
!endif

# delete the installed files
!ifmacrodef customRemoveFiles
!insertmacro customRemoveFiles
!else
RMDir /r $INSTDIR
!endif

Var /GLOBAL isDeleteAppData
StrCpy $isDeleteAppData "0"

Expand Down