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.

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

Application updates #66

Open
takluyver opened this issue May 26, 2016 · 18 comments
Open

Application updates #66

takluyver opened this issue May 26, 2016 · 18 comments

Comments

@takluyver
Copy link
Owner

I've mostly ignored the question of updating applications. Running a new version of the installer will presumably overwrite existing files and update existing folders, so it should mostly work, although old files will not be removed. At a minimum, we should investigate and check this behaviour.

Another super simple approach would be to look for an existing uninstall.exe and run that from the installer before installing a new version; this is not very efficient.

There are more advanced possibilities like delivering patch updates, but I doubt that's worth the added complexity at the moment.

Should Pynsist also provide any support for 'check for updates' features inside applications?

@takluyver
Copy link
Owner Author

Potentially useful pieces:

  • The Update Framework (TUF) focuses on securely checking for and downloading updates (doesn't handle applying them).
  • Esky is an update framework for frozen Python applications. I don't think Pynsist can use it directly, but there may be useful ideas/code in there.

@prc33
Copy link

prc33 commented Aug 30, 2016

I don't have any concrete steps to reproduce this, but I have seen examples where running a new version of the installer on top of an existing installation does not work correctly, whilst a clean installation does. On both occasions, the updated installer has contained a newer version of simplejson than the existing installation. Manually removing the pyc files for simplejson module from the installation directory (without running the installer again) fixes this. I have not double checked but I believe running the installer again after this is also ok.

@Siecje
Copy link
Contributor

Siecje commented Jan 14, 2017

If the application updates itself and adds new files, will uninstalling still remove all of the files?

@takluyver
Copy link
Owner Author

@prc33 that sounds like it's not compiling the .pyc files correctly. I can imagine that may be the case if the timestamps of the .pyc files (from when it was first installed) are newer than the timestamps of the new .py files.

@Siecje yes, I think so. The new install will rewrite the uninstaller. If the new version has fewer files, then it's possible for the files that were only in the earlier version to be missed on uninstall. But Pynsist mostly tries to remove entire directories to minimise that problem.

@Siecje
Copy link
Contributor

Siecje commented Jan 16, 2017

I meant without using a new installer, just replacing and adding files in the install directory. But it sounds like it just deletes the install directory so it shouldn't be a problem.

@takluyver
Copy link
Owner Author

takluyver commented Jan 16, 2017 via email

@SecT0uch
Copy link

SecT0uch commented Feb 7, 2019

I actually like this behaviour because my app writes a .conf file in the install directory. It allows to preserve the configuration across updates.
A solution would be to add an option to remove everything, including configurations in the uninstaller.

@takluyver
Copy link
Owner Author

In general, it's probably not advisable to write config in your application directory. If a user installs it systemwide ('for all users'), the application may not be able to write files in its install directory. There are other places you can store config. See the Note here: https://pynsist.readthedocs.io/en/latest/faq.html#using-data-files

@SecT0uch
Copy link

SecT0uch commented Feb 7, 2019

That's the reason why I created a custom .nsi file wich install my app in C:\MyApp. I wanted it systemwide with the ability for my users to write in.
I admit it's not the best practice
Guess I will have to keep a track on this issue to avoid future issues due to deletion of the config

@takluyver
Copy link
Owner Author

Why not save config and user created data elsewhere? There are folders for these purposes already, like %APPDATA%.

@SecT0uch
Copy link

SecT0uch commented Feb 8, 2019

You're right, just moved my configuration to %PROGRAMDATA% (for all users).

@andreymal
Copy link

old files will not be removed

Yes, this case is important. For example, if an application discovers its submodules (e.g. Django automatically loads models.py, admin.py etc. if they are present) and if some module was removed in newer version, it will not removed after upgrade, and the old version of the module will automatically imported, and the app will not work. Old files should be removed.

Fun fact: pip had a similar bug a few years ago.

@fohrloop
Copy link
Contributor

Another super simple approach would be to look for an existing uninstall.exe and run that from the installer before installing a new version; this is not very efficient.

+1 for this. It's much better than nothing, and a good starting point!

There could be an entry for this in the config file:

[Installer]
uninstall_old=False

or something similar.

@bastimeyer
Copy link
Contributor

I've been working on rewriting our project's Windows installer build config and came accross this issue which we've been working around with custom logic that overwrites files with empty content, which is far from ideal, so I was checking whether the issue has been fixed so I don't have to re-implement this ugly workaround. Apparently the issue is still not resolved.

According to the linked issues / pull requests on this thread, the certbot repo has applied a fix for that issue in this pull request:
certbot/certbot#8836

This change adds

RMDir /r "$INSTDIR\pkgs"

at the start of the sec_app section of the template's sections block, and what it does is cleaning up the entire pkgs directory before installing, which fixes the issue when running the installer without uninstalling first. Unfortunately, certbot is completely rewriting their NSIS template and can therefore easily add this command at this specific line because no template block exists for custom overrides at this part of the section, similar to install_files for example.

This means that one of these three solutions need be implemented here in order to fix the issue for everyone:

  1. Add another template block called install_pkgs, so that the initial pkgs copy commands can be extended with custom logic and the RMDir command can be prepended
    diff --git a/nsist/pyapp.nsi b/nsist/pyapp.nsi
    index 279a8c5..d049d3d 100644
    --- a/nsist/pyapp.nsi
    +++ b/nsist/pyapp.nsi
    @@ -66,8 +66,10 @@ Section "!${PRODUCT_NAME}" sec_app
       SetRegView [[ib.py_bitness]]
       SectionIn RO
       File ${PRODUCT_ICON}
    -  SetOutPath "$INSTDIR\pkgs"
    -  File /r "pkgs\*.*"
    +  [% block install_pkgs %]
    +    SetOutPath "$INSTDIR\pkgs"
    +    File /r "pkgs\*.*"
    +  [% endblock install_pkgs %]
       SetOutPath "$INSTDIR"
     
       ; Marker file for per-user install
  2. Add an option for this to the pynsist config and run the RMDir command if the option's value is true
    diff --git a/nsist/pyapp.nsi b/nsist/pyapp.nsi
    index 279a8c5..8627aa9 100644
    --- a/nsist/pyapp.nsi
    +++ b/nsist/pyapp.nsi
    @@ -66,6 +66,9 @@ Section "!${PRODUCT_NAME}" sec_app
       SetRegView [[ib.py_bitness]]
       SectionIn RO
       File ${PRODUCT_ICON}
    +  [% if clean_pkgs %]
    +    RMDir /r "$INSTDIR\pkgs"
    +  [% endif %]
       SetOutPath "$INSTDIR\pkgs"
       File /r "pkgs\*.*"
       SetOutPath "$INSTDIR"
  3. Always try to remove the pkgs dir before installing the files.
    diff --git a/nsist/pyapp.nsi b/nsist/pyapp.nsi
    index 279a8c5..f46f310 100644
    --- a/nsist/pyapp.nsi
    +++ b/nsist/pyapp.nsi
    @@ -66,6 +66,7 @@ Section "!${PRODUCT_NAME}" sec_app
       SetRegView [[ib.py_bitness]]
       SectionIn RO
       File ${PRODUCT_ICON}
    +  RMDir /r "$INSTDIR\pkgs"
       SetOutPath "$INSTDIR\pkgs"
       File /r "pkgs\*.*"
       SetOutPath "$INSTDIR"

@takluyver Could you please give me your opinion on this before I open a PR?

@takluyver
Copy link
Owner Author

Thanks for the information. :-)

I'm least keen on option 2 (adding a config parameter). Between 1 and 3, I'm less sure - I might go for 3, as this is the same command run by the uninstaller, and doesn't seem to have caused any problems yet. I seem to remember reading somewhere that your uninstaller shouldn't just delete $INSTDIR because it might be shared by multiple applications, but in practice I think each application usually goes in its own directory (this is certainly what Pynsist aims for).

I guess it's worth noting that removing $INSTDIR/pkgs covers most of this question - that's where most of the installed files are - but not all of it. E.g. if the newer version has fewer start menu entries, or fewer scripts, the old ones will not be deleted. Running the uninstall.exe from the target folder might do this more comprehensively, though it's slower and has more room for something to go wrong. 🤷

@takluyver
Copy link
Owner Author

I should probably add that I don't have much time to spend on Pynsist these days, and I'm not currently using it myself. I haven't abandoned it, but so far as possible it's ticking over in a fairly low maintenance mode.

@bastimeyer
Copy link
Contributor

I seem to remember reading somewhere that your uninstaller shouldn't just delete $INSTDIR because it might be shared by multiple applications

The correct behavior would be capturing a list of all files written by the installer and deleting each file and directory explicitly in the uninstaller in reverse (which is what a package manager on sane OSes would do). Recursive deletion is dangerous when done on the root install dir. I've already done this mistake in the past in a different non-pynsist NSIS build script and caused one of my users to nuke their entire programs dir, because they changed the install location and didn't install it in a separate directory, which is what I didn't think about. For the pkgs subdirectory, this is however not too big of a concern.

Regarding additional files written by the installer, this can probably only be fixed by running the (previously created) uninstaller with an explicit files+dirs list, as just mentioned. But since I'm not affected by this issue, my solution here only focuses on the pkgs subdir, which causes issues when python modules get loaded dynamically, hence why we need to overwrite certain files with empty content in our case.

I would prefer adding a simple template block for now and letting pynsist users override/extend it themselves. Then they can decide themselves if they want a recursive delete command in their installer. Would that be okay?

@takluyver
Copy link
Owner Author

I would prefer adding a simple template block for now and letting pynsist users override/extend it themselves. Then they can decide themselves if they want a recursive delete command in their installer. Would that be okay?

That's fine. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants