Add Dedicated Backup Directory - SAVE_CONFIG Now Applies To Includes

[lraithel15133]

#149

Submitting PR for https://github.com/MasturMynd

[MasturMynd]

Thank you! I am dum dum when it comes to git

[rogerlz]

I like that.. what do you think about putting that behind a danger_option?

[MasturMynd]

I like that.. what do you think about putting that behind a danger_option?

I don't think that's necessary, as with proper testing it shouldn't pose any "danger" and should be the default behavior, IMO.

[bwnance]

@MasturMynd what happens if an include includes an include?

[bwnance]

@MasturMynd @rogerlz i would like this to be hidden under a danger flag, at least initially - though it likely shouldn't pose an issue, i'd rather keep stuff flagged until it's been validated by a wide range of users

[MasturMynd]

@MasturMynd what happens if an include includes an include?

Welp, that was an oversight, but an easily remedied one. I'll get it updated so that it does recursive checks rather than only checking printer.cfg for includes

[MasturMynd]

@MasturMynd @rogerlz i would like this to be hidden under a danger flag, at least initially - though it likely shouldn't pose an issue, i'd rather keep stuff flagged until it's been validated by a wide range of users

Let me know what I need to do to put this behind a danger flag and I'll get it done. Haven't had time to sift through the code base to see how those work yet

[rogerlz]

If I understand correctly, this PR is trying to do two things:

• fix a bug / implement a feature to SAVE_CONFIG to consider included files
• create backups in a backup directory

It would be nice to clarify that.
I'd be fine with the SAVE_CONFIG changes going as is, but I'd prefer to have the backup directory change behind a danger_option.

to create a new danger_option parameter:

• add it to the danger_options.py module here with a default that doesn't change the original behavior if possible

• load it on the module you are working on

self.printer = config.get_printer()
self.danger_options = self.printer.lookup_object("danger_options")

• use the parameter to validate something

if self.danger_options.adc_ignore_limits:
    continue

don't forget:

• add tests to tests/klippy/danger_options.cfg
• changes to the Config_Reference.md
• changes to README.md to list the new feature so users can know about it

[MasturMynd]

If I understand correctly, this PR is trying to do two things:

• fix a bug / implement a feature to SAVE_CONFIG to consider included files
• create backups in a backup directory

It would be nice to clarify that. I'd be fine with the SAVE_CONFIG changes going as is, but I'd prefer to have the backup directory change behind a danger_option.

to create a new danger_option parameter:

• add it to the danger_options.py module here with a default that doesn't change the original behavior if possible
• load it on the module you are working on

self.printer = config.get_printer()
self.danger_options = self.printer.lookup_object("danger_options")

• use the parameter to validate something

if self.danger_options.adc_ignore_limits:
    continue

don't forget:

• add tests to tests/klippy/danger_options.cfg
• changes to the Config_Reference.md
• changes to README.md to list the new feature so users can know about it

I'd like to make sure that I've addressed all of the concerns to this point and have provided information on why certain things are the way they are.

First is the desire to process included files during SAVE_CONFIG. As I'm sure is known, SAVE_CONFIG in its current state will only apply autosave data to printer.cfg. This results in conflicts when something such as PID values are stored in a separate config file. The proposed changes that I've brought forward will sift through all of the config files used to instruct machine operation for conflicts with autosave data and comment them out.

The second change, being the config_backups directory was brought along out of necessity. During testing, @lraithel15133 was using a machine that had all of his additional config files in a single folder. Then, a [include foldername/*.cfg] was used for simplicity and to ensure that any new config additions were automatically loaded. This presented a unique error due to the naming scheme that klipper uses for backing up configs. The .cfg file extension is retained, meaning that backups saved to the foldername directory were loaded via the include block. Rather than change the naming format that klipper uses for backup files and potentially causing issues downstream with mainsail / fluid, the decision was made to add a directory specific for backup configs.

I still haven't had time to look into exactly what I need to do to submit a proper pull request, so all I can do at this point is provide the necessary snippets here and request that someone more knowledgeable than I proceed forward with them. I'm available in to discuss the changes via discord @mastur_mynd, whether it be via DMs or in the DK channels, or wherever.

test\klippy\danger_options.cfg

[poopybutt]
[danger_options]
log_statistics: False
log_config_file_at_startup: False
error_on_unused_config_options: False
log_bed_mesh_at_startup: False
log_shutdown_info: False
allow_plugin_override: True
multi_mcu_trsync_timeout: 0.05
adc_ignore_limits: True
autosave_includes: True

docs\Config_Reference.md

#autosave_includes: False
#   When set to true, SAVE_CONFIG will recursively read [include ...] blocks
#   for conflicts to autosave data. Any configurations updated will be backed
#   up to configs/config_backups.

klippy\extras\danger_options.py

self.autosave_includes = config.getboolean("autosave_includes", False)

klippy\configfile.py

    def _write_backup(self, cfgpath, cfgdata, gcode):
        printercfg = self.printer.get_start_args()["config_file"]
        configdir = os.path.dirname(printercfg)
        # Define a directory for configuration backups so that include blocks
        # using a wildcard to reference all files in a directory don't throw
        # errors
        backupdir = os.path.join(configdir, 'config_backups')
        # Create the backup directory if it doesn't already exist
        if not os.path.exists(backupdir):
            os.mkdir(backupdir)
        
        # Generate the name of the backup file by stripping the leading path in
        # `cfgpath` and appending to it. Then add it to the config_backups dir
        datestr = time.strftime("-%Y%m%d_%H%M%S")
        cfgname = os.path.basename(cfgpath)
        backup_path = backupdir + "/" + cfgname + datestr
        if cfgpath.endswith(".cfg"):
            backup_path = backupdir + "/" + cfgname[:-4] + datestr + ".cfg"
        logging.info(
            "SAVE_CONFIG to '%s' (backup in '%s')", cfgpath, backup_path
        )
        try:
            # Read the current config into the backup before making changes to
            # the original file
            currentconfig = open(cfgpath, "r")
            backupconfig = open(backup_path, "w")
            backupconfig.write(currentconfig.read())
            backupconfig.close()
            currentconfig.close()
            # With the backup created, write the new data to the original file
            currentconfig = open(cfgpath, "w")
            currentconfig.write(cfgdata)
            currentconfig.close()
        except:
            msg = "Unable to write config file during SAVE_CONFIG"
            logging.exception(msg)
            raise gcode.error(msg)

    def _save_includes(self, cfgpath, data, visitedpaths, gcode):
        # Prevent an infinite loop in the event of configs circularly
        # referencing each other
        if cfgpath in visitedpaths:
            return

        visitedpaths.add(cfgpath)
        dirname = os.path.dirname(cfgpath)
        # Read the data as individual lines so we can find include blocks
        lines = data.split("\n")
        for line in lines:
            # Strip trailing comment
            pos = line.find("#")
            if pos >= 0:
                line = line[:pos]

            mo = configparser.RawConfigParser.SECTCRE.match(line)
            header = mo and mo.group("header")
            if header and header.startswith("include "):
                include_spec = header[8:].strip()
                include_glob = os.path.join(dirname, include_spec)
                # retrieve all filenames associated with the absolute path of
                # the include header
                include_filenames = glob.glob(include_glob)
                if not include_filenames and not glob.has_magic(include_glob):
                    # Empty set is OK if wildcard but not for direct file
                    # reference
                    raise error("Include file '%s' does not exist"
                                % (include_glob,))
                include_filenames.sort()
                # Read the include files and check them against autosave data.
                # If autosave data overwites anything we'll update the file
                # and create a backup.
                for include_filename in include_filenames:
                    # Recursively check for includes. No need to check for looping
                    # includes as klipper checks this at startup.
                    include_predata = self._read_config_file(include_filename)
                    self._save_includes(include_filename, include_predata,
                                        visitedpaths, gcode)
                    
                    include_postdata = self._strip_duplicates(
                        include_predata, self.autosave)
                    # Only write and backup data that's been changed
                    if include_predata != include_postdata:
                        self._write_backup(include_filename, include_postdata,
                                           gcode)

    def cmd_SAVE_CONFIG(self, gcmd):
        if not self.autosave.fileconfig.sections():
            return
        gcode = self.printer.lookup_object("gcode")
        # Create string containing autosave data
        autosave_data = self._build_config_string(self.autosave)
        lines = [("#*# " + l).strip() for l in autosave_data.split("\n")]
        lines.insert(0, "\n" + AUTOSAVE_HEADER.rstrip())
        lines.append("")
        autosave_data = "\n".join(lines)
        # Read in and validate current config file
        cfgname = self.printer.get_start_args()["config_file"]
        try:
            data = self._read_config_file(cfgname)
            regular_data, old_autosave_data = self._find_autosave_data(data)
            config = self._build_config_wrapper(regular_data, cfgname)
        except error as e:
            msg = "Unable to parse existing config on SAVE_CONFIG"
            logging.exception(msg)
            raise gcode.error(msg)
        regular_data = self._strip_duplicates(regular_data, self.autosave)
        
        self.danger_options = self.printer.lookup_object("danger_options")
        if self.danger_options.autosave_includes:
            self._save_includes(cfgname, data, gcode, set())
        
        # NOW we're safe to check for conflicts
        self._disallow_include_conflicts(regular_data, cfgname, gcode)
        data = regular_data.rstrip() + autosave_data
        self._write_backup(cfgname, data, gcode)

        # Request a restart
        gcode.request_restart("restart")

[lraithel15133]

refer to #153