From 25c8f40bef4b2fb9e410023998cf97d5ab24483d Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jun 16 2021 09:37:39 +0000 Subject: [PATCH 1/8] Enable not overwriting original spec file Fixes: #121 Signed-off-by: Nils Philippsen --- diff --git a/rpmautospec/process_distgit.py b/rpmautospec/process_distgit.py index 80ccca0..d35c942 100644 --- a/rpmautospec/process_distgit.py +++ b/rpmautospec/process_distgit.py @@ -35,10 +35,15 @@ def register_subcommand(subparsers): help="Path to package worktree or the spec file within", ) + process_distgit_parser.add_argument( + "target", + help="Path where to write processed spec file", + ) + return subcmd_name -def process_distgit(spec_or_path: Union[Path, str]) -> bool: +def process_distgit(spec_or_path: Union[Path, str], target: Union[Path, str] = None) -> bool: """Process an RPM spec file in a distgit repository. :param spec_or_path: the spec file or path of the repository @@ -46,6 +51,11 @@ def process_distgit(spec_or_path: Union[Path, str]) -> bool: """ processor = PkgHistoryProcessor(spec_or_path) + if target is None: + target = processor.specfile + elif isinstance(target, Path): + target = Path(target) + features = check_specfile_features(processor.specfile) processing_necessary = ( features.has_autorelease or features.has_autochangelog or not features.changelog_lineno @@ -98,10 +108,10 @@ def process_distgit(spec_or_path: Union[Path, str]) -> bool: tmp_specfile.flush() # ...and copy it back (potentially across device boundaries) - shutil.copy2(tmp_specfile.name, processor.specfile) + shutil.copy2(tmp_specfile.name, target) def main(args): """Main method.""" spec_or_path = args.spec_or_path.rstrip(os.path.sep) - process_distgit(spec_or_path) + process_distgit(spec_or_path, args.target) diff --git a/tests/rpmautospec/test_process_distgit.py b/tests/rpmautospec/test_process_distgit.py index 76574b6..6ca2cc9 100644 --- a/tests/rpmautospec/test_process_distgit.py +++ b/tests/rpmautospec/test_process_distgit.py @@ -103,6 +103,7 @@ class TestProcessDistgit: env=env, ) + @pytest.mark.parametrize("overwrite_specfile", (False, True)) @pytest.mark.parametrize("dirty_worktree", (False, True)) @pytest.mark.parametrize("autorelease_case", ("unchanged", "with braces", "optional")) @pytest.mark.parametrize( @@ -118,7 +119,9 @@ class TestProcessDistgit: "optional", ), ) - def test_process_distgit(self, dirty_worktree, autorelease_case, autochangelog_case): + def test_process_distgit( + self, overwrite_specfile, dirty_worktree, autorelease_case, autochangelog_case + ): """Test the process_distgit() function""" with tempfile.TemporaryDirectory() as workdir: with tarfile.open( @@ -146,7 +149,20 @@ class TestProcessDistgit: run_git_amend=not dirty_worktree, ) - process_distgit.process_distgit(unpacked_repo_dir) + if overwrite_specfile: + target_spec_file_path = None + else: + target_spec_file_path = os.path.join(workdir, "test-this-specfile-please.spec") + + orig_test_spec_file_stat = os.stat(test_spec_file_path) + process_distgit.process_distgit(unpacked_repo_dir, target_spec_file_path) + if not overwrite_specfile: + test_spec_file_stat = os.stat(test_spec_file_path) + # we can't compare stat_results directly because st_atime has changed + for attr in ("mode", "ino", "dev", "uid", "gid", "size", "mtime", "ctime"): + assert getattr(test_spec_file_stat, "st_" + attr) == getattr( + orig_test_spec_file_stat, "st_" + attr + ) expected_spec_file_path = os.path.join( __here__, @@ -178,7 +194,10 @@ class TestProcessDistgit: rpm_cmd = ["rpm", "--define", "dist .fc32", "--specfile"] - test_cmd = rpm_cmd + [test_spec_file_path] + if target_spec_file_path: + test_cmd = rpm_cmd + [target_spec_file_path] + else: + test_cmd = rpm_cmd + [test_spec_file_path] expected_cmd = rpm_cmd + [expected_spec_file_path] q_release = ["--qf", "%{release}\n"] From 5514d9f555d52fdb9a5f52acf2787e14824ec373 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jun 16 2021 09:37:42 +0000 Subject: [PATCH 2/8] Move subcommand modules into their own package This is needed so that exposing process_distgit() (the function) doesn't make process_distgit (the module) inaccessible (and is better namespacing anyway). Signed-off-by: Nils Philippsen --- diff --git a/koji_plugins/rpmautospec_builder.py b/koji_plugins/rpmautospec_builder.py index 58c51a4..2f757d4 100644 --- a/koji_plugins/rpmautospec_builder.py +++ b/koji_plugins/rpmautospec_builder.py @@ -2,7 +2,7 @@ import logging from koji.plugin import callback -from rpmautospec.process_distgit import process_distgit +from rpmautospec.subcommands.process_distgit import process_distgit log = logging.getLogger(__name__) diff --git a/rpmautospec/changelog.py b/rpmautospec/changelog.py deleted file mode 100644 index 45b1f53..0000000 --- a/rpmautospec/changelog.py +++ /dev/null @@ -1,60 +0,0 @@ -import logging -from typing import Any, Dict, Optional, Union - -from .pkg_history import PkgHistoryProcessor - - -log = logging.getLogger(__name__) - - -def register_subcommand(subparsers): - subcmd_name = "generate-changelog" - - gen_changelog_parser = subparsers.add_parser( - subcmd_name, - help="Generate changelog entries from git commit logs", - ) - - gen_changelog_parser.add_argument( - "spec_or_path", - default=".", - nargs="?", - help="Path to package worktree or the spec file within", - ) - - return subcmd_name - - -def _coerce_to_str(str_or_bytes: Union[str, bytes]) -> str: - if isinstance(str_or_bytes, bytes): - str_or_bytes = str_or_bytes.decode("utf-8", errors="replace") - return str_or_bytes - - -def _coerce_to_bytes(str_or_bytes: Union[str, bytes]) -> str: - if isinstance(str_or_bytes, str): - str_or_bytes = str_or_bytes.encode("utf-8") - return str_or_bytes - - -def collate_changelog( - processor_results: Dict[str, Any], result_type: Optional[type] = str -) -> Union[str, bytes]: - changelog = processor_results["changelog"] - if result_type == str: - entry_strings = (_coerce_to_str(entry["data"]) for entry in changelog) - else: - entry_strings = (_coerce_to_bytes(entry["data"]) for entry in changelog) - return "\n\n".join(entry_strings) - - -def produce_changelog(spec_or_repo): - processor = PkgHistoryProcessor(spec_or_repo) - result = processor.run(visitors=(processor.release_number_visitor, processor.changelog_visitor)) - return collate_changelog(result) - - -def main(args): - """Main method.""" - changelog = produce_changelog(args.spec_or_path) - log.info(changelog) diff --git a/rpmautospec/cli.py b/rpmautospec/cli.py index b49f5e1..6bc28d5 100644 --- a/rpmautospec/cli.py +++ b/rpmautospec/cli.py @@ -3,7 +3,7 @@ import logging import sys import typing -from . import changelog, process_distgit, release +from .subcommands import changelog, process_distgit, release subcmd_modules_by_name = {} diff --git a/rpmautospec/process_distgit.py b/rpmautospec/process_distgit.py deleted file mode 100644 index d35c942..0000000 --- a/rpmautospec/process_distgit.py +++ /dev/null @@ -1,117 +0,0 @@ -import logging -import os -import shutil -import tempfile -from pathlib import Path -from typing import Union - -from .misc import check_specfile_features -from .pkg_history import PkgHistoryProcessor - - -log = logging.getLogger(__name__) -__here__ = os.path.dirname(__file__) - -autorelease_template = """## START: Set by rpmautospec -%define autorelease(e:s:pb:) %{{?-p:0.}}%{{lua: - release_number = {autorelease_number:d}; - base_release_number = tonumber(rpm.expand("%{{?-b*}}%{{!?-b:1}}")); - print(release_number + base_release_number - 1); -}}%{{?-e:.%{{-e*}}}}%{{?-s:.%{{-s*}}}}%{{?dist}} -## END: Set by rpmautospec -""" # noqa: E501 - - -def register_subcommand(subparsers): - subcmd_name = "process-distgit" - - process_distgit_parser = subparsers.add_parser( - subcmd_name, - help="Modify the contents of the specfile according to the repo", - ) - - process_distgit_parser.add_argument( - "spec_or_path", - help="Path to package worktree or the spec file within", - ) - - process_distgit_parser.add_argument( - "target", - help="Path where to write processed spec file", - ) - - return subcmd_name - - -def process_distgit(spec_or_path: Union[Path, str], target: Union[Path, str] = None) -> bool: - """Process an RPM spec file in a distgit repository. - - :param spec_or_path: the spec file or path of the repository - :return: whether or not the spec file needed processing - """ - processor = PkgHistoryProcessor(spec_or_path) - - if target is None: - target = processor.specfile - elif isinstance(target, Path): - target = Path(target) - - features = check_specfile_features(processor.specfile) - processing_necessary = ( - features.has_autorelease or features.has_autochangelog or not features.changelog_lineno - ) - if not processing_necessary: - return False - - needs_autochangelog = ( - features.changelog_lineno is None - and features.autochangelog_lineno is None - or features.has_autochangelog - ) - - visitors = [processor.release_number_visitor] - if needs_autochangelog: - visitors.append(processor.changelog_visitor) - result = processor.run(visitors=visitors) - - autorelease_number = result["release-number"] - - with processor.specfile.open("r") as specfile, tempfile.NamedTemporaryFile("w") as tmp_specfile: - # Process the spec file into a temporary file... - if features.has_autorelease: - # Write %autorelease macro header - print( - autorelease_template.format(autorelease_number=autorelease_number), - file=tmp_specfile, - ) - - for lineno, line in enumerate(specfile, start=1): - if features.changelog_lineno: - if features.has_autochangelog and lineno > features.changelog_lineno: - break - - else: - if features.has_autochangelog and lineno == features.autochangelog_lineno: - print("%changelog\n", file=tmp_specfile, end="") - break - print(line, file=tmp_specfile, end="") - - if not features.has_autochangelog and features.changelog_lineno is None: - print("\n%changelog\n", file=tmp_specfile, end="") - - if needs_autochangelog: - print( - "\n\n".join(entry["data"] for entry in result["changelog"]), - file=tmp_specfile, - ) - - tmp_specfile.flush() - - # ...and copy it back (potentially across device boundaries) - shutil.copy2(tmp_specfile.name, target) - - -def main(args): - """Main method.""" - spec_or_path = args.spec_or_path.rstrip(os.path.sep) - process_distgit(spec_or_path, args.target) diff --git a/rpmautospec/release.py b/rpmautospec/release.py deleted file mode 100644 index dee5b61..0000000 --- a/rpmautospec/release.py +++ /dev/null @@ -1,38 +0,0 @@ -import logging -from pathlib import Path -from typing import Union - -from .pkg_history import PkgHistoryProcessor - - -log = logging.getLogger(__name__) - - -def register_subcommand(subparsers): - subcmd_name = "calculate-release" - - calc_release_parser = subparsers.add_parser( - subcmd_name, - help="Calculate the next release tag for a package build", - ) - - calc_release_parser.add_argument( - "spec_or_path", - default=".", - nargs="?", - help="Path to package worktree or the spec file within", - ) - - return subcmd_name - - -def calculate_release(spec_or_path: Union[str, Path]) -> int: - processor = PkgHistoryProcessor(spec_or_path) - result = processor.run(visitors=(processor.release_number_visitor,)) - return result["release-complete"] - - -def main(args): - """Main method.""" - release = calculate_release(args.spec_or_path) - log.info("calculate_release release: %s", release) diff --git a/rpmautospec/subcommands/__init__.py b/rpmautospec/subcommands/__init__.py new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/rpmautospec/subcommands/__init__.py diff --git a/rpmautospec/subcommands/changelog.py b/rpmautospec/subcommands/changelog.py new file mode 100644 index 0000000..a1607ca --- /dev/null +++ b/rpmautospec/subcommands/changelog.py @@ -0,0 +1,60 @@ +import logging +from typing import Any, Dict, Optional, Union + +from ..pkg_history import PkgHistoryProcessor + + +log = logging.getLogger(__name__) + + +def register_subcommand(subparsers): + subcmd_name = "generate-changelog" + + gen_changelog_parser = subparsers.add_parser( + subcmd_name, + help="Generate changelog entries from git commit logs", + ) + + gen_changelog_parser.add_argument( + "spec_or_path", + default=".", + nargs="?", + help="Path to package worktree or the spec file within", + ) + + return subcmd_name + + +def _coerce_to_str(str_or_bytes: Union[str, bytes]) -> str: + if isinstance(str_or_bytes, bytes): + str_or_bytes = str_or_bytes.decode("utf-8", errors="replace") + return str_or_bytes + + +def _coerce_to_bytes(str_or_bytes: Union[str, bytes]) -> str: + if isinstance(str_or_bytes, str): + str_or_bytes = str_or_bytes.encode("utf-8") + return str_or_bytes + + +def collate_changelog( + processor_results: Dict[str, Any], result_type: Optional[type] = str +) -> Union[str, bytes]: + changelog = processor_results["changelog"] + if result_type == str: + entry_strings = (_coerce_to_str(entry["data"]) for entry in changelog) + else: + entry_strings = (_coerce_to_bytes(entry["data"]) for entry in changelog) + return "\n\n".join(entry_strings) + + +def produce_changelog(spec_or_repo): + processor = PkgHistoryProcessor(spec_or_repo) + result = processor.run(visitors=(processor.release_number_visitor, processor.changelog_visitor)) + return collate_changelog(result) + + +def main(args): + """Main method.""" + changelog = produce_changelog(args.spec_or_path) + log.info(changelog) diff --git a/rpmautospec/subcommands/process_distgit.py b/rpmautospec/subcommands/process_distgit.py new file mode 100644 index 0000000..c90cf8c --- /dev/null +++ b/rpmautospec/subcommands/process_distgit.py @@ -0,0 +1,117 @@ +import logging +import os +import shutil +import tempfile +from pathlib import Path +from typing import Union + +from ..misc import check_specfile_features +from ..pkg_history import PkgHistoryProcessor + + +log = logging.getLogger(__name__) +__here__ = os.path.dirname(__file__) + +autorelease_template = """## START: Set by rpmautospec +%define autorelease(e:s:pb:) %{{?-p:0.}}%{{lua: + release_number = {autorelease_number:d}; + base_release_number = tonumber(rpm.expand("%{{?-b*}}%{{!?-b:1}}")); + print(release_number + base_release_number - 1); +}}%{{?-e:.%{{-e*}}}}%{{?-s:.%{{-s*}}}}%{{?dist}} +## END: Set by rpmautospec +""" # noqa: E501 + + +def register_subcommand(subparsers): + subcmd_name = "process-distgit" + + process_distgit_parser = subparsers.add_parser( + subcmd_name, + help="Modify the contents of the specfile according to the repo", + ) + + process_distgit_parser.add_argument( + "spec_or_path", + help="Path to package worktree or the spec file within", + ) + + process_distgit_parser.add_argument( + "target", + help="Path where to write processed spec file", + ) + + return subcmd_name + + +def process_distgit(spec_or_path: Union[Path, str], target: Union[Path, str] = None) -> bool: + """Process an RPM spec file in a distgit repository. + + :param spec_or_path: the spec file or path of the repository + :return: whether or not the spec file needed processing + """ + processor = PkgHistoryProcessor(spec_or_path) + + if target is None: + target = processor.specfile + elif isinstance(target, Path): + target = Path(target) + + features = check_specfile_features(processor.specfile) + processing_necessary = ( + features.has_autorelease or features.has_autochangelog or not features.changelog_lineno + ) + if not processing_necessary: + return False + + needs_autochangelog = ( + features.changelog_lineno is None + and features.autochangelog_lineno is None + or features.has_autochangelog + ) + + visitors = [processor.release_number_visitor] + if needs_autochangelog: + visitors.append(processor.changelog_visitor) + result = processor.run(visitors=visitors) + + autorelease_number = result["release-number"] + + with processor.specfile.open("r") as specfile, tempfile.NamedTemporaryFile("w") as tmp_specfile: + # Process the spec file into a temporary file... + if features.has_autorelease: + # Write %autorelease macro header + print( + autorelease_template.format(autorelease_number=autorelease_number), + file=tmp_specfile, + ) + + for lineno, line in enumerate(specfile, start=1): + if features.changelog_lineno: + if features.has_autochangelog and lineno > features.changelog_lineno: + break + + else: + if features.has_autochangelog and lineno == features.autochangelog_lineno: + print("%changelog\n", file=tmp_specfile, end="") + break + print(line, file=tmp_specfile, end="") + + if not features.has_autochangelog and features.changelog_lineno is None: + print("\n%changelog\n", file=tmp_specfile, end="") + + if needs_autochangelog: + print( + "\n\n".join(entry["data"] for entry in result["changelog"]), + file=tmp_specfile, + ) + + tmp_specfile.flush() + + # ...and copy it back (potentially across device boundaries) + shutil.copy2(tmp_specfile.name, target) + + +def main(args): + """Main method.""" + spec_or_path = args.spec_or_path.rstrip(os.path.sep) + process_distgit(spec_or_path, args.target) diff --git a/rpmautospec/subcommands/release.py b/rpmautospec/subcommands/release.py new file mode 100644 index 0000000..8e5de13 --- /dev/null +++ b/rpmautospec/subcommands/release.py @@ -0,0 +1,38 @@ +import logging +from pathlib import Path +from typing import Union + +from ..pkg_history import PkgHistoryProcessor + + +log = logging.getLogger(__name__) + + +def register_subcommand(subparsers): + subcmd_name = "calculate-release" + + calc_release_parser = subparsers.add_parser( + subcmd_name, + help="Calculate the next release tag for a package build", + ) + + calc_release_parser.add_argument( + "spec_or_path", + default=".", + nargs="?", + help="Path to package worktree or the spec file within", + ) + + return subcmd_name + + +def calculate_release(spec_or_path: Union[str, Path]) -> int: + processor = PkgHistoryProcessor(spec_or_path) + result = processor.run(visitors=(processor.release_number_visitor,)) + return result["release-complete"] + + +def main(args): + """Main method.""" + release = calculate_release(args.spec_or_path) + log.info("calculate_release release: %s", release) diff --git a/tests/rpmautospec/subcommands/__init__.py b/tests/rpmautospec/subcommands/__init__.py new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/tests/rpmautospec/subcommands/__init__.py diff --git a/tests/rpmautospec/subcommands/test_process_distgit.py b/tests/rpmautospec/subcommands/test_process_distgit.py new file mode 100644 index 0000000..0e4fc71 --- /dev/null +++ b/tests/rpmautospec/subcommands/test_process_distgit.py @@ -0,0 +1,240 @@ +import difflib +import os +import re +import shutil +from subprocess import run, check_output +import tarfile +import tempfile + +import pytest + +from rpmautospec.subcommands import process_distgit + + +__here__ = os.path.dirname(__file__) + + +class TestProcessDistgit: + """Test the rpmautospec.subcommands.process_distgit module""" + + autorelease_autochangelog_cases = [ + (autorelease_case, autochangelog_case) + for autorelease_case in ("unchanged", "with braces", "optional") + for autochangelog_case in ( + "unchanged", + "changelog case insensitive", + "changelog trailing garbage", + "line in between", + "trailing line", + "with braces", + "missing", + "optional", + ) + ] + + relnum_re = re.compile("^(?P[0-9]+)(?P.*)$") + + @classmethod + def relnum_split(cls, release): + match = cls.relnum_re.match(release) + # let this fail if the regex doesn't match + return int(match.group("relnum")), match.group("rest") + + @staticmethod + def fuzz_spec_file(spec_file_path, autorelease_case, autochangelog_case, run_git_amend): + """Fuzz a spec file in ways which shouldn't change the outcome""" + + with open(spec_file_path, "r") as orig, open(spec_file_path + ".new", "w") as new: + for line in orig: + if line.startswith("Release:") and autorelease_case != "unchanged": + if autorelease_case == "with braces": + print("Release: %{autorelease}", file=new) + elif autorelease_case == "optional": + print("Release: %{?autorelease}", file=new) + else: + raise ValueError(f"Unknown autorelease_case: {autorelease_case}") + elif line.strip() == "%changelog" and autochangelog_case != "unchanged": + if autochangelog_case == "changelog case insensitive": + print("%ChAnGeLoG", file=new) + elif autochangelog_case == "changelog trailing garbage": + print("%changelog with trailing garbage yes this works", file=new) + elif autochangelog_case == "line in between": + print("%changelog\n\n%autochangelog", file=new) + break + elif autochangelog_case == "trailing line": + print("%changelog\n%autochangelog\n", file=new) + break + elif autochangelog_case == "with braces": + print("%changelog\n%{autochangelog}", file=new) + break + elif autochangelog_case == "missing": + # do nothing, i.e. don't print a %changelog to file + break + elif autochangelog_case == "optional": + print("%changelog\n%{?autochangelog}", file=new) + break + else: + raise ValueError(f"Unknown autochangelog_case: {autochangelog_case}") + else: + print(line, file=new, end="") + + os.rename(spec_file_path + ".new", spec_file_path) + + if run_git_amend: + # Ensure worktree doesn't differ + workdir = os.path.dirname(spec_file_path) + commit_timestamp = check_output( + ["git", "log", "-1", "--pretty=format:%cI"], + cwd=workdir, + encoding="ascii", + ).strip() + env = os.environ.copy() + # Set name and email explicitly so CI doesn't trip over them being unset. + env.update( + { + "GIT_COMMITTER_NAME": "Test User", + "GIT_COMMITTER_EMAIL": "", + "GIT_COMMITTER_DATE": commit_timestamp, + } + ) + run( + ["git", "commit", "--all", "--allow-empty", "--amend", "--no-edit"], + cwd=workdir, + env=env, + ) + + @pytest.mark.parametrize("overwrite_specfile", (False, True)) + @pytest.mark.parametrize("dirty_worktree", (False, True)) + @pytest.mark.parametrize("autorelease_case", ("unchanged", "with braces", "optional")) + @pytest.mark.parametrize( + "autochangelog_case", + ( + "unchanged", + "changelog case insensitive", + "changelog trailing garbage", + "line in between", + "trailing line", + "with braces", + "missing", + "optional", + ), + ) + def test_process_distgit( + self, overwrite_specfile, dirty_worktree, autorelease_case, autochangelog_case + ): + """Test the process_distgit() function""" + with tempfile.TemporaryDirectory() as workdir: + with tarfile.open( + os.path.join( + __here__, + os.path.pardir, + os.path.pardir, + "test-data", + "repodata", + "dummy-test-package-gloster-git.tar.gz", + ) + ) as tar: + tar.extractall(path=workdir) + + unpacked_repo_dir = os.path.join(workdir, "dummy-test-package-gloster") + test_spec_file_path = os.path.join( + unpacked_repo_dir, + "dummy-test-package-gloster.spec", + ) + + if autorelease_case != "unchanged" or autochangelog_case != "unchanged": + self.fuzz_spec_file( + test_spec_file_path, + autorelease_case, + autochangelog_case, + run_git_amend=not dirty_worktree, + ) + + if overwrite_specfile: + target_spec_file_path = None + else: + target_spec_file_path = os.path.join(workdir, "test-this-specfile-please.spec") + + orig_test_spec_file_stat = os.stat(test_spec_file_path) + process_distgit.process_distgit(unpacked_repo_dir, target_spec_file_path) + if not overwrite_specfile: + test_spec_file_stat = os.stat(test_spec_file_path) + # we can't compare stat_results directly because st_atime has changed + for attr in ("mode", "ino", "dev", "uid", "gid", "size", "mtime", "ctime"): + assert getattr(test_spec_file_stat, "st_" + attr) == getattr( + orig_test_spec_file_stat, "st_" + attr + ) + + expected_spec_file_path = os.path.join( + __here__, + os.path.pardir, + os.path.pardir, + "test-data", + "repodata", + "dummy-test-package-gloster.spec.expected", + ) + + with tempfile.NamedTemporaryFile() as tmpspec: + shutil.copy2(expected_spec_file_path, tmpspec.name) + if autorelease_case != "unchanged" or autochangelog_case != "unchanged": + if autochangelog_case not in ( + "changelog case insensitive", + "changelog trailing garbage", + ): + # "%changelog", "%ChAnGeLoG", ... stay verbatim, trick fuzz_spec_file() to + # leave the rest of the cases as is, the %autorelease macro is expanded. + fuzz_autochangelog_case = "unchanged" + else: + fuzz_autochangelog_case = autochangelog_case + expected_spec_file_path = tmpspec.name + self.fuzz_spec_file( + expected_spec_file_path, + autorelease_case, + fuzz_autochangelog_case, + run_git_amend=False, + ) + + rpm_cmd = ["rpm", "--define", "dist .fc32", "--specfile"] + + if target_spec_file_path: + test_cmd = rpm_cmd + [target_spec_file_path] + else: + test_cmd = rpm_cmd + [test_spec_file_path] + expected_cmd = rpm_cmd + [expected_spec_file_path] + + q_release = ["--qf", "%{release}\n"] + test_output = check_output(test_cmd + q_release, encoding="utf-8").strip() + test_relnum, test_rest = self.relnum_split(test_output) + expected_output = check_output(expected_cmd + q_release, encoding="utf-8").strip() + expected_relnum, expected_rest = self.relnum_split(expected_output) + + if dirty_worktree and ( + autorelease_case != "unchanged" or autochangelog_case != "unchanged" + ): + assert test_relnum == expected_relnum + 1 + else: + assert test_relnum == expected_relnum + + assert test_rest == expected_rest + + q_changelog = ["--changelog"] + test_output = check_output(test_cmd + q_changelog, encoding="utf-8") + expected_output = check_output(expected_cmd + q_changelog, encoding="utf-8") + + if dirty_worktree and ( + autorelease_case != "unchanged" or autochangelog_case != "unchanged" + ): + diff = list( + difflib.ndiff(expected_output.splitlines(), test_output.splitlines()) + ) + # verify entry for uncommitted changes + assert all(line.startswith("+ ") for line in diff[:3]) + assert diff[0].endswith(f"-{expected_relnum + 1}") + assert diff[1] == "+ - Uncommitted changes" + assert diff[2] == "+ " + + # verify the rest is the expected changelog + assert all(line.startswith(" ") for line in diff[3:]) + assert expected_output.splitlines() == [line[2:] for line in diff[3:]] + else: + assert test_output == expected_output diff --git a/tests/rpmautospec/subcommands/test_release.py b/tests/rpmautospec/subcommands/test_release.py new file mode 100644 index 0000000..42727f1 --- /dev/null +++ b/tests/rpmautospec/subcommands/test_release.py @@ -0,0 +1,45 @@ +import logging +import os.path +import tarfile +import tempfile +from pathlib import Path +from unittest.mock import Mock + +import pytest + +from rpmautospec.subcommands import release + + +__here__ = os.path.dirname(__file__) + + +class TestRelease: + """Test the rpmautospec.subcommands.release module""" + + @pytest.mark.parametrize("method_to_test", ("calculate_release", "main")) + def test_calculate_release(self, method_to_test, caplog): + with tempfile.TemporaryDirectory() as workdir: + with tarfile.open( + os.path.join( + __here__, + os.path.pardir, + os.path.pardir, + "test-data", + "repodata", + "dummy-test-package-gloster-git.tar.gz", + ) + ) as tar: + tar.extractall(path=workdir) + + unpacked_repo_dir = Path(workdir) / "dummy-test-package-gloster" + + expected_release = "11" + + if method_to_test == "calculate_release": + assert release.calculate_release(unpacked_repo_dir) == expected_release + else: + with caplog.at_level(logging.INFO): + args = Mock() + args.spec_or_path = unpacked_repo_dir + release.main(args) + assert f"calculate_release release: {expected_release}" in caplog.text diff --git a/tests/rpmautospec/test_process_distgit.py b/tests/rpmautospec/test_process_distgit.py deleted file mode 100644 index 6ca2cc9..0000000 --- a/tests/rpmautospec/test_process_distgit.py +++ /dev/null @@ -1,238 +0,0 @@ -import difflib -import os -import re -import shutil -from subprocess import run, check_output -import tarfile -import tempfile - -import pytest - -from rpmautospec import process_distgit - - -__here__ = os.path.dirname(__file__) - - -class TestProcessDistgit: - """Test the rpmautospec.process_distgit module""" - - autorelease_autochangelog_cases = [ - (autorelease_case, autochangelog_case) - for autorelease_case in ("unchanged", "with braces", "optional") - for autochangelog_case in ( - "unchanged", - "changelog case insensitive", - "changelog trailing garbage", - "line in between", - "trailing line", - "with braces", - "missing", - "optional", - ) - ] - - relnum_re = re.compile("^(?P[0-9]+)(?P.*)$") - - @classmethod - def relnum_split(cls, release): - match = cls.relnum_re.match(release) - # let this fail if the regex doesn't match - return int(match.group("relnum")), match.group("rest") - - @staticmethod - def fuzz_spec_file(spec_file_path, autorelease_case, autochangelog_case, run_git_amend): - """Fuzz a spec file in ways which shouldn't change the outcome""" - - with open(spec_file_path, "r") as orig, open(spec_file_path + ".new", "w") as new: - for line in orig: - if line.startswith("Release:") and autorelease_case != "unchanged": - if autorelease_case == "with braces": - print("Release: %{autorelease}", file=new) - elif autorelease_case == "optional": - print("Release: %{?autorelease}", file=new) - else: - raise ValueError(f"Unknown autorelease_case: {autorelease_case}") - elif line.strip() == "%changelog" and autochangelog_case != "unchanged": - if autochangelog_case == "changelog case insensitive": - print("%ChAnGeLoG", file=new) - elif autochangelog_case == "changelog trailing garbage": - print("%changelog with trailing garbage yes this works", file=new) - elif autochangelog_case == "line in between": - print("%changelog\n\n%autochangelog", file=new) - break - elif autochangelog_case == "trailing line": - print("%changelog\n%autochangelog\n", file=new) - break - elif autochangelog_case == "with braces": - print("%changelog\n%{autochangelog}", file=new) - break - elif autochangelog_case == "missing": - # do nothing, i.e. don't print a %changelog to file - break - elif autochangelog_case == "optional": - print("%changelog\n%{?autochangelog}", file=new) - break - else: - raise ValueError(f"Unknown autochangelog_case: {autochangelog_case}") - else: - print(line, file=new, end="") - - os.rename(spec_file_path + ".new", spec_file_path) - - if run_git_amend: - # Ensure worktree doesn't differ - workdir = os.path.dirname(spec_file_path) - commit_timestamp = check_output( - ["git", "log", "-1", "--pretty=format:%cI"], - cwd=workdir, - encoding="ascii", - ).strip() - env = os.environ.copy() - # Set name and email explicitly so CI doesn't trip over them being unset. - env.update( - { - "GIT_COMMITTER_NAME": "Test User", - "GIT_COMMITTER_EMAIL": "", - "GIT_COMMITTER_DATE": commit_timestamp, - } - ) - run( - ["git", "commit", "--all", "--allow-empty", "--amend", "--no-edit"], - cwd=workdir, - env=env, - ) - - @pytest.mark.parametrize("overwrite_specfile", (False, True)) - @pytest.mark.parametrize("dirty_worktree", (False, True)) - @pytest.mark.parametrize("autorelease_case", ("unchanged", "with braces", "optional")) - @pytest.mark.parametrize( - "autochangelog_case", - ( - "unchanged", - "changelog case insensitive", - "changelog trailing garbage", - "line in between", - "trailing line", - "with braces", - "missing", - "optional", - ), - ) - def test_process_distgit( - self, overwrite_specfile, dirty_worktree, autorelease_case, autochangelog_case - ): - """Test the process_distgit() function""" - with tempfile.TemporaryDirectory() as workdir: - with tarfile.open( - os.path.join( - __here__, - os.path.pardir, - "test-data", - "repodata", - "dummy-test-package-gloster-git.tar.gz", - ) - ) as tar: - tar.extractall(path=workdir) - - unpacked_repo_dir = os.path.join(workdir, "dummy-test-package-gloster") - test_spec_file_path = os.path.join( - unpacked_repo_dir, - "dummy-test-package-gloster.spec", - ) - - if autorelease_case != "unchanged" or autochangelog_case != "unchanged": - self.fuzz_spec_file( - test_spec_file_path, - autorelease_case, - autochangelog_case, - run_git_amend=not dirty_worktree, - ) - - if overwrite_specfile: - target_spec_file_path = None - else: - target_spec_file_path = os.path.join(workdir, "test-this-specfile-please.spec") - - orig_test_spec_file_stat = os.stat(test_spec_file_path) - process_distgit.process_distgit(unpacked_repo_dir, target_spec_file_path) - if not overwrite_specfile: - test_spec_file_stat = os.stat(test_spec_file_path) - # we can't compare stat_results directly because st_atime has changed - for attr in ("mode", "ino", "dev", "uid", "gid", "size", "mtime", "ctime"): - assert getattr(test_spec_file_stat, "st_" + attr) == getattr( - orig_test_spec_file_stat, "st_" + attr - ) - - expected_spec_file_path = os.path.join( - __here__, - os.path.pardir, - "test-data", - "repodata", - "dummy-test-package-gloster.spec.expected", - ) - - with tempfile.NamedTemporaryFile() as tmpspec: - shutil.copy2(expected_spec_file_path, tmpspec.name) - if autorelease_case != "unchanged" or autochangelog_case != "unchanged": - if autochangelog_case not in ( - "changelog case insensitive", - "changelog trailing garbage", - ): - # "%changelog", "%ChAnGeLoG", ... stay verbatim, trick fuzz_spec_file() to - # leave the rest of the cases as is, the %autorelease macro is expanded. - fuzz_autochangelog_case = "unchanged" - else: - fuzz_autochangelog_case = autochangelog_case - expected_spec_file_path = tmpspec.name - self.fuzz_spec_file( - expected_spec_file_path, - autorelease_case, - fuzz_autochangelog_case, - run_git_amend=False, - ) - - rpm_cmd = ["rpm", "--define", "dist .fc32", "--specfile"] - - if target_spec_file_path: - test_cmd = rpm_cmd + [target_spec_file_path] - else: - test_cmd = rpm_cmd + [test_spec_file_path] - expected_cmd = rpm_cmd + [expected_spec_file_path] - - q_release = ["--qf", "%{release}\n"] - test_output = check_output(test_cmd + q_release, encoding="utf-8").strip() - test_relnum, test_rest = self.relnum_split(test_output) - expected_output = check_output(expected_cmd + q_release, encoding="utf-8").strip() - expected_relnum, expected_rest = self.relnum_split(expected_output) - - if dirty_worktree and ( - autorelease_case != "unchanged" or autochangelog_case != "unchanged" - ): - assert test_relnum == expected_relnum + 1 - else: - assert test_relnum == expected_relnum - - assert test_rest == expected_rest - - q_changelog = ["--changelog"] - test_output = check_output(test_cmd + q_changelog, encoding="utf-8") - expected_output = check_output(expected_cmd + q_changelog, encoding="utf-8") - - if dirty_worktree and ( - autorelease_case != "unchanged" or autochangelog_case != "unchanged" - ): - diff = list( - difflib.ndiff(expected_output.splitlines(), test_output.splitlines()) - ) - # verify entry for uncommitted changes - assert all(line.startswith("+ ") for line in diff[:3]) - assert diff[0].endswith(f"-{expected_relnum + 1}") - assert diff[1] == "+ - Uncommitted changes" - assert diff[2] == "+ " - - # verify the rest is the expected changelog - assert all(line.startswith(" ") for line in diff[3:]) - assert expected_output.splitlines() == [line[2:] for line in diff[3:]] - else: - assert test_output == expected_output diff --git a/tests/rpmautospec/test_release.py b/tests/rpmautospec/test_release.py deleted file mode 100644 index 7f65cc0..0000000 --- a/tests/rpmautospec/test_release.py +++ /dev/null @@ -1,44 +0,0 @@ -import logging -import os.path -import tarfile -import tempfile -from pathlib import Path -from unittest.mock import Mock - -import pytest - -from rpmautospec import release - - -__here__ = os.path.dirname(__file__) - - -class TestRelease: - """Test the rpmautospec.release module""" - - @pytest.mark.parametrize("method_to_test", ("calculate_release", "main")) - def test_calculate_release(self, method_to_test, caplog): - with tempfile.TemporaryDirectory() as workdir: - with tarfile.open( - os.path.join( - __here__, - os.path.pardir, - "test-data", - "repodata", - "dummy-test-package-gloster-git.tar.gz", - ) - ) as tar: - tar.extractall(path=workdir) - - unpacked_repo_dir = Path(workdir) / "dummy-test-package-gloster" - - expected_release = "11" - - if method_to_test == "calculate_release": - assert release.calculate_release(unpacked_repo_dir) == expected_release - else: - with caplog.at_level(logging.INFO): - args = Mock() - args.spec_or_path = unpacked_repo_dir - release.main(args) - assert f"calculate_release release: {expected_release}" in caplog.text From 6b3301093442f62da4e59fd52481bf901527be91 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jun 16 2021 09:37:42 +0000 Subject: [PATCH 3/8] Expose process_distgit() in top-level package This should be used by fedpkg/rpkg to preprocess a spec file where necessary (e.g. fedpkg srpm, fedpkg local, fedpkg mockbuild). Signed-off-by: Nils Philippsen --- diff --git a/koji_plugins/rpmautospec_builder.py b/koji_plugins/rpmautospec_builder.py index 2f757d4..b1087dd 100644 --- a/koji_plugins/rpmautospec_builder.py +++ b/koji_plugins/rpmautospec_builder.py @@ -2,7 +2,7 @@ import logging from koji.plugin import callback -from rpmautospec.subcommands.process_distgit import process_distgit +from rpmautospec import process_distgit log = logging.getLogger(__name__) diff --git a/rpmautospec/__init__.py b/rpmautospec/__init__.py index 2429bae..4a55e58 100644 --- a/rpmautospec/__init__.py +++ b/rpmautospec/__init__.py @@ -1 +1,2 @@ from .misc import specfile_uses_rpmautospec # noqa: F401 +from .subcommands.process_distgit import process_distgit # noqa: F401 From 7cdc523f59400324f7e6a277e298d0c9ec4fb1a1 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jun 16 2021 09:52:33 +0000 Subject: [PATCH 4/8] Cache check_specfile_features() results This is to allow 3rd party users call specfile_uses_rpmautospec() multiple times about different sets of used features without having to parse the spec file every time. It assumes that the spec file doesn't change over the runtime of the process, if this can't be guaranteed, call check_specfile_features.__wrapped__() directly or disable caching when calling process_distgit() (like in the Koji builder plugin). Signed-off-by: Nils Philippsen --- diff --git a/koji_plugins/rpmautospec_builder.py b/koji_plugins/rpmautospec_builder.py index b1087dd..0575f75 100644 --- a/koji_plugins/rpmautospec_builder.py +++ b/koji_plugins/rpmautospec_builder.py @@ -16,5 +16,5 @@ def process_distgit_cb(cb_type, *, srcdir, taskinfo, **kwargs): # i.e. maven and image builds don't have spec-files return - if not process_distgit(srcdir): + if not process_distgit(srcdir, enable_caching=False): log.info("No %autorelease/%autochangelog features used, skipping.") diff --git a/rpmautospec/misc.py b/rpmautospec/misc.py index b22343e..2bfd312 100644 --- a/rpmautospec/misc.py +++ b/rpmautospec/misc.py @@ -1,5 +1,6 @@ import re from collections import namedtuple +from functools import lru_cache from pathlib import Path from typing import Union @@ -18,6 +19,7 @@ SpecfileFeatures = namedtuple( ) +@lru_cache(maxsize=None) def check_specfile_features(specpath: Union[Path, str]) -> SpecfileFeatures: if not isinstance(specpath, Path): specpath = Path(specpath) diff --git a/rpmautospec/subcommands/process_distgit.py b/rpmautospec/subcommands/process_distgit.py index c90cf8c..9b2597e 100644 --- a/rpmautospec/subcommands/process_distgit.py +++ b/rpmautospec/subcommands/process_distgit.py @@ -43,10 +43,15 @@ def register_subcommand(subparsers): return subcmd_name -def process_distgit(spec_or_path: Union[Path, str], target: Union[Path, str] = None) -> bool: +def process_distgit( + spec_or_path: Union[Path, str], target: Union[Path, str] = None, *, enable_caching: bool = True +) -> bool: """Process an RPM spec file in a distgit repository. :param spec_or_path: the spec file or path of the repository + :param enable_caching: whether or not spec file feature test results + should be cached (disable in long-running + processes) :return: whether or not the spec file needed processing """ processor = PkgHistoryProcessor(spec_or_path) @@ -56,7 +61,10 @@ def process_distgit(spec_or_path: Union[Path, str], target: Union[Path, str] = N elif isinstance(target, Path): target = Path(target) - features = check_specfile_features(processor.specfile) + if enable_caching: + features = check_specfile_features(processor.specfile) + else: + features = check_specfile_features.__wrapped__(processor.specfile) processing_necessary = ( features.has_autorelease or features.has_autochangelog or not features.changelog_lineno ) From 883ad2b01c730e4ba3b35dfb2def52cf8774a8f5 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jun 16 2021 10:31:59 +0000 Subject: [PATCH 5/8] Allow setting the release number in the RPM macro This enabled 3rd party users (or anyone really) to set the %_rpmautospec_release_number macro to a value other than one to have the locally installed %autorelease macro do the right thing. Signed-off-by: Nils Philippsen --- diff --git a/rpm/macros.d/macros.rpmautospec b/rpm/macros.d/macros.rpmautospec index dfee6e0..5b39b99 100644 --- a/rpm/macros.d/macros.rpmautospec +++ b/rpm/macros.d/macros.rpmautospec @@ -1,4 +1,8 @@ -%autorelease(e:s:pb:) %{?-p:0.}%{?-b*}%{!?-b:1}%{?-e:.%{-e*}}%{?-s:.%{-s*}}%{?dist} +%autorelease(e:s:pb:) %{?-p:0.}%{lua: + release_number = tonumber(rpm.expand("%{?_rpmautospec_release_number}%{!?_rpmautospec_release_number:1}")); + base_release_number = tonumber(rpm.expand("%{?-b*}%{!?-b:1}")); + print(release_number + base_release_number - 1); +}%{?-e:.%{-e*}}%{?-s:.%{-s*}}%{?dist} %autochangelog %{lua: locale = os.setlocale(nil) os.setlocale("C.utf8") From d3ffcfa67248e4416e0fe03d4a956961f7dccc37 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jun 16 2021 10:38:09 +0000 Subject: [PATCH 6/8] Add choice between number only or complete release Signed-off-by: Nils Philippsen --- diff --git a/rpmautospec/subcommands/release.py b/rpmautospec/subcommands/release.py index 8e5de13..18a3a26 100644 --- a/rpmautospec/subcommands/release.py +++ b/rpmautospec/subcommands/release.py @@ -23,16 +23,37 @@ def register_subcommand(subparsers): help="Path to package worktree or the spec file within", ) + complete_release_group = calc_release_parser.add_mutually_exclusive_group() + + complete_release_group.add_argument( + "-c", + "--complete-release", + action="store_true", + default=True, + help="Print the complete release with flags (without dist tag)", + ) + + complete_release_group.add_argument( + "-n", + "--number-only", + action="store_false", + dest="complete_release", + default=False, + help="Print only the calculated release number", + ) + return subcmd_name -def calculate_release(spec_or_path: Union[str, Path]) -> int: +def calculate_release( + spec_or_path: Union[str, Path], *, complete_release: bool = True +) -> Union[str, int]: processor = PkgHistoryProcessor(spec_or_path) result = processor.run(visitors=(processor.release_number_visitor,)) - return result["release-complete"] + return result["release-complete" if complete_release else "release-number"] def main(args): """Main method.""" - release = calculate_release(args.spec_or_path) + release = calculate_release(args.spec_or_path, complete_release=args.complete_release) log.info("calculate_release release: %s", release) From 573bb1f1dbb926d3e2cbc5e72625d5d258373daa Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jun 16 2021 10:42:04 +0000 Subject: [PATCH 7/8] Expose calculate_release*() in top-level package Signed-off-by: Nils Philippsen --- diff --git a/rpmautospec/__init__.py b/rpmautospec/__init__.py index 4a55e58..09e1ca5 100644 --- a/rpmautospec/__init__.py +++ b/rpmautospec/__init__.py @@ -1,2 +1,3 @@ from .misc import specfile_uses_rpmautospec # noqa: F401 from .subcommands.process_distgit import process_distgit # noqa: F401 +from .subcommands.release import calculate_release, calculate_release_number # noqa: F401 diff --git a/rpmautospec/subcommands/release.py b/rpmautospec/subcommands/release.py index 18a3a26..9491ff0 100644 --- a/rpmautospec/subcommands/release.py +++ b/rpmautospec/subcommands/release.py @@ -53,6 +53,10 @@ def calculate_release( return result["release-complete" if complete_release else "release-number"] +def calculate_release_number(spec_or_path: Union[str, Path]) -> int: + return calculate_release(spec_or_path, complete_release=False) + + def main(args): """Main method.""" release = calculate_release(args.spec_or_path, complete_release=args.complete_release) From 860e5f8e37598fbe43c070a76e7c399921de9fe0 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jun 16 2021 10:49:13 +0000 Subject: [PATCH 8/8] Add/complete docstring of exposed functions Signed-off-by: Nils Philippsen --- diff --git a/rpmautospec/misc.py b/rpmautospec/misc.py index 2bfd312..aa1ce95 100644 --- a/rpmautospec/misc.py +++ b/rpmautospec/misc.py @@ -52,7 +52,13 @@ def check_specfile_features(specpath: Union[Path, str]) -> SpecfileFeatures: def specfile_uses_rpmautospec( specpath: Union[Path, str], check_autorelease: bool = True, check_autochangelog: bool = True ) -> bool: - """Check whether or not an RPM spec file uses rpmautospec features.""" + """Check whether or not an RPM spec file uses rpmautospec features. + + :param specpath: Path to the RPM spec file + :param check_autorelease: Whether to check for use of %autorelease + :param check_autochangelog: Whether to check for use of %autochangelog + :return: Whether the spec file uses the specified features + """ if not check_autorelease and not check_autochangelog: raise ValueError("One of check_autorelease and check_autochangelog must be set") diff --git a/rpmautospec/subcommands/release.py b/rpmautospec/subcommands/release.py index 9491ff0..0a1c0fe 100644 --- a/rpmautospec/subcommands/release.py +++ b/rpmautospec/subcommands/release.py @@ -48,12 +48,27 @@ def register_subcommand(subparsers): def calculate_release( spec_or_path: Union[str, Path], *, complete_release: bool = True ) -> Union[str, int]: + """Calculate release value (or number) of a package. + + :param spec_or_path: The spec file or directory it is located in. + :param complete_release: Whether to return the complete release (without + dist tag) or just the number. + :return: the release value or number + """ processor = PkgHistoryProcessor(spec_or_path) result = processor.run(visitors=(processor.release_number_visitor,)) return result["release-complete" if complete_release else "release-number"] def calculate_release_number(spec_or_path: Union[str, Path]) -> int: + """Calculate release number of a package. + + This number can be passed into the %autorelease macro as + %_rpmautospec_release_number. + + :param spec_or_path: The spec file or directory it is located in. + :return: the release number + """ return calculate_release(spec_or_path, complete_release=False)