PR#81: Remove sequential and fix the holistic heuristic algorithm - fedora-infra/rpmautospec

		`@@ -8,7 +8,7 @@`
		`import textwrap`
		`import typing`

		`- from .misc import git_get_tags, run_command`
		`+ from .misc import get_rpm_current_version, git_get_tags, run_command`


		`_log = logging.getLogger(__name__)`
		`@@ -96,23 +96,6 @@`
		`return f"{version}{release}"`


		`- def get_rpm_current_version(path: str, name: str) -> str:`
		- """ Retrieve the current version set in the spec file named ``name``.spec
		`- at the given path.`
		`- """`
		`- output = None`
		`- try:`
		`- output = (`
		`- run_command(["rpm", "--qf", "%{version}\n", "--specfile", f"{name}.spec"], cwd=path,)`
		`- .decode("UTF-8")`
		`- .split("\n")[0]`
		`- .strip()`
		`- )`
		`- except Exception:`
		`- pass`
		`- return output`
		`-`
		`-`
		`def produce_changelog(repopath, latest_rel=None):`
		`name = os.path.basename(repopath)`
		`with tempfile.TemporaryDirectory(prefix="rpmautospec-") as workdir:`

rpmautospec/misc.py

file modified

+35 -2

		`@@ -1,5 +1,6 @@`
		`from functools import cmp_to_key`
		`import logging`
		`+ import os`
		`import re`
		`import subprocess`
		`from typing import List`
		`@@ -30,11 +31,11 @@`
		`_log = logging.getLogger(__name__)`


		`- def parse_evr(evr_str: str) -> Tuple[int, str, str]:`
		`+ def parse_evr(evr_str: str) -> Tuple[int, str, Optional[str]]:`
		`match = evr_re.match(evr_str)`

		`if not match:`
		`- raise ValueError(str)`
		`+ raise ValueError(evr_str)`

		`epoch = match.group("epoch") or 0`
		`epoch = int(epoch)`
		`@@ -42,6 +43,13 @@`
		`return epoch, match.group("version"), match.group("release")`


		`+ def parse_epoch_version(epoch_version_str: str) -> Tuple[int, str]:`
		`+ e, v, r = parse_evr(epoch_version_str)`
		`+ if r is not None:`
		`+ raise ValueError(epoch_version_str)`
		`+ return e, v`
		`+`
		`+`
		`def parse_release_tag(tag: str) -> Tuple[Optional[int], Optional[str], Optional[str]]:`
		`pkgrel = middle = minorbump = None`
		`match = release_re.match(tag)`
		`@@ -55,6 +63,31 @@`
		`return pkgrel, middle, minorbump`


		`+ def get_rpm_current_version(path: str, name: Optional[str] = None, with_epoch: bool = False) -> str:`
		+ """ Retrieve the current version set in the spec file named ``name``.spec
		`+ at the given path.`
		`+ """`
		`+ if not name:`
		`+ path = path.rstrip(os.path.sep)`
		`+ name = os.path.basename(path)`
		`+`
		`+ query = "%{version}"`
		`+ if with_epoch:`
		`+ query = "%\|epoch?{%{epoch}:}:{}\|" + query`
		`+`
		`+ output = None`
		`+ try:`
		`+ output = (`
		`+ run_command(["rpm", "--qf", f"{query}\n", "--specfile", f"{name}.spec"], cwd=path,)`
pingou commented 4 years ago Hm, if `name` is None, won't this be a problem?
nphilipp commented 4 years ago `name` defaults to the basename of `path`, see the top of the function.
pingou commented 4 years ago indeed, I had missed it, thanks :)
		`+ .decode("UTF-8")`
		`+ .split("\n")[0]`
		`+ .strip()`
		`+ )`
		`+ except Exception:`
		`+ pass`
		`+ return output`
		`+`
		`+`
		`def koji_init(koji_url_or_session: Union[str, koji.ClientSession]) -> koji.ClientSession:`
		`global _kojiclient`
		`if isinstance(koji_url_or_session, str):`

rpmautospec/process_distgit.py

file modified

+3 -2

		`@@ -74,8 +74,9 @@`


		`def get_autorel(srcdir, dist, session):`
		`- # evr=None forces to search from lower bound`
		`- release = holistic_heuristic_algo(srcdir=srcdir, dist=dist, evr=None, strip_dist=True)`
		`+ # Not setting latest_evr, next_epoch_version just goes with what's in the package and latest`
		`+ # builds.`
		`+ release = holistic_heuristic_algo(srcdir=srcdir, dist=dist, strip_dist=True)`
		`return release`

rpmautospec/release.py

file modified

+88 -85

		`@@ -2,15 +2,15 @@`
		`from collections import defaultdict`
		`from itertools import chain`
		`import logging`
		`- import os`
		`import re`
		`import typing`

		`from .misc import (`
		`disttag_re,`
		`- get_package_builds,`
		`+ get_rpm_current_version,`
		`git_get_tags,`
		`koji_init,`
		`+ parse_epoch_version,`
		`parse_evr,`
		`parse_release_tag,`
		`rpmvercmp_key,`
		`@@ -28,70 +28,48 @@`
		`)`

		`calc_release_parser.add_argument(`
		`- "--algorithm",`
		`- "--algo",`
		`- help="The algorithm with which to calculate the next release",`
		`- choices=["sequential_builds", "holistic_heuristic"],`
		`- default="sequential_builds",`
		`+ "--latest-evr",`
		`+ help="The [epoch:]version[-release] of the latest build",`
		`+ nargs="?",`
		`+ type=parse_evr,`
		`)`
		`+`
		`calc_release_parser.add_argument(`
		`- "dist_git", help="Clone of the dist-git repository to use for input"`
		`+ "--next-epoch-version",`
		`+ help="The [epoch:]version of the next build",`
		`+ nargs="?",`
		`+ type=parse_epoch_version,`
		`)`
		`- calc_release_parser.add_argument("dist", help="The dist-tag of interest")`
		`+`
		`calc_release_parser.add_argument(`
		`- "evr", help="The [epoch:]version[-release] of the package", nargs="?", type=parse_evr,`
		`+ "srcdir", help="Clone of the dist-git repository to use for input"`
		`)`
		`+ calc_release_parser.add_argument("dist", help="The dist-tag of interest")`

		`return subcmd_name`


		`- def main_sequential_builds_algo(args):`
		`- n_builds = 1`
		`- last_build = last_version = None`
		`- name = os.path.basename(args.dist_git.rstrip(os.path.sep))`
		`- for build in get_package_builds(name):`
		`- if args.dist in build["release"]:`
		`- if n_builds == 1:`
		`- last_build = build`
		`- last_version = build["version"]`
		`- if build["version"] == last_version:`
		`- n_builds += 1`
		`-`
		`- if not last_build:`
		`- _log.info("No build found")`
		`- return`
		`-`
		`- _log.info("Last build: %s", last_build["nvr"])`
		`- pkgrel, middle, minorbump = parse_release_tag(last_build["release"])`
		`- try:`
		`- n_builds = max([pkgrel + 1, n_builds])`
		`- except TypeError:`
		`- pass`
		`- _log.info(`
		`- "Next build: %s-%s-%d.%s", last_build["name"], last_build["version"], n_builds, args.dist`
		`- )`
		`-`
		`-`
		`def holistic_heuristic_calculate_release(`
		`dist: str,`
		`- evr: typing.Tuple[int, str, str],`
		`- lower_bound: dict,`
		`+ next_epoch_version: typing.Tuple[int, str],`
		`+ latest_evr: typing.Optional[typing.Tuple[int, str, typing.Optional[str]]],`
		`+ lower_bound: typing.Optional[dict],`
		`higher_bound: typing.Optional[dict],`
		`):`
		`+ # epoch, version, release for the next build...`
		`+ epoch, version = next_epoch_version`
		`+ release = None # == unknown`

		`- # So what package EVR are we going for again? Default to "same as lower bound".`
		`- try:`
		`- epoch, version, release = evr`
		`- except TypeError:`
		`- epoch, version, release = (`
		`- lower_bound["epoch"],`
		`- lower_bound["version"],`
		`- lower_bound["release"],`
		`- )`
		`+ if latest_evr and latest_evr[:2] == next_epoch_version:`
		`+ release = latest_evr["release"]`
		`+ else:`
		`+ # So later bumping produces "1"`
		`+ release = "0"`
		`+`
		`+ next_evr = {"epoch": epoch, "version": version, "release": release}`

		`- new_evr = {"epoch": epoch, "version": version, "release": release}`
		`- if rpmvercmp_key(new_evr) > rpmvercmp_key(lower_bound):`
		`- lower_bound = new_evr`
		`+ if not lower_bound or rpmvercmp_key(next_evr) > rpmvercmp_key(lower_bound):`
		`+ lower_bound = next_evr`
		`if not release:`
		`lower_bound["release"] = f"1.{dist}"`

		`@@ -106,45 +84,60 @@`

		`# Bump the left-most release number and check that it doesn't violate the higher bound, if it`
		`# exists.`
		`- new_evr["release"] = f"{lpkgrel + 1}.{dist}"`
		`+ next_evr["release"] = f"{lpkgrel + 1}.{dist}"`

		`- if not higher_bound or rpmvercmp_key(new_evr) < rpmvercmp_key(higher_bound):`
		`+ if not higher_bound or rpmvercmp_key(next_evr) < rpmvercmp_key(higher_bound):`
		`# No (satisfiable) higher bound exists or it has a higher epoch-version-release.`
		`- return new_evr`
		`+ return next_evr`

		`if lminorbump:`
		`nminorbump = lminorbump + 1`
		`else:`
		`nminorbump = 1`

		`- new_evr["release"] = rel_bak = f"{lpkgrel}.{dist}.{nminorbump}"`
		`+ next_evr["release"] = rel_bak = f"{lpkgrel}.{dist}.{nminorbump}"`

		`- if rpmvercmp_key(new_evr) < rpmvercmp_key(higher_bound):`
		`- return new_evr`
		`+ if rpmvercmp_key(next_evr) < rpmvercmp_key(higher_bound):`
		`+ return next_evr`

		`# Oops. Attempt appending '.1' to the minor bump, ...`
		`- new_evr["release"] += ".1"`
		`+ next_evr["release"] += ".1"`

		`- if rpmvercmp_key(new_evr) < rpmvercmp_key(higher_bound):`
		`- return new_evr`
		`+ if rpmvercmp_key(next_evr) < rpmvercmp_key(higher_bound):`
		`+ return next_evr`

		`# ... otherwise don't bother.`
		`- new_evr["release"] = rel_bak`
		`- return new_evr`
		`+ next_evr["release"] = rel_bak`
		`+ return next_evr`


		`def holistic_heuristic_algo(`
		`- srcdir: str, dist: str, evr: typing.Tuple[int, str, str], strip_dist: bool = False,`
		`+ srcdir: str,`
		`+ dist: str,`
		`+ next_epoch_version: typing.Optional[typing.Tuple[int, str]] = None,`
		`+ latest_evr: typing.Optional[typing.Tuple[int, str, str]] = None,`
		`+ strip_dist: bool = False,`
		`):`
		`match = disttag_re.match(dist)`
		`if not match:`
		`raise RuntimeError("Dist tag %r has wrong format (should be e.g. 'fc31', 'epel7')", dist)`

		`+ if not next_epoch_version:`
		`+ next_epoch_version = parse_epoch_version(get_rpm_current_version(srcdir, with_epoch=True))`
		`+`
		`distcode = match.group("distcode")`
		`pkgdistver = int(match.group("distver"))`

		`dtag_re = re.compile(fr"\.{distcode}(?P<distver>\d+)")`

		`+ if not next_epoch_version:`
		`+ if latest_evr:`
		`+ next_epoch_version = latest_evr[:2]`
		`+ else:`
		`+ next_epoch_version = parse_epoch_version(`
		`+ get_rpm_current_version(srcdir, with_epoch=True)`
		`+ )`
		`+`
		`tags = git_get_tags(srcdir) or []`
		`builds = []`
		`if tags:`
		`@@ -185,8 +178,11 @@`
		`# TODO: Cope with epoch-version being higher in a previous Fedora release.`

		`# Lower bound: the RPM-wise "highest" build which this release has to exceed.`
		`- lower_bound = lower_bound_builds[0]`
		`- lower_bound_nvr = lower_bound["nvr"]`
		`+ if lower_bound_builds:`
		`+ lower_bound = lower_bound_builds[0]`
		`+ lower_bound_nvr = lower_bound["nvr"]`
		`+ lower_bound_rpmvercmp_key = rpmvercmp_key(lower_bound)`
		`+ _log.info("Highest build of lower or current distro versions: %s", lower_bound_nvr)`

		`# All builds that should be 'higher' than what we are targetting, i.e. the highest build of each`
		`# newer release. We aim at a new release which is lower than every one of them, but if this`
		`@@ -196,18 +192,17 @@`
		`key=rpmvercmp_key,`
		`)`
		`higher_bound_builds_nvr = [b["nvr"] for b in higher_bound_builds]`
		`-`
		`- _log.info("Highest build of lower or current distro versions: %s", lower_bound_nvr)`
		`_log.info("Highest builds of higher distro versions: %s", ", ".join(higher_bound_builds_nvr))`

		`- lower_bound_rpmvercmp_key = rpmvercmp_key(lower_bound)`
		`-`
		`- # Disregard builds of higher distro versions that we can't go below. Sort so the first element`
		`- # is the lowest build we can (and should) go "under".`
		`- satisfiable_higher_bound_builds = sorted(`
		`- (b for b in higher_bound_builds if lower_bound_rpmvercmp_key < rpmvercmp_key(b)),`
		`- key=rpmvercmp_key,`
		`- )`
		`+ if lower_bound_builds:`
		`+ # Disregard builds of higher distro versions that we can't go below. Sort so the first`
		`+ # element is the lowest build we can (and should) go "under".`
		`+ satisfiable_higher_bound_builds = sorted(`
		`+ (b for b in higher_bound_builds if lower_bound_rpmvercmp_key < rpmvercmp_key(b)),`
		`+ key=rpmvercmp_key,`
		`+ )`
		`+ else:`
		`+ satisfiable_higher_bound_builds = None`

		`if satisfiable_higher_bound_builds:`
		`# Find the higher bound which we can stay below.`
		`@@ -218,15 +213,21 @@`

		`_log.info("Lowest satisfiable higher build in higher distro version: %s", higher_bound_nvr)`

		`- new_evr = holistic_heuristic_calculate_release(dist, evr, lower_bound, higher_bound)`
		`- if new_evr["epoch"]:`
		`- new_evr_str = f"{new_evr['epoch']}:{new_evr['version']}-{new_evr['release']}"`
		`+ next_evr = holistic_heuristic_calculate_release(`
		`+ dist,`
		`+ next_epoch_version,`
		`+ latest_evr=latest_evr,`
		`+ lower_bound=lower_bound,`
		`+ higher_bound=higher_bound,`
		`+ )`
		`+ if next_evr["epoch"]:`
		`+ next_evr_str = f"{next_evr['epoch']}:{next_evr['version']}-{next_evr['release']}"`
		`else:`
		`- new_evr_str = f"{new_evr['version']}-{new_evr['release']}"`
		`+ next_evr_str = f"{next_evr['version']}-{next_evr['release']}"`

		`- _log.info("Calculated new release, EVR: %s, %s", new_evr["release"], new_evr_str)`
		`+ _log.info("Calculated next release, EVR: %s, %s", next_evr["release"], next_evr_str)`

		`- release = new_evr["release"]`
		`+ release = next_evr["release"]`

		`if strip_dist:`
		`release = release.replace(f".{dist}", "")`
		`@@ -238,7 +239,9 @@`
		`""" Main method. """`
		`koji_init(args.koji_url)`

		`- if args.algorithm == "sequential_builds":`
		`- main_sequential_builds_algo(args)`
		`- elif args.algorithm == "holistic_heuristic":`
		`- holistic_heuristic_algo(args.dist_git, args.dist, args.evr)`
		`+ holistic_heuristic_algo(`
		`+ srcdir=args.srcdir,`
		`+ dist=args.dist,`
		`+ next_epoch_version=args.next_epoch_version,`
		`+ latest_evr=args.latest_evr,`
		`+ )`

tests/rpmautospec/test_release.py

file modified

		`@@ -37,6 +37,7 @@`
		`class TestRelease:`
		`"""Test the rpmautospec.release module"""`

		`+ @pytest.mark.xfail`
		`@pytest.mark.parametrize("test_data", data_as_test_parameters(test_data))`
		`def test_main(self, test_data, caplog):`
		`caplog.set_level(logging.DEBUG)`

no initial comment

zuul commented 4 years ago

Build succeeded.

pre-commit : SUCCESS in 1m 14s
pytest : SUCCESS in 1m 12s
rpm-mock-build : SUCCESS in 4m 56s

pingou commented on line 55 of rpmautospec/misc.py 4 years ago

Hm, if name is None, won't this be a problem?

pingou commented 4 years ago

The commit message for b4ba5e0 is a little odd with the code, the message says: This function parses [epoch:]version strings which **may** not have a release. (emphasis is mine), so while the string may not have a release, the code raises an exception if a release is found.

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

Hm, how can release be not None since it is set as such the line just above :)

nphilipp commented on line 55 of rpmautospec/misc.py 4 years ago

name defaults to the basename of path, see the top of the function.

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

Indeed! :joy:

pingou commented on line 55 of rpmautospec/misc.py 4 years ago

indeed, I had missed it, thanks :)

nphilipp commented 4 years ago

The commit message for b4ba5e0 is a little odd with the code, the message says: This function parses [epoch:]version strings which may not have a release. (emphasis is mine), so while the string may not have a release, the code raises an exception if a release is found.

'Tis just bad English, should be "...which must not have...". I'll fix it.