From c365d345503be86e4ee3930973aa8fdbdae05caa Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 14:47:36 +0000 Subject: [PATCH 1/14] Remove superfluous `if True:` used for debugging Signed-off-by: Nils Philippsen --- diff --git a/tests/rpmautospec/subcommands/test_process_distgit.py b/tests/rpmautospec/subcommands/test_process_distgit.py index d5e5894..fafc0ee 100644 --- a/tests/rpmautospec/subcommands/test_process_distgit.py +++ b/tests/rpmautospec/subcommands/test_process_distgit.py @@ -137,126 +137,123 @@ class TestProcessDistgit: pytest.xfail("invalid parameter combination") workdir = str(tmp_path) - if True: - 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", - ) - - cwd = os.getcwd() - os.chdir(unpacked_repo_dir) - run(["git", "checkout", branch]) - os.chdir(cwd) - - 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( + with tarfile.open( + os.path.join( __here__, os.path.pardir, os.path.pardir, "test-data", "repodata", - "dummy-test-package-gloster.spec.expected", + "dummy-test-package-gloster-git.tar.gz", ) + ) as tar: + tar.extractall(path=workdir) - 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" - ): - expected_relnum += 1 - - if branch == "epel8": - expected_relnum += 1 + 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", + ) - assert test_relnum == expected_relnum + cwd = os.getcwd() + os.chdir(unpacked_repo_dir) + run(["git", "checkout", branch]) + os.chdir(cwd) + + 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, + ) - assert test_rest == expected_rest + 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 + ) - 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") + expected_spec_file_path = os.path.join( + __here__, + os.path.pardir, + os.path.pardir, + "test-data", + "repodata", + "dummy-test-package-gloster.spec.expected", + ) - if dirty_worktree and ( - autorelease_case != "unchanged" or autochangelog_case != "unchanged" + 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", ): - 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}") - 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:]] + # "%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: - assert test_output == expected_output + 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" + ): + expected_relnum += 1 + + if branch == "epel8": + expected_relnum += 1 + + 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}") + 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 From 887508f8fedc7f452b1f3f20a876ce51a03a55be Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 15:51:45 +0000 Subject: [PATCH 2/14] Pre-generate valid test combinations Previously, test_process_distgit() would check for invalid combinations at the top of the test and pytest.xfail() invalid combinations. This needlessly clutters test output. Signed-off-by: Nils Philippsen --- diff --git a/tests/rpmautospec/subcommands/test_process_distgit.py b/tests/rpmautospec/subcommands/test_process_distgit.py index fafc0ee..aa35d17 100644 --- a/tests/rpmautospec/subcommands/test_process_distgit.py +++ b/tests/rpmautospec/subcommands/test_process_distgit.py @@ -14,6 +14,32 @@ from rpmautospec.subcommands import process_distgit __here__ = os.path.dirname(__file__) +def _generate_branch_autorelease_autochangelog_case_combinations(): + """Pre-generate valid combinations to avoid cluttering pytest output. + + Only run fuzzing tests on the Rawhide branch because merge + commits (which it doesn't have) make them fail.""" + valid_combinations = [ + (branch, autorelease_case, autochangelog_case) + for branch in ("rawhide", "epel8") + 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", + ) + if branch == "rawhide" + or autorelease_case == "unchanged" + and autochangelog_case == "unchanged" + ] + return valid_combinations + + class TestProcessDistgit: """Test the rpmautospec.subcommands.process_distgit module""" @@ -104,21 +130,10 @@ class TestProcessDistgit: ) @pytest.mark.parametrize("overwrite_specfile", (False, True)) - @pytest.mark.parametrize("branch", ("rawhide", "epel8")) @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", - ), + "branch, autorelease_case, autochangelog_case", + _generate_branch_autorelease_autochangelog_case_combinations(), ) def test_process_distgit( self, @@ -130,12 +145,6 @@ class TestProcessDistgit: autochangelog_case, ): """Test the process_distgit() function""" - if branch != "rawhide" and ( - autorelease_case != "unchanged" or autochangelog_case != "unchanged" - ): - # Fuzzing makes the tests fail when applied to a merge commit - pytest.xfail("invalid parameter combination") - workdir = str(tmp_path) with tarfile.open( os.path.join( From 5dcc47c6d8ed73024fe566d809bfc4fa7c214e02 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 15:51:47 +0000 Subject: [PATCH 3/14] Don't trim changelogs from tested spec files Per default, RPM trims changelog entries older than two years. This makes tests fail when comparing RPM output with fixed contents. Signed-off-by: Nils Philippsen --- diff --git a/tests/rpmautospec/subcommands/test_process_distgit.py b/tests/rpmautospec/subcommands/test_process_distgit.py index aa35d17..33d1240 100644 --- a/tests/rpmautospec/subcommands/test_process_distgit.py +++ b/tests/rpmautospec/subcommands/test_process_distgit.py @@ -221,7 +221,16 @@ class TestProcessDistgit: run_git_amend=False, ) - rpm_cmd = ["rpm", "--define", "dist .fc32", "--specfile"] + rpm_cmd = [ + "rpm", + "--define", + "dist .fc32", + "--define", + "_changelog_trimage 0", + "--define", + "_changelog_trimtime 0", + "--specfile", + ] if target_spec_file_path: test_cmd = rpm_cmd + [target_spec_file_path] From 766a577cdc45ce4cfe0c4104d6035ad207fccd1a Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 15:51:47 +0000 Subject: [PATCH 4/14] Initialize locale in CLI We don't actually care about the locale (yet), but not setting this hides bugs when running the CLI tool where, if the locale is initialized, generated text is localized (which it shouldn't be). Signed-off-by: Nils Philippsen --- diff --git a/rpmautospec/cli.py b/rpmautospec/cli.py index 534071a..43142d0 100644 --- a/rpmautospec/cli.py +++ b/rpmautospec/cli.py @@ -1,4 +1,5 @@ import argparse +import locale import logging import sys import typing @@ -91,6 +92,8 @@ def get_cli_args(args: typing.List[str]) -> argparse.Namespace: def main(): + locale.setlocale(locale.LC_ALL, "") + args = get_cli_args(sys.argv[1:]) setup_logging(log_level=args.log_level) From 17e2e684f6642e51c106dd9ce10b5cdc73e695fa Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 17:20:39 +0000 Subject: [PATCH 5/14] Ignore locale when formatting dates Dates in RPM changelogs must use a set format with abbreviated English names of weekdays and months. Previously, we used datetime.strftime() which always honours locale settings, so when run under e.g. `fedpkg local` with a locale that uses another format or language, this generated invalid RPM changelog entries. Because the CLI didn't initialize locale, this never caught anyone's eye before. Fixes: #104 Signed-off-by: Nils Philippsen --- diff --git a/ci/pytest.yaml b/ci/pytest.yaml index 6442218..0bac22b 100644 --- a/ci/pytest.yaml +++ b/ci/pytest.yaml @@ -9,6 +9,9 @@ become: yes package: name: + - glibc-langpack-de + - glibc-langpack-en + - python3-babel - python3-koji - python3-pygit2 - python3-pytest diff --git a/python-rpmautospec.spec b/python-rpmautospec.spec index 942eaf8..7d08191 100644 --- a/python-rpmautospec.spec +++ b/python-rpmautospec.spec @@ -20,6 +20,9 @@ URL: https://pagure.io/fedora-infra/rpmautospec Source0: https://releases.pagure.org/fedora-infra/rpmautospec/rpmautospec-%{version}.tar.gz BuildArch: noarch +# the langpacks are needed for tests +BuildRequires: glibc-langpack-de +BuildRequires: glibc-langpack-en BuildRequires: python3-devel >= 3.6.0 BuildRequires: python3-setuptools %if %{with epel_le_7} @@ -30,6 +33,7 @@ BuildRequires: python2-devel # python3-koji %if ! %{with epel_le_7} BuildRequires: koji +BuildRequires: python%{python3_pkgversion}-babel BuildRequires: python3-koji BuildRequires: python3-pygit2 BuildRequires: python%{python3_pkgversion}-pytest @@ -52,6 +56,7 @@ Summary: %{summary} %{?python_provide:%python_provide python3-%{srcname}} Requires: koji +Requires: python3-babel Requires: python3-koji Requires: python3-pygit2 Requires: rpm @@ -141,6 +146,9 @@ install -m 644 rpm/macros.d/macros.rpmautospec %{buildroot}%{rpmmacrodir}/ %endif %changelog +* Sun Nov 07 2021 Nils Philippsen +- require python3-babel and glibc langpacks (the latter for testing) + * Fri Aug 06 2021 Nils Philippsen - 0.2.5-1 - Update to 0.2.5 diff --git a/requirements.txt b/requirements.txt index 23dc10f..0395e16 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,2 +1,3 @@ +babel>=2.9 koji pygit2>=1.2.1 diff --git a/rpmautospec/pkg_history.py b/rpmautospec/pkg_history.py index 4490bb6..768c31e 100644 --- a/rpmautospec/pkg_history.py +++ b/rpmautospec/pkg_history.py @@ -12,6 +12,7 @@ from textwrap import TextWrapper from typing import Any, Dict, Optional, Sequence, Union import pygit2 +from babel.dates import format_datetime from .misc import AUTORELEASE_MACRO @@ -302,7 +303,12 @@ class PkgHistoryProcessor: } changelog_author = f"{commit.author.name} <{commit.author.email}>" - changelog_date = dt.datetime.utcfromtimestamp(commit.commit_time).strftime("%a %b %d %Y") + changelog_date = format_datetime( + dt.datetime.utcfromtimestamp(commit.commit_time), + format="EEE MMM dd Y", + locale="en", + ) + changelog_evr = f"{commit_result['epoch-version']}-{commit_result['release-complete']}" changelog_header = f"* {changelog_date} {changelog_author} {changelog_evr}" @@ -634,7 +640,9 @@ class PkgHistoryProcessor: changelog_author = f"{signature.name} <{signature.email}>" except KeyError: changelog_author = "Unknown User " - changelog_date = dt.datetime.utcnow().strftime("%a %b %d %Y") + changelog_date = format_datetime( + dt.datetime.utcnow(), format="EEE MMM dd Y", locale="en" + ) changelog_evr = f"{epoch_version}-{release_complete}" changelog_header = f"* {changelog_date} {changelog_author} {changelog_evr}" diff --git a/tests/rpmautospec/test_pkg_history.py b/tests/rpmautospec/test_pkg_history.py index 0a81281..16b63b2 100644 --- a/tests/rpmautospec/test_pkg_history.py +++ b/tests/rpmautospec/test_pkg_history.py @@ -1,6 +1,9 @@ +import datetime as dt +import locale import os import re import stat +from calendar import LocaleTextCalendar from shutil import rmtree from unittest.mock import patch @@ -16,6 +19,21 @@ def processor(repo): return processor +@pytest.fixture +def setlocale(): + """Allow temporary modification of locale settings.""" + saved_locale_settings = { + category: locale.getlocale(getattr(locale, category)) + for category in dir(locale) + if category.startswith("LC_") and category != "LC_ALL" + } + + yield locale.setlocale + + for category, locale_settings in saved_locale_settings.items(): + locale.setlocale(getattr(locale, category), locale_settings) + + class TestPkgHistoryProcessor: version_re = re.compile(r"^Version: .*$", flags=re.MULTILINE) @@ -128,8 +146,13 @@ class TestPkgHistoryProcessor: else: assert processor._get_rpmverflags_for_commit(head_commit)["epoch-version"] == "1.0" - @pytest.mark.parametrize("testcase", ("without commit", "with commit", "all results")) - def test_run(self, testcase, repo, processor): + @pytest.mark.parametrize( + "testcase", ("without commit", "with commit", "all results", "locale set") + ) + def test_run(self, testcase, repo, processor, setlocale): + if testcase == "locale set": + setlocale(locale.LC_ALL, "de_DE.UTF-8") + all_results = "all results" in testcase head_commit = repo[repo.head.target] @@ -166,4 +189,16 @@ class TestPkgHistoryProcessor: ): assert snippet in top_entry["data"] + cal = LocaleTextCalendar(firstweekday=0, locale="C.UTF-8") + commit_time = dt.datetime.utcfromtimestamp(head_commit.commit_time) + weekdayname = cal.formatweekday(day=commit_time.weekday(), width=3) + monthname = cal.formatmonthname( + theyear=commit_time.year, + themonth=commit_time.month, + width=1, + withyear=False, + )[:3] + expected_date_blurb = f"* {weekdayname} {monthname} {commit_time.day:02} {commit_time.year}" + assert top_entry["data"].startswith(expected_date_blurb) + assert all("error" not in entry for entry in changelog) From 6b98d43de179d9a83268b4c0bf74c3fc210a4246 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 17:20:44 +0000 Subject: [PATCH 6/14] Document potential jump of initial release number Fixes: #200 Signed-off-by: Nils Philippsen --- diff --git a/doc/opting-in.rst b/doc/opting-in.rst index 0046ed9..5993016 100644 --- a/doc/opting-in.rst +++ b/doc/opting-in.rst @@ -19,6 +19,11 @@ with the ``%autorelease`` macro, such as: Release: %autorelease +.. note:: + Often, changing to automatic releases will result in an initial jump of the release number + because the number of commits since the last version change is higher than the number of builds + up to here. This is expected and not a sign that the product is defective. + There are different options you can associate with this macro which are documented in: :ref:`using-autorelease`. From 6a9a591fa9c6dad3c4b7455c3830f67d273d8b56 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 17:20:44 +0000 Subject: [PATCH 7/14] Document how to rebuild without changes Fixes: #217 Signed-off-by: Nils Philippsen --- diff --git a/doc/peculiarities.rst b/doc/peculiarities.rst index 84e768b..dd7896c 100644 --- a/doc/peculiarities.rst +++ b/doc/peculiarities.rst @@ -42,3 +42,16 @@ more parents, which e.g. happens if branches are merged with ``git merge branch is used, disregarding the contents of other branches. In this case, rpmautospec will follow the first parent it encounters which has the same tree as the merge commit and disregard the others. + + +Rebuilding a package without changing it +---------------------------------------- + +In the past, rebuilding a package to pick up changed dependencies, or in the context of mass +rebuilds was accomplished by bumping the release and adding a suitable changelog entry. With +`rpmautospec`, you have to tell git that you really want to add a commit without any changes in +content to accomplish the equivalent, e.g.:: + + git commit --allow-empty + +The resulting empty head commit can be pushed into the repository of the package and built normally. From b476f2b253abc26d9891ea5c68b5db48e422ac08 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 17:20:44 +0000 Subject: [PATCH 8/14] Cope with spec files outside git repos Fixes: #214 Signed-off-by: Nils Philippsen --- diff --git a/rpmautospec/pkg_history.py b/rpmautospec/pkg_history.py index 768c31e..8f58c10 100644 --- a/rpmautospec/pkg_history.py +++ b/rpmautospec/pkg_history.py @@ -71,6 +71,21 @@ class PkgHistoryProcessor: except pygit2.GitError: self.repo = None + @staticmethod + def _get_rpm_packager() -> str: + fallback = "John Doe " + try: + return ( + subprocess.check_output( + ("rpm", "--eval", f"%{{?packager}}%{{!?packager:{fallback}}}"), + stderr=subprocess.DEVNULL, + ) + .decode("UTF-8") + .strip() + ) + except Exception: + return fallback + @classmethod def _get_rpmverflags(cls, path: str, name: Optional[str] = None) -> Optional[str]: """Retrieve the epoch/version and %autorelease flags set in spec file. @@ -578,17 +593,23 @@ class PkgHistoryProcessor: # whether or not the worktree differs and this needs to be reflected in the result(s) reflect_worktree = False - if not head: - head = self.repo[self.repo.head.target] - diff_to_head = self.repo.diff(head) - reflect_worktree = diff_to_head.stats.files_changed > 0 - elif isinstance(head, str): - head = self.repo[head] + if self.repo: + if not head: + head = self.repo[self.repo.head.target] + diff_to_head = self.repo.diff(head) + reflect_worktree = diff_to_head.stats.files_changed > 0 + elif isinstance(head, str): + head = self.repo[head] - visited_results = self._run_on_history(head, visitors=visitors) - head_result = visited_results[head] + visited_results = self._run_on_history(head, visitors=visitors) + head_result = visited_results[head] + else: + reflect_worktree = True + visited_results = {} + head_result = {} if reflect_worktree: + # Not a git repository, or the git worktree isn't clean. worktree_result = {} verflags = self._get_rpmverflags(self.path, name=self.name) @@ -607,7 +628,7 @@ class PkgHistoryProcessor: # Mimic the bottom half of release_visitor worktree_result["epoch-version"] = epoch_version = verflags["epoch-version"] - if epoch_version == head_result["epoch-version"]: + if head_result and epoch_version == head_result["epoch-version"]: release_number = head_result["release-number"] + 1 else: release_number = 1 @@ -628,16 +649,22 @@ class PkgHistoryProcessor: changelog = () else: previous_changelog = head_result.get("changelog", ()) - changed_files = self._files_changed_in_diff(diff_to_head) - skip_for_changelog = all( - any(fnmatchcase(f, path) for path in self.changelog_ignore_patterns) - for f in changed_files - ) + if self.repo: + changed_files = self._files_changed_in_diff(diff_to_head) + skip_for_changelog = all( + any(fnmatchcase(f, path) for path in self.changelog_ignore_patterns) + for f in changed_files + ) + else: + skip_for_changelog = False if not skip_for_changelog: try: signature = self.repo.default_signature changelog_author = f"{signature.name} <{signature.email}>" + except AttributeError: + # self.repo == None -> no git repo + changelog_author = self._get_rpm_packager() except KeyError: changelog_author = "Unknown User " changelog_date = format_datetime( diff --git a/tests/rpmautospec/test_pkg_history.py b/tests/rpmautospec/test_pkg_history.py index 16b63b2..72c53e6 100644 --- a/tests/rpmautospec/test_pkg_history.py +++ b/tests/rpmautospec/test_pkg_history.py @@ -147,7 +147,7 @@ class TestPkgHistoryProcessor: assert processor._get_rpmverflags_for_commit(head_commit)["epoch-version"] == "1.0" @pytest.mark.parametrize( - "testcase", ("without commit", "with commit", "all results", "locale set") + "testcase", ("without commit", "with commit", "all results", "locale set", "without repo") ) def test_run(self, testcase, repo, processor, setlocale): if testcase == "locale set": @@ -155,7 +155,12 @@ class TestPkgHistoryProcessor: all_results = "all results" in testcase - head_commit = repo[repo.head.target] + if testcase == "without repo": + rmtree(repo.path) + processor = PkgHistoryProcessor(repo.workdir) + head_commit = None + else: + head_commit = repo[repo.head.target] if testcase == "with commit": args = [head_commit] @@ -176,29 +181,40 @@ class TestPkgHistoryProcessor: else: assert all(isinstance(key, str) for key in res) - assert res["commit-id"] == head_commit.id - assert res["release-number"] == 2 - changelog = res["changelog"] top_entry = changelog[0] - assert top_entry["commit-id"] == head_commit.id - for snippet in ( - "Jane Doe ", - "- Did nothing!", - ): - assert snippet in top_entry["data"] - - cal = LocaleTextCalendar(firstweekday=0, locale="C.UTF-8") - commit_time = dt.datetime.utcfromtimestamp(head_commit.commit_time) - weekdayname = cal.formatweekday(day=commit_time.weekday(), width=3) - monthname = cal.formatmonthname( - theyear=commit_time.year, - themonth=commit_time.month, - width=1, - withyear=False, - )[:3] - expected_date_blurb = f"* {weekdayname} {monthname} {commit_time.day:02} {commit_time.year}" - assert top_entry["data"].startswith(expected_date_blurb) + if testcase == "without repo": + assert res["release-number"] == 1 + + for snippet in ( + processor._get_rpm_packager(), + "- Uncommitted changes", + ): + assert snippet in top_entry["data"] + else: + assert res["commit-id"] == head_commit.id + assert res["release-number"] == 2 + assert top_entry["commit-id"] == head_commit.id + + for snippet in ( + "Jane Doe ", + "- Did nothing!", + ): + assert snippet in top_entry["data"] + + cal = LocaleTextCalendar(firstweekday=0, locale="C.UTF-8") + commit_time = dt.datetime.utcfromtimestamp(head_commit.commit_time) + weekdayname = cal.formatweekday(day=commit_time.weekday(), width=3) + monthname = cal.formatmonthname( + theyear=commit_time.year, + themonth=commit_time.month, + width=1, + withyear=False, + )[:3] + expected_date_blurb = ( + f"* {weekdayname} {monthname} {commit_time.day:02} {commit_time.year}" + ) + assert top_entry["data"].startswith(expected_date_blurb) assert all("error" not in entry for entry in changelog) From 18bd285b28a1d57526d990d609a08beaa2e8012d Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 17:20:44 +0000 Subject: [PATCH 9/14] Document DCO, Signed-off-by requirement Fixes: #220 Signed-off-by: Nils Philippsen --- diff --git a/DCO.txt b/DCO.txt new file mode 100644 index 0000000..49b8cb0 --- /dev/null +++ b/DCO.txt @@ -0,0 +1,34 @@ +Developer Certificate of Origin +Version 1.1 + +Copyright (C) 2004, 2006 The Linux Foundation and its contributors. + +Everyone is permitted to copy and distribute verbatim copies of this +license document, but changing it is not allowed. + + +Developer's Certificate of Origin 1.1 + +By making a contribution to this project, I certify that: + +(a) The contribution was created in whole or in part by me and I + have the right to submit it under the open source license + indicated in the file; or + +(b) The contribution is based upon previous work that, to the best + of my knowledge, is covered under an appropriate open source + license and I have the right under that license to submit that + work with modifications, whether created in whole or in part + by me, under the same open source license (unless I am + permitted to submit under a different license), as indicated + in the file; or + +(c) The contribution was provided directly to me by some other + person who certified (a), (b) or (c) and I have not modified + it. + +(d) I understand and agree that this project and the contribution + are public and that a record of the contribution (including all + personal information I submit with it, including my sign-off) is + maintained indefinitely and may be redistributed consistent with + this project or the open source license(s) involved. diff --git a/README.rst b/README.rst index 16cfbad..21e2c78 100644 --- a/README.rst +++ b/README.rst @@ -53,6 +53,15 @@ E.g.: python run-rpmautospec.py calculate-release bash +Contributing +------------ + +You need to be legally allowed to submit any contribution to this project. What this +means in detail is laid out in the file ``DCO.txt`` next to this file. The mechanism by which you +certify this is adding a ``Signed-off-by`` trailer to git commit log messages, you can do this by +using the ``--signoff/-s`` option to ``git commit``. + + --- License: MIT From 4f5ba212c6ffb6cdff94c2db171c25dfbb539af7 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 17:20:44 +0000 Subject: [PATCH 10/14] Pass around more info between commit visitors Previously the visitors only passed around whether or not they needed further history for processing in the forward stage of processing git histories. This lets us pass around other information, e.g. if a changelog file is removed, so that its contents can be disregarded when processing the respective parent commit that introduced it. Related: #210 Signed-off-by: Nils Philippsen --- diff --git a/rpmautospec/pkg_history.py b/rpmautospec/pkg_history.py index 8f58c10..4701b74 100644 --- a/rpmautospec/pkg_history.py +++ b/rpmautospec/pkg_history.py @@ -179,7 +179,7 @@ class PkgHistoryProcessor: return self._get_rpmverflags(workdir, self.name) - def release_number_visitor(self, commit: pygit2.Commit, children_must_continue: bool): + def release_number_visitor(self, commit: pygit2.Commit, child_info: Dict[str, Any]): """Visit a commit to determine its release number. The coroutine returned first determines if the parent chain(s) must be @@ -205,18 +205,18 @@ class PkgHistoryProcessor: tag_string = "" if not epoch_version: - must_continue = True + child_must_continue = True else: epoch_versions_to_check = [] for p in commit.parents: verflags = self._get_rpmverflags_for_commit(p) if verflags: epoch_versions_to_check.append(verflags["epoch-version"]) - must_continue = epoch_version in epoch_versions_to_check + child_must_continue = epoch_version in epoch_versions_to_check # Suspend execution, yield whether caller should continue, and get back the (partial) result # for this commit and parent results as dictionaries on resume. - commit_result, parent_results = yield must_continue + commit_result, parent_results = yield {"child_must_continue": child_must_continue} commit_result["epoch-version"] = epoch_version @@ -248,7 +248,7 @@ class PkgHistoryProcessor: files.add(delta.new_file.path) return files - def changelog_visitor(self, commit: pygit2.Commit, children_must_continue: bool): + def changelog_visitor(self, commit: pygit2.Commit, child_info: Dict[str, Any]): """Visit a commit to generate changelog entries for it and its parents. It first determines if parent chain(s) must be followed, i.e. if the @@ -257,6 +257,7 @@ class PkgHistoryProcessor: modified) and full results of parents back (as dictionaries), which get processed and the results for this commit yielded again. """ + child_must_continue = child_info["child_must_continue"] # Check if the spec file exists, if not, there will be no changelog. specfile_present = f"{self.name}.spec" in commit.tree @@ -299,19 +300,19 @@ class PkgHistoryProcessor: # care. If it didn't change, we don't know how to continue and need to flag that. merge_unresolvable = not changelog_changed - child_must_continue = ( + our_child_must_continue = ( not (changelog_changed or merge_unresolvable) and specfile_present - and children_must_continue + and child_must_continue ) log.debug("\tchangelog changed: %s", changelog_changed) log.debug("\tmerge unresolvable: %s", merge_unresolvable) log.debug("\tspec file present: %s", specfile_present) - log.debug("\tchildren must continue: %s", children_must_continue) - log.debug("\tchild must continue: %s", child_must_continue) + log.debug("\tchild must continue (incoming): %s", child_must_continue) + log.debug("\tchild must continue (outgoing): %s", our_child_must_continue) - commit_result, parent_results = yield child_must_continue + commit_result, parent_results = yield {"child_must_continue": our_child_must_continue} changelog_entry = { "commit-id": commit.id, @@ -399,6 +400,21 @@ class PkgHistoryProcessor: yield commit_result + @staticmethod + def _merge_info(f1: Dict[str, Any], f2: Dict[str, Any]) -> Dict[str, Any]: + mf = f1.copy() + for k, v2 in f2.items(): + try: + v1 = mf[k] + except KeyError: + mf[k] = v2 + else: + if k == "child_must_continue": + mf[k] = v1 or v2 + else: + raise KeyError(f"Unknown information key: {k}") + return mf + def _run_on_history( self, head: pygit2.Commit, *, visitors: Sequence = () ) -> Dict[pygit2.Commit, Dict[str, Any]]: @@ -406,7 +422,7 @@ class PkgHistoryProcessor: # maps visited commits to their (in-flight) visitors and if they must # continue commit_coroutines = {} - commit_coroutines_must_continue = {} + commit_coroutines_info = {} # keep track of branches branch_heads = [head] @@ -448,7 +464,7 @@ class PkgHistoryProcessor: log.debug("commit %s: %s", commit.short_id, commit.message.split("\n", 1)[0]) if commit == head: - children_visitors_must_continue = [True for v in visitors] + children_visitors_info = [{"child_must_continue": True} for v in visitors] else: this_children = commit_children[commit] if not all(child in commit_coroutines for child in this_children): @@ -472,19 +488,22 @@ class PkgHistoryProcessor: ) break - # For all visitor coroutines, determine if any of the children must continue. - children_visitors_must_continue = [ + # For all visitor coroutines, merge their produced info, e.g. to determine if + # any of the children must continue. + children_visitors_info = [ reduce( - lambda must_continue, child: ( - must_continue or commit_coroutines_must_continue[child][vindex] + lambda info, child: self._merge_info( + info, commit_coroutines_info[child][vindex] ), this_children, - False, + {}, ) for vindex, v in enumerate(visitors) ] - keep_processing = keep_processing and any(children_visitors_must_continue) + keep_processing = keep_processing and any( + info["child_must_continue"] for info in children_visitors_info + ) branch.append(commit) @@ -493,17 +512,18 @@ class PkgHistoryProcessor: # method. Pass the ordered list of "is there a child whose coroutine of the same # visitor wants to continue" into it. commit_coroutines[commit] = coroutines = [ - v(commit, children_visitors_must_continue[vi]) - for vi, v in enumerate(visitors) + v(commit, children_visitors_info[vi]) for vi, v in enumerate(visitors) ] # Consult all visitors for the commit on whether we should continue and store # the results. - commit_coroutines_must_continue[commit] = [next(c) for c in coroutines] + commit_coroutines_info[commit] = [next(c) for c in coroutines] else: # Only traverse this commit. commit_coroutines[commit] = coroutines = None - commit_coroutines_must_continue[commit] = [False for v in visitors] + commit_coroutines_info[commit] = [ + {"child_must_continue": False} for v in visitors + ] if not commit.parents: log.debug("\tno parents, bailing out") From 96ee85b4e72967347283240bf91328957071aa02 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 17:20:44 +0000 Subject: [PATCH 11/14] Handle removal of `changelog` file better Previously, removal of the changelog file would stop processing the commit logs of previous commits which is clearly wrong. Now, the removal is recorded and used to disregard its contents when processing the commit in which it was added. Fixes: #210 Signed-off-by: Nils Philippsen --- diff --git a/rpmautospec/pkg_history.py b/rpmautospec/pkg_history.py index 4701b74..cf78de5 100644 --- a/rpmautospec/pkg_history.py +++ b/rpmautospec/pkg_history.py @@ -268,6 +268,8 @@ class PkgHistoryProcessor: except KeyError: changelog_blob = None + child_changelog_removed = child_info.get("changelog_removed") + our_changelog_removed = False if commit.parents: changelog_changed = True for parent in commit.parents: @@ -275,6 +277,8 @@ class PkgHistoryProcessor: par_changelog_blob = parent.tree["changelog"] except KeyError: par_changelog_blob = None + else: + our_changelog_removed = our_changelog_removed or not changelog_blob if changelog_blob == par_changelog_blob: changelog_changed = False else: @@ -301,18 +305,24 @@ class PkgHistoryProcessor: merge_unresolvable = not changelog_changed our_child_must_continue = ( - not (changelog_changed or merge_unresolvable) + not (changelog_changed and changelog_blob or merge_unresolvable) and specfile_present and child_must_continue ) log.debug("\tchangelog changed: %s", changelog_changed) + log.debug("\tchild changelog removed: %s", child_changelog_removed) + log.debug("\tour changelog removed: %s", our_changelog_removed) log.debug("\tmerge unresolvable: %s", merge_unresolvable) log.debug("\tspec file present: %s", specfile_present) log.debug("\tchild must continue (incoming): %s", child_must_continue) log.debug("\tchild must continue (outgoing): %s", our_child_must_continue) - commit_result, parent_results = yield {"child_must_continue": our_child_must_continue} + commit_result, parent_results = yield { + "child_must_continue": our_child_must_continue, + "changelog_removed": not (changelog_blob and changelog_changed) + and (child_changelog_removed or our_changelog_removed), + } changelog_entry = { "commit-id": commit.id, @@ -341,18 +351,15 @@ class PkgHistoryProcessor: changelog_entry["error"] = "unresolvable merge" previous_changelog = () commit_result["changelog"] = (changelog_entry,) - elif changelog_changed: + elif changelog_changed and changelog_blob: log.debug("\tchangelog file changed") - if changelog_blob: + if not child_changelog_removed: changelog_entry["data"] = changelog_blob.data.decode("utf-8", errors="replace") + commit_result["changelog"] = (changelog_entry,) else: - # Changelog removed. Oops. - log.debug("\tchangelog file removed") - changelog_entry[ - "data" - ] = f"{changelog_header}\n- RPMAUTOSPEC: changelog file removed" - changelog_entry["error"] = "changelog file removed" - commit_result["changelog"] = (changelog_entry,) + # The `changelog` file was removed in a later commit, stop changelog generation. + log.debug("\t skipping") + commit_result["changelog"] = () else: # Pull previous changelog entries from parent result (if any). if len(commit.parents) == 1: @@ -411,14 +418,17 @@ class PkgHistoryProcessor: else: if k == "child_must_continue": mf[k] = v1 or v2 + elif k == "changelog_removed": + mf[k] = v1 and v2 else: raise KeyError(f"Unknown information key: {k}") return mf def _run_on_history( - self, head: pygit2.Commit, *, visitors: Sequence = () + self, head: pygit2.Commit, *, visitors: Sequence = (), seed_info: Dict[str, Any] = None ) -> Dict[pygit2.Commit, Dict[str, Any]]: """Process historical commits with visitors and gather results.""" + seed_info = {"child_must_continue": True, **(seed_info or {})} # maps visited commits to their (in-flight) visitors and if they must # continue commit_coroutines = {} @@ -464,7 +474,7 @@ class PkgHistoryProcessor: log.debug("commit %s: %s", commit.short_id, commit.message.split("\n", 1)[0]) if commit == head: - children_visitors_info = [{"child_must_continue": True} for v in visitors] + children_visitors_info = [seed_info for v in visitors] else: this_children = commit_children[commit] if not all(child in commit_coroutines for child in this_children): @@ -614,14 +624,21 @@ class PkgHistoryProcessor: reflect_worktree = False if self.repo: + seed_info = None if not head: head = self.repo[self.repo.head.target] diff_to_head = self.repo.diff(head) reflect_worktree = diff_to_head.stats.files_changed > 0 + if ( + reflect_worktree + and not (self.specfile.parent / "changelog").exists() + and "changelog" in head.tree + ): + seed_info = {"changelog_removed": True} elif isinstance(head, str): head = self.repo[head] - visited_results = self._run_on_history(head, visitors=visitors) + visited_results = self._run_on_history(head, visitors=visitors, seed_info=seed_info) head_result = visited_results[head] else: reflect_worktree = True diff --git a/tests/rpmautospec/subcommands/test_process_distgit.py b/tests/rpmautospec/subcommands/test_process_distgit.py index 33d1240..6430347 100644 --- a/tests/rpmautospec/subcommands/test_process_distgit.py +++ b/tests/rpmautospec/subcommands/test_process_distgit.py @@ -14,13 +14,13 @@ from rpmautospec.subcommands import process_distgit __here__ = os.path.dirname(__file__) -def _generate_branch_autorelease_autochangelog_case_combinations(): +def _generate_branch_testcase_combinations(): """Pre-generate valid combinations to avoid cluttering pytest output. Only run fuzzing tests on the Rawhide branch because merge commits (which it doesn't have) make them fail.""" valid_combinations = [ - (branch, autorelease_case, autochangelog_case) + (branch, autorelease_case, autochangelog_case, remove_changelog_file) for branch in ("rawhide", "epel8") for autorelease_case in ("unchanged", "with braces", "optional") for autochangelog_case in ( @@ -33,9 +33,11 @@ def _generate_branch_autorelease_autochangelog_case_combinations(): "missing", "optional", ) + for remove_changelog_file in (False, True) if branch == "rawhide" or autorelease_case == "unchanged" and autochangelog_case == "unchanged" + and not remove_changelog_file ] return valid_combinations @@ -67,11 +69,14 @@ class TestProcessDistgit: 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""" + def fuzz_spec_file(spec_file_path, autorelease_case, autochangelog_case, remove_changelog_file): + """Fuzz a spec file in ways which (often) shouldn't change the outcome""" with open(spec_file_path, "r") as orig, open(spec_file_path + ".new", "w") as new: + encountered_first_after_conversion = False for line in orig: + if remove_changelog_file and encountered_first_after_conversion: + break if line.startswith("Release:") and autorelease_case != "unchanged": if autorelease_case == "with braces": print("Release: %{autorelease}", file=new) @@ -102,38 +107,40 @@ class TestProcessDistgit: else: raise ValueError(f"Unknown autochangelog_case: {autochangelog_case}") else: + if line == "- Honour the tradition of antiquated encodings!\n": + encountered_first_after_conversion = True 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, - ) + @staticmethod + def run_git_amend(worktree_dir): + # Ensure worktree doesn't differ + commit_timestamp = check_output( + ["git", "log", "-1", "--pretty=format:%cI"], + cwd=worktree_dir, + 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=worktree_dir, + env=env, + ) @pytest.mark.parametrize("overwrite_specfile", (False, True)) @pytest.mark.parametrize("dirty_worktree", (False, True)) @pytest.mark.parametrize( - "branch, autorelease_case, autochangelog_case", - _generate_branch_autorelease_autochangelog_case_combinations(), + "branch, autorelease_case, autochangelog_case, remove_changelog_file", + _generate_branch_testcase_combinations(), ) def test_process_distgit( self, @@ -143,6 +150,7 @@ class TestProcessDistgit: dirty_worktree, autorelease_case, autochangelog_case, + remove_changelog_file, ): """Test the process_distgit() function""" workdir = str(tmp_path) @@ -174,9 +182,19 @@ class TestProcessDistgit: test_spec_file_path, autorelease_case, autochangelog_case, - run_git_amend=not dirty_worktree, + remove_changelog_file, ) + if remove_changelog_file: + os.unlink(os.path.join(unpacked_repo_dir, "changelog")) + + if ( + autorelease_case != "unchanged" + or autochangelog_case != "unchanged" + or remove_changelog_file + ) and not dirty_worktree: + self.run_git_amend(unpacked_repo_dir) + if overwrite_specfile: target_spec_file_path = None else: @@ -203,7 +221,11 @@ class TestProcessDistgit: with tempfile.NamedTemporaryFile() as tmpspec: shutil.copy2(expected_spec_file_path, tmpspec.name) - if autorelease_case != "unchanged" or autochangelog_case != "unchanged": + if ( + autorelease_case != "unchanged" + or autochangelog_case != "unchanged" + or remove_changelog_file + ): if autochangelog_case not in ( "changelog case insensitive", "changelog trailing garbage", @@ -218,7 +240,7 @@ class TestProcessDistgit: expected_spec_file_path, autorelease_case, fuzz_autochangelog_case, - run_git_amend=False, + remove_changelog_file, ) rpm_cmd = [ @@ -245,7 +267,9 @@ class TestProcessDistgit: expected_relnum, expected_rest = self.relnum_split(expected_output) if dirty_worktree and ( - autorelease_case != "unchanged" or autochangelog_case != "unchanged" + autorelease_case != "unchanged" + or autochangelog_case != "unchanged" + or remove_changelog_file ): expected_relnum += 1 @@ -261,7 +285,9 @@ class TestProcessDistgit: expected_output = check_output(expected_cmd + q_changelog, encoding="utf-8") if dirty_worktree and ( - autorelease_case != "unchanged" or autochangelog_case != "unchanged" + autorelease_case != "unchanged" + or autochangelog_case != "unchanged" + or remove_changelog_file ): diff = list(difflib.ndiff(expected_output.splitlines(), test_output.splitlines())) # verify entry for uncommitted changes From 814ad80cbb9d36ec8dd1a8e083d5feb61c1dc923 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 17:20:44 +0000 Subject: [PATCH 12/14] Paper over gaps in epoch-versions Gaps can occur when the spec file is missing or is unparseable. This change allows rpmautospec to pick up release numbering again when a retired package is unretired, without requiring to manually add a "base release number". Fixes: #237 Signed-off-by: Nils Philippsen --- diff --git a/rpmautospec/pkg_history.py b/rpmautospec/pkg_history.py index cf78de5..2826366 100644 --- a/rpmautospec/pkg_history.py +++ b/rpmautospec/pkg_history.py @@ -210,9 +210,17 @@ class PkgHistoryProcessor: epoch_versions_to_check = [] for p in commit.parents: verflags = self._get_rpmverflags_for_commit(p) - if verflags: - epoch_versions_to_check.append(verflags["epoch-version"]) - child_must_continue = epoch_version in epoch_versions_to_check + if not verflags: + child_must_continue = True + break + epoch_versions_to_check.append(verflags["epoch-version"]) + else: + child_must_continue = ( + epoch_version in epoch_versions_to_check or epoch_version is None + ) + + log.debug("\tepoch_version: %s", epoch_version) + log.debug("\tchild must continue: %s", child_must_continue) # Suspend execution, yield whether caller should continue, and get back the (partial) result # for this commit and parent results as dictionaries on resume. @@ -220,18 +228,38 @@ class PkgHistoryProcessor: commit_result["epoch-version"] = epoch_version - # Find the maximum applicable parent release number and increment by one. - commit_result["release-number"] = release_number = ( - max( - ( - res["release-number"] if res and epoch_version == res["epoch-version"] else 0 - for res in parent_results - ), - default=0, - ) - + 1 + log.debug("\tepoch_version: %s", epoch_version) + log.debug( + "\tparent rel numbers: %s", + ", ".join(str(res["release-number"]) if res else "none" for res in parent_results), + ) + + # Find the maximum applicable parent release number and increment by one if the + # epoch-version can be parsed from the spec file. + release_number = max( + ( + res["release-number"] + if res + and ( + # Paper over gaps in epoch-versions, these could be simple syntax errors in + # the spec file, or a retired, then unretired package. + epoch_version is None + or res["epoch-version"] is None + or epoch_version == res["epoch-version"] + ) + else 0 + for res in parent_results + ), + default=0, ) + if epoch_version is not None: + release_number += 1 + + commit_result["release-number"] = release_number + + log.debug("\trelease_number: %s", release_number) + prerel_str = "0." if prerelease else "" release_number_with_base = release_number + base - 1 commit_result["release-complete"] = f"{prerel_str}{release_number_with_base}{tag_string}" @@ -305,9 +333,7 @@ class PkgHistoryProcessor: merge_unresolvable = not changelog_changed our_child_must_continue = ( - not (changelog_changed and changelog_blob or merge_unresolvable) - and specfile_present - and child_must_continue + not (changelog_changed and changelog_blob or merge_unresolvable) and child_must_continue ) log.debug("\tchangelog changed: %s", changelog_changed) @@ -335,17 +361,17 @@ class PkgHistoryProcessor: locale="en", ) - changelog_evr = f"{commit_result['epoch-version']}-{commit_result['release-complete']}" + epoch_version = commit_result["epoch-version"] + if epoch_version: + changelog_evr = f" {epoch_version}-{commit_result['release-complete']}" + else: + changelog_evr = "" - changelog_header = f"* {changelog_date} {changelog_author} {changelog_evr}" + changelog_header = f"* {changelog_date} {changelog_author}{changelog_evr}" - skip_for_changelog = False + skip_for_changelog = not specfile_present - if not specfile_present: - # no spec file => start fresh - log.debug("\tno spec file present") - commit_result["changelog"] = () - elif merge_unresolvable: + if merge_unresolvable: log.debug("\tunresolvable merge") changelog_entry["data"] = f"{changelog_header}\n- RPMAUTOSPEC: unresolvable merge" changelog_entry["error"] = "unresolvable merge" @@ -391,6 +417,8 @@ class PkgHistoryProcessor: for f in changed_files ) + log.debug("\tskip_for_changelog: %s", skip_for_changelog) + if not skip_for_changelog: commit_subject = commit.message.split("\n", 1)[0].strip() if commit_subject.startswith("-"): From d28775208b44e7e6c233ce9012178b7957bc579c Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 17:20:44 +0000 Subject: [PATCH 13/14] Refactor changelog entry generation Previously, a changelog entry was pieced together directly in the changelog visitor and a slightly tweaked copy of that code existed in run() which processed uncommitted changes. This moved changelog generation into its own module containing ChangelogEntry, an augmented special purpose dictionary. It carries all information necessary to generate a changelog entry and contains the logic in its format() method. Signed-off-by: Nils Philippsen --- diff --git a/rpmautospec/changelog.py b/rpmautospec/changelog.py new file mode 100644 index 0000000..eb4cdbf --- /dev/null +++ b/rpmautospec/changelog.py @@ -0,0 +1,46 @@ +from textwrap import TextWrapper + +from babel.dates import format_datetime + + +class ChangelogEntry(dict): + """Dictionary holding changelog entry details.""" + + linewrapper = TextWrapper(width=75, subsequent_indent=" ") + + def format(self, **overrides): + entry_info = {**self, **overrides} + + if "error" not in entry_info: + entry_info["error"] = None + if isinstance(entry_info["error"], str): + entry_info["error"] = list(entry_info["error"]) + + if "data" in entry_info: + # verbatim data from the changed `changelog` file + return entry_info["data"] + + changelog_date = format_datetime( + entry_info["timestamp"], format="EEE MMM dd Y", locale="en" + ) + + if entry_info["epoch-version"]: + changelog_evr = f" {entry_info['epoch-version']}-{entry_info['release-complete']}" + else: + changelog_evr = "" + changelog_header = f"* {changelog_date} {entry_info['authorblurb']}{changelog_evr}" + + commit_subject = entry_info["commitlog"].split("\n", 1)[0].strip() + if commit_subject.startswith("-"): + commit_subject = commit_subject.lstrip("-").lstrip() + if not commit_subject: + entry_info["error"].append("empty commit log subject after stripping") + + if entry_info["error"]: + changelog_items = [f"RPMAUTOSPEC: {detail}" for detail in entry_info["error"]] + else: + changelog_items = [commit_subject] + + changelog_body = "\n".join(self.linewrapper.fill(f"- {item}") for item in changelog_items) + + return f"{changelog_header}\n{changelog_body}" diff --git a/rpmautospec/pkg_history.py b/rpmautospec/pkg_history.py index 2826366..5c97389 100644 --- a/rpmautospec/pkg_history.py +++ b/rpmautospec/pkg_history.py @@ -8,12 +8,11 @@ from fnmatch import fnmatchcase from functools import lru_cache, reduce from pathlib import Path from tempfile import TemporaryDirectory -from textwrap import TextWrapper from typing import Any, Dict, Optional, Sequence, Union import pygit2 -from babel.dates import format_datetime +from .changelog import ChangelogEntry from .misc import AUTORELEASE_MACRO @@ -276,6 +275,9 @@ class PkgHistoryProcessor: files.add(delta.new_file.path) return files + def _changelog_for_commit(self, commit, commit_result): + """Generate the changelog entry text from a commit.""" + def changelog_visitor(self, commit: pygit2.Commit, child_info: Dict[str, Any]): """Visit a commit to generate changelog entries for it and its parents. @@ -350,30 +352,21 @@ class PkgHistoryProcessor: and (child_changelog_removed or our_changelog_removed), } - changelog_entry = { - "commit-id": commit.id, - } - - changelog_author = f"{commit.author.name} <{commit.author.email}>" - changelog_date = format_datetime( - dt.datetime.utcfromtimestamp(commit.commit_time), - format="EEE MMM dd Y", - locale="en", + changelog_entry = ChangelogEntry( + { + "commit-id": commit.id, + "authorblurb": f"{commit.author.name} <{commit.author.email}>", + "timestamp": dt.datetime.utcfromtimestamp(commit.commit_time), + "commitlog": commit.message, + "epoch-version": commit_result["epoch-version"], + "release-complete": commit_result["release-complete"], + } ) - epoch_version = commit_result["epoch-version"] - if epoch_version: - changelog_evr = f" {epoch_version}-{commit_result['release-complete']}" - else: - changelog_evr = "" - - changelog_header = f"* {changelog_date} {changelog_author}{changelog_evr}" - skip_for_changelog = not specfile_present if merge_unresolvable: log.debug("\tunresolvable merge") - changelog_entry["data"] = f"{changelog_header}\n- RPMAUTOSPEC: unresolvable merge" changelog_entry["error"] = "unresolvable merge" previous_changelog = () commit_result["changelog"] = (changelog_entry,) @@ -419,16 +412,9 @@ class PkgHistoryProcessor: log.debug("\tskip_for_changelog: %s", skip_for_changelog) + changelog_entry["skip"] = skip_for_changelog + if not skip_for_changelog: - commit_subject = commit.message.split("\n", 1)[0].strip() - if commit_subject.startswith("-"): - commit_subject = commit_subject[1:].lstrip() - if not commit_subject: - commit_subject = "RPMAUTOSPEC: empty commit log subject after stripping" - changelog_entry["error"] = "empty commit log subject" - wrapper = TextWrapper(width=75, subsequent_indent=" ") - wrapped_msg = wrapper.fill(f"- {commit_subject}") - changelog_entry["data"] = f"{changelog_header}\n{wrapped_msg}" commit_result["changelog"] = (changelog_entry,) + previous_changelog else: commit_result["changelog"] = previous_changelog @@ -726,24 +712,24 @@ class PkgHistoryProcessor: if not skip_for_changelog: try: signature = self.repo.default_signature - changelog_author = f"{signature.name} <{signature.email}>" + authorblurb = f"{signature.name} <{signature.email}>" except AttributeError: # self.repo == None -> no git repo - changelog_author = self._get_rpm_packager() + authorblurb = self._get_rpm_packager() except KeyError: - changelog_author = "Unknown User " - changelog_date = format_datetime( - dt.datetime.utcnow(), format="EEE MMM dd Y", locale="en" + authorblurb = "Unknown User " + + changelog_entry = ChangelogEntry( + { + "commit-id": None, + "authorblurb": authorblurb, + "timestamp": dt.datetime.utcnow(), + "commitlog": "Uncommitted changes", + "epoch-version": epoch_version, + "release-complete": release_complete, + } ) - changelog_evr = f"{epoch_version}-{release_complete}" - changelog_header = f"* {changelog_date} {changelog_author} {changelog_evr}" - changelog_item = "- Uncommitted changes" - - changelog_entry = { - "commit-id": None, - "data": f"{changelog_header}\n{changelog_item}", - } changelog = (changelog_entry,) + previous_changelog else: changelog = previous_changelog diff --git a/rpmautospec/subcommands/changelog.py b/rpmautospec/subcommands/changelog.py index a1607ca..46dfcd8 100644 --- a/rpmautospec/subcommands/changelog.py +++ b/rpmautospec/subcommands/changelog.py @@ -42,9 +42,9 @@ def collate_changelog( ) -> Union[str, bytes]: changelog = processor_results["changelog"] if result_type == str: - entry_strings = (_coerce_to_str(entry["data"]) for entry in changelog) + entry_strings = (_coerce_to_str(entry.format()) for entry in changelog) else: - entry_strings = (_coerce_to_bytes(entry["data"]) for entry in changelog) + entry_strings = (_coerce_to_bytes(entry.format()) for entry in changelog) return "\n\n".join(entry_strings) diff --git a/rpmautospec/subcommands/process_distgit.py b/rpmautospec/subcommands/process_distgit.py index fd0f243..50757c1 100644 --- a/rpmautospec/subcommands/process_distgit.py +++ b/rpmautospec/subcommands/process_distgit.py @@ -112,7 +112,7 @@ def process_distgit( if needs_autochangelog: print( - "\n\n".join(entry["data"] for entry in result["changelog"]), + "\n\n".join(entry.format() for entry in result["changelog"]), file=tmp_specfile, ) diff --git a/tests/rpmautospec/test_changelog.py b/tests/rpmautospec/test_changelog.py new file mode 100644 index 0000000..9849a8c --- /dev/null +++ b/tests/rpmautospec/test_changelog.py @@ -0,0 +1,220 @@ +import datetime as dt +import locale +import os +import re +import stat +from calendar import LocaleTextCalendar +from shutil import rmtree +from unittest.mock import patch + +import pygit2 +import pytest + +from rpmautospec.pkg_history import PkgHistoryProcessor + + +@pytest.fixture +def processor(repo): + processor = PkgHistoryProcessor(repo.workdir) + return processor + + +@pytest.fixture +def setlocale(): + """Allow temporary modification of locale settings.""" + saved_locale_settings = { + category: locale.getlocale(getattr(locale, category)) + for category in dir(locale) + if category.startswith("LC_") and category != "LC_ALL" + } + + yield locale.setlocale + + for category, locale_settings in saved_locale_settings.items(): + locale.setlocale(getattr(locale, category), locale_settings) + + +class TestPkgHistoryProcessor: + + version_re = re.compile(r"^Version: .*$", flags=re.MULTILINE) + + @pytest.mark.parametrize( + "testcase", + ( + "str, is file", + "str, is dir", + "path, is file", + "path, is file, wrong extension", + "path, is dir", + "doesn't exist", + "spec doesn't exist, is dir", + "no git repo", + "not a regular file", + ), + ) + @patch("rpmautospec.pkg_history.pygit2") + def test___init__(self, pygit2, testcase, specfile): + if "wrong extension" in testcase: + # Path.rename() only returns the new path from Python 3.8 on. + specfile.rename(specfile.with_suffix(".foo")) + specfile = specfile.with_suffix(".foo") + + spec_or_path = specfile + + if "is dir" in testcase: + spec_or_path = spec_or_path.parent + + if "spec doesn't exist" in testcase: + specfile.unlink() + elif "doesn't exist" in testcase: + rmtree(specfile.parent) + + if "str" in testcase: + spec_or_path = str(spec_or_path) + + if "doesn't exist" in testcase: + with pytest.raises(RuntimeError) as excinfo: + PkgHistoryProcessor(spec_or_path) + if "spec doesn't exist" in testcase: + expected_message = f"Spec file '{specfile}' doesn't exist in '{specfile.parent}'." + else: + expected_message = f"Spec file or path '{spec_or_path}' doesn't exist." + assert str(excinfo.value) == expected_message + return + + if "not a regular file" in testcase: + specfile.unlink() + os.mknod(specfile, stat.S_IFIFO | stat.S_IRUSR | stat.S_IWUSR) + with pytest.raises(RuntimeError) as excinfo: + PkgHistoryProcessor(spec_or_path) + assert str(excinfo.value) == "File specified as `spec_or_path` is not a regular file." + return + + if "wrong extension" in testcase: + with pytest.raises(ValueError) as excinfo: + PkgHistoryProcessor(spec_or_path) + assert str(excinfo.value) == ( + "File specified as `spec_or_path` must have '.spec' as an extension." + ) + return + + if "no git repo" in testcase: + + class GitError(Exception): + pass + + pygit2.GitError = GitError + pygit2.Repository.side_effect = GitError + + processor = PkgHistoryProcessor(spec_or_path) + + assert processor.specfile == specfile + assert processor.path == specfile.parent + + pygit2.Repository.assert_called_once() + + if "no git repo" in testcase: + assert processor.repo is None + else: + assert processor.repo + + @pytest.mark.parametrize("testcase", ("normal", "no spec file")) + def test__get_rpmverflags_for_commit(self, testcase, specfile, repo, processor): + head_commit = repo[repo.head.target] + + if testcase == "no spec file": + index = repo.index + index.remove(specfile.name) + index.write() + + tree = index.write_tree() + + parent, ref = repo.resolve_refish(repo.head.name) + + head_commit = repo[ + repo.create_commit( + ref.name, + repo.default_signature, + repo.default_signature, + "Be gone, spec file!", + tree, + [parent.oid], + ) + ] + + assert processor._get_rpmverflags_for_commit(head_commit) is None + else: + assert processor._get_rpmverflags_for_commit(head_commit)["epoch-version"] == "1.0" + + @pytest.mark.parametrize( + "testcase", ("without commit", "with commit", "all results", "locale set", "without repo") + ) + def test_run(self, testcase, repo, processor, setlocale): + if testcase == "locale set": + setlocale(locale.LC_ALL, "de_DE.UTF-8") + + all_results = "all results" in testcase + + if testcase == "without repo": + rmtree(repo.path) + processor = PkgHistoryProcessor(repo.workdir) + head_commit = None + else: + head_commit = repo[repo.head.target] + + if testcase == "with commit": + args = [head_commit] + else: + args = [] + + res = processor.run( + *args, + visitors=[processor.release_number_visitor, processor.changelog_visitor], + all_results=all_results, + ) + + assert isinstance(res, dict) + if all_results: + assert all(isinstance(key, pygit2.Commit) for key in res) + # only verify outcome for head commit below + res = res[head_commit] + else: + assert all(isinstance(key, str) for key in res) + + changelog = res["changelog"] + top_entry = changelog[0] + + if testcase == "without repo": + assert res["release-number"] == 1 + + for snippet in ( + processor._get_rpm_packager(), + "- Uncommitted changes", + ): + assert snippet in top_entry.format() + else: + assert res["commit-id"] == head_commit.id + assert res["release-number"] == 2 + assert top_entry["commit-id"] == head_commit.id + + for snippet in ( + "Jane Doe ", + "- Did nothing!", + ): + assert snippet in top_entry.format() + + cal = LocaleTextCalendar(firstweekday=0, locale="C.UTF-8") + commit_time = dt.datetime.utcfromtimestamp(head_commit.commit_time) + weekdayname = cal.formatweekday(day=commit_time.weekday(), width=3) + monthname = cal.formatmonthname( + theyear=commit_time.year, + themonth=commit_time.month, + width=1, + withyear=False, + )[:3] + expected_date_blurb = ( + f"* {weekdayname} {monthname} {commit_time.day:02} {commit_time.year}" + ) + assert top_entry.format().startswith(expected_date_blurb) + + assert all("error" not in entry for entry in changelog) diff --git a/tests/rpmautospec/test_pkg_history.py b/tests/rpmautospec/test_pkg_history.py index 72c53e6..9849a8c 100644 --- a/tests/rpmautospec/test_pkg_history.py +++ b/tests/rpmautospec/test_pkg_history.py @@ -191,7 +191,7 @@ class TestPkgHistoryProcessor: processor._get_rpm_packager(), "- Uncommitted changes", ): - assert snippet in top_entry["data"] + assert snippet in top_entry.format() else: assert res["commit-id"] == head_commit.id assert res["release-number"] == 2 @@ -201,7 +201,7 @@ class TestPkgHistoryProcessor: "Jane Doe ", "- Did nothing!", ): - assert snippet in top_entry["data"] + assert snippet in top_entry.format() cal = LocaleTextCalendar(firstweekday=0, locale="C.UTF-8") commit_time = dt.datetime.utcfromtimestamp(head_commit.commit_time) @@ -215,6 +215,6 @@ class TestPkgHistoryProcessor: expected_date_blurb = ( f"* {weekdayname} {monthname} {commit_time.day:02} {commit_time.year}" ) - assert top_entry["data"].startswith(expected_date_blurb) + assert top_entry.format().startswith(expected_date_blurb) assert all("error" not in entry for entry in changelog) From 7abff9b07ffa60031703483dbf73b9f15565ed07 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mar 04 2022 17:20:44 +0000 Subject: [PATCH 14/14] Enable longer changelog entries Previously, only the git commit log subject would be considered for changelog generation, which is quite restrictive because it's usually limited to 50 characters in length. Also, people might want one commit to contain more than one change and have one item for each. This change enables both: - By prepending an ellipsis (three dots '...' or the equivalent Unicode character '…') to the body, the subsequent text will be appended to the first item of a changelog entry. - Dashed list items following the first item will be added to the changelog entry. For example, this commit log text ...: --- 8< --- This is the first item but I'm feeling verbose ... today so it doesn't fit into the subject. - And here I describe another change. --- >8 --- ... would result in a changelog entry like this: --- 8< --- * Fri Mar 04 2022 Nils Philippsen - 1.0-1 - This is the first item but I'm feeling verbose today so it doesn't fit into the subject. - And here I describe another change. --- >8 --- Additionally, it quotes percent signs in generated changelog entries (but not in the `changelog` file, if it exists it will be included verbatim) so RPM doesn't attempt to expand them as macros. Fixes: #189, #234 Signed-off-by: Nils Philippsen --- diff --git a/ci/pytest.yaml b/ci/pytest.yaml index 0bac22b..4c5367b 100644 --- a/ci/pytest.yaml +++ b/ci/pytest.yaml @@ -16,6 +16,7 @@ - python3-pygit2 - python3-pytest - python3-pytest-cov + - python3-pyyaml - python3-rpm state: present diff --git a/python-rpmautospec.spec b/python-rpmautospec.spec index 7d08191..2070a72 100644 --- a/python-rpmautospec.spec +++ b/python-rpmautospec.spec @@ -38,6 +38,7 @@ BuildRequires: python3-koji BuildRequires: python3-pygit2 BuildRequires: python%{python3_pkgversion}-pytest BuildRequires: python%{python3_pkgversion}-pytest-cov +BuildRequires: python%{python3_pkgversion}-pyyaml BuildRequires: git %endif @@ -146,6 +147,9 @@ install -m 644 rpm/macros.d/macros.rpmautospec %{buildroot}%{rpmmacrodir}/ %endif %changelog +* Fri Mar 04 2022 Nils Philippsen +- require python3-pyyaml for building + * Sun Nov 07 2021 Nils Philippsen - require python3-babel and glibc langpacks (the latter for testing) diff --git a/rpmautospec/changelog.py b/rpmautospec/changelog.py index eb4cdbf..a48e482 100644 --- a/rpmautospec/changelog.py +++ b/rpmautospec/changelog.py @@ -1,12 +1,91 @@ +import re +from enum import Enum, auto from textwrap import TextWrapper +from typing import List from babel.dates import format_datetime +class CommitLogParseState(int, Enum): + before_subject = auto() + subject = auto() + before_body = auto() + in_continuation = auto() + body = auto() + + class ChangelogEntry(dict): """Dictionary holding changelog entry details.""" linewrapper = TextWrapper(width=75, subsequent_indent=" ") + ellipsis_re = re.compile(r"^(?P\.{3,}|…+)\s*(?P.*)$") + + @classmethod + def commitlog_to_changelog_items(cls, commitlog: str) -> List[str]: + changelog_items_lines: List[List[str]] = [[]] + + state: CommitLogParseState = CommitLogParseState.before_subject + + for line in commitlog.split("\n"): + # quote percent characters in the commit log + line = line.replace("%", "%%").strip() + + if state == CommitLogParseState.before_subject: + if not line: # pragma: no cover + # fast-forward to subject if it's not right at the beginning + # (unlikely) + continue + + state = CommitLogParseState.subject + # strip off leading dash from subject, if any + if line.startswith("-"): + line = line[1:].lstrip() + + if state == CommitLogParseState.subject: + if line: + changelog_items_lines[0].append(line) + continue + else: + state = CommitLogParseState.before_body + + if state == CommitLogParseState.before_body: + if not line: + # fast-forward to body + continue + + match = cls.ellipsis_re.match(line) + if match: + state = CommitLogParseState.in_continuation + changelog_items_lines[0].append(match.group("rest")) + continue + else: + if not line.startswith("-"): + # bail out + break + state = CommitLogParseState.body + + if state == CommitLogParseState.in_continuation: + if not line or line.startswith("-"): + state = CommitLogParseState.body + else: + changelog_items_lines[0].append(line) + continue + + # state == CommitLogParseState.body + + if not line: + # outta here, we're done + break + + if line.startswith("-"): + line = line[1:].lstrip() + changelog_items_lines.append([]) + + changelog_items_lines[-1].append(line) + + # Now changelog_items_lines should contain one list per changelog item, containing all lines + # (stripped of prefixes and such). Merge these lines into a single one per item. + return [" ".join(lines) for lines in changelog_items_lines] def format(self, **overrides): entry_info = {**self, **overrides} @@ -14,7 +93,7 @@ class ChangelogEntry(dict): if "error" not in entry_info: entry_info["error"] = None if isinstance(entry_info["error"], str): - entry_info["error"] = list(entry_info["error"]) + entry_info["error"] = [entry_info["error"]] if "data" in entry_info: # verbatim data from the changed `changelog` file @@ -25,21 +104,17 @@ class ChangelogEntry(dict): ) if entry_info["epoch-version"]: - changelog_evr = f" {entry_info['epoch-version']}-{entry_info['release-complete']}" + changelog_evr = f" {entry_info['epoch-version']}" + if entry_info["release-complete"]: + changelog_evr += f"-{entry_info['release-complete']}" else: changelog_evr = "" changelog_header = f"* {changelog_date} {entry_info['authorblurb']}{changelog_evr}" - commit_subject = entry_info["commitlog"].split("\n", 1)[0].strip() - if commit_subject.startswith("-"): - commit_subject = commit_subject.lstrip("-").lstrip() - if not commit_subject: - entry_info["error"].append("empty commit log subject after stripping") - if entry_info["error"]: changelog_items = [f"RPMAUTOSPEC: {detail}" for detail in entry_info["error"]] else: - changelog_items = [commit_subject] + changelog_items = self.commitlog_to_changelog_items(entry_info["commitlog"]) changelog_body = "\n".join(self.linewrapper.fill(f"- {item}") for item in changelog_items) diff --git a/test_requirements.txt b/test_requirements.txt index 9955dec..3c4c74d 100644 --- a/test_requirements.txt +++ b/test_requirements.txt @@ -1,2 +1,3 @@ +PyYAML pytest pytest-cov diff --git a/tests/rpmautospec/test_changelog.py b/tests/rpmautospec/test_changelog.py index 9849a8c..0fed82b 100644 --- a/tests/rpmautospec/test_changelog.py +++ b/tests/rpmautospec/test_changelog.py @@ -1,220 +1,153 @@ import datetime as dt -import locale -import os import re -import stat -from calendar import LocaleTextCalendar -from shutil import rmtree -from unittest.mock import patch +from pathlib import Path -import pygit2 import pytest +import yaml + +from rpmautospec.changelog import ChangelogEntry + +HERE = Path(__file__).parent +COMMITLOG_CHANGELOG_DIR = HERE.parent / "test-data" / "commitlog-to-changelog" +COMMITLOGFILE_RE = re.compile(r"^commit-(?P.*)\.txt$") +TESTDATA = {} + + +def _read_commitlog_changelog_testdata(): + if not TESTDATA: + for commitlog_path in sorted(COMMITLOG_CHANGELOG_DIR.glob("commit*.txt")): + match = COMMITLOGFILE_RE.match(commitlog_path.name) + variant = match.group("variant") + chlog_items_path = commitlog_path.with_name(f"expected-{variant}.yaml") + chlog_entry_path = commitlog_path.with_name(f"expected-{variant}.txt") + with open(chlog_items_path, "r") as chlog_items_fp: + TESTDATA[variant] = ( + commitlog_path.read_text(), + yaml.safe_load(chlog_items_fp)["changelog_items"], + chlog_entry_path.read_text(), + ) + return TESTDATA -from rpmautospec.pkg_history import PkgHistoryProcessor - - -@pytest.fixture -def processor(repo): - processor = PkgHistoryProcessor(repo.workdir) - return processor - - -@pytest.fixture -def setlocale(): - """Allow temporary modification of locale settings.""" - saved_locale_settings = { - category: locale.getlocale(getattr(locale, category)) - for category in dir(locale) - if category.startswith("LC_") and category != "LC_ALL" - } - - yield locale.setlocale - - for category, locale_settings in saved_locale_settings.items(): - locale.setlocale(getattr(locale, category), locale_settings) - - -class TestPkgHistoryProcessor: - - version_re = re.compile(r"^Version: .*$", flags=re.MULTILINE) - - @pytest.mark.parametrize( - "testcase", - ( - "str, is file", - "str, is dir", - "path, is file", - "path, is file, wrong extension", - "path, is dir", - "doesn't exist", - "spec doesn't exist, is dir", - "no git repo", - "not a regular file", - ), - ) - @patch("rpmautospec.pkg_history.pygit2") - def test___init__(self, pygit2, testcase, specfile): - if "wrong extension" in testcase: - # Path.rename() only returns the new path from Python 3.8 on. - specfile.rename(specfile.with_suffix(".foo")) - specfile = specfile.with_suffix(".foo") - - spec_or_path = specfile - - if "is dir" in testcase: - spec_or_path = spec_or_path.parent - - if "spec doesn't exist" in testcase: - specfile.unlink() - elif "doesn't exist" in testcase: - rmtree(specfile.parent) - - if "str" in testcase: - spec_or_path = str(spec_or_path) - - if "doesn't exist" in testcase: - with pytest.raises(RuntimeError) as excinfo: - PkgHistoryProcessor(spec_or_path) - if "spec doesn't exist" in testcase: - expected_message = f"Spec file '{specfile}' doesn't exist in '{specfile.parent}'." - else: - expected_message = f"Spec file or path '{spec_or_path}' doesn't exist." - assert str(excinfo.value) == expected_message - return - - if "not a regular file" in testcase: - specfile.unlink() - os.mknod(specfile, stat.S_IFIFO | stat.S_IRUSR | stat.S_IWUSR) - with pytest.raises(RuntimeError) as excinfo: - PkgHistoryProcessor(spec_or_path) - assert str(excinfo.value) == "File specified as `spec_or_path` is not a regular file." - return - - if "wrong extension" in testcase: - with pytest.raises(ValueError) as excinfo: - PkgHistoryProcessor(spec_or_path) - assert str(excinfo.value) == ( - "File specified as `spec_or_path` must have '.spec' as an extension." - ) - return - - if "no git repo" in testcase: - class GitError(Exception): - pass +def pytest_generate_tests(metafunc): + if ( + "commitlog_chlogitems" in metafunc.fixturenames + or "commitlog_chlogentry" in metafunc.fixturenames + ): + _read_commitlog_changelog_testdata() - pygit2.GitError = GitError - pygit2.Repository.side_effect = GitError + if "commitlog_chlogitems" in metafunc.fixturenames: + metafunc.parametrize( + "commitlog_chlogitems", + [(val[0], val[1]) for val in TESTDATA.values()], + ids=(f"commitlog-{variant}" for variant in TESTDATA), + ) - processor = PkgHistoryProcessor(spec_or_path) + if "commitlog_chlogentry" in metafunc.fixturenames: + metafunc.parametrize( + "commitlog_chlogentry", + [(val[0], val[2]) for val in TESTDATA.values()], + ids=(f"commitlog-{variant}" for variant in TESTDATA), + ) - assert processor.specfile == specfile - assert processor.path == specfile.parent - pygit2.Repository.assert_called_once() +class TestChangelogEntry: + @staticmethod + def _parametrize_commitlog(commitlog, *, subject_with_dash, trailing_newline): + if subject_with_dash: + commitlog = f"- {commitlog}" - if "no git repo" in testcase: - assert processor.repo is None + if trailing_newline: + if commitlog[-1] != "\n": + commitlog += "\n" else: - assert processor.repo + commitlog = commitlog.rstrip("\n") - @pytest.mark.parametrize("testcase", ("normal", "no spec file")) - def test__get_rpmverflags_for_commit(self, testcase, specfile, repo, processor): - head_commit = repo[repo.head.target] + return commitlog - if testcase == "no spec file": - index = repo.index - index.remove(specfile.name) - index.write() + @pytest.mark.parametrize("subject_with_dash", (True, False)) + @pytest.mark.parametrize("trailing_newline", (True, False)) + def test_commitlog_to_changelog_items( + self, subject_with_dash, trailing_newline, commitlog_chlogitems + ): + commitlog, expected_changelog_items = commitlog_chlogitems - tree = index.write_tree() - - parent, ref = repo.resolve_refish(repo.head.name) - - head_commit = repo[ - repo.create_commit( - ref.name, - repo.default_signature, - repo.default_signature, - "Be gone, spec file!", - tree, - [parent.oid], - ) - ] - - assert processor._get_rpmverflags_for_commit(head_commit) is None - else: - assert processor._get_rpmverflags_for_commit(head_commit)["epoch-version"] == "1.0" + commitlog = self._parametrize_commitlog( + commitlog, subject_with_dash=subject_with_dash, trailing_newline=trailing_newline + ) - @pytest.mark.parametrize( - "testcase", ("without commit", "with commit", "all results", "locale set", "without repo") - ) - def test_run(self, testcase, repo, processor, setlocale): - if testcase == "locale set": - setlocale(locale.LC_ALL, "de_DE.UTF-8") + changelog_items = ChangelogEntry.commitlog_to_changelog_items(commitlog) + assert changelog_items == expected_changelog_items + + @pytest.mark.parametrize("with_epoch_version_release", (True, "epoch-version", False)) + @pytest.mark.parametrize("with_error_is_none", (False, True)) + @pytest.mark.parametrize("subject_with_dash", (True, False)) + @pytest.mark.parametrize("trailing_newline", (True, False)) + def test_format( + self, + with_epoch_version_release, + with_error_is_none, + subject_with_dash, + trailing_newline, + commitlog_chlogentry, + ): + commitlog, expected_changelog_entry = commitlog_chlogentry + + commitlog = self._parametrize_commitlog( + commitlog, subject_with_dash=subject_with_dash, trailing_newline=trailing_newline + ) - all_results = "all results" in testcase + changelog_entry = ChangelogEntry( + { + "timestamp": dt.datetime(1970, 1, 1, 0, 0, 0), + "authorblurb": "An Author ", + "epoch-version": None, + "release-complete": None, + "commitlog": commitlog, + } + ) - if testcase == "without repo": - rmtree(repo.path) - processor = PkgHistoryProcessor(repo.workdir) - head_commit = None - else: - head_commit = repo[repo.head.target] + expected_evr = "" + if with_epoch_version_release: + changelog_entry["epoch-version"] = "1.0" + expected_evr = " 1.0" + if with_epoch_version_release != "epoch-version": + changelog_entry["release-complete"] = "1" + expected_evr = " 1.0-1" - if testcase == "with commit": - args = [head_commit] - else: - args = [] + if with_error_is_none: + changelog_entry["error"] = None - res = processor.run( - *args, - visitors=[processor.release_number_visitor, processor.changelog_visitor], - all_results=all_results, + expected_changelog_entry = ( + f"* Thu Jan 01 1970 An Author {expected_evr}\n" + + expected_changelog_entry ) - assert isinstance(res, dict) - if all_results: - assert all(isinstance(key, pygit2.Commit) for key in res) - # only verify outcome for head commit below - res = res[head_commit] - else: - assert all(isinstance(key, str) for key in res) + formatted_changelog_entry = changelog_entry.format() + assert formatted_changelog_entry == expected_changelog_entry.rstrip("\n") + + @pytest.mark.parametrize("error", ("string", "list")) + def test_format_error(self, error): + changelog_entry = ChangelogEntry( + { + "timestamp": dt.datetime(1970, 1, 1, 0, 0, 0), + "authorblurb": "An Author ", + "epoch-version": "1.0", + "release-complete": "1", + } + ) - changelog = res["changelog"] - top_entry = changelog[0] + if error == "string": + changelog_entry["error"] = "a string" + else: # error == "list" + changelog_entry["error"] = ["a string", "and another"] - if testcase == "without repo": - assert res["release-number"] == 1 + expected_changelog_entry = ( + "* Thu Jan 01 1970 An Author 1.0-1\n- RPMAUTOSPEC: a string" + ) - for snippet in ( - processor._get_rpm_packager(), - "- Uncommitted changes", - ): - assert snippet in top_entry.format() - else: - assert res["commit-id"] == head_commit.id - assert res["release-number"] == 2 - assert top_entry["commit-id"] == head_commit.id - - for snippet in ( - "Jane Doe ", - "- Did nothing!", - ): - assert snippet in top_entry.format() - - cal = LocaleTextCalendar(firstweekday=0, locale="C.UTF-8") - commit_time = dt.datetime.utcfromtimestamp(head_commit.commit_time) - weekdayname = cal.formatweekday(day=commit_time.weekday(), width=3) - monthname = cal.formatmonthname( - theyear=commit_time.year, - themonth=commit_time.month, - width=1, - withyear=False, - )[:3] - expected_date_blurb = ( - f"* {weekdayname} {monthname} {commit_time.day:02} {commit_time.year}" - ) - assert top_entry.format().startswith(expected_date_blurb) + if error == "list": + expected_changelog_entry += "\n- RPMAUTOSPEC: and another" - assert all("error" not in entry for entry in changelog) + formatted_changelog_entry = changelog_entry.format() + assert formatted_changelog_entry == expected_changelog_entry.rstrip("\n") diff --git a/tests/test-data/commitlog-to-changelog/commit-1.txt b/tests/test-data/commitlog-to-changelog/commit-1.txt new file mode 100644 index 0000000..b2ce905 --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/commit-1.txt @@ -0,0 +1,13 @@ +A commit subject on a pretty long line so that it + +… would overshoot limits continued here. +- Some item which is also pretty verbose and contains a lot of detail so that + it would be continued in the next line but it should still become one +changelog item. And a %macro. +- Some +- more +- but +- shorter +- items + +This should be ignored. diff --git a/tests/test-data/commitlog-to-changelog/commit-2.txt b/tests/test-data/commitlog-to-changelog/commit-2.txt new file mode 100644 index 0000000..d845516 --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/commit-2.txt @@ -0,0 +1,3 @@ +A commit subject on a pretty long line two three + +This should be ignored. diff --git a/tests/test-data/commitlog-to-changelog/commit-3.txt b/tests/test-data/commitlog-to-changelog/commit-3.txt new file mode 100644 index 0000000..c793f26 --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/commit-3.txt @@ -0,0 +1,5 @@ +A commit subject + +- Another item + +This should be ignored. diff --git a/tests/test-data/commitlog-to-changelog/commit-4.txt b/tests/test-data/commitlog-to-changelog/commit-4.txt new file mode 100644 index 0000000..200b8f7 --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/commit-4.txt @@ -0,0 +1,6 @@ +The subject + +...continues here. And it continues +even further. + +Here's more detail, but it should be ignored. diff --git a/tests/test-data/commitlog-to-changelog/expected-1.txt b/tests/test-data/commitlog-to-changelog/expected-1.txt new file mode 100644 index 0000000..6d99794 --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/expected-1.txt @@ -0,0 +1,10 @@ +- A commit subject on a pretty long line so that it would overshoot limits + continued here. +- Some item which is also pretty verbose and contains a lot of detail so + that it would be continued in the next line but it should still become + one changelog item. And a %%macro. +- Some +- more +- but +- shorter +- items diff --git a/tests/test-data/commitlog-to-changelog/expected-1.yaml b/tests/test-data/commitlog-to-changelog/expected-1.yaml new file mode 100644 index 0000000..4489a1e --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/expected-1.yaml @@ -0,0 +1,12 @@ +--- +changelog_items: + - A commit subject on a pretty long line so that it would overshoot limits + continued here. + - Some item which is also pretty verbose and contains a lot of detail so + that it would be continued in the next line but it should still become + one changelog item. And a %%macro. + - Some + - more + - but + - shorter + - items diff --git a/tests/test-data/commitlog-to-changelog/expected-2.txt b/tests/test-data/commitlog-to-changelog/expected-2.txt new file mode 100644 index 0000000..57e78db --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/expected-2.txt @@ -0,0 +1 @@ +- A commit subject on a pretty long line two three diff --git a/tests/test-data/commitlog-to-changelog/expected-2.yaml b/tests/test-data/commitlog-to-changelog/expected-2.yaml new file mode 100644 index 0000000..fd974fa --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/expected-2.yaml @@ -0,0 +1,3 @@ +--- +changelog_items: + - A commit subject on a pretty long line two three diff --git a/tests/test-data/commitlog-to-changelog/expected-3.txt b/tests/test-data/commitlog-to-changelog/expected-3.txt new file mode 100644 index 0000000..d17cdc7 --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/expected-3.txt @@ -0,0 +1,2 @@ +- A commit subject +- Another item diff --git a/tests/test-data/commitlog-to-changelog/expected-3.yaml b/tests/test-data/commitlog-to-changelog/expected-3.yaml new file mode 100644 index 0000000..00d4d42 --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/expected-3.yaml @@ -0,0 +1,4 @@ +--- +changelog_items: + - A commit subject + - Another item diff --git a/tests/test-data/commitlog-to-changelog/expected-4.txt b/tests/test-data/commitlog-to-changelog/expected-4.txt new file mode 100644 index 0000000..9eeaa16 --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/expected-4.txt @@ -0,0 +1 @@ +- The subject continues here. And it continues even further. diff --git a/tests/test-data/commitlog-to-changelog/expected-4.yaml b/tests/test-data/commitlog-to-changelog/expected-4.yaml new file mode 100644 index 0000000..8a7006e --- /dev/null +++ b/tests/test-data/commitlog-to-changelog/expected-4.yaml @@ -0,0 +1,3 @@ +--- +changelog_items: + - The subject continues here. And it continues even further.