[innosetup] Add code to uninstall the installed version when upgrading #19769
Conversation
|
Let's not merge this right before release. There's no chance to test it and I'd rather not spring it on the users blindly, let alone find out there is a bug that we didn't have a chance to catch. I'm planning on building with the deprecated installer, since I haven't been able to find build instructions for the new installer, even though I've asked (#19544). The weekly builds (darktable windows insider program on pixls.us) serve 2 purposes. One is to get windows users to test. The seconds is to verify the build and installer work correctly. |
@wpferguson: This is actually a great idea. Although nightly builds are published on the GitHub releases page, some forum users probably don't have the habit of checking there for fresh builds. So your weekly builds definitely attract additional users to test. The more users who test the builds, the more bugs will be found and fixed. |
I will definitely write these instructions for the end user and I promise that it will happen before the release. At the time of your request and even now I still have hope for the innosetup package to be included in the MSYS2 package base (maybe Milos would find time for it, as I hope, maybe I would figure out how to do it...), and that would definitely change (and simplify) the instructions. In the meantime, I'm always quietly lurking in darktable Matrix room and ready to answer any questions you may have about the installer. BTW, the "Create Inno Setup installer" step in the nightly build task is pretty self-contained in terms of what is needed to build the Inno Setup installer. See: darktable/.github/workflows/nightly.yml Lines 289 to 298 in 1c51391 Since Inno Setup installer is not yet packaged in MSYS2, we need to install it on the Windows host system, copy darktable.iss installer script to the directory where CMake installed the files, create a multi-resolution icon for darktable installer executable, and run the darktable.iss script compilation. EDIT: I recalled that choco adds iscc to the PATH. So when installing Inno Setup not through choco, such as using installer from the program's website, you will need to specify the full path to iscc.exe. If we are talking about a non-automated, interactive build, then perhaps the easiest way is to click on the .iss file. This will launch the interactive compiler associated with this extension, in which you need to press Ctrl+F9 to start the compilation. |
@wpferguson: It's up to you to decide as the person in charge of creating the official release for Windows. There is no rush, the new installer has several advantages, but if we do not have a developer who is willing to review and test and we do not trust the user testing of nightly builds, then it maybe indeed be better to postpone its use in the official build until later. However, I would like to emphasize that the extra time will only bring us somewhat more user testing. If we don't trust this, then what are we waiting for?
But, if you don't review/test this code and are against merging this code, how can it be tested? I think it's obvious that improving the code of the new installer is a separate process from deciding which installer to use in the next official release. For my part, I undertake to write a lengthy text about why uninstalling the previous installation is safer than just upgrading to the same place, and a testing plan, as well as explaining what and why is done in my code in the Inno Setup scripting language (which is essentially Pascal, BTW). |
I'm not against merging it. I'm against merging it NOW. We are in feature freeze and this isn't a bug fix, it's an enhancement. How does merging this code now make the release better, versus merging at the start of 5.5.? Let's merge it at the start of 5.5 and then transition the weekly builds to it. That way users have a chance to get used to the differences, whatever they may be, it gets tested, and I have time to figure out how to fit it into my build process. The new installer also introduces new packages to the build machine (chocolatey, innosetup). I build on a VM and it has the absolute minimum necessary to build darktable installed and nothing else just to minimize the chance of anything infecting the machine and getting passed on to users.
The current installer let's you do that (and I always do :-) ). |
|
@wpferguson: I have the impression that you didn't see my comment that I don't suggest making an installer for 5.4 using Inno Setup. I copy it below:
This PR is not about the build of the next release at all, so I don't understand why you're explaining your opposition to this PR by saying what would be better for the release...
In fact, on the contrary, it should be merged as early as possible. The sooner this is done, the more likely it is that early testers will report any flaws that may exist.
It's irrelevant how we classify it, it's not a change in darktable code and it has nothing to do with the release, so our release preparation traditions don't apply to it.
This is a wrong question. Precisely because merging this code has no impact on the release, there is no reason to block it just because the release is coming soon. |
|
@victoryforce : I thought this would affect the 5.4 release, no? Or are you saying that @wpferguson binaries won't be affected? Please bear with me, I'm not a Windows user and I'm a bit lost about the way it is distributed for our releases. My point is that if this does not affect 5.4 installer it should be merged now otherwise it should be merged after the 5.4 release is made. Please @victoryforce & @wpferguson can you clarify? TIA. |
|
I guess that maybe I don't understand the "we are in feature freeze, only bug fixes will be accepted". I've learned the hard way, both first and second hand, that this is a really good policy to adhere to. If you don't merge code that you don't need for the release, then no problems can arise from it. If you do merge code that shouldn't affect the release... As I said, I've leaned that lesson the hard way. I know next to nothing about the new installer, I do know that the current installer is marked as deprecated, so I'll have to rename the generated package to that users don't get confused. @TurboGit, @victoryforce you guys hash it out. I can,build with the current "deprecated" installer just fine. @TurboGit, this is why we pay you the big bucks ;-D |
The purpose of renaming the new installer to have the default name was to encourage more nightly users to try the new installer, as we need to test it. According to the download statistics, it has been downloaded more often than when the installer had the "non-default" name. If this is a problem, I can roll back this change.
I also think this is a really good policy. All I'm trying to explain is that this PR does not violate the policy in any way. I have to repeat myself: this PR does not make any changes to the darktable code and has absolutely no impact on the release, as none of the code changed in this PR will be used in the next release. |
3 participants
We should suggest to users that they uninstall the previous installation before upgrading. This is the right way to go, as a simple in-place upgrading may preserve files from the previous version in the new installation if they are not present in the new one. This can bring surprises if the behavior of the program depends on the presence or absence of certain files (the most obvious example is translation files).
This code handles upgrades from a previous version installed by both the NSIS installer and the Inno Setup installer. We currently install development snapshots and releases into different directories by default, so they are treated as separate applications. Upgrading a development snapshot will not prompt you to uninstall a release, and vice versa.