Skip to content

Bugfix/version windows integration improve install - #8178

Merged
mgallien merged 6 commits into
masterfrom
bugfix/versionWindowsIntegrationImproveInstall
May 13, 2025
Merged

Bugfix/version windows integration improve install#8178
mgallien merged 6 commits into
masterfrom
bugfix/versionWindowsIntegrationImproveInstall

Conversation

@mgallien

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread admin/win/msi/Nextcloud.wxs Outdated
Comment thread VERSION.cmake Outdated
set(MIRALL_SOVERSION 0)
set(MIRALL_PREVERSION_HUMAN "3.17.0 alpha") # For preversions where PATCH>=50. Use version + alpha, rc1, rc2, etc.
set(WINDOWS_INTEGRATION_VERSION 4)
set(WINDOWS_INTEGRATION_SOVERSION 4)

@nilsding nilsding Apr 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

where does the 4 come from?

NCExt/Version.h defines the NCEXT_BUILD_NUM constant with 47 that is part of the NCEXT_VERSION constant

and how does setting these as target properties differ from having it be defined in the .rc files? e.g.

FILEVERSION NCEXT_VERSION
PRODUCTVERSION NCEXT_VERSION

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fwiw I didn't see anything different on the resulting DLLs with these two settings

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes
that is a leftover
I can double check but I probably have a stash upgrading the DLL embedded version number from a more centralized working place

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

adding the version(s) of the shell integration to the centralised VERSION.cmake file is a good idea :>

would be cool if you still have the stash somewhere. otherwise I can take care of that as well

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

no
I will do it again

@nilsding

nilsding commented May 7, 2025

Copy link
Copy Markdown
Member

first round of testing with omus -- I built the MSI twice which allows me to test what an upgrade will be like. I opened up an explorer window (with the overlay icons visible), and looked at the loaded DLLs via Process Explorer.

an upgrade from an install without this change still asked for Windows Explorer to exit. I'd say this is expected though

I noticed this during an upgrade from an install with this change:

  • at first, the installer asks only for Nextcloud to quit. NCOverlays.dll is loaded by explorer.exe
    Screenshot_20250507_123931
  • after accepting that, it wants to restart Windows Explorer again
    Screenshot_20250507_123951
  • accepted that one again. the Explorer window disappeared and NCOverlays.dll no longer showed up in the list of loaded DLLs. the DLL got moved to C:\Config.Msi\ by the installer and is now called 1a2e5f.rbf. However the explorer.exe process never quits so it won't be restarted by the installer again...
    Screenshot_20250507_124047

I'll try to dig further into this

@nilsding
nilsding force-pushed the bugfix/versionWindowsIntegrationImproveInstall branch from 543cc4a to 5310707 Compare May 7, 2025 14:49
@nilsding

nilsding commented May 7, 2025

Copy link
Copy Markdown
Member

...well well well :D

I read some more documentation about the Windows Installer itself and this led me to try out a different value for MajorUpgrade.

I created two new MSIs which use that value: one for the client version 3.16.50 and another one for 3.16.51.

Upgrading the installation from e.g. 3.16.3 to 3.16.50 will still request a restart of Windows Explorer. But once that new version is installed, a future upgrade to 3.16.51 only wants to try to restart Nextcloud itself (if it's running). This is the only restart dialog that appears during the entire installation -- and the extension stays loaded by explorer.exe

Screenshot_20250507_164604

In my tests this also worked when downgrading from 3.16.51 to 3.16.50.

Of course, when uninstalling Nextcloud the installer still wants to restart Windows Explorer. The installer still asks for a reboot after a successful installation/upgrade as well.

@nilsding
nilsding force-pushed the bugfix/versionWindowsIntegrationImproveInstall branch from 5310707 to 3733fcb Compare May 8, 2025 12:31
@nilsding

nilsding commented May 8, 2025

Copy link
Copy Markdown
Member

The last commit affects the way the Restart Manager handles the termination of processes.

Previously the Windows Explorer process would not terminate completely, after ~30s the installer displays an error message about that, and Explorer would never come back again.
With that change the installer now forces the process to exit after that time. After the installation the Windows Explorer finally reappears 🎉

Comment thread shell_integration/windows/NCContextMenu/CMakeLists.txt Outdated
@mgallien
mgallien force-pushed the bugfix/versionWindowsIntegrationImproveInstall branch 3 times, most recently from 14cca8e to c0e0d57 Compare May 12, 2025 10:10
Comment thread src/libsync/vfs/cfapi/shellext/CMakeLists.txt
@mgallien
mgallien force-pushed the bugfix/versionWindowsIntegrationImproveInstall branch 2 times, most recently from f07b112 to ac3131b Compare May 12, 2025 15:02
@Rello Rello moved this to ☑️ Done in 💻 Desktop Clients team May 13, 2025
mgallien and others added 5 commits May 13, 2025 17:18
shoudl enable replacing files during install happen only when needed
instead of always

will need to take care of migration from older releases

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Jyrki Gadinger <nilsding@nilsding.org>
…ating

The MSIs we distribute perform so-called "major upgrades".

From the Microsoft docs, we learn that...

> A typical major upgrade **removes a previous version** of an
> application and installs a new version.

Indeed that was the case -- the previous version is uninstalled
completely before the new version gets installed.  Since we install an
extension that's loaded by explorer, the installer will ask the user to
restart Windows Explorer during the uninstallation of the previous
version (and restarting that might not always work as expected).

There is a way to change this behaviour though, using Wix3's
`MajorUpgrade` element.  Through its `Schedule` property it's possible
to define when the removal of the previous version should occur.

The previous value `afterInstallInitialize` is one of two values that
perform a complete uninstall/install sequence.  The new value
`afterInstallExecute` will install new files on top of the previous
installation (depending on the `REINSTALLMODE` I guess) and remove
older files that are not present.
In the case of the extensions this means that the .dlls from the
previous installation will not be touched, as long as the version
embedded in the DLL info didn't change.  This results in the installer
no longer requesting Windows Explorer to restart, which is a big plus :D

I also changed the REINSTALLMODE to `dmus` to allow downgrades as well.

With this change, the request to restart Windows Explorer should
hopefully happen only once an installation was made using an MSI that
contains this change.  Further updates will only request a restart if
the DLL version of these extensions change...

See also:
  - https://learn.microsoft.com/en-us/windows/win32/msi/major-upgrades
  - https://docs.firegiant.com/wix/schema/wxs/majorupgrade/#attributes:~:text=Schedule

Signed-off-by: Jyrki Gadinger <nilsding@nilsding.org>
Stopping the Windows Explorer process does not always work.  Per default
the MSI Restart Manager will display a warning message after a timeout,
and not attempt to restart Explorer afterwards -- resulting in a black
screen if no other window is open.

Setting the property `MSIRMSHUTDOWN` to `1` will force the termination
of the process when the timeout is reached, which finally allows the
Explorer to be restarted.

Signed-off-by: Jyrki Gadinger <nilsding@nilsding.org>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
let's have the MSI RestartManager take care of that

Signed-off-by: Jyrki Gadinger <nilsding@nilsding.org>
@mgallien
mgallien force-pushed the bugfix/versionWindowsIntegrationImproveInstall branch from ac3131b to d28b8e3 Compare May 13, 2025 15:21
@github-actions

Copy link
Copy Markdown
Contributor

Artifact containing the AppImage: nextcloud-appimage-pr-8178.zip

SHA256 checksum: 6b92c6808f0be2cdbd97c21eb77168c3a6afcd1fc03d89f9516babbdad20dfcd

To test this change/fix you can download the above artifact file, unzip it, and run it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarqubecloud

Copy link
Copy Markdown

@mgallien
mgallien merged commit 8ee4f47 into master May 13, 2025
@mgallien
mgallien deleted the bugfix/versionWindowsIntegrationImproveInstall branch May 13, 2025 16:29
@nilsding

Copy link
Copy Markdown
Member

/backport to stable-3.16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants