From fb4f86a1c3f81642a6fe1b358376e7d29aaa29cc Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Thu, 13 Nov 2025 14:03:16 +0100 Subject: [PATCH 1/9] Moved displaying of a message informing about creating a commit to the helper function This improved how the program messages read (especially when in interactive mode, where there will be a prompt about editing the commit message) and reduced code verbosity. Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/commands.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/cfbs/commands.py b/cfbs/commands.py index b98a797f..925ab60a 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -1127,6 +1127,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 +1264,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 +1310,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: @@ -1374,10 +1371,8 @@ def cfbs_convert_git_commit( 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) 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": From 34ecbbf6cf930990f9336ced866b020f96e31191 Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Fri, 14 Nov 2025 14:13:27 +0100 Subject: [PATCH 2/9] Added a helper function that checks whether a CLI tool is present on the system Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/git.py | 8 ++------ cfbs/utils.py | 11 +++++++++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/cfbs/git.py b/cfbs/git.py index d9ea07a3..d79b1205 100644 --- a/cfbs/git.py +++ b/cfbs/git.py @@ -15,7 +15,7 @@ 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): @@ -23,11 +23,7 @@ class CFBSGitError(Exception): 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): diff --git a/cfbs/utils.py b/cfbs/utils.py index abb9afc7..7b76b0a8 100644 --- a/cfbs/utils.py +++ b/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 From 70cb36c6b458cc05b155da2f42185449ba5ffddb Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Fri, 14 Nov 2025 15:04:56 +0100 Subject: [PATCH 3/9] Added the ability to optionally specify a different path when writing a unified diff Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/utils.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/cfbs/utils.py b/cfbs/utils.py index 7b76b0a8..615b2b03 100644 --- a/cfbs/utils.py +++ b/cfbs/utils.py @@ -123,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 From 35d85f699a6b7566c2b15402e8e41a1b4ae33d5f Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Fri, 14 Nov 2025 15:28:19 +0100 Subject: [PATCH 4/9] Added the patch build step, used to patch built masterfiles files Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- JSON.md | 4 ++++ cfbs/build.py | 39 +++++++++++++++++++++++++++++++++++++++ cfbs/validate.py | 7 +++++++ 3 files changed, 50 insertions(+) diff --git a/JSON.md b/JSON.md index 332ebe26..4333293e 100644 --- a/JSON.md +++ b/JSON.md @@ -281,6 +281,10 @@ In `cfbs.json`'s `"steps"`, the build step name must be separated from the rest - `replace_version ` - 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 ` + - 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: diff --git a/cfbs/build.py b/cfbs/build.py index 03276f30..06f4db89 100644 --- a/cfbs/build.py +++ b/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: diff --git a/cfbs/validate.py b/cfbs/validate.py index 099d5ccd..15ff5755 100644 --- a/cfbs/validate.py +++ b/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 From 62bee748fcd88e990a9cad1e8ec4dbdd31d7862e Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Fri, 14 Nov 2025 15:28:40 +0100 Subject: [PATCH 5/9] Implemented support for converting modified files into patch files in cfbs-convert Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/cfbs_config.py | 20 ++++++++++++ cfbs/commands.py | 79 +++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/cfbs/cfbs_config.py b/cfbs/cfbs_config.py index 7d0e5565..bec6b3e1 100644 --- a/cfbs/cfbs_config.py +++ b/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/" + ( diff --git a/cfbs/commands.py b/cfbs/commands.py index 925ab60a..121ecfe0 100644 --- a/cfbs/commands.py +++ b/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, @@ -1319,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.") + + patches_module_present = False print("The following files have custom modifications:") modified_files = analyzed_files.modified for modified_file in modified_files: @@ -1363,10 +1368,9 @@ 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) @@ -1377,6 +1381,75 @@ def cfbs_convert_git_commit( 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 + "/" + + actual_path_modified_file = os.path.join(dir_name, modified_file) + actual_path_original_file = mpf_filepath + + file_diff_data = file_diff_text( + actual_path_original_file, + actual_path_modified_file, + 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 not patches_module_present: + print("Adding patches local module...") + mkdir(patches_dir) + try: + r = add_command( + [patches_module], + added_by="cfbs convert", + explicit_build_steps=["patch " + patch_filename], + ) + # `explicit_build_steps=[]` would fail validation + except Exception as e: + log.warning( + "Adding the patches local module failed (%s), continuing..." + % str(e) + ) + if r != 0: + log.warning("Adding the patches local module failed, continuing...") + + # no need to `cfbs_convert_git_commit` here, the `add_command` will Git commit the added patches local module + patches_module_present = True + else: + config = CFBSConfig.get_instance() + config.add_patch_step(patches_module, patch_filename) + + print("Deleting './%s'..." % modified_file) + rm(actual_path_modified_file) + + try: + cfbs_convert_git_commit( + "Converted './%s' into a .patch file" % modified_file + ) + except: + log.warning("Git commit failed, continuing without committing...") print("Conversion finished successfully.") From 00857655ed22ec0060d930ea5e4bebd70819082b Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Mon, 17 Nov 2025 14:01:51 +0100 Subject: [PATCH 6/9] Improved code quality of original and modified file path variables Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/commands.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/cfbs/commands.py b/cfbs/commands.py index 121ecfe0..335ce914 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -1337,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: @@ -1349,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..." @@ -1374,7 +1375,7 @@ def cfbs_convert_git_commit( if response == "1": print("Deleting './%s'..." % modified_file) - rm(os.path.join(dir_name, modified_file)) + rm(modified_file_path) try: cfbs_convert_git_commit("Deleted './%s'" % modified_file) except: @@ -1386,12 +1387,9 @@ def cfbs_convert_git_commit( patches_dir = "custom-masterfiles-patches" patches_module = "./" + patches_dir + "/" - actual_path_modified_file = os.path.join(dir_name, modified_file) - actual_path_original_file = mpf_filepath - file_diff_data = file_diff_text( - actual_path_original_file, - actual_path_modified_file, + mpf_filepath, + modified_file_path, modified_file, modified_file, ) @@ -1442,7 +1440,7 @@ def cfbs_convert_git_commit( config.add_patch_step(patches_module, patch_filename) print("Deleting './%s'..." % modified_file) - rm(actual_path_modified_file) + rm(modified_file_path) try: cfbs_convert_git_commit( From 389bee55fcc01a5b38bc5048a736b7d5ed96e7eb Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Mon, 17 Nov 2025 16:14:01 +0100 Subject: [PATCH 7/9] Left a note to follow up on the issue of poor Git history for the first file converted to a patch file Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/commands.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cfbs/commands.py b/cfbs/commands.py index 335ce914..295ea3f3 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -1425,6 +1425,7 @@ def cfbs_convert_git_commit( explicit_build_steps=["patch " + patch_filename], ) # `explicit_build_steps=[]` would fail validation + # TODO: rewrite this to temporarily avoid validation to fix the poor Git history for the first file converted to a patch file except Exception as e: log.warning( "Adding the patches local module failed (%s), continuing..." From 2e21c63208c2dbbbdfafd29020bb537b1767fc6b Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Tue, 18 Nov 2025 14:26:33 +0100 Subject: [PATCH 8/9] Do not continue if adding the patches local module failed Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/commands.py | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/cfbs/commands.py b/cfbs/commands.py index 295ea3f3..1a630401 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -1418,21 +1418,13 @@ def cfbs_convert_git_commit( if not patches_module_present: print("Adding patches local module...") mkdir(patches_dir) - try: - r = add_command( - [patches_module], - added_by="cfbs convert", - explicit_build_steps=["patch " + patch_filename], - ) - # `explicit_build_steps=[]` would fail validation - # TODO: rewrite this to temporarily avoid validation to fix the poor Git history for the first file converted to a patch file - except Exception as e: - log.warning( - "Adding the patches local module failed (%s), continuing..." - % str(e) - ) - if r != 0: - log.warning("Adding the patches local module failed, continuing...") + add_command( + [patches_module], + added_by="cfbs convert", + explicit_build_steps=["patch " + patch_filename], + ) + # `explicit_build_steps=[]` would fail validation + # TODO: rewrite this to temporarily avoid validation to fix the poor Git history for the first file converted to a patch file # no need to `cfbs_convert_git_commit` here, the `add_command` will Git commit the added patches local module patches_module_present = True From a0a833c008883e2c3bb7bbb49df8ad727012a78e Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Tue, 18 Nov 2025 14:45:50 +0100 Subject: [PATCH 9/9] Commit the adding of the patches local module and the first patch file conversion together This makes sense since a module with no build steps is not considered valid. Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/commands.py | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/cfbs/commands.py b/cfbs/commands.py index 1a630401..5ed4ed09 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -1323,7 +1323,7 @@ def cfbs_convert_git_commit( if not prompt_user_yesno(non_interactive, "Do you want to continue?"): raise CFBSExitError("User did not proceed, exiting.") - patches_module_present = False + first_patch_conversion = True print("The following files have custom modifications:") modified_files = analyzed_files.modified for modified_file in modified_files: @@ -1415,19 +1415,18 @@ def cfbs_convert_git_commit( continue # make the patches local module on first use - if not patches_module_present: + if first_patch_conversion: print("Adding patches local module...") mkdir(patches_dir) - add_command( + + 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], ) - # `explicit_build_steps=[]` would fail validation - # TODO: rewrite this to temporarily avoid validation to fix the poor Git history for the first file converted to a patch file - - # no need to `cfbs_convert_git_commit` here, the `add_command` will Git commit the added patches local module - patches_module_present = True + config.save() else: config = CFBSConfig.get_instance() config.add_patch_step(patches_module, patch_filename) @@ -1436,12 +1435,20 @@ def cfbs_convert_git_commit( rm(modified_file_path) try: - cfbs_convert_git_commit( - "Converted './%s' into a .patch file" % modified_file - ) + 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.") return 0