From 802937cdc6c62cce36106c8ec293a86da0b340c5 Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Wed, 16 Jul 2025 14:40:57 +0200 Subject: [PATCH 1/4] Made cfbs update strictly validate new versions of modules The rationale for this is that if there are validation errors, there is a good chance something is wrong and the build will fail when it gets to the hub. We want to do what we can to prevent the hub from having a project it cannot build correctly. Signed-off-by: Ole Herman Schumacher Elgesem --- cfbs/commands.py | 34 +++++++++++++++++++++++++++++----- cfbs/validate.py | 48 +++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/cfbs/commands.py b/cfbs/commands.py index 1053e2e5..ab912bf1 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -45,7 +45,11 @@ perform_build, ) from cfbs.cfbs_config import CFBSConfig, CFBSReturnWithoutCommit -from cfbs.validate import validate_config, validate_config_raise_exceptions +from cfbs.validate import ( + validate_config, + validate_config_raise_exceptions, + validate_single_module, +) from cfbs.internal_file_management import ( clone_url_repo, SUPPORTED_URI_SCHEMES, @@ -582,6 +586,8 @@ def update_command(to_update) -> Result: module_updates = ModuleUpdates(config) index = None + old_modules = [] + new_modules = [] for update in to_update: old_module = config.get_module_from_build(update.name) assert ( @@ -611,7 +617,7 @@ def update_command(to_update) -> Result: if not old_module: log.warning("Module '%s' not in build. Skipping its update." % update.name) continue - + old_modules.append(old_module) if "url" in old_module: path, commit = clone_url_repo(old_module["url"]) remote_config = CFBSJson( @@ -663,11 +669,28 @@ def update_command(to_update) -> Result: continue new_module = index_info + new_modules.append(new_module) + + assert len(old_modules) == len(to_update) + assert len(old_modules) == len(new_modules) + + for name, module in zip(to_update, old_modules): + # TODO: Consider removing this, it should not be necessary - + # the old modeules from cfbs.json should already be validated. + # However, it also shouldn't hurt. + # We can also consider if we want validation to be slightly different + # in cfbs update, i.e. allow you to run cfbs update even with an + # invalid config, so long as your updated module actually fix the errors. + validate_single_module( + context="build", name=name, module=module, config=None, local_check=True + ) + for name, module in zip(to_update, new_modules): + validate_single_module( + context="build", name=name, module=module, config=None, local_check=True + ) + for old_module, new_module, update in zip(old_modules, new_modules, to_update): update_module(old_module, new_module, module_updates, update) - - # add new items - updated.append(update) if module_updates.new_deps: @@ -677,6 +700,7 @@ def update_command(to_update) -> Result: for d in module_updates.new_deps ] config.add_with_dependencies(objects) + validate_config_raise_exceptions(config, empty_build_list_ok=False) config.save() if module_updates.changes_made: diff --git a/cfbs/validate.py b/cfbs/validate.py index 5a3eae82..fbde1a72 100644 --- a/cfbs/validate.py +++ b/cfbs/validate.py @@ -210,11 +210,11 @@ def validate_config_raise_exceptions(config, empty_build_list_ok=False): if "index" in raw_data and type(raw_data["index"]) in (dict, OrderedDict): for name, module in raw_data["index"].items(): - _validate_module_object("index", name, module, config) + validate_single_module("index", name, module, config) if "provides" in raw_data: for name, module in raw_data["provides"].items(): - _validate_module_object("provides", name, module, config) + validate_single_module("provides", name, module, config) if config["type"] == "module": validate_module_name_content(config["name"]) @@ -416,8 +416,18 @@ def _validate_module_by(name, module): raise CFBSValidationError(name, '"by" must be non-empty') -def _validate_module_dependencies(name, module, config, context): - if context == "build": +def _validate_module_dependencies(name, module, config, context, local_check=False): + assert name + assert module + assert context in ("build", "provides", "index") + if local_check: + assert config is None + else: + assert config + + if local_check: + search_in = None + elif context == "build": search_in = ("build",) elif context == "provides": search_in = ("index", "provides") @@ -432,6 +442,9 @@ def _validate_module_dependencies(name, module, config, context): for dependency in module["dependencies"]: if type(dependency) is not str: raise CFBSValidationError(name, '"dependencies" must be a list of strings') + if local_check: + continue + assert config if not config.can_reach_dependency(dependency, search_in): raise CFBSValidationError( name, @@ -631,12 +644,33 @@ def _validate_module_input(name, module): ) -def _validate_module_object(context, name, module, config): +def validate_single_module(context, name, module, config, local_check=False): + """Function to validate one module object. + + Called repeatedly for each module in index, provides, and build. + Also called from other places, like in the update command, + before "accepting" new versions of a module object. + Planned to be used by cfbs add as well in the future. + + local_check can be set to True if you don't want to check + references to other parts of the cfbs.json i.e. to disable + checks for dependencies and aliases. In that case, the function must be + called with config = None. + """ assert context in ("index", "provides", "build") + if local_check: + assert config is None + else: + assert config # Step 1 - Handle special cases (alias): if "alias" in module: + if local_check: + raise CFBSValidationError( + "The 'alias' field is not allowed in '%s', inside '$%s'" + % (name, context) + ) # Needs to be validated first because it's missing the other fields: _validate_module_alias(name, module, context, config) return # alias entries would fail the other validation below @@ -682,7 +716,7 @@ def _validate_module_object(context, name, module, config): if "by" in module: _validate_module_by(name, module) if "dependencies" in module: - _validate_module_dependencies(name, module, config, context) + _validate_module_dependencies(name, module, config, context, local_check) if "index" in module: _validate_module_index(name, module) if "version" in module: @@ -722,7 +756,7 @@ def _validate_config_for_build_field(config, empty_build_list_ok=False): # If there are modules in "build" validate them: for index, module in enumerate(config["build"]): name = module["name"] if "name" in module else index - _validate_module_object("build", name, module, config) + validate_single_module("build", name, module, config) elif not empty_build_list_ok: raise CFBSExitError( "The \"build\" field in ./cfbs.json is empty - add modules with 'cfbs add'" From 6c12239a61acb78ca95d493bbb1d7fbadebbbd88 Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Wed, 16 Jul 2025 14:57:40 +0200 Subject: [PATCH 2/4] Allowed cfbs update to fix an invalid cfbs.json If we introduce stricter validation rules inside modules, I think it's nice if cfbs update works so it can get the new valid version and automatically make your cfbs.json valid. Avoided the partial approach of trying to measure if your cfbs.json is more valid than before, that can quickly get hairy. Signed-off-by: Ole Herman Schumacher Elgesem --- cfbs/commands.py | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/cfbs/commands.py b/cfbs/commands.py index ab912bf1..4d24cbc5 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -19,6 +19,7 @@ from cfbs.utils import ( CFBSNetworkError, CFBSUserError, + CFBSValidationError, cfbs_filename, is_cfbs_repo, read_json, @@ -572,7 +573,15 @@ def _someone_needs_me(this) -> bool: @commit_after_command("Updated module%s", [PLURAL_S]) def update_command(to_update) -> Result: config = CFBSConfig.get_instance() - validate_config_raise_exceptions(config, empty_build_list_ok=True) + r = validate_config(config, empty_build_list_ok=True) + valid_before = r == 0 + if not valid_before: + log.warning( + "Your cfbs.json fails validation before update " + + "(see messages above) - " + + "We will attempt update anyway, hoping newer " + + "versions of modules might fix the issues" + ) build = config["build"] # Update all modules in build if none specified @@ -674,16 +683,10 @@ def update_command(to_update) -> Result: assert len(old_modules) == len(to_update) assert len(old_modules) == len(new_modules) - for name, module in zip(to_update, old_modules): - # TODO: Consider removing this, it should not be necessary - - # the old modeules from cfbs.json should already be validated. - # However, it also shouldn't hurt. - # We can also consider if we want validation to be slightly different - # in cfbs update, i.e. allow you to run cfbs update even with an - # invalid config, so long as your updated module actually fix the errors. - validate_single_module( - context="build", name=name, module=module, config=None, local_check=True - ) + # We don't validate old modules here because we want to allow + # cfbs update to fix invalid modules with a newer valid version. + + # Validate new modules, we don't want to add them unless they are valid: for name, module in zip(to_update, new_modules): validate_single_module( context="build", name=name, module=module, config=None, local_check=True @@ -700,7 +703,21 @@ def update_command(to_update) -> Result: for d in module_updates.new_deps ] config.add_with_dependencies(objects) - validate_config_raise_exceptions(config, empty_build_list_ok=False) + r = validate_config(config, empty_build_list_ok=False) + valid_after = r == 0 + if not valid_after: + if valid_before: + raise CFBSValidationError( + "The cfbs.json was valid before update, " + + "but is invalid after adding new versions " + + "of modules - aborting update " + + "(see validation error messages above)" + ) + raise CFBSValidationError( + "The cfbs.json was invalid before update, " + + "but updating modules did not fix it - aborting update" + + "(see validation error messages above)" + ) config.save() if module_updates.changes_made: From 0d1147d89dbdf9f718fa48882c441919d818e4cd Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Wed, 16 Jul 2025 16:00:08 +0200 Subject: [PATCH 3/4] Fixed handling of failing asserts to show failing expression and location Example error message: ``` $ cfbs --non-interactive update promise-type-ansible@0.1.0 Error: Assertion failed - assert name == module["name"] (cfbs/validate.py:377) This is an unexpected error indicating a bug, please create a ticket at: https://northerntech.atlassian.net/ (Rerun with CFBACKTRACE=1 in front of your command to show the full backtrace) ``` Signed-off-by: Ole Herman Schumacher Elgesem --- cfbs/main.py | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/cfbs/main.py b/cfbs/main.py index 4506a37d..d21f2f44 100644 --- a/cfbs/main.py +++ b/cfbs/main.py @@ -6,6 +6,8 @@ import logging as log import sys import os +import traceback +import pathlib from typing import Union from cfbs.result import Result @@ -272,19 +274,50 @@ def main() -> int: return r except CFBSValidationError as e: print("Error: " + str(e)) + return 1 except CFBSExitError as e: print("Error: " + str(e)) + return 1 except CFBSUserError as e: print("Error: " + str(e)) + return 1 except CFBSNetworkError as e: print("Error: " + str(e)) - except (AssertionError, CFBSProgrammerError) as e: - print("Error: " + str(e)) - print( - "This is an unexpected error indicating a bug, please create a ticket at:" + return 1 + # AssertionError and CFBSProgrammerError are not expected, print extra info: + except AssertionError as e: + tb = traceback.extract_tb(e.__traceback__) + frame = tb[-1] + this_file = pathlib.Path(__file__) + cfbs_prefix = os.path.abspath(this_file.parent.parent.resolve()) + filename = os.path.abspath(frame.filename) + # Opportunistically cut off beginning of path if possible: + if filename.startswith(cfbs_prefix): + filename = filename[len(cfbs_prefix) :] + if filename.startswith("/"): + filename = filename[1:] + line = frame.lineno + # Avoid using frame.colno - it was not available in python 3.5, + # and even in the latest version, it is not declared in the + # docstring, so you will get linting warnings; + # https://github.com/python/cpython/blob/v3.13.5/Lib/traceback.py#L276-L288 + # column = frame.colno + assertion = frame.line + explanation = str(e) + message = "Assertion failed - %s%s (%s:%s)" % ( + assertion, + (" - " + explanation) if explanation else "", + filename, + line, ) - print("https://northerntech.atlassian.net/") - print("(Rerun with CFBACKTRACE=1 in front of your command to show backtraces)") + print("Error: " + message) + except CFBSProgrammerError as e: + print("Error: " + str(e)) + print("This is an unexpected error indicating a bug, please create a ticket at:") + print("https://northerntech.atlassian.net/") + print( + "(Rerun with CFBACKTRACE=1 in front of your command to show the full backtrace)" + ) # TODO: Handle other exceptions return 1 From 97c1d630aa62d5283116abd9dda86cab019c36b4 Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Wed, 16 Jul 2025 20:06:20 +0200 Subject: [PATCH 4/4] Fixed mistakes introduced in update logic Signed-off-by: Ole Herman Schumacher Elgesem --- cfbs/commands.py | 16 +++++++++++----- cfbs/validate.py | 6 +++++- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/cfbs/commands.py b/cfbs/commands.py index 4d24cbc5..e2f45643 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -597,6 +597,7 @@ def update_command(to_update) -> Result: old_modules = [] new_modules = [] + update_objects = [] for update in to_update: old_module = config.get_module_from_build(update.name) assert ( @@ -626,7 +627,6 @@ def update_command(to_update) -> Result: if not old_module: log.warning("Module '%s' not in build. Skipping its update." % update.name) continue - old_modules.append(old_module) if "url" in old_module: path, commit = clone_url_repo(old_module["url"]) remote_config = CFBSJson( @@ -678,21 +678,27 @@ def update_command(to_update) -> Result: continue new_module = index_info + update_objects.append(update) + old_modules.append(old_module) new_modules.append(new_module) - assert len(old_modules) == len(to_update) + assert len(old_modules) == len(update_objects) assert len(old_modules) == len(new_modules) # We don't validate old modules here because we want to allow # cfbs update to fix invalid modules with a newer valid version. # Validate new modules, we don't want to add them unless they are valid: - for name, module in zip(to_update, new_modules): + for update, module in zip(update_objects, new_modules): validate_single_module( - context="build", name=name, module=module, config=None, local_check=True + context="build", + name=update.name, + module=module, + config=None, + local_check=True, ) - for old_module, new_module, update in zip(old_modules, new_modules, to_update): + for old_module, new_module, update in zip(old_modules, new_modules, update_objects): update_module(old_module, new_module, module_updates, update) updated.append(update) diff --git a/cfbs/validate.py b/cfbs/validate.py index fbde1a72..dbde1655 100644 --- a/cfbs/validate.py +++ b/cfbs/validate.py @@ -372,8 +372,12 @@ def _validate_module_alias(name, module, context, config): raise CFBSValidationError(name, '"alias" cannot reference another alias') -def _validate_module_name(name, module): +def _validate_module_name(name: str, module): assert "name" in module + assert name + assert type(name) is str + assert module["name"] + assert type(module["name"]) is str assert name == module["name"] if type(module["name"]) is not str: raise CFBSValidationError(name, '"name" must be of type string')