feat!: Debug output for scheduled jobs (#1616)

Activate debug output in schedule settings. Breaking Change: GUI started with --debug does no longer add --debug to the crontab for scheduled profiles by default. Additionally some refactoring and also pylint tweaks. Thanks to Kosta Vukicevic (@stcksmsh) for submitting this PR. Close #1616 --------- Co-authored-by: Christian Buhtz <[email protected]> Co-authored-by: aryoda <[email protected]>
bit-team · May 13, 2024 · 99081af · 99081af
1 parent c9c805a
commit 99081af
Show file tree

Hide file tree

Showing 15 changed files with 259 additions and 41 deletions.
diff --git a/CHANGES b/CHANGES
@@ -1,14 +1,17 @@
 Back In Time
 
 Version 1.4.4-dev (development of upcoming release)
+* Breaking Change: GUI started with --debug does no longer add --debug to the crontab for scheduled profiles.
+                   Use the new "enable logging for debug messages" in the 'Schedule' section of the 'Manage profiles' GUI instead.
+* Feature: Profile and GUI allow to activate debug output for scheduled jobs by adding '--debug' to crontab entry (#1616, contributed by @stcksmsh Kosta Vukicevic)
 * Removed: Field "filesystem_mount" and "snapshot_version" in "info" file (#1684)
 * Feature: Support SSH proxy (jump) host (#1688) (@cgrinham, Christie Grinham)
 * Removed: Context menu in LogViewDialog (#1578)
 * Refactor: Replace Config.user() with getpass.getuser() (#1694)
 * Fix: Validation of diff command settings in compare snapshots dialog (#1662) (@stcksmsh Kosta Vukicevic)
 * Fix bug: Open symlinked folders in file view (#1476)
 * Fix bug: Respect dark mode using color roles (#1601)
-* Fix: "Highly recommended" exclusion pattern in "Manage Profile" dialog's "Exclude" tab show missings only (#1620)
+* Fix: "Highly recommended" exclusion pattern in "Manage Profile" dialog's "Exclude" tab show missing only (#1620)
 * Build: Activate PyLint error E0401 (import-error)
 * Dependency: Migration to PyQt6
 * Build: PyLint unit test is skipped if PyLint isn't installed, but will always run on TravisCI (#1634)

diff --git a/FAQ.md b/FAQ.md
@@ -458,7 +458,7 @@ Otherwise, kill the process. After that look into the folder
 For more details see the developer documentation: [Usage of control files (locks, flocks, logs and others)](common/doc-dev/4_Control_files_usage_(locks_flocks_logs_and_others).md)
 
 ### Switching to dark or light mode in the desktop environment is ignored by BIT
-After restart _Back In Time_ it should addapt to the desktops current used
+After restart _Back In Time_ it should adapt to the desktops current used
 color theme.
 
 It happens because Qt does not detect theme modifications out of the

diff --git a/common/config.py b/common/config.py
@@ -989,6 +989,13 @@ def scheduleMode(self, profile_id = None):
     def setScheduleMode(self, value, profile_id = None):
         self.setProfileIntValue('schedule.mode', value, profile_id)
 
+    def scheduleDebug(self, profile_id = None):
+        #?Enable debug output to system log for schedule mode.
+        return self.profileBoolValue('schedule.debug', False, profile_id)
+
+    def setScheduleDebug(self, value, profile_id = None):
+        self.setProfileBoolValue('schedule.debug', value, profile_id)
+
     def scheduleTime(self, profile_id = None):
         #?Position-coded number with the format "hhmm" to specify the hour
         #?and minute the cronjob should start (eg. 2015 means a quarter
@@ -1787,28 +1794,59 @@ def cronLine(self, profile_id):
         return cron_line
 
     def cronCmd(self, profile_id):
-        if not tools.checkCommand('backintime'):
-            logger.error("Command 'backintime' not found", self)
-            return
+        """Generates the command used in the crontab file based on the settings
+        for the current profile.
+
+        Returns:
+            str: The crontab line.
+        """
+
+        # buhtz (2024-04): IMHO meaningless in productive environments.
+        # if not tools.checkCommand('backintime'):
+        #     logger.error("Command 'backintime' not found", self)
+        #     return
+
+        # Get full path of the Back In Time binary
         cmd = tools.which('backintime') + ' '
+
+        # The "--profile-id" argument is used only for profiles different from
+        # first profile
         if profile_id != '1':
             cmd += '--profile-id %s ' % profile_id
+
+        # User defined path to config file
         if not self._LOCAL_CONFIG_PATH is self._DEFAULT_CONFIG_PATH:
             cmd += '--config %s ' % self._LOCAL_CONFIG_PATH
-        if logger.DEBUG:
+
+        # Enable debug output
+        if self.scheduleDebug(profile_id):
             cmd += '--debug '
+
+        # command
         cmd += 'backup-job'
+
+        # Redirect stdout to nirvana
         if self.redirectStdoutInCron(profile_id):
             cmd += ' >/dev/null'
+
+        # Redirect stderr ...
         if self.redirectStderrInCron(profile_id):
+
             if self.redirectStdoutInCron(profile_id):
+                # ... to stdout
                 cmd += ' 2>&1'
             else:
+                # ... to nirvana
                 cmd += ' 2>/dev/null'
+
+        # IO priority: low (-n7) in "best effort" class (-c2)
         if self.ioniceOnCron(profile_id) and tools.checkCommand('ionice'):
             cmd = tools.which('ionice') + ' -c2 -n7 ' + cmd
+
+        # CPU priority: very low
         if self.niceOnCron(profile_id) and tools.checkCommand('nice'):
             cmd = tools.which('nice') + ' -n19 ' + cmd
+
         return cmd
 
 

diff --git a/common/snapshots.py b/common/snapshots.py
@@ -52,7 +52,7 @@ class Snapshots:
     the class `SID` which represents a snapshot in the "data layer".
 
     BUHTZ 2024-02-23: Not sure but it seems to be one concret snapshot and
-    not a collection of snapshots. In this case the class name is missleading
+    not a collection of snapshots. In this case the class name is misleading
     because it is in plural form.
 
     Args:
@@ -103,7 +103,7 @@ def takeSnapshotMessage(self):
         Dev note (buhtz):
             Too many try..excepts in here.
         """
-        # Dev note (buhtz): Not sure what happens here or why this is usefull.
+        # Dev note (buhtz): Not sure what happens here or why this is useful.
         wait = datetime.datetime.now() - datetime.timedelta(seconds=5)
 
         if self.lastBusyCheck < wait:
@@ -172,7 +172,7 @@ def setTakeSnapshotMessage(self, type_id, message, timeout=-1):
         message_fn = self.config.takeSnapshotMessageFile()
 
         try:
-            # Write message to file (and overwrites the previos one)
+            # Write message to file (and overwrites the previous one)
             with open(message_fn, 'wt') as f:
                 f.write(str(type_id) + '\n' + message)
 

diff --git a/common/test/test_crontab.py b/common/test/test_crontab.py
@@ -0,0 +1,104 @@
+# Back In Time
+# Copyright (C) 2024 Kosta Vukicevic, Christian Buhtz
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation,Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+import unittest
+import pyfakefs.fake_filesystem_unittest as pyfakefs_ut
+import sys
+import os
+import tempfile
+import inspect
+from pathlib import Path
+from unittest import mock
+sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
+import backintime
+import config
+import snapshots
+import tools
+import logger
+
+
+class CrontabDebug(pyfakefs_ut.TestCase):
+    """Debug behavior when scheduled via crontab"""
+    def setUp(self):
+        """Setup a fake filesystem with a config file."""
+        self.setUpPyfakefs(allow_root_user=False)
+
+        # cleanup() happens automatically
+        self._temp_dir = tempfile.TemporaryDirectory(prefix='bit.')
+         # Workaround: tempfile and pathlib not compatible yet
+        self.temp_path = Path(self._temp_dir.name)
+
+        self.config_fp = self._create_config_file(parent_path=self.temp_path)
+
+    def _create_config_file(cls, parent_path):
+        """Minimal config file"""
+        cfg_content = inspect.cleandoc('''
+            config.version=6
+            profile1.snapshots.include.1.type=0
+            profile1.snapshots.include.1.value=rootpath/source
+            profile1.snapshots.include.size=1
+            profile1.snapshots.no_on_battery=false
+            profile1.snapshots.notify.enabled=true
+            profile1.snapshots.path=rootpath/destination
+            profile1.snapshots.path.host=test-host
+            profile1.snapshots.path.profile=1
+            profile1.snapshots.path.user=test-user
+            profile1.snapshots.preserve_acl=false
+            profile1.snapshots.preserve_xattr=false
+            profile1.snapshots.remove_old_snapshots.enabled=true
+            profile1.snapshots.remove_old_snapshots.unit=80
+            profile1.snapshots.remove_old_snapshots.value=10
+            profile1.snapshots.rsync_options.enabled=false
+            profile1.snapshots.rsync_options.value=
+            profiles.version=1
+        ''')
+
+        # config file location
+        config_fp = parent_path / 'config_path' / 'config'
+        config_fp.parent.mkdir()
+        config_fp.write_text(cfg_content, 'utf-8')
+
+        return config_fp
+
+    @mock.patch('tools.which', return_value='backintime')
+    def test_crontab_contains_debug(self, mock_which):
+        """
+        About mock_which: A workaround because the function gives
+        false-negative when using a fake filesystem.
+        """
+        conf = config.Config(str(self.config_fp))
+
+        # set and assert start conditions
+        conf.setScheduleDebug(True)
+        self.assertTrue(conf.scheduleDebug())
+
+        sut = conf.cronCmd(profile_id='1')
+        self.assertIn('--debug', sut)
+
+    @mock.patch('tools.which', return_value='backintime')
+    def test_crontab_without_debug(self, mock_which):
+        """No debug output in crontab line.
+
+        About mock_which: See test_crontab_with_debug().
+        """
+        conf = config.Config(str(self.config_fp))
+
+        # set and assert start conditions
+        conf.setScheduleDebug(False)
+        self.assertFalse(conf.scheduleDebug())
+
+        sut = conf.cronCmd(profile_id='1')
+        self.assertNotIn('--debug', sut)
diff --git a/common/test/test_diagnostics.py b/common/test/test_diagnostics.py
@@ -28,7 +28,7 @@ def test_content_minimal(self):
         self.assertCountEqual(sut['host-setup'].keys(), ['OS'])
 
     def test_some_content(self):
-        """Some containted elements"""
+        """Some contained elements"""
         result = diagnostics.collect_diagnostics()
 
         # 1st level keys

diff --git a/common/test/test_lint.py b/common/test/test_lint.py
@@ -65,22 +65,49 @@ def test_with_pylint(self):
             # Deactivate all checks by default
             '--disable=all',
             # prevent false-positive no-module-member errors
-            '--extension-pkg-whitelist=PyQt6,PyQt6.QtCore',
+            '--extension-pkg-allow-list=PyQt6,PyQt6.QtCore',
             # Because of globally installed GNU gettext functions
             '--additional-builtins=_,ngettext',
             # PEP8 conform line length (see PyLint Issue #3078)
             '--max-line-length=79',
             # Whitelist variable names
             '--good-names=idx,fp',
+            # '--reports=yes',
         ]
 
         # Explicit activate checks
         err_codes = [
+            'E0401',  # import-error
             'E0602',  # undefined-variable
             'E1101',  # no-member
+            # 'W0611',  # unused-import
+            'W1301',  # unused-format-string-key
             'W1401',  # anomalous-backslash-in-string (invalid escape sequence)
-            'E0401',  # import-error
             'I0021',  # useless-suppression
+
+            # Enable asap. This list is selection of existing (not all!)
+            # problems currently exiting in the BIT code base. Quit easy to fix
+            # because there count is low.
+            # 'C0303',  # trailing-whitespace
+            # 'C0305',  # trailing-newlines
+            # 'C0324',  # superfluous-parens
+            # 'C0410',  # multiple-imports
+            # 'E0213',  # no-self-argument
+            # 'R0201',  # no-self-use
+            # 'R0202',  # no-classmethod-decorator
+            # 'R0203',  # no-staticmethod-decorator
+            # 'R0801',  # duplicate-code
+            # 'W0123',  # eval-used
+            # 'W0237',  # arguments-renamed
+            # 'W0221',  # arguments-differ
+            # 'W0311',  # bad-indentation
+            # 'W0404',  # reimported
+            # 'W4902',  # deprecated-method
+            # 'W4904',  # deprecated-class
+            # 'W0603',  # global-statement
+            # 'W0614',  # unused-wildcard-import
+            # 'W0612',  # unused-variable
+            # 'W0707',  # raise-missing-from
         ]
 
         if ON_TRAVIS_PPC64LE:
@@ -105,3 +132,6 @@ def test_with_pylint(self):
         print(r.stdout)
 
         self.assertEqual(0, error_n, f'PyLint found {error_n} problems.')
+
+        # any other errors?
+        self.assertEqual(r.stderr, '')
diff --git a/common/test/test_tools.py b/common/test/test_tools.py
@@ -812,7 +812,7 @@ def setUp(self):
         self.setUpPyfakefs(allow_root_user=False)
 
     def test_git_repo_info_none(self):
-        """Acutally not a git repo"""
+        """Actually not a git repo"""
 
         self.assertEqual(tools.get_git_repository_info(), None)
 

diff --git a/common/tools.py b/common/tools.py
@@ -357,11 +357,15 @@ def registerBackintimePath(*path):
 
 
 def runningFromSource():
-    """
-    Check if BackInTime is running from source (without installing).
+    """Check if BackInTime is running from source (without installing).
+
+    Dev notes by buhtz (2024-04): This function is dangerous and will give a
+    false-negative in fake filesystems (e.g. PyFakeFS). The function should
+    not exist. Beside unit tests it is used only two times. Remove it until
+    migration to pyproject.toml based project packaging (#1575).
 
     Returns:
-        bool:   ``True`` if BackInTime is running from source
+        bool: ``True`` if BackInTime is running from source.
     """
     return os.path.isfile(backintimePath('common', 'backintime'))
 
@@ -493,14 +497,13 @@ def readFileLines(path, default = None):
 
 
 def checkCommand(cmd):
-    """
-    Check if command ``cmd`` is a file in 'PATH' environ.
+    """Check if command ``cmd`` is a file in 'PATH' environment.
 
     Args:
-        cmd (str):  command
+        cmd (str): The command.
 
     Returns:
-        bool:       ``True`` if command ``cmd`` is in 'PATH' environ
+        bool: ``True`` if ``cmd`` is in 'PATH' environment otherwise ``False``.
     """
     cmd = cmd.strip()
 
@@ -510,23 +513,27 @@ def checkCommand(cmd):
     if os.path.isfile(cmd):
         return True
 
-    return not which(cmd) is None
-
+    return which(cmd) is not None
 
+
 def which(cmd):
-    """
-    Get the fullpath of executable command ``cmd``. Works like
-    command-line 'which' command.
+    """Get the fullpath of executable command ``cmd``.
+
+    Works like command-line 'which' command.
+
+    Dev note by buhtz (2024-04): Give false-negative results in fake
+    filesystems. Quit often use in the whole code base. But not sure why
+    can we replace it with "which" from shell?
 
     Args:
-        cmd (str):  command
+        cmd (str): The command.
 
     Returns:
-        str:        fullpath of command ``cmd`` or ``None`` if command is
-                    not available
+        str: Fullpath of command ``cmd`` or ``None`` if command is not
+             available.
     """
     pathenv = os.getenv('PATH', '')
-    path = pathenv.split(":")
+    path = pathenv.split(':')
     common = backintimePath('common')
 
     if runningFromSource() and common not in path: