Made cfbs update strictly validate new versions of modules

cfbs/commands.py

@@ -19,6 +19,7 @@
 from cfbs.utils import (
     CFBSNetworkError,
     CFBSUserError,
+    CFBSValidationError,
     cfbs_filename,
     is_cfbs_repo,
     read_json,
@@ -45,7 +46,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,
@@ -568,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
@@ -582,6 +595,9 @@ def update_command(to_update) -> Result:
     module_updates = ModuleUpdates(config)
     index = None
 
+    old_modules = []
+    new_modules = []
+    update_objects = []
     for update in to_update:
         old_module = config.get_module_from_build(update.name)
         assert (
@@ -611,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
-
         if "url" in old_module:
             path, commit = clone_url_repo(old_module["url"])
             remote_config = CFBSJson(
@@ -663,11 +678,28 @@ 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(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 update, module in zip(update_objects, new_modules):
+        validate_single_module(
+            context="build",
+            name=update.name,
+            module=module,
+            config=None,
+            local_check=True,
+        )
 
+    for old_module, new_module, update in zip(old_modules, new_modules, update_objects):
         update_module(old_module, new_module, module_updates, update)
-
-        # add new items
-
         updated.append(update)
 
     if module_updates.new_deps:
@@ -677,6 +709,21 @@ def update_command(to_update) -> Result:
             for d in module_updates.new_deps
         ]
         config.add_with_dependencies(objects)
+    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:

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

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"])
@@ -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')
@@ -416,8 +420,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 +446,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 +648,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 +720,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 +760,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'"