PR#41: Inject the real %autorel macro into spec files (+ misc fixes) - fedora-infra/rpmautospec

fedora-infra / rpmautospec

#41 Inject the real %autorel macro into spec files (+ misc fixes)

Merged 4 years ago by pingou. Opened 4 years ago by nphilipp.

fedora-infra/ nphilipp/rpmautospec master--use-real-autorel-macro into master

Inject the real %autorel macro into spec files

Nils Philippsen • 4 years ago

0ad947f

Allow to strip the dist tag off the release

Nils Philippsen • 4 years ago

de37fe2

Make `run_command()` generally accessible

Nils Philippsen • 4 years ago

6f0480b

Rename the directory with the plugins, too

Nils Philippsen • 4 years ago

c318aa5

MANIFEST.in

file modified

+1 -1

		`@@ -4,6 +4,6 @@`
		`include *.txt`
		`include *.rst`
		`recursive-include rpmautospec *.py`
		`- recursive-include koji_plugin *.py`
		`+ recursive-include koji_plugins *.py`
		`recursive-include tests .py .tar.gz .spec.expected .json`
		`recursive-excludes *.pyc`

koji_plugins/__init__.py ~~koji_plugin/__init__.py~~

file renamed

file was moved with no change to the file

koji_plugins/rpmautospec_builder.py ~~koji_plugin/rpmautospec_builder.py~~

file renamed

file was moved with no change to the file

koji_plugins/rpmautospec_hub.py ~~koji_plugin/rpmautospec_hub.py~~

file renamed

file was moved with no change to the file

python-rpmautospec.spec

file modified

+1 -1

		`@@ -89,7 +89,7 @@`
		`%py3_install`
		`for plugin_type in builder hub; do`
		`mkdir -p %{buildroot}%{_prefix}/lib/koji-${plugin_type}-plugins/`
		`- install -m 0644 koji_plugin/rpmautospec_${plugin_type}.py \`
		`+ install -m 0644 koji_plugins/rpmautospec_${plugin_type}.py \`
		`%{buildroot}%{_prefix}/lib/koji-${plugin_type}-plugins/`
		`done`

rpmautospec/changelog.py

file modified

+1 -17

		`@@ -4,11 +4,11 @@`
		`import os`
		`import re`
		`import shutil`
		`- import subprocess`
		`import tempfile`
		`import textwrap`
		`import typing`

		`+ from .misc import run_command`
		`from .py2compat.escape_tags import unescape_tag`

		`_log = logging.getLogger(__name__)`
		`@@ -26,22 +26,6 @@`
		`return subcmd_name`


		`- def run_command(command: list, cwd: typing.Optional[str] = None) -> bytes:`
		`- """ Run the specified command in a specific working directory if one`
		`- is specified.`
		`- """`
		`- output = None`
		`- try:`
		`- output = subprocess.check_output(command, cwd=cwd, stderr=subprocess.PIPE)`
		`- except subprocess.CalledProcessError as e:`
		- _log.error("Command `{}` return code: `{}`".format(" ".join(command), e.returncode))
		`- _log.error("stdout:\n-------\n{}".format(e.stdout))`
		`- _log.error("stderr:\n-------\n{}".format(e.stderr))`
		`- raise`
		`-`
		`- return output`
		`-`
		`-`
		`def git_get_log(`
		`path: str,`
		`log_options: typing.Optional[typing.List[str]] = None,`

rpmautospec/etc/autorel-macro.txt

file added

+41

		`@@ -0,0 +1,41 @@`
		`+ ## START: Automatic release macro`
		`+ %define autorel(e:s:hp) %{lua:`
		`+ has_extraver = rpm.expand("%{-e}") ~= ""`
		`+ has_snapinfo = rpm.expand("%{-s}") ~= ""`
		`+ is_prerelease = rpm.expand("%{-p}") ~= ""`
		`+ is_hotfix = rpm.expand("%{-h}") ~= ""`
		`+`
		`+ if has_extraver`
		`+ then`
		`+ rpm.define("_autorel_extraver .%{-e*}")`
		`+ else`
		`+ rpm.define("_autorel_extraver %{nil}")`
		`+ end`
		`+`
		`+ if has_snapinfo`
		`+ then`
		`+ rpm.define("_autorel_snapinfo .%{-s*}")`
		`+ else`
		`+ rpm.define("_autorel_snapinfo %{nil}")`
		`+ end`
		`+`
		`+ if is_prerelease`
		`+ then`
		`+ if is_hotfix`
		`+ then`
		`+ option = "prerel_hotfix_cadence"`
		`+ else`
		`+ option = "prerel_cadence"`
		`+ end`
		`+ else`
		`+ if is_hotfix`
		`+ then`
		`+ option = "hotfix_cadence"`
		`+ else`
		`+ option = "normal_cadence"`
		`+ end`
		`+ end`
		`+`
		`+ print(rpm.expand("%_autorel_" .. option))`
		`+ }`
		`+ ## END: Automatic release macro`

rpmautospec/misc.py

file modified

+20

		`@@ -1,5 +1,7 @@`
		`from functools import cmp_to_key`
		`+ import logging`
		`import re`
		`+ import subprocess`
		`from typing import List`
		`from typing import Optional`
		`from typing import Tuple`
		`@@ -22,6 +24,8 @@`

		`_kojiclient = None`

		`+ _log = logging.getLogger(__name__)`
		`+`

		`def parse_evr(evr_str: str) -> Tuple[int, str, str]:`
		`match = evr_re.match(evr_str)`
		`@@ -65,3 +69,19 @@`
		`raise ValueError(f"Package {pkgname!r} not found!")`

		`return _kojiclient.listBuilds(pkgid, type="rpm", queryOpts={"order": "-nvr"})`
		`+`
		`+`
		`+ def run_command(command: list, cwd: Optional[str] = None) -> bytes:`
		`+ """ Run the specified command in a specific working directory if one`
		`+ is specified.`
		`+ """`
		`+ output = None`
		`+ try:`
		`+ output = subprocess.check_output(command, cwd=cwd, stderr=subprocess.PIPE)`
		`+ except subprocess.CalledProcessError as e:`
		+ _log.error("Command `{}` return code: `{}`".format(" ".join(command), e.returncode))
		`+ _log.error("stdout:\n-------\n{}".format(e.stdout))`
		`+ _log.error("stderr:\n-------\n{}".format(e.stderr))`
		`+ raise`
		`+`
		`+ return output`

rpmautospec/process_distgit.py

file modified

+19 -9

		`@@ -11,6 +11,20 @@`
		`from rpmautospec.release import holistic_heuristic_algo`

		`_log = logging.getLogger(__name__)`
		`+ __here__ = os.path.dirname(__file__)`
		`+`
		`+ autorel_macro_path = os.path.join(__here__, "etc", "autorel-macro.txt")`
		`+ autorel_template = """## START: Set by rpmautospec`
		`+ %define _autorel_normal_cadence {autorel_normal}%{{?_autorel_extraver}}%{{?_autorel_snapinfo}}%{{?dist}}`
		`+ %define _autorel_hotfix_cadence %nil`
		`+ %define _autorel_prerel_cadence %nil`
		`+ %define _autorel_prerel_hotfix_cadence %nil`
		`+ ## END: Set by rpmautospec`
		`+ """ # noqa: E501`
		`+`
		`+ autorel_re = re.compile(r"^\s(?i:Release)\s:\s+%(?:autorel(?:\s\|$)\|\{autorel[^\}]*\})")`
		`+ changelog_re = re.compile(r"^%changelog(?:\s.*)?$", re.IGNORECASE)`
		`+ autochangelog_re = re.compile(r"^\s%(?:autochangelog\|\{autochangelog\})\s$")`


		`def register_subcommand(subparsers):`
		`@@ -26,13 +40,6 @@`
		`return subcmd_name`


		`- autorel_template = """%define autorel {autorel}"""`
		`-`
		`- autorel_re = re.compile(r"^\s(?i:Release)\s:\s+%(?:autorel(?:\s\|$)\|\{autorel[^\}]*\})")`
		`- changelog_re = re.compile(r"^%changelog(?:\s.*)?$", re.IGNORECASE)`
		`- autochangelog_re = re.compile(r"^\s%(?:autochangelog\|\{autochangelog\})\s$")`
		`-`
		`-`
		`def is_autorel(line):`
		`return autorel_re.match(line)`

		`@@ -40,7 +47,7 @@`
		`def get_autorel(name, dist, session):`
		`koji_init(session)`
		`# evr=None forces to search from lower bound`
		`- release = holistic_heuristic_algo(package=name, dist=dist, evr=None)`
		`+ release = holistic_heuristic_algo(package=name, dist=dist, evr=None, strip_dist=True)`
		`return release`


		`@@ -82,7 +89,10 @@`
		`# Process the spec file into a temporary file...`
		`if has_autorel:`
		`# Write %autorel macro header`
		`- print(autorel_template.format(autorel=new_rel), file=tmp_specfile)`
		`+ with open(autorel_macro_path, "r") as autorel_macro_file:`
		`+ print(autorel_template.format(autorel_normal=new_rel), file=tmp_specfile)`
		`+ for line in autorel_macro_file:`
		`+ print(line, file=tmp_specfile, end="")`

		`for lineno, line in enumerate(specfile, start=1):`
		`if has_autochangelog and lineno > changelog_lineno:`

rpmautospec/release.py

file modified

+9 -2

		`@@ -126,7 +126,9 @@`
		`return new_evr`


		`- def holistic_heuristic_algo(package: str, dist: str, evr: typing.Tuple[int, str, str]):`
		`+ def holistic_heuristic_algo(`
		`+ package: str, dist: str, evr: typing.Tuple[int, str, str], strip_dist: bool = False,`
		`+ ):`
		`match = disttag_re.match(dist)`
		`if not match:`
		`print(`
		`@@ -214,7 +216,12 @@`

		`print(f"Calculated new release, EVR: {new_evr['release']}, {new_evr_str}")`

		`- return new_evr["release"]`
		`+ release = new_evr["release"]`
		`+`
		`+ if strip_dist:`
		`+ release = release.replace(f".{dist}", "")`
pingou commented 4 years ago What about epel? Note, I have code doing this in the changelog piece
nphilipp commented 4 years ago Hmm, I missed that you did comment inline. :wink: The `dist` variable would be e.g. `fc32` at this point, so I don't see a reason why this shouldn't work for EPEL (would be e.g. `el8` then). I introduced `strip_dist` because we need to inject the release number without `%dist` at the top of the spec file, should I attempt to make the changelog code use this, too?
pingou commented 4 years ago Oh, sorry I missed that this is an f-string, nevermind then, good to go!
		`+`
		`+ return release`


		`def main(args):`

rpmautospec/tag_package.py

file modified

+1 -21

		`@@ -2,9 +2,8 @@`

		`import logging`
		`import os`
		`- import subprocess`

		`- from .misc import get_package_builds, koji_init`
		`+ from .misc import get_package_builds, koji_init, run_command`
		`from .py2compat.escape_tags import escape_tag, tag_prefix`


		`@@ -23,25 +22,6 @@`
		`return subcmd_name`


		`- def run_command(command, cwd=None):`
		`- """ Run the specified command in a specific working directory if one`
		`- is specified.`
		`- """`
		`- output = None`
		`- try:`
		`- output = subprocess.check_output(command, cwd=cwd, stderr=subprocess.PIPE)`
		`- except subprocess.CalledProcessError as e:`
		`- command_str = " ".join(command)`
		- _log.error("Command `{}` return code: `{}`".format(command_str, e.returncode))
		`- _log.error("stdout:\n-------\n{}".format(e.stdout))`
		`- _log.error("stderr:\n-------\n{}".format(e.stderr))`
		`- raise RuntimeError(`
		- "Command `{}` failed to run, returned {}".format(command_str, e.returncode)
		`- )`
		`-`
		`- return output`
		`-`
		`-`
		`def main(args):`
		`""" Main method. """`
		`kojiclient = koji_init(args.koji_url)`

setup.cfg

file modified

+1 -1

		`@@ -5,5 +5,5 @@`
		`#ignore =`

		`# Configure flake8-import-order`
		`- application-import-names = rpmautospec,koji_plugin`
		`+ application-import-names = rpmautospec,koji_plugins`
		`import-order-style = google`

tests/koji_plugins/__init__.py ~~tests/koji_plugin/__init__.py~~

file renamed

file was moved with no change to the file

tests/koji_plugins/test_rpmautospec_builder.py ~~tests/koji_plugin/test_rpmautospec_builder.py~~

file renamed

+1 -1

		`@@ -7,7 +7,7 @@`

		`import pytest`

		`- from koji_plugin.rpmautospec_builder import process_distgit_cb`
		`+ from koji_plugins.rpmautospec_builder import process_distgit_cb`


		`__here__ = os.path.dirname(__file__)`

tests/koji_plugins/test_rpmautospec_hub.py ~~tests/koji_plugin/test_rpmautospec_hub.py~~

file renamed

+3 -3

		`@@ -1,6 +1,6 @@`
		`from unittest import mock`

		`- from koji_plugin import rpmautospec_hub`
		`+ from koji_plugins import rpmautospec_hub`


		`test_config = {`
		`@@ -17,8 +17,8 @@`
		`class TestRpmautospecHub:`
		`"""Test the rpmautospec hub plugin for Koji."""`

		`- @mock.patch("koji_plugin.rpmautospec_hub.requests.post")`
		`- @mock.patch("koji_plugin.rpmautospec_hub.CONFIG", MockConfig())`
		`+ @mock.patch("koji_plugins.rpmautospec_hub.requests.post")`
		`+ @mock.patch("koji_plugins.rpmautospec_hub.CONFIG", MockConfig())`
		`def test_tag_in_pagure(self, mock_post):`
		`mock_post.return_value.ok = True`

tests/rpmautospec/test_misc.py

file added

+31

		`@@ -0,0 +1,31 @@`
		`+ import logging`
		`+ import subprocess`
		`+ from unittest import mock`
		`+`
		`+ import pytest`
		`+`
		`+ from rpmautospec import misc`
		`+`
		`+`
		`+ class TestMisc:`
		`+ """Test the rpmautospec.misc module"""`
		`+`
		`+ @mock.patch("rpmautospec.misc.subprocess.check_output")`
		`+ @pytest.mark.parametrize("raise_exception", (False, True))`
		`+ def test_run_command(self, check_output, raise_exception, caplog):`
		`+ """Test run_command()"""`
		`+ caplog.set_level(logging.DEBUG)`
		`+`
		`+ if not raise_exception:`
		`+ check_output.return_value = "Some output"`
		`+ assert misc.run_command(["command"]) == "Some output"`
		`+ check_output.assert_called_once_with(["command"], cwd=None, stderr=subprocess.PIPE)`
		`+ assert not any(rec.levelno >= logging.WARNING for rec in caplog.records)`
		`+ else:`
		`+ check_output.side_effect = subprocess.CalledProcessError(`
		`+ returncode=139, cmd=["command"], output="Some command", stderr="And it failed!",`
		`+ )`
		`+ with pytest.raises(subprocess.CalledProcessError) as excinfo:`
		`+ misc.run_command(["command"])`
		`+ assert str(excinfo.value) == "Command '['command']' returned non-zero exit status 139."`
		`+ assert any(rec.levelno == logging.ERROR for rec in caplog.records)`

tests/rpmautospec/test_tag_package.py

file modified

-22

		`@@ -1,5 +1,3 @@`
		`- import logging`
		`- import subprocess`
		`from unittest import mock`

		`import pytest`
		`@@ -66,26 +64,6 @@`
		`class TestTagPackage:`
		`"""Test the rpmautospec.tag_package module."""`

		`- @mock.patch("rpmautospec.tag_package.subprocess.check_output")`
		`- @pytest.mark.parametrize("raise_exception", (False, True))`
		`- def test_run_command(self, check_output, raise_exception, caplog):`
		`- """Test run_command()"""`
		`- caplog.set_level(logging.DEBUG)`
		`-`
		`- if not raise_exception:`
		`- check_output.return_value = "Some output"`
		`- assert tag_package.run_command(["command"]) == "Some output"`
		`- check_output.assert_called_once_with(["command"], cwd=None, stderr=subprocess.PIPE)`
		`- assert not any(rec.levelno >= logging.WARNING for rec in caplog.records)`
		`- else:`
		`- check_output.side_effect = subprocess.CalledProcessError(`
		`- returncode=139, cmd=["command"], output="Some command", stderr="And it failed!",`
		`- )`
		`- with pytest.raises(RuntimeError) as excinfo:`
		`- tag_package.run_command(["command"])`
		- assert str(excinfo.value) == "Command `command` failed to run, returned 139"
		`- assert any(rec.levelno == logging.ERROR for rec in caplog.records)`
		`-`
		`@pytest.mark.parametrize(`
		`"phenomena",`
		`(`

nphilipp commented 4 years ago

Additional fixes:

missed renaming the plugin directory
we don't need two copies of run_command(), consolidate in rpmautospec.misc

zuul commented 4 years ago

Build failed.

pre-commit : FAILURE in 1m 15s
pytest : SUCCESS in 1m 08s

rebased onto c318aa5

4 years ago

zuul commented 4 years ago

Build succeeded.

pre-commit : SUCCESS in 1m 13s
pytest : SUCCESS in 1m 05s

pingou commented on line 20 of rpmautospec/release.py 4 years ago

What about epel?

Note, I have code doing this in the changelog piece

nphilipp commented on line 20 of rpmautospec/release.py 4 years ago

Hmm, I missed that you did comment inline. :wink:

The dist variable would be e.g. fc32 at this point, so I don't see a reason why this shouldn't work for EPEL (would be e.g. el8 then).

I introduced strip_dist because we need to inject the release number without %dist at the top of the spec file, should I attempt to make the changelog code use this, too?