PR#13: Process spec file line by line, be more lenient (and minor cleanups) - fedora-infra/rpmautospec

fedora-infra / rpmautospec

#13 Process spec file line by line, be more lenient (and minor cleanups)

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

fedora-infra/ nphilipp/rpmautospec master--process-spec-line-by-line into master

Process spec file line by line, be more lenient

Nils Philippsen • 4 years ago

6a559ad

koji_plugin/rpmautospec_plugin.py

file modified

+49 -21

		`@@ -1,6 +1,8 @@`
		`from glob import glob`
		`import os`
		`import re`
		`+ import shutil`
		`+ import tempfile`


		`from koji.plugin import callback`
		`@@ -15,6 +17,8 @@`
		`"""`

		`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):`
		`@@ -37,28 +41,52 @@`

		`dist = build_tag["tag_name"]`
		`name = os.path.basename(srcdir)`
		`- autospec = False`
		`- autorel = False`
		`new_rel = get_autorel(name, dist, session)`
		`- specfiles = glob(f"{srcdir}/*.spec")`
		`- if len(specfiles) != 1:`
		`+ specfile_names = glob(f"{srcdir}/*.spec")`
		`+ if len(specfile_names) != 1:`
		`# callback should be run only in if there is a single spec-file`
		`return`

		`- with open(specfiles[0], "r") as specfile:`
		`- content = specfile.read()`
		`- # we remove trailing lines`
		`- lines = content.rstrip().splitlines(keepends=False)`
		`- autorel = any(is_autorel(l) for l in lines)`
		`-`
		`- if lines[-1] == "%{autochangelog}":`
		`- autospec = True`
		`-`
		`- if autorel:`
		`- lines.insert(0, autorel_template.format(autorel=new_rel))`
		`- if autospec:`
		`- del lines[-1]`
		`- lines += produce_changelog(srcdir)`
		`- if autorel or autospec:`
		`- with open(f"{srcdir}/{name}.spec", "w") as specfile:`
		`- specfile.writelines(l + "\n" for l in lines)`
		`+ specfile_name = specfile_names[0]`
		`+`
		`+ has_autorel = False`
		`+ changelog_lineno = None`
		`+ has_autochangelog = None`
		`+`
		`+ # Detect if %autorel, %autochangelog are in use`
		`+ with open(specfile_name, "r") as specfile:`
		`+ # Process line by line to cope with large files`
		`+ for lineno, line in enumerate(iter(specfile), start=1):`
		`+ line = line.rstrip("\n")`
		`+`
		`+ if not has_autorel and is_autorel(line):`
		`+ has_autorel = True`
		`+`
		`+ if changelog_lineno is None:`
		`+ if changelog_re.match(line):`
		`+ changelog_lineno = lineno`
		`+ elif has_autochangelog is None:`
		`+ if autochangelog_re.match(line):`
		`+ has_autochangelog = True`
		`+ elif line.strip():`
		`+ # Anything else than %autochangelog after %changelog -> hands off`
		`+ has_autochangelog = False`
		`+`
		`+ if has_autorel or has_autochangelog:`
		`+ with open(specfile_name, "r") as specfile, tempfile.NamedTemporaryFile("w") as tmp_specfile:`
		`+ # 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)`
		`+`
		`+ for lineno, line in enumerate(specfile, start=1):`
		`+ if has_autochangelog and lineno > changelog_lineno:`
		`+ break`
		`+`
		`+ print(line, file=tmp_specfile, end="")`
		`+`
		`+ if has_autochangelog:`
asaleh commented 4 years ago Haven't seen used print instead of file.write/e.t.c.
nphilipp commented 4 years ago I think it just uses the `.write()` method of the file in the background. Prior to Python 3, `print` was a statement (not a function), and printing to a file other than `sys.stdout` looked pretty wild :wink:. The line above would have looked like this with the old syntax: print >>tmp_specfile, line, Note the trailing comma which tells it to not add a newline.
pingou commented 4 years ago I wonder if this works in py2 with the `from future import print_function`
nphilipp commented 4 years ago It should work.
		`+ print("\n".join(produce_changelog(srcdir)), file=tmp_specfile)`
		`+`
		`+ # ...and copy it back (potentially across device boundaries)`
		`+ shutil.copy2(tmp_specfile.name, specfile_name)`

tests/koji_plugin/test_rpmautospec_plugin.py

file modified

+83 -11

		`@@ -1,9 +1,12 @@`
		`import filecmp`
		`import os`
		`+ import shutil`
		`import tarfile`
		`import tempfile`
		`from unittest.mock import MagicMock`

		`+ import pytest`
		`+`
		`from koji_plugin.rpmautospec_plugin import autospec_cb, is_autorel`


		`@@ -29,6 +32,19 @@`
		`class TestRpmautospecPlugin:`
		`"""Test the koji_plugin.rpmautospec_plugin module"""`

		`+ autorel_autochangelog_cases = [`
		`+ (autorel_case, autochangelog_case)`
		`+ for autorel_case in ("unchanged", "without braces")`
		`+ for autochangelog_case in (`
		`+ "unchanged",`
		`+ "changelog case insensitive",`
		`+ "changelog trailing garbage",`
		`+ "line in between",`
		`+ "trailing line",`
		`+ "without braces",`
		`+ )`
		`+ ]`
		`+`
		`def test_is_autorel(self):`
		`assert is_autorel("Release: %{autorel}")`
		`assert is_autorel("Release: %autorel")`
		`@@ -39,9 +55,43 @@`
		`assert not is_autorel("NotRelease: %{autorel}")`
		`assert not is_autorel("release: 1%{?dist}")`

		`- def test_autospec_cb(self):`
		`+ @staticmethod`
		`+ def fuzz_spec_file(spec_file_path, autorel_case, autochangelog_case):`
		`+ """Fuzz a spec file in ways which shouldn't change the outcome"""`
		`+`
		`+ with open(spec_file_path, "r") as orig, open(spec_file_path + ".new", "w") as new:`
		`+ for line in orig:`
		`+ if line.startswith("Release:") and autorel_case != "unchanged":`
		`+ if autorel_case == "without braces":`
		`+ print("Release: %autorel", file=new)`
		`+ else:`
		`+ raise ValueError(f"Unknown autorel_case: {autorel_case}")`
		`+ elif line.strip() == "%changelog" and autochangelog_case != "unchanged":`
		`+ if autochangelog_case == "changelog case insensitive":`
		`+ print("%ChAnGeLoG", file=new)`
		`+ elif autochangelog_case == "changelog trailing garbage":`
		`+ print("%changelog with trailing garbage yes this works", file=new)`
		`+ elif autochangelog_case == "line in between":`
		`+ print("%changelog\n\n%{autochangelog}", file=new)`
		`+ break`
		`+ elif autochangelog_case == "trailing line":`
		`+ print("%changelog\n%{autochangelog}\n", file=new)`
		`+ break`
		`+ elif autochangelog_case == "without braces":`
		`+ print("%changelog\n%autochangelog", file=new)`
		`+ break`
		`+ else:`
		`+ raise ValueError(f"Unknown autochangelog_case: {autochangelog_case}")`
		`+ else:`
		`+ print(line, file=new, end="")`
		`+`
		`+ os.rename(spec_file_path + ".new", spec_file_path)`
		`+`
		`+ @pytest.mark.parametrize("autorel_case,autochangelog_case", autorel_autochangelog_cases)`
		`+ def test_autospec_cb(self, autorel_case, autochangelog_case):`
		`"""Test the autospec_cb() function"""`
		`with tempfile.TemporaryDirectory() as workdir:`
		`+ workdir = tempfile.mkdtemp()`
		`with tarfile.open(`
		`os.path.join(`
		`__here__,`
		`@@ -53,6 +103,14 @@`
		`) as tar:`
		`tar.extractall(path=workdir)`

		`+ unpacked_repo_dir = os.path.join(workdir, "dummy-test-package-gloster")`
		`+ unprocessed_spec_file_path = os.path.join(`
		`+ unpacked_repo_dir, "dummy-test-package-gloster.spec",`
		`+ )`
		`+`
		`+ if autorel_case != "unchanged" or autochangelog_case != "unchanged":`
		`+ self.fuzz_spec_file(unprocessed_spec_file_path, autorel_case, autochangelog_case)`
		`+`
		`koji_session = MagicMock()`
		`koji_session.getPackageID.return_value = 30489`
		`name = "dummy-test-package-gloster"`
		`@@ -72,18 +130,32 @@`
		`"scminfo": data_scm_info,`
		`"build_tag": data_build_tag,`
		`"scratch": MagicMock(),`
		`- "srcdir": os.path.join(workdir, "dummy-test-package-gloster"),`
		`+ "srcdir": unpacked_repo_dir,`
		`"taskinfo": {"method": "buildSRPMFromSCM"},`
		`"session": koji_session,`
		`}`
		`+`
		`autospec_cb(args, *kwargs)`
		`- assert filecmp.cmp(`
		`- os.path.join(kwargs["srcdir"], "dummy-test-package-gloster.spec"),`
		`- os.path.join(`
		`- __here__,`
		`- os.path.pardir,`
		`- "test-data",`
		`- "repodata",`
		`- "dummy-test-package-gloster.spec",`
		`- ),`
		`+`
		`+ expected_spec_file_path = os.path.join(`
		`+ __here__,`
		`+ os.path.pardir,`
		`+ "test-data",`
		`+ "repodata",`
		`+ "dummy-test-package-gloster.spec",`
		`)`
		`+`
		`+ with tempfile.NamedTemporaryFile() as tmpspec:`
		`+ if autorel_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 %autorel macro is expanded.`
		`+ autochangelog_case = "unchanged"`
		`+ shutil.copy2(expected_spec_file_path, tmpspec.name)`
		`+ expected_spec_file_path = tmpspec.name`
		`+ self.fuzz_spec_file(expected_spec_file_path, autorel_case, autochangelog_case)`
		`+`
		`+ assert filecmp.cmp(unprocessed_spec_file_path, expected_spec_file_path)`

tests/test-data/repodata/dummy-test-package-gloster.spec

file modified

-1

		`@@ -368,4 +368,3 @@`

		`* Thu Jan 09 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-1`
		`- Initial import post review`
		`-`

nphilipp commented 4 years ago

Mainly this avoids reading the whole spec file into memory and makes the callback a bit more lenient when detecting the use of %autochangelog.

Additionally, it makes flake8 and black happy.

Fixes https://pagure.io/Fedora-Infra/rpmautospec/issue/14

zuul commented 4 years ago

Build succeeded.

pre-commit : SUCCESS in 1m 45s
pytest : SUCCESS in 1m 35s

pingou commented 4 years ago

Looks good to me (I didn't check the tests we are running to see if we should expand them though)

asaleh commented on line 88 of koji_plugin/rpmautospec_plugin.py 4 years ago

Haven't seen used print instead of file.write/e.t.c.

asaleh commented 4 years ago

Definitely would add at least one test where we have a spec-file fir %changelong, %{autochangelog} and whitespace to be ignored.

zuul commented 4 years ago

Build succeeded.

pre-commit : SUCCESS in 1m 43s
pytest : SUCCESS in 1m 37s

nphilipp commented on line 88 of koji_plugin/rpmautospec_plugin.py 4 years ago

I think it just uses the .write() method of the file in the background. Prior to Python 3, print was a statement (not a function), and printing to a file other than sys.stdout looked pretty wild :wink:. The line above would have looked like this with the old syntax: