Merge pull request #289 from jakub-nt/CFE-4561

CFE-4561: Implemented support for converting modified files into patch files in cfbs-convert

Author: larsewi

JSON.md

@@ -281,6 +281,10 @@ In `cfbs.json`'s `"steps"`, the build step name must be separated from the rest
 - `replace_version <n> <to_replace> <filename>`
   - Replace the string inside the file with the version number of that module.
   - Works identically to `replace`, except the string to replace with, i.e. the version number, is found automatically.
+- `patch <patch_file>`
+  - Apply a unified diff patch file.
+    The patches are applied to files in `out/masterfiles` during build.
+    Paths inside the .patch file should be given relative to `out/masterfiles`.
 
 When `def.json` is modified during a `json`, `input`, `directory`, `bundles`, or `policy_files` build step, the values of some lists of strings are deduplicated, when this does not make any difference in behavior.
 These cases are:

cfbs/build.py

@@ -13,9 +13,12 @@
 import os
 import logging as log
 import shutil
+import subprocess
 from cfbs.cfbs_config import CFBSConfig
 from cfbs.utils import (
+    CFBSUserError,
     canonify,
+    cli_tool_present,
     cp,
     cp_dry_overwrites,
     deduplicate_def_json,
@@ -126,6 +129,27 @@ def _perform_replacement(n, a, b, filename):
         raise CFBSExitError("Failed to write to '%s'" % (filename,))
 
 
+def _apply_masterfiles_patch(patch_path):
+    if not cli_tool_present("patch"):
+        raise CFBSUserError("Working with .patch files requires the 'patch' utility")
+
+    if not os.path.isfile(patch_path):
+        raise CFBSExitError("Patch at path '%s' not found" % patch_path)
+
+    patch_path = os.path.relpath(patch_path, "out/masterfiles")
+
+    # reasoning for used flags:
+    # * `-t`: do not interactively ask the user for another path if the patch fails to apply due to the path not being found
+    # * `-p0`: use paths in the form specified in the .patch files
+    cmd = "patch -u -t -p0 -i" + patch_path
+    # the cwd needs to be the base path of the relative paths specified in the .patch files
+    # currently, the output of the patch command is displayed
+    cp = subprocess.run(cmd, shell=True, cwd="out/masterfiles")
+
+    if cp.returncode != 0:
+        raise CFBSExitError("Failed to apply patch '%s'" % patch_path)
+
+
 def _perform_copy_step(args, source, destination, prefix):
     src, dst = args
     if dst in [".", "./"]:
@@ -358,6 +382,19 @@ def _perform_replace_version_step(module, i, args, name, destination, prefix):
     _perform_replacement(n, to_replace, version, filename)
 
 
+def _perform_patch_step(module, i, args, name, source, prefix):
+    assert len(args) == 1
+
+    patch_relpath = args[0]
+    print("%s patch '%s'" % (prefix, patch_relpath))
+    # New build step so let's be a bit strict about validating it:
+    validate_build_step(name, module, i, "patch", args, strict=True)
+
+    patch_path = os.path.join(source, patch_relpath)
+
+    _apply_masterfiles_patch(patch_path)
+
+
 def perform_build(config: CFBSConfig, diffs_filename=None) -> int:
     if not config.get("build"):
         raise CFBSExitError("No 'build' key found in the configuration")
@@ -430,6 +467,8 @@ def perform_build(config: CFBSConfig, diffs_filename=None) -> int:
                 _perform_replace_version_step(
                     module, i, args, name, destination, prefix
                 )
+            elif operation == "patch":
+                _perform_patch_step(module, i, args, name, source, prefix)
 
     if diffs_filename is not None:
         try:

cfbs/cfbs_config.py

@@ -24,6 +24,7 @@
 from cfbs.cfbs_types import CFBSCommandGitResult
 from cfbs.utils import (
     CFBSExitError,
+    CFBSProgrammerError,
     CFBSUserError,
     is_a_commit_hash,
     read_file,
@@ -264,6 +265,25 @@ def _find_dependencies(self, modules, exclude):
             dependencies += self._find_dependencies(dependencies, exclude)
         return dependencies
 
+    def _module_by_name(self, name):
+        for module in self["build"]:
+            if module["name"] == name:
+                return module
+
+        raise CFBSProgrammerError("Module of name %s not found" % name)
+
+    def add_patch_step(self, module_name, patch_filepath):
+        """Adds a patch step to a module's `"steps"` list, and saves the `CFBSConfig`.
+
+        API note: currently only used for a local module.
+
+        Error handling:
+        * excepts when module of `module_name` is not present."""
+        module = self._module_by_name(module_name)
+        step = "patch " + patch_filepath
+        module["steps"].append(step)
+        self.save()
+
     def _add_policy_files_build_step(self, module):
         name = module["name"]
         step = "policy_files services/cfbs/" + (

cfbs/commands.py

@@ -69,9 +69,12 @@ def search_command(terms: List[str]):
     cfbs_dir,
     cfbs_filename,
     display_diff,
+    file_diff_text,
     is_cfbs_repo,
+    mkdir,
     read_json,
     CFBSExitError,
+    save_file,
     strip_right,
     pad_right,
     CFBSProgrammerError,
@@ -1127,6 +1130,7 @@ def cfbs_convert_cleanup():
     def cfbs_convert_git_commit(
         commit_message: str, add_scope: Union[str, Iterable[str]] = "all"
     ):
+        print("Creating Git commit...")
         try:
             git_commit_maybe_prompt(commit_message, non_interactive, scope=add_scope)
         except CFBSGitError:
@@ -1263,7 +1267,6 @@ def cfbs_convert_git_commit(
         for unmodified_mpf_file in analyzed_files.unmodified:
             rm(os.path.join(dir_name, unmodified_mpf_file))
 
-        print("Creating Git commit...")
         cfbs_convert_git_commit("Deleted unmodified policy files")
 
     print(
@@ -1310,9 +1313,6 @@ def cfbs_convert_git_commit(
             for file_d in files_to_delete:
                 rm(os.path.join(dir_name, file_d))
 
-            print(
-                "Creating Git commit with deletion of policy files from other versions..."
-            )
             cfbs_convert_git_commit("Deleted policy files from other versions")
             print("Done.", end=" ")
     else:
@@ -1322,6 +1322,8 @@ def cfbs_convert_git_commit(
     )
     if not prompt_user_yesno(non_interactive, "Do you want to continue?"):
         raise CFBSExitError("User did not proceed, exiting.")
+
+    first_patch_conversion = True
     print("The following files have custom modifications:")
     modified_files = analyzed_files.modified
     for modified_file in modified_files:
@@ -1335,6 +1337,7 @@ def cfbs_convert_git_commit(
             mpf_dir_path, masterfiles_version, "tarball", "masterfiles"
         )
         mpf_filepath = os.path.join(mpf_version_dir_path, modified_file)
+        modified_file_path = os.path.join(dir_name, modified_file)
         display_diffs = True
         if not os.path.exists(mpf_version_dir_path):
             try:
@@ -1347,7 +1350,7 @@ def cfbs_convert_git_commit(
                 display_diffs = False
         if display_diffs:
             try:
-                display_diff(mpf_filepath, os.path.join(dir_name, modified_file))
+                display_diff(mpf_filepath, modified_file_path)
             except:
                 log.warning(
                     "Displaying a diff between your file and the default file failed, continuing without displaying a diff..."
@@ -1366,22 +1369,85 @@ def cfbs_convert_git_commit(
         prompt_str = "\nChoose an option:\n"
         prompt_str += "1) Drop modifications - They are not important, or can be achieved in another way.\n"
         prompt_str += "2) Keep modified file - File is kept as is, and can be handled later. Can make future upgrades more complicated.\n"
-        prompt_str += "3) (Not implemented yet) Keep patch file - "
-        prompt_str += "File is converted into a patch file (diff) that hopefully will apply to future versions as well.\n"
+        prompt_str += "3) Keep patch file - File is converted into a patch file (diff) that hopefully will apply to future versions as well.\n"
 
-        response = prompt_user(non_interactive, prompt_str, ["1", "2"], "1")
+        response = prompt_user(non_interactive, prompt_str, ["1", "2", "3"], "1")
 
         if response == "1":
             print("Deleting './%s'..." % modified_file)
-            rm(os.path.join(dir_name, modified_file))
-            commit_message = "Deleted './%s'" % modified_file
-            print("Creating Git commit - %s..." % commit_message)
+            rm(modified_file_path)
             try:
-                cfbs_convert_git_commit(commit_message)
+                cfbs_convert_git_commit("Deleted './%s'" % modified_file)
             except:
                 log.warning("Git commit failed, continuing without committing...")
         if response == "2":
             print("Keeping file as is, nothing to do.")
+        if response == "3":
+            print("Converting './%s' into a patch file..." % modified_file)
+            patches_dir = "custom-masterfiles-patches"
+            patches_module = "./" + patches_dir + "/"
+
+            file_diff_data = file_diff_text(
+                mpf_filepath,
+                modified_file_path,
+                modified_file,
+                modified_file,
+            )
+
+            patch_filename = modified_file.replace("/", "_") + ".patch"
+            patch_path = os.path.join(patches_dir, patch_filename)
+            # append a number if there are multiple files with the same filename
+            i = 1
+            while os.path.exists(patch_path):
+                patch_filename = (
+                    modified_file.replace("/", "_") + "-" + str(i) + ".patch"
+                )
+                patch_path = os.path.join(patches_dir, patch_filename)
+                i += 1
+
+            # saving the .patch file should be done before creating the patches module
+            try:
+                save_file(patch_path, file_diff_data)
+            except:
+                log.warning(
+                    "Saving the patch file failed - keeping the modified file as is instead and continuing..."
+                )
+                continue
+
+            # make the patches local module on first use
+            if first_patch_conversion:
+                print("Adding patches local module...")
+                mkdir(patches_dir)
+
+                config = CFBSConfig.get_instance()
+                # `explicit_build_steps=[]` would fail validation, therefore also add the first build step during the module's creation
+                config.add_command(
+                    [patches_module],
+                    added_by="cfbs convert",
+                    explicit_build_steps=["patch " + patch_filename],
+                )
+                config.save()
+            else:
+                config = CFBSConfig.get_instance()
+                config.add_patch_step(patches_module, patch_filename)
+
+            print("Deleting './%s'..." % modified_file)
+            rm(modified_file_path)
+
+            try:
+                if first_patch_conversion:
+                    cfbs_convert_git_commit(
+                        "Added patches local module, converted './%s' into a .patch file"
+                        % modified_file
+                    )
+                else:
+                    cfbs_convert_git_commit(
+                        "Converted './%s' into a .patch file" % modified_file
+                    )
+            except:
+                log.warning("Git commit failed, continuing without committing...")
+
+            first_patch_conversion = False
 
     print("Conversion finished successfully.")
 

cfbs/git.py

@@ -15,19 +15,15 @@
 from typing import Iterable, Union
 
 from cfbs.prompts import prompt_user
-from cfbs.utils import CFBSExitError, are_paths_equal
+from cfbs.utils import CFBSExitError, are_paths_equal, cli_tool_present
 
 
 class CFBSGitError(Exception):
     pass
 
 
 def git_exists():
-    try:
-        check_call(["git", "--version"], stdout=DEVNULL)
-        return True
-    except FileNotFoundError:
-        return False
+    return cli_tool_present("git")
 
 
 def ls_remote(remote, branch):

cfbs/utils.py

@@ -103,6 +103,17 @@ def sh(cmd: str, directory=None):
     return _sh(cmd)
 
 
+def cli_tool_present(name: str):
+    """Returns whether the CLI tool `name` exists on the system."""
+    try:
+        subprocess.check_call(
+            ["command -v " + name], stdout=subprocess.DEVNULL, shell=True
+        )
+        return True
+    except subprocess.CalledProcessError:
+        return False
+
+
 def display_diff(path_A, path_B):
     """Also displays `stderr`."""
     cmd = "diff -u " + path_A + " " + path_B
@@ -112,19 +123,24 @@ def display_diff(path_A, path_B):
         raise
 
 
-def file_diff(filepath_A, filepath_B):
+def file_diff(filepath_A, filepath_B, target_A=None, target_B=None):
     with open(filepath_A) as f:
         lines_A = f.readlines()
     with open(filepath_B) as f:
         lines_B = f.readlines()
 
-    u_diff = difflib.unified_diff(lines_A, lines_B, filepath_A, filepath_B)
+    if target_A is None:
+        target_A = filepath_A
+    if target_B is None:
+        target_B = filepath_B
+
+    u_diff = difflib.unified_diff(lines_A, lines_B, target_A, target_B)
 
     return u_diff
 
 
-def file_diff_text(filepath_A, filepath_B):
-    u_diff = file_diff(filepath_A, filepath_B)
+def file_diff_text(filepath_A, filepath_B, target_A=None, target_B=None):
+    u_diff = file_diff(filepath_A, filepath_B, target_A, target_B)
     diff_text = "".join(u_diff)
 
     return diff_text

cfbs/validate.py

@@ -57,6 +57,7 @@
     "bundles": "1+",
     "replace": 4,  # n, a, b, filename
     "replace_version": 3,  # n, string to replace, filename
+    "patch": 1,
 }
 
 # Constants / regexes / limits for validating build steps:
@@ -356,6 +357,12 @@ def validate_build_step(name, module, i, operation, args, strict=False):
         validate_build_step(
             name, module, i, "replace", [n, to_replace, version, filename], strict
         )
+    elif operation == "patch":
+        # TODO: consider what this should validate (CFE-4607):
+        # * perhaps check that the patch file exists here instead of in `apply_patch`?
+        # * what kinds of paths should be legal?
+        # * more validation?
+        pass
     else:
         # TODO: Add more validation of other build steps.
         pass