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

Check if Include folders/files do exists (in case they are removed) #1718

Draft
wants to merge 20 commits into
base: dev
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGES
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
Back In Time

Version 1.4.4-dev (development of upcoming release)
* Fix bug: Check if Include folders/files do exists (in case they are removed) (#1586) (@rafaelhdr)
* Fix: Fix Qt segmentation fault with uninstall ExtraMouseButtonEventFilter when closing main window (#1095)
* Feature: Warn if Cron is not running (#1747)
* Build: Enable PyLint rules (#1755)
Expand Down
30 changes: 30 additions & 0 deletions common/backintime.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@

parsers = {}


def takeSnapshotAsync(cfg, checksum = False):
"""
Fork a new backintime process with 'backup' command which will
Expand Down Expand Up @@ -82,6 +83,7 @@ def takeSnapshotAsync(cfg, checksum = False):
pass
subprocess.Popen(cmd, env = env)


def takeSnapshot(cfg, force = True):
"""
Take a new snapshot.
Expand All @@ -98,6 +100,7 @@ def takeSnapshot(cfg, force = True):
ret = snapshots.Snapshots(cfg).backup(force)
return ret


def _mount(cfg):
"""
Mount external filesystems.
Expand All @@ -113,6 +116,7 @@ def _mount(cfg):
else:
cfg.setCurrentHashId(hash_id)


def _umount(cfg):
"""
Unmount external filesystems.
Expand All @@ -125,6 +129,7 @@ def _umount(cfg):
except MountException as ex:
logger.error(str(ex))


def createParsers(app_name = 'backintime'):
"""
Define parsers for commandline arguments.
Expand Down Expand Up @@ -620,6 +625,7 @@ def join(args, subArgs):

return args


def printHeader():
"""
Print application name, version and legal notes.
Expand All @@ -633,6 +639,7 @@ def printHeader():
print("under certain conditions; type `backintime --license' for details.")
print('')


class PseudoAliasAction(argparse.Action):
"""
Translate '--COMMAND' into 'COMMAND' for backwards compatibility.
Expand All @@ -659,6 +666,7 @@ def __call__(self, parser, namespace, values, option_string=None):
setattr(namespace, 'replace', replace)
setattr(namespace, 'alias', alias)


def aliasParser(args):
"""
Call commands which where given with leading -- for backwards
Expand All @@ -676,6 +684,7 @@ def aliasParser(args):
if 'func' in dir(newArgs):
newArgs.func(newArgs)


def getConfig(args, check = True):
"""
Load config and change to profile selected on commandline.
Expand Down Expand Up @@ -713,6 +722,7 @@ def getConfig(args, check = True):
cfg.forceUseChecksum = args.checksum
return cfg


def setQuiet(args):
"""
Redirect :py:data:`sys.stdout` to ``/dev/null`` if ``--quiet`` was set on
Expand All @@ -734,6 +744,7 @@ def setQuiet(args):
atexit.register(force_stdout.close)
return force_stdout


class printLicense(argparse.Action):
"""
Print custom license
Expand All @@ -746,6 +757,7 @@ def __call__(self, *args, **kwargs):
print(license_path.read_text('utf-8'))
sys.exit(RETURN_OK)


class printDiagnostics(argparse.Action):
"""
Print information that is helpful for the support team
Expand All @@ -764,6 +776,7 @@ def __call__(self, *args, **kwargs):

sys.exit(RETURN_OK)


def backup(args, force = True):
"""
Command for force taking a new snapshot.
Expand All @@ -783,6 +796,7 @@ def backup(args, force = True):
ret = takeSnapshot(cfg, force)
sys.exit(int(ret))


def backupJob(args):
"""
Command for taking a new snapshot in background. Mainly used for cronjobs.
Expand All @@ -798,6 +812,7 @@ def backupJob(args):
"""
cli.BackupJobDaemon(backup, args).start()


def shutdown(args):
"""
Command for shutting down the computer after the current snapshot has
Expand Down Expand Up @@ -842,6 +857,7 @@ def shutdown(args):
sd.shutdown()
sys.exit(RETURN_OK)


def snapshotsPath(args):
"""
Command for printing the full snapshot path of current profile.
Expand All @@ -864,6 +880,7 @@ def snapshotsPath(args):
print(msg.format(cfg.snapshotsFullPath()), file=force_stdout)
sys.exit(RETURN_OK)


def snapshotsList(args):
"""
Command for printing a list of all snapshots in current profile.
Expand Down Expand Up @@ -894,6 +911,7 @@ def snapshotsList(args):
_umount(cfg)
sys.exit(RETURN_OK)


def snapshotsListPath(args):
"""
Command for printing a list of all snapshots paths in current profile.
Expand Down Expand Up @@ -924,6 +942,7 @@ def snapshotsListPath(args):
_umount(cfg)
sys.exit(RETURN_OK)


def lastSnapshot(args):
"""
Command for printing the very last snapshot in current profile.
Expand All @@ -950,6 +969,7 @@ def lastSnapshot(args):
_umount(cfg)
sys.exit(RETURN_OK)


def lastSnapshotPath(args):
"""
Command for printing the path of the very last snapshot in
Expand Down Expand Up @@ -978,6 +998,7 @@ def lastSnapshotPath(args):
_umount(cfg)
sys.exit(RETURN_OK)


def unmount(args):
"""
Command for unmounting all filesystems.
Expand All @@ -995,6 +1016,7 @@ def unmount(args):
_umount(cfg)
sys.exit(RETURN_OK)


def benchmarkCipher(args):
"""
Command for transferring a file with scp to remote host with all
Expand All @@ -1018,6 +1040,7 @@ def benchmarkCipher(args):
logger.error("SSH is not configured for profile '%s'!" % cfg.profileName())
sys.exit(RETURN_ERR)


def pwCache(args):
"""
Command for starting password cache daemon.
Expand Down Expand Up @@ -1048,6 +1071,7 @@ def pwCache(args):
daemon.run()
sys.exit(ret)


def decode(args):
"""
Command for decoding paths given paths with 'encfsctl'.
Expand Down Expand Up @@ -1082,6 +1106,7 @@ def decode(args):
_umount(cfg)
sys.exit(RETURN_OK)


def remove(args, force = False):
"""
Command for removing snapshots.
Expand All @@ -1102,6 +1127,7 @@ def remove(args, force = False):
_umount(cfg)
sys.exit(RETURN_OK)


def removeAndDoNotAskAgain(args):
"""
Command for removing snapshots without asking before remove
Expand All @@ -1116,6 +1142,7 @@ def removeAndDoNotAskAgain(args):
"""
remove(args, True)


def smartRemove(args):
"""
Command for running Smart-Removal from Terminal.
Expand Down Expand Up @@ -1149,6 +1176,7 @@ def smartRemove(args):
logger.error('Smart Removal is not configured.')
sys.exit(RETURN_NO_CFG)


def restore(args):
"""
Command for restoring files from snapshots.
Expand Down Expand Up @@ -1178,6 +1206,7 @@ def restore(args):
_umount(cfg)
sys.exit(RETURN_OK)


def checkConfig(args):
"""
Command for checking the config file.
Expand Down Expand Up @@ -1205,5 +1234,6 @@ def checkConfig(args):
file = force_stdout)
sys.exit(RETURN_ERR)


if __name__ == '__main__':
startApp()
29 changes: 29 additions & 0 deletions common/snapshots.py
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,15 @@ def remove(self, sid):

return True

def _warning_on_take_snapshot(self, config):
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to add a comment header to describe the purpose and arguments of the function

Copy link
Contributor

Choose a reason for hiding this comment

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

This function name (_warning_on_take_snapshot) does indicate that a warning is issued but it is unclear under which condition (or what is checked).

I suggest to rename the function to eg. _check_included_sources_exist_on_take_snapshot (perhaps you find a better word than "sources")

missing = has_missing(config.include())

if missing:
msg = ', '.join(missing)
msg = f'The following folders are missing: {msg}'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "includes" could be folders but also files so I would add "... following **files/**folders are missing..." to the message

logger.warning(msg)
self.setTakeSnapshotMessage(1, msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

"1" means "error" ("0" would be "info"), but we do not yet have a value for "warnings".
I am not quite sure about the impact of using "error" here (eg. does this mean that the taken snapshot is reported as "error"?). I have to test this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I think I used 1 in order to show properly in the Systray (but now systray is not working fine for me anymore :/)

But sure. Let me know if you want me to change :)


# TODO Refactor: This functions is extremely difficult to understand:
# - Nested "if"s
# - Fuzzy names of classes, attributes and methods
Expand Down Expand Up @@ -806,6 +815,7 @@ def backup(self, force=False):
else:
self.config.setCurrentHashId(hash_id)

self._warning_on_take_snapshot(self.config)
include_folders = self.config.include()

if not include_folders:
Expand Down Expand Up @@ -3087,6 +3097,25 @@ def lastSnapshot(cfg):
return sids[0]


def has_missing(included):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add what is missing to make the purpose of the function clear: has_missing_includes()

Copy link
Contributor

Choose a reason for hiding this comment

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

So I suggest to rename the function to has_missing_includes()

"""
Check if there are missing files or folders in a snapshot.
Args:
included (list): list of tuples (item, info)
Returns:
tuple: (bool, str) where bool is ``True`` if there are
missing files or folders and str is a message
describing the missing files or folders
"""
not_found = []
for path, info in included:
if not os.path.exists(path):
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if os.path.exists() does also work for files (though folders are files), could you please (manually) check this with a configuration that contains a "missing" and an "existing" file (positive and negative test)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I just did it and it worked fine :)

I tested via CLI:

➜  /tmp cd /tmp                                                                                                                                                                                                                          /0.0s
➜  /tmp touch test.txt                                                                                                                                                                                                                   /0.0s
➜  /tmp python
Python 3.12.4 (main, Jun  7 2024, 06:33:07) [GCC 14.1.1 20240522] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.exists("/tmp")
True
>>> os.path.exists("/tmp/test.txt")
True
>>> os.path.exists("/wrong")
False
>>> os.path.exists("/tmp/wrong.txt")
False

I tested w/ CLI, and got correct results w/ this warning:

WARNING: The following **files/**folders are missing: /home/rafaelhdr/RMFolder, /home/rafaelhdr/RMFIle

And also via GUI with real snapshots (w/ existent folder, removed folder, existent file and removed file).

Screenshot from 2024-07-26 10-06-02

not_found.append(path)
return not_found


if __name__ == '__main__':
config = config.Config()
snapshots = Snapshots(config)
Expand Down
24 changes: 21 additions & 3 deletions qt/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -1211,12 +1211,27 @@ def updateTimeLine(self, refreshSnapshotsList=True):
item = self.timeLine.addSnapshot(sid)
self.timeLine.checkSelection()

def validate_on_take_snapshot(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

excellent function name 👍 !!!

missing = snapshots.has_missing(self.config.include())
if missing:
msg_missing = '\n'.join(missing)
msg = _('The following folders are missing: {folders} Do you want to proceed?'.format(
Copy link
Contributor

Choose a reason for hiding this comment

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

The def _warning_on_take_snapshot() in snapshots.py also has a similar message creation logic so this code partially looks like duplicated code (and "includes" could also be files so I would add "files/folders` here too.

If you'd find a simple way to de-duplicate the code it would be great but if it is too complicated I could live with the existing code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About this, a similar code was made with the missing = snapshots.has_missing(self.config.include()) (which is now renamed to has_missing_includes)

Only the text generation is different (also not translated the CLI version) and how we warn the user.

Ok?

folders=f'\n{msg_missing}\n\n'))
answer = messagebox.warningYesNo(self, msg)
buhtz marked this conversation as resolved.
Show resolved Hide resolved
return answer == QMessageBox.StandardButton.Yes
return True

def btnTakeSnapshotClicked(self):
backintime.takeSnapshotAsync(self.config)
self.updateTakeSnapshot(True)
self._take_snapshot_clicked(checksum=False)

def btnTakeSnapshotChecksumClicked(self):
backintime.takeSnapshotAsync(self.config, checksum = True)
self._take_snapshot_clicked(checksum=True)

def _take_snapshot_clicked(self, checksum):
if not self.validate_on_take_snapshot():
return

backintime.takeSnapshotAsync(self.config, checksum=checksum)
self.updateTakeSnapshot(True)

def btnStopTakeSnapshotClicked(self):
Expand Down Expand Up @@ -1957,6 +1972,7 @@ def eventFilter(self, receiver, event):
return super(ExtraMouseButtonEventFilter, self) \
.eventFilter(receiver, event)


class RemoveSnapshotThread(QThread):
"""
remove snapshots in background thread so GUI will not freeze
Expand Down Expand Up @@ -1992,6 +2008,7 @@ def run(self):
if self.config.inhibitCookie:
self.config.inhibitCookie = tools.unInhibitSuspend(*self.config.inhibitCookie)


class FillTimeLineThread(QThread):
"""
add snapshot IDs to timeline in background
Expand All @@ -2009,6 +2026,7 @@ def run(self):

self.parent.snapshotsList.sort()


class SetupCron(QThread):
"""
Check crontab entries on startup.
Expand Down