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

force clean-up of temporary extract dir #56

Open
dennisvang opened this issue Nov 18, 2022 · 1 comment · May be fixed by #91
Open

force clean-up of temporary extract dir #56

dennisvang opened this issue Nov 18, 2022 · 1 comment · May be fixed by #91
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@dennisvang
Copy link
Owner

dennisvang commented Nov 18, 2022

Background

The tufup client extracts new update archives to a temporary directory. By default, on Windows, this is a per-user temp dir: AppData\Local\Temp\tufup

Tufup then uses a batch script to move the extracted files to the installation directory. The temporary dir is then assumed to be empty, and is left in place for future use.

Note that Windows does not automatically clear content of its default temporary directories.

Problem

The problem with this approach is that any files that are left-over in the tufup temporary dir (for whatever reason), will get moved along with newly extracted archives. This can happen e.g. when files are not moved because they already exist in the install dir.

This can lead to stale files cluttering the new install directory, as mentioned here. This is not a problem per se, but can be confusing, and may lead to unexpected issues in the future.

How to reproduce

Run an update with the tufup-example. After the update, some files that have not changed between app versions have not been moved, so they remain in the AppData\Local\Temp\tufup.

These files will be moved along with future update files, even if the app content has changed completely.

Proposed solution

The best solution would be to properly clean up the tufup temp dir after moving the files.

However, users may override the install process, so we cannot always be sure the tufup temp dir has been cleaned up.

Therefore, we should also make sure the tufup temp dir is empty before extracting a new archive.

@dennisvang dennisvang added the enhancement New feature or request label Nov 18, 2022
@dennisvang dennisvang self-assigned this Nov 18, 2022
@dennisvang dennisvang added the bug Something isn't working label Nov 18, 2022
@dennisvang
Copy link
Owner Author

dennisvang commented Nov 8, 2023

Some additional considerations:

  • Although the default extract dir is a temp dir, it can easily be overridden by the user via the Client's extract_dir argument
  • Deleting existing files from the user's system is always scary, because we do not want to accidentally remove unrelated files (user may have specified an improper extract_dir, or may have otherwise messed with install process and/or paths in such a way that this becomes a possibility)
  • A similar risk applies to the app_install_dir, see e.g. All files in same folder deleted #38
  • Perhaps the above could be mitigated using a manifest of some sort: only remove files that are in the manifest
  • Alternatively, if we hard-code the name of the tufup temp dir, then clear the dir by that name, it might be relatively safe. Actually we already use a namespaced temp dir, although the user might have overridden this:
    DEFAULT_EXTRACT_DIR = pathlib.Path(tempfile.gettempdir()) / 'tufup'
  • In any case, we should probably make clearing the temp dir optional, to prevent surprises for existing users.

The basic approach of extracting to an intermediate directory (extract_dir) before moving the files into place (app_install_dir) was based on PyUpdater's implementation.

It would probably be a lot simpler to extract directly into the app_install_dir, not using an extract_dir at all. Downside is that this would be more fragile, so we would need to consider rollback options, which would complicate matters again.

@dennisvang dennisvang linked a pull request Nov 8, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant