PR#628: Refuse import of packages processed by rpmautospec - rpkg

rpkg

#628 Refuse import of packages processed by rpmautospec

Merged 2 years ago by onosek. Opened 2 years ago by zbyszek.

zbyszek/rpkg rpmautospec-reject into master

Refuse import of packages processed by rpmautospec

Zbigniew Jędrzejewski-Szmek • 2 years ago

c2385ba

pyrpkg/__init__.py

file modified

+7 -1

		`@@ -50,7 +50,7 @@`
		`from pyrpkg.spec import SpecFile`
		`from pyrpkg.utils import (cached_property, extract_srpm, find_me,`
		`is_file_tracked, is_lookaside_eligible_file,`
		`- log_result)`
		`+ spec_file_processed_by_rpmautospec, log_result)`

		`from .gitignore import GitIgnore`

		`@@ -1445,10 +1445,16 @@`
		`# the test of eligibility`
		`self.log.warning("SRPM file contains a directory: '{0}'. "`
		`"Skipping it.".format(file))`
		`+`
		`if is_lookaside_eligible_file(file, target_dir):`
		`uploadfiles.append(file)`
		`else:`
		`files.append(file)`
		`+`
		`+ if (file == self.spec and`
		`+ spec_file_processed_by_rpmautospec(file, target_dir)):`
		`+ raise rpkgError('srpm was processed by rpmautospec')`
		`+`
		`finally:`
		`shutil.rmtree(target_dir)`

pyrpkg/utils.py

file modified

+20

		`@@ -320,3 +320,23 @@`
		`# output contains encoding ("binary", "us-ascii", ...)`
		`encoding = output.strip() # strip newline at the end`
		`return encoding == "binary"`
		`+`
		`+ def spec_file_processed_by_rpmautospec(file_name, dir_path=None):`
		`+ file_path = os.path.join(dir_path or "", file_name)`
		`+`
		`+ try:`
		`+ contents = open(file_path).readlines()`
		`+ except Exception:`
		`+ # if we can't read it, let's assume the answer is "no".`
		`+ return False`
		`+`
		`+ # Check for the %autorelease header prepended to the file`
		`+ if any('START: Set by rpmautospec' in line for line in contents[:10]):`
		`+ return True`
		`+`
		`+ # It seems that currently there's no mechanism to detect`
		`+ # %autochangelog processing. But most packages would use both`
		`+ # %autochangelog and %autorelease together, so we should catch`
		`+ # most cases by checking for %autorelease only.`
		`+ # https://pagure.io/fedora-infra/rpmautospec/issue/269`
		`+ return False`

zbyszek commented 2 years ago

People occasionally try to do 'fedpkg/rpm import' on an srpm which was
built from a .spec file with %autorelease and %autochangelog fields.
rpmautospec works by replacing those macros with generated content.
Importing and srpm after such processing is generally the wrong thing
to do, because for %autorelease and %autochangelog to work, the spec
file with (unexpanded) macros must be stored in dist-git. But even if
somebody wanted to stop using rpmautospec, and wanted to import the
processed spec file, the headers that were inserted by rpmautospec
should be removed. Thus, just refuse such imports.

Note that I also opened https://pagure.io/fedora-infra/rpmautospec/issue/269
to provide an "official" way to detect that the file was processed.
But even the current imperfect check is better than nothing because
it seems to be a fairly common mistake because 'fedpkg import' is
a very common workflow, and %autorelease/%autochangelog are becoming
the norm too.

Test:
$ PYTHONPATH=$HOME/python/rpkg fedpkg import --offline package-notes-0.5-3.fc38.src.rpm
Could not execute import_srpm: srpm was processed by rpmautospec

Signed-off-by: Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl

pretty please pagure-ci rebuild

2 years ago

onosek commented 2 years ago

Hi. I looked at your code.
fedpkg import allows running in a repository without any specfile (in case of a new repository). In this situation self.spec fails (IncompleteLayout is selected).
What about checking every specfile found in the srpm instead? I can make this change, just confirm whether it is OK for you.