From 89bc57df4f2dcc22004c3e412df3013b62b70ffb Mon Sep 17 00:00:00 2001 From: Otto Urpelainen Date: Mar 01 2021 17:50:04 +0000 Subject: Add config option for writing dist-git build results to a subdirectory Currently, when using the dist-git layout, build results are written directly to layout root, since all of builddir, srpmdir and rpmdir are set to root. This pattern is unnecessarily difficult to cover in gitignore rules and has led to situation in Fedora where, for many packages, running `fedpkg local` pollutes `git status`. Instead of writing to repository root, it is much cleaner to write to a single subdirectory which is easy to add to ignore rules. To enable this without interfering with current usage, new config option `results_dir` is added. Default value preserving current behavior is `root`, whereas setting `subdir` leads to selecting an alternative dist-git layout that uses subdirectory `results` instead. This topic has been discussed in a Fedora mailing list thread: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/FNDBDD5TRXOIUYSC23QV2GN7ZKAHRM3C/ There, it was proposed that the subdirectory would be the same that the `mockbuild` command uses. Unfortunately, resolving mockbuild results dir name requires parsing the spec file, whereas layout selection happens before. Because of this difficulty, a simpler method of fixed name "results" is used here. Earlier, layout selection was based solely on directory contents. Since build results directory is only created after something is built, that method is not adequate. Thus, additional `hint` input is added to layout `from_path` method, allowing selecting the subdir variant from standard dist-git structure. To keep the build result directory as simple as possible, and as similar to mockbuild results, architecture specific subdirectories are not created. This requires adding new member to layouts: rpmfilename. It is passed directly to rpmdefines. From the viewpoint of easy writing of ignore rules, it would make sense to set also sourcedir to the new `results` directory. However, patches are stored in repository root everywhere and rpmbuild expects to find them in the sourcedir. Thus, sourcedir must be set to repository root like it is in the standard dist-git layout. Signed-off-by: Otto Urpelainen --- diff --git a/pyrpkg/__init__.py b/pyrpkg/__init__.py index 126f0db..afc525a 100644 --- a/pyrpkg/__init__.py +++ b/pyrpkg/__init__.py @@ -92,7 +92,7 @@ class Commands(object): build_client, user=None, dist=None, target=None, quiet=False, distgit_namespaced=False, realms=None, lookaside_namespaced=False, - git_excludes=None): + git_excludes=None, results_dir='root'): """Init the object and some configuration details.""" # Path to operate on, most often pwd @@ -212,8 +212,11 @@ class Commands(object): self.block_retire_ns = ['rpms'] # Git excludes patterns self.git_excludes = git_excludes or [] + # Results directory type + self.results_dir = results_dir # Layout setup - self.layout = layout.build(self.path) + self.layout = layout.build(self.path, + 'resultsdir' if self.results_dir == 'subdir' else None) # Define properties here # Properties allow us to "lazy load" various attributes, which also means @@ -771,6 +774,7 @@ class Commands(object): "--define '_builddir %s'" % self.layout.builddir, "--define '_srcrpmdir %s'" % self.layout.srcrpmdir, "--define '_rpmdir %s'" % self.layout.rpmdir, + "--define '_rpmfilename %s'" % self.layout.rpmfilename, "--define 'dist .%s'" % self._disttag, "--define '%s %s'" % (self._distvar, self._distval.split('_')[0]), @@ -2932,7 +2936,7 @@ class Commands(object): the rpmbuild command. """ - self.srpmname = os.path.join(self.path, + self.srpmname = os.path.join(self.layout.srcrpmdir, "%s-%s-%s.src.rpm" % (self.repo_name, self.ver, self.rel)) diff --git a/pyrpkg/cli.py b/pyrpkg/cli.py index 5d05ba1..55d111c 100644 --- a/pyrpkg/cli.py +++ b/pyrpkg/cli.py @@ -259,6 +259,10 @@ class cliClient(object): for excl in items.get("git_excludes", '').split('\n') if excl] + results_dir = 'root' + if self.config.has_option(self.name, 'results_dir'): + results_dir = self.config.get(self.name, 'results_dir') + # Create the cmd object self._cmd = self.site.Commands(self.args.path, items['lookaside'], @@ -276,7 +280,8 @@ class cliClient(object): distgit_namespaced=dg_namespaced, realms=realms, lookaside_namespaced=la_namespaced, - git_excludes=git_excludes + git_excludes=git_excludes, + results_dir=results_dir ) if self.args.module_name: diff --git a/pyrpkg/layout/__init__.py b/pyrpkg/layout/__init__.py index 2bd3e61..762af0d 100644 --- a/pyrpkg/layout/__init__.py +++ b/pyrpkg/layout/__init__.py @@ -16,7 +16,7 @@ from .layouts import (DistGitLayout, IncompleteLayout, # noqa: F401 RetiredLayout, SRPMLayout) -def build(path): +def build(path, hint=None): """ Tries to create a layout instance based on MetaLayout._layouts and will return an instance in the first successfull attempt to @@ -26,7 +26,7 @@ def build(path): """ for layout in MetaLayout._layouts: try: - return layout.from_path(path) + return layout.from_path(path, hint) except LayoutError: continue return None diff --git a/pyrpkg/layout/base.py b/pyrpkg/layout/base.py index 48a80d6..4d7e38b 100644 --- a/pyrpkg/layout/base.py +++ b/pyrpkg/layout/base.py @@ -62,12 +62,13 @@ class BaseLayout(ABC): specdir = None builddir = None rpmdir = None + rpmfilename = None srcrpmdir = None sources_file_template = None @classmethod @abstractmethod - def from_path(cls, path): + def from_path(cls, path, hint=None): """ Class constructor based on a package path. diff --git a/pyrpkg/layout/layouts.py b/pyrpkg/layout/layouts.py index a7b9f9c..30fd128 100644 --- a/pyrpkg/layout/layouts.py +++ b/pyrpkg/layout/layouts.py @@ -29,20 +29,58 @@ class DistGitLayout(BaseLayout): self.specdir = root_dir self.builddir = root_dir self.rpmdir = root_dir + self.rpmfilename = '%%{ARCH}/%%{NAME}-%%{VERSION}-%%{RELEASE}.%%{ARCH}.rpm' self.srcrpmdir = root_dir self.sources_file_template = sources_file_template @classmethod - def from_path(cls, path): + def from_path(cls, path, hint=None): """ Creates a new object instance from a valid path in the file system. Raises exception if path or package structure is invalid. """ - super(DistGitLayout, cls).from_path(path) + super(DistGitLayout, cls).from_path(path, hint) if len([f for f in os.listdir(path) if f.endswith('.spec')]) == 0: raise LayoutError('spec file not found.') + if (hint == 'resultsdir'): + raise LayoutError('resultsdir hint given') + return cls(root_dir=path) + + +class DistGitResultsDirLayout(BaseLayout): + """ + This class represents a dist-git package layout using results directory. + """ + def __init__(self, root_dir=None, sources_file_template='sources'): + """ + Default class constructor to create a new object instance. + """ + results_dir = os.path.join(root_dir, 'results') + + self.root_dir = root_dir + self.sourcedir = root_dir + self.specdir = root_dir + self.builddir = results_dir + self.rpmdir = results_dir + self.rpmfilename = '%%{NAME}-%%{VERSION}-%%{RELEASE}.%%{ARCH}.rpm' + self.srcrpmdir = results_dir + self.sources_file_template = sources_file_template + + @classmethod + def from_path(cls, path, hint=None): + """ + Creates a new object instance from a valid path in the file system. + + Raises exception if path or package structure is invalid. + """ + super(DistGitResultsDirLayout, cls).from_path(path, hint) + + if len([f for f in os.listdir(path) if f.endswith('.spec')]) == 0: + raise LayoutError('spec file not found.') + if (hint != 'resultsdir'): + raise LayoutError('resultsdir hint not given') return cls(root_dir=path) @@ -59,17 +97,18 @@ class SRPMLayout(BaseLayout): self.specdir = os.path.join(root_dir, 'SPECS') self.builddir = os.path.join(root_dir, 'BUILD') self.rpmdir = os.path.join(root_dir, 'RPMS') + self.rpmfilename = '%%{ARCH}/%%{NAME}-%%{VERSION}-%%{RELEASE}.%%{ARCH}.rpm' self.srcrpmdir = os.path.join(root_dir, 'SRPMS') self.sources_file_template = sources_file_template @classmethod - def from_path(cls, path): + def from_path(cls, path, hint=None): """ Creates a new object instance from a valid path in the file system. Raises exception if path or package structure is invalid. """ - super(SRPMLayout, cls).from_path(path) + super(SRPMLayout, cls).from_path(path, hint) if not os.path.exists(os.path.join(path, 'SPECS')): raise LayoutError('SPECS dir not found.') @@ -94,13 +133,13 @@ class IncompleteLayout(BaseLayout): self.sources_file_template = sources_file_template @classmethod - def from_path(cls, path): + def from_path(cls, path, hint=None): """ Creates a new object instance from a valid path in the file system. Raises exception if package is already retired. """ - super(IncompleteLayout, cls).from_path(path) + super(IncompleteLayout, cls).from_path(path, hint) if cls(root_dir=path).is_retired(): raise LayoutError('Retired marker found.') @@ -119,13 +158,13 @@ class RetiredLayout(BaseLayout): self.root_dir = root_dir @classmethod - def from_path(cls, path): + def from_path(cls, path, hint=None): """ Creates a new object instance from a valid path in the file system. Raises exception if no marker is found. """ - super(RetiredLayout, cls).from_path(path) + super(RetiredLayout, cls).from_path(path, hint) if not cls(root_dir=path).is_retired(): raise LayoutError('Retired marker not found.') diff --git a/tests/test_commands.py b/tests/test_commands.py index e82a5d2..57b3bc8 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -242,6 +242,7 @@ class LoadRPMDefinesTest(CommandTestCase): '_builddir': self.cloned_repo_path, '_srcrpmdir': self.cloned_repo_path, '_rpmdir': self.cloned_repo_path, + '_rpmfilename': '%%{ARCH}/%%{NAME}-%%{VERSION}-%%{RELEASE}.%%{ARCH}.rpm', 'dist': u'.el6', 'rhel': u'6', 'el6': u'1', @@ -260,6 +261,7 @@ class LoadRPMDefinesTest(CommandTestCase): '_builddir': self.cloned_repo_path, '_srcrpmdir': self.cloned_repo_path, '_rpmdir': self.cloned_repo_path, + '_rpmfilename': '%%{ARCH}/%%{NAME}-%%{VERSION}-%%{RELEASE}.%%{ARCH}.rpm', 'dist': u'.el6_5', 'rhel': u'6', 'el6_5': u'1', diff --git a/tests/test_layout_distgit.py b/tests/test_layout_distgit.py index cc70014..dd078ab 100644 --- a/tests/test_layout_distgit.py +++ b/tests/test_layout_distgit.py @@ -10,7 +10,7 @@ fixtures_dir = os.path.join(os.path.dirname(__file__), 'fixtures') class DistGitLayoutTestCase(unittest.TestCase): def setUp(self): self.workdir = os.path.join(fixtures_dir, 'layouts/dist-git') - self.layout = layouts.DistGitLayout.from_path(self.workdir) + self.layout = layouts.DistGitLayout.from_path(self.workdir, None) def test_layout_data(self): self.assertEqual(self.layout.sourcedir, self.workdir) @@ -18,6 +18,8 @@ class DistGitLayoutTestCase(unittest.TestCase): self.assertEqual(self.layout.root_dir, self.workdir) self.assertEqual(self.layout.builddir, self.workdir) self.assertEqual(self.layout.rpmdir, self.workdir) + self.assertEqual(self.layout.rpmfilename, + '%%{ARCH}/%%{NAME}-%%{VERSION}-%%{RELEASE}.%%{ARCH}.rpm') self.assertEqual(self.layout.srcrpmdir, self.workdir) self.assertEqual(self.layout.sources_file_template, 'sources') @@ -38,3 +40,8 @@ class DistGitLayoutErrorsTestCase(unittest.TestCase): with self.assertRaises(errors.LayoutError) as e: layouts.DistGitLayout.from_path(os.path.join(self.workdir, 'specless')) self.assertEqual('spec file not found.', e.exception.args[0]) + + def test_resultsdirhint_error(self): + with self.assertRaises(errors.LayoutError) as e: + layouts.DistGitLayout.from_path(os.path.join(self.workdir, 'dist-git'), 'resultsdir') + self.assertEqual('resultsdir hint given', e.exception.args[0]) diff --git a/tests/test_layout_distgit_resultsdir.py b/tests/test_layout_distgit_resultsdir.py new file mode 100644 index 0000000..ed1e0ec --- /dev/null +++ b/tests/test_layout_distgit_resultsdir.py @@ -0,0 +1,47 @@ +import os +import unittest + +from pyrpkg import errors +from pyrpkg.layout import layouts + +fixtures_dir = os.path.join(os.path.dirname(__file__), 'fixtures') + + +class DistGitResultsDirLayoutTestCase(unittest.TestCase): + def setUp(self): + self.workdir = os.path.join(fixtures_dir, 'layouts/dist-git') + self.layout = layouts.DistGitResultsDirLayout.from_path(self.workdir, 'resultsdir') + + def test_layout_data(self): + self.assertEqual(self.layout.sourcedir, self.workdir) + self.assertEqual(self.layout.specdir, self.workdir) + self.assertEqual(self.layout.root_dir, self.workdir) + self.assertEqual(self.layout.builddir, os.path.join(self.workdir, 'results')) + self.assertEqual(self.layout.rpmdir, os.path.join(self.workdir, 'results')) + self.assertEqual(self.layout.rpmfilename, '%%{NAME}-%%{VERSION}-%%{RELEASE}.%%{ARCH}.rpm') + self.assertEqual(self.layout.srcrpmdir, os.path.join(self.workdir, 'results')) + self.assertEqual(self.layout.sources_file_template, 'sources') + + def test_layout_retired(self): + self.assertEqual(None, self.layout.is_retired()) + + +class DistGitResultsDirLayoutErrorsTestCase(unittest.TestCase): + def setUp(self): + self.workdir = os.path.join(fixtures_dir, 'layouts') + + def test_path_error(self): + with self.assertRaises(errors.LayoutError) as e: + layouts.DistGitResultsDirLayout.from_path(os.path.join(self.workdir, 'notfound')) + self.assertEqual('package path does not exist', e.exception.args[0]) + + def test_specless_error(self): + with self.assertRaises(errors.LayoutError) as e: + layouts.DistGitResultsDirLayout.from_path(os.path.join(self.workdir, 'specless')) + self.assertEqual('spec file not found.', e.exception.args[0]) + + def test_wronghint_error(self): + with self.assertRaises(errors.LayoutError) as e: + layouts.DistGitResultsDirLayout.from_path(os.path.join(self.workdir, 'dist-git'), + 'wronghint') + self.assertEqual('resultsdir hint not given', e.exception.args[0]) diff --git a/tests/test_layout_incomplete.py b/tests/test_layout_incomplete.py index 0cef423..59f3e2b 100644 --- a/tests/test_layout_incomplete.py +++ b/tests/test_layout_incomplete.py @@ -18,6 +18,7 @@ class IncompleteLayoutTestCase(unittest.TestCase): self.assertEqual(self.layout.root_dir, self.workdir) self.assertEqual(self.layout.builddir, None) self.assertEqual(self.layout.rpmdir, None) + self.assertEqual(self.layout.rpmfilename, None) self.assertEqual(self.layout.srcrpmdir, None) self.assertEqual(self.layout.sources_file_template, 'sources') diff --git a/tests/test_layout_retired.py b/tests/test_layout_retired.py index 4e854ea..bb5cc6c 100644 --- a/tests/test_layout_retired.py +++ b/tests/test_layout_retired.py @@ -18,6 +18,7 @@ class RetiredLayoutTestCase(unittest.TestCase): self.assertEqual(self.layout.root_dir, self.workdir) self.assertEqual(self.layout.builddir, None) self.assertEqual(self.layout.rpmdir, None) + self.assertEqual(self.layout.rpmfilename, None) self.assertEqual(self.layout.srcrpmdir, None) self.assertEqual(self.layout.sources_file_template, None) diff --git a/tests/test_layout_srpm.py b/tests/test_layout_srpm.py index 241ef38..e252bdb 100644 --- a/tests/test_layout_srpm.py +++ b/tests/test_layout_srpm.py @@ -18,6 +18,8 @@ class SRPMLayoutTestCase(unittest.TestCase): self.assertEqual(self.layout.root_dir, self.workdir) self.assertEqual(self.layout.builddir, os.path.join(self.workdir, 'BUILD')) self.assertEqual(self.layout.rpmdir, os.path.join(self.workdir, 'RPMS')) + self.assertEqual(self.layout.rpmfilename, + '%%{ARCH}/%%{NAME}-%%{VERSION}-%%{RELEASE}.%%{ARCH}.rpm') self.assertEqual(self.layout.srcrpmdir, os.path.join(self.workdir, 'SRPMS')) self.assertEqual(self.layout.sources_file_template, '.{0.repo_name}.metadata')