From cd86c954e521b937a3a26b436ccd7111385eb411 Mon Sep 17 00:00:00 2001 From: Silke Hofstra Date: Sat, 30 Mar 2024 16:16:37 +0100 Subject: [PATCH 1/4] ci/package_checks: Improve output when called outside of CI **Summary** Configure a decent logger and log results when not running in the CI. --- common/CI/package_checks.py | 65 +++++++++++++++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 2 deletions(-) diff --git a/common/CI/package_checks.py b/common/CI/package_checks.py index fc16ee2894..bdae4659dc 100755 --- a/common/CI/package_checks.py +++ b/common/CI/package_checks.py @@ -2,9 +2,11 @@ import argparse import glob import json +import logging import os.path import re import subprocess +import sys from dataclasses import dataclass from datetime import datetime, timezone from enum import Enum @@ -15,6 +17,11 @@ from xml.etree import ElementTree import yaml +def in_ci() -> bool: + """Returns true if running in GitHub Actions or GitLab CI.""" + return os.environ.get('CI') == 'true' + + @dataclass class FreezeConfig: start: Optional[datetime] @@ -89,6 +96,28 @@ class Git: return self.run_lines('diff', '--name-only', '--diff-filter=AM') +class LogFormatter(logging.Formatter): + fmt = '\033[0m \033[94m%(pathname)s:%(lineno)d:\033[0m %(message)s' + formatters = { + logging.DEBUG: logging.Formatter('\033[1;30mDBG' + fmt), + logging.INFO: logging.Formatter('\033[34mINF' + fmt), + logging.WARNING: logging.Formatter('\033[33mWRN' + fmt), + logging.ERROR: logging.Formatter('\033[31mERR' + fmt), + logging.CRITICAL: logging.Formatter('\033[31mCRI' + fmt), + } + + def format(self, record: logging.LogRecord) -> str: + return self.formatters[record.levelno].format(record) + + @staticmethod + def handler() -> logging.Handler: + handler = logging.StreamHandler(sys.stderr) + handler.setLevel(logging.DEBUG) + handler.setFormatter(LogFormatter()) + + return handler + + class Level(str, Enum): __str__ = Enum.__str__ DEBUG = 'debug' @@ -96,6 +125,20 @@ class Level(str, Enum): ERROR = 'error' WARNING = 'warning' + @property + def log_level(self) -> int: + match self: + case Level.DEBUG: + return logging.DEBUG + case Level.NOTICE: + return logging.INFO + case Level.WARNING: + return logging.WARNING + case Level.ERROR: + return logging.ERROR + case _: + return 0 + @dataclass class Result: @@ -109,7 +152,14 @@ class Result: endLine: Optional[int] = None def __str__(self) -> str: - return f'::{self.level}{self._meta}::{self._message}' + return f'::{self.level.value}{self._meta}::{self._message}' + + def log(self) -> None: + if in_ci(): + print(str(self)) + return + + logging.getLogger(__name__).handle(self._record) @property def _message(self) -> str: @@ -132,6 +182,15 @@ class Result: return f'{key}={value}' + @property + def _record(self) -> logging.LogRecord: + return logging.LogRecord('', + self.level.log_level, + self.file or '', + self.line or 1, + self.message, + None, None) + class PullRequestCheck: _package_files = ['package.yml'] @@ -644,7 +703,7 @@ class Checker: print(f"Found {len(results)} result(s), {len(errors)} error(s)") for result in results: - print(result) + result.log() self.write_summary() @@ -663,6 +722,8 @@ class Checker: if __name__ == "__main__": + logging.basicConfig(level=logging.INFO, handlers=[LogFormatter.handler()]) + parser = argparse.ArgumentParser() parser.add_argument('--base', type=str, help='Optional reference to the base branch') From fae0cf387723dc32ca32ace4b2da470a69f0ea19 Mon Sep 17 00:00:00 2001 From: Silke Hofstra Date: Sat, 30 Mar 2024 17:28:35 +0100 Subject: [PATCH 2/4] hooks/pre-commit: Show output from package checks **Summary** Show the output from the package checks so the committer is aware that there are warnings or informational messages for the package. --- common/Hooks/pre-commit.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/common/Hooks/pre-commit.py b/common/Hooks/pre-commit.py index 50ba88142a..597e796a1c 100755 --- a/common/Hooks/pre-commit.py +++ b/common/Hooks/pre-commit.py @@ -8,11 +8,9 @@ package_checks = os.path.join(common_dir, 'common', 'CI', 'package_checks.py') def _run(name: str, *args: str) -> bool: - res = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True) + res = subprocess.run(args, stdout=subprocess.PIPE, text=True) if res.returncode != 0: print(f'{name} failed: {" ".join(args)}') - print('Output:') - print("\n".join([' ' + line for line in res.stdout.split("\n")])) return res.returncode == 0 From 0f280537337e3615b298cdf6315c368378dc7431 Mon Sep 17 00:00:00 2001 From: Silke Hofstra Date: Sat, 30 Mar 2024 16:26:46 +0100 Subject: [PATCH 3/4] ci/package_checks: Create and use classes for `package.yml` and `pspec.xml` --- common/CI/package_checks.py | 109 ++++++++++++++++++++++++++---------- 1 file changed, 78 insertions(+), 31 deletions(-) diff --git a/common/CI/package_checks.py b/common/CI/package_checks.py index bdae4659dc..f1d4dc786f 100755 --- a/common/CI/package_checks.py +++ b/common/CI/package_checks.py @@ -22,6 +22,55 @@ def in_ci() -> bool: return os.environ.get('CI') == 'true' +class PackageYML: + """Represents a Package YML file.""" + + def __init__(self, stream: Any): + self._data = dict(yaml.safe_load(stream)) + + @property + def name(self) -> str: + return str(self._data['name']) + + @property + def version(self) -> str: + return str(self._data['version']) + + @property + def release(self) -> int: + return int(self._data['release']) + + @property + def homepage(self) -> Optional[str]: + return self._data.get('homepage') + + def get(self, key: str, default: Any = None) -> Any: + return self._data.get(key, default) + + +class PspecXML: + """Represents a Pspec XML file.""" + + def __init__(self, data: str): + self._xml = ElementTree.fromstring(data) + + @property + def release(self) -> int: + return int(self._xml.findall('.//Update')[0].attrib['release']) + + @property + def homepage(self) -> Optional[str]: + xml_homepage_element = self._xml.find('.//Homepage') + if xml_homepage_element is not None: + return xml_homepage_element.text + + return None + + @property + def files(self) -> List[str]: + return [str(element.text) for element in self._xml.findall('.//Path')] + + @dataclass class FreezeConfig: start: Optional[datetime] @@ -228,12 +277,22 @@ class PullRequestCheck: def _open(self, path: str) -> TextIO: return open(self._path(path), 'r') - def load_package_yml(self, file: str) -> Dict[str, Any]: - with self._open(file) as f: - return dict(yaml.safe_load(f)) + def _read(self, path: str) -> str: + with self._open(path) as f: + return str(f.read()) - def load_package_yml_from_commit(self, ref: str, file: str) -> Dict[str, Any]: - return dict(yaml.safe_load(self.git.file_from_commit(ref, file))) + def load_package_yml(self, file: str) -> PackageYML: + with self._open(file) as f: + return PackageYML(f) + + def load_package_yml_from_commit(self, ref: str, file: str) -> PackageYML: + return PackageYML(self.git.file_from_commit(ref, file)) + + def load_pspec_xml(self, file: str) -> PspecXML: + return PspecXML(self._read(file)) + + def load_pspec_xml_from_commit(self, ref: str, file: str) -> PspecXML: + return PspecXML(self.git.file_from_commit(ref, file)) def file_line(self, file: str, expr: str) -> Optional[int]: with self._open(file) as f: @@ -352,13 +411,13 @@ class PackageBumped(PullRequestCheck): try: old = self.load_package_yml_from_commit(base, file) - if int(old['release']) + 1 != int(new['release']): + if old.release + 1 != new.release: return Result(level=self._level, file=file, message=self._msg) return None except Exception as e: if 'exists on disk, but not in' in str(e): - if int(new['release']) != 1: + if new.release != 1: return Result(level=self._level, file=file, message=self._msg_new) return None @@ -442,9 +501,7 @@ class PackageVersion(PullRequestCheck): if not self._check_version(path)] def _check_version(self, path: str) -> bool: - version = self.load_package_yml(path)['version'] - - return isinstance(version, str) + return isinstance(self.load_package_yml(path).version, str) class Patch(PullRequestCheck): @@ -481,7 +538,7 @@ class SPDXLicense(PullRequestCheck): for r in self._validate_spdx(f) if r] def _validate_spdx(self, file: str) -> List[Optional[Result]]: - license = self.load_package_yml(file)['license'] + license = self.load_package_yml(file).get('license') if isinstance(license, list): return [self._validate_license(file, id) for id in license] @@ -551,26 +608,16 @@ class Pspec(PullRequestCheck): if not self._check_consistent(path)] def _check_consistent(self, package_dir: str) -> bool: - xml = ElementTree.parse(self._xml_file(package_dir)) - xml_release = int(xml.findall('.//Update')[0].attrib['release']) - xml_homepage: str = '' - xml_homepage_element = xml.find('.//Homepage') + xml = self._xml_file(package_dir) + yml = self._yml_file(package_dir) - if xml_homepage_element is not None: - xml_homepage = xml_homepage_element.text or '' + return bool(yml.release == xml.release and yml.homepage == xml.homepage) - with open(self._yml_file(package_dir), 'r') as f: - yml = yaml.safe_load(f) - yml_release = yml.get('release', '') - yml_homepage = yml.get('homepage', '') + def _yml_file(self, package_dir: str) -> PackageYML: + return self.load_package_yml(os.path.join(package_dir, 'package.yml')) - return bool(yml_release == xml_release and yml_homepage == xml_homepage) - - def _yml_file(self, package_dir: str) -> str: - return self._path(os.path.join(package_dir, 'package.yml')) - - def _xml_file(self, package_dir: str) -> str: - return self._path(os.path.join(package_dir, 'pspec_x86_64.xml')) + def _xml_file(self, package_dir: str) -> PspecXML: + return self.load_pspec_xml(os.path.join(package_dir, 'pspec_x86_64.xml')) class SystemDependencies(PullRequestCheck): @@ -588,7 +635,7 @@ class SystemDependencies(PullRequestCheck): return self._validate_deps(pkg) if self._components_match(pkg_components) else [] def _package_components(self, pkg: str) -> List[str]: - return self._unwrap_component(self.load_package_yml(self.package_yml_path(pkg))['component']) + return self._unwrap_component(self.load_package_yml(self.package_yml_path(pkg)).get('component')) def _unwrap_component(self, component: Any) -> List[str]: if isinstance(component, dict): @@ -646,9 +693,9 @@ class SummaryGenerator(PullRequestCheck): if package is None: return None - return f"{package['name']}-{package['version']}-{package['release']}" + return f"{package.name}-{package.version}-{package.release}" - def _commit_package_yaml(self, ref: str) -> Optional[Dict[str, Any]]: + def _commit_package_yaml(self, ref: str) -> Optional[PackageYML]: files = [f for f in self.git.files_in_commit(ref) if os.path.basename(f) == 'package.yml'] if len(files) == 0: return None From 05ff11fecabf0a0e69d43617b93846ae36774b30 Mon Sep 17 00:00:00 2001 From: Silke Hofstra Date: Sat, 30 Mar 2024 16:29:24 +0100 Subject: [PATCH 4/4] ci/package_checks: Show an error when the pspec has not been bumped --- common/CI/package_checks.py | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/common/CI/package_checks.py b/common/CI/package_checks.py index f1d4dc786f..900d02ec33 100755 --- a/common/CI/package_checks.py +++ b/common/CI/package_checks.py @@ -10,12 +10,15 @@ import sys from dataclasses import dataclass from datetime import datetime, timezone from enum import Enum -from typing import Any, Dict, List, Optional, TextIO, Tuple, Union +from typing import Any, Callable, Dict, List, Optional, TextIO, Tuple, Union from urllib import request from xml.etree import ElementTree import yaml +"""Package is either a Package YML file or Pspec XML file.""" +Package = Union['PackageYML', 'PspecXML'] + def in_ci() -> bool: """Returns true if running in GitHub Actions or GitLab CI.""" @@ -398,27 +401,35 @@ class Homepage(PullRequestCheck): class PackageBumped(PullRequestCheck): _msg = 'Package release is not incremented by 1' _msg_new = 'Package release is not 1' - _level = Level.WARNING def run(self) -> List[Result]: results = [self._check_commit(self.base or 'HEAD', file) - for file in self.package_files] + for file in self.files] return [result for result in results if result is not None] def _check_commit(self, base: str, file: str) -> Optional[Result]: - new = self.load_package_yml(file) + match os.path.basename(file): + case 'package.yml': + return self._check(base, file, PackageYML, Level.WARNING) + case 'pspec_x86_64.xml': + return self._check(base, file, PspecXML, Level.ERROR) + case _: + return None + + def _check(self, base: str, file: str, loader: Callable[[str], Package], level: Level) -> Optional[Result]: + new = loader(self._read(file)) try: - old = self.load_package_yml_from_commit(base, file) + old = loader(self.git.file_from_commit(base, file)) if old.release + 1 != new.release: - return Result(level=self._level, file=file, message=self._msg) + return Result(level=level, file=file, message=self._msg) return None except Exception as e: if 'exists on disk, but not in' in str(e): if new.release != 1: - return Result(level=self._level, file=file, message=self._msg_new) + return Result(level=level, file=file, message=self._msg_new) return None