PR#540: Add config option for writing dist-git build results to a subdirectory - rpkg

rpkg

#540 Add config option for writing dist-git build results to a subdirectory

Merged 3 years ago by onosek. Opened 3 years ago by oturpe.

oturpe/rpkg new-layout into master

Add config option for writing dist-git build results to a subdirectory

Otto Urpelainen • 3 years ago

89bc57d

pyrpkg/__init__.py

file modified

+7 -3

		`@@ -92,7 +92,7 @@`
		`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 @@`
		`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 @@`
		`"--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 @@`
		`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))`

pyrpkg/cli.py

file modified

+6 -1

		`@@ -259,6 +259,10 @@`
		`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 @@`
		`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:`

pyrpkg/layout/__init__.py

file modified

+2 -2

		`@@ -16,7 +16,7 @@`
		`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 @@`
		`"""`
		`for layout in MetaLayout._layouts:`
		`try:`
		`- return layout.from_path(path)`
		`+ return layout.from_path(path, hint)`
		`except LayoutError:`
		`continue`
		`return None`

pyrpkg/layout/base.py

file modified

+2 -1

		`@@ -62,12 +62,13 @@`
		`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.`

pyrpkg/layout/layouts.py

file modified

+47 -8

		`@@ -29,20 +29,58 @@`
		`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 @@`
		`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 @@`
		`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 @@`
		`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.')`

tests/test_commands.py

file modified

		`@@ -242,6 +242,7 @@`
		`'_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 @@`
		`'_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',`

tests/test_layout_distgit.py

file modified

+8 -1

		`@@ -10,7 +10,7 @@`
		`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 @@`
		`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 @@`
		`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])`

tests/test_layout_distgit_resultsdir.py

file added

+47

		`@@ -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])`

tests/test_layout_incomplete.py

file modified

		`@@ -18,6 +18,7 @@`
		`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')`

tests/test_layout_retired.py

file modified

		`@@ -18,6 +18,7 @@`
		`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)`

tests/test_layout_srpm.py

file modified

		`@@ -18,6 +18,8 @@`
		`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')`

oturpe commented 3 years ago

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_diris 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 Fedora mailing list thread. 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, additionalhint 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.

Signed-off-by: Otto Urpelainen oturpe@iki.fi

rebased onto 353bfbd246efdee8376e0696bf9babaf678c2031

3 years ago

oturpe commented 3 years ago

This could also have started as discussion in the Issues section like suggested in the contribution guidelines. But since I did not have any prior knowledge of the codebase, I had to dig quite deep before I could propose a solution that would work. Thus, when I was ready to propose something, the implementation was already more or less done. Thus the approach of starting with a pull request.

onosek commented 3 years ago

Looks promising. I like your approach of sending the PR directly (and even with unittests).

So far it doesn't work for me. But maybe I missed something. I tested it in "setup.py development" mode. And changed rhpkg.conf. I can not see "results" dir after "rhpkg local".
There is a commit message that uses long lines and syntax features that Pagure supports. But the message is broken in "git log" view. Description in Pagure is OK, would it be possible to modify the commit message?
rpkg.conf - maybe it is not necessary (but it is not wrong either). After installation of rpkg this config is not really used. Proper config has to be modified in rhpkg/fedpkg after release.
Jenkins tests (https://jenkins-fedora-infra.apps.ci.centos.org/job/pyrpkg/473/Builders=F29/console) fails on Python2.7 (this has to be supported because of RHEL7). I am not sure what happened and I will have to look at it more. Currently, tests are not triggered automatically because of some external infrastructure issue.
Also flake8 shows some minor syntax issues.

rebased onto 85a8eca9f7fdcddd5c91f02247521e03a989ca47

3 years ago

oturpe commented 3 years ago

Ondrej,

Thank you reviewing this.

I added line break to commit message and removed some Markdown that was not really plaintext friendly. I hope the message now looks better.
I removed the rpkg.confchange. No need to have it there if it is not necessary. It would be great if there was a place to document different configuration options. Unfortunately, doc/source/configuration.rst is empty, I did not see the point in adding only one option there, and did not know what to write about many existing ones, so putting something to rpkg.conf was my best attempt at documentation, as inadequate as it was.
I went through flake8's complaints and fixed everything.
Regarding it not working...well, it does not work for me either if I remove the fedpkg patch I had to apply to get it working. I added the patch in my fedpkg fork so you can take a look. Basically, fedpkg was ignoring the whole rpg layout setup and settings its own rpmdefines. And since this pull request is based on layouts, that patch was necessary. I though that this was a bug in fedpkg and I could submit the fix to fedpkg next, in case this pull request gets accepted. But, if you say this pull request does not work with rhpkg either, could it be that the same thing is going on there, too? If so, maybe it is not so much that fedpkg has a bug, but that they are (in practice at least) an obsolete feature that no user of rpkg should depend on? If so, this pull request needs more than minor changes (and layouts could be removed from the codebase as dead code).

Edited 3 years ago by oturpe

rebased onto bd68008e10914b54a377fa802a5727f5c1a05b86

3 years ago

rebased onto ebc004e4e80b2176b6783b1fd008e2f5eeaf3d07

3 years ago

oturpe commented 3 years ago

Now that I took another look at this, I realized that in addition to items that were already sent to results directory, also sources from lookaside cache should go there. I added that, too. Probably all this should be a single commit if the pull request is merged, but since I am making changes in middle of review, I added another (temporary) commit so the addition can be viewed separately.

It turns out source download was also bypassing the layout completely, always using rootdir as source location.

2 new commits added

Extend resultsdir config to apply to sources, too
Add config option for writing dist-git build results to a subdirectory

3 years ago

onosek commented 3 years ago

commit messages look better now
thanks for fixing flake8 complaints and unittests failures.
Unfortunately, there is no such documentation so far. I describe newly added functionality in a releases section of web documentation. I know that it is not fortunate in case you are looking for a general summary regarding configuration.
I patched the fedpkg to try the functionality, but fedpkg prep stopped working to me. It was unable to find patches. I tried to build rpkg dist-git repository:

Cannot read /repo/rpkg-fedora/results/remove-koji-and-rpm-py-installer-from-requires.patch

rebased onto 884a19b7cf22d80b20c4ba553ca9c6d017936201

3 years ago

oturpe commented 3 years ago

Oh, I had a bad example package that did not have any patches. I now realize that patches should be in the sourcedir. Since patches are already at repository root (in at least Fedora repositories), sourcedir has to be the root. I reverted the sourcedir change. Now, for me, all this runs well:

$ fedpkg clone rpkg
$ cd rpkg
$ fedpkg prep
$ fedpkg srpm
$ fedpkg mockbuild

fedpkg local runs otherwise, but produces a lot of test failures. They seem to be related to Fedora's recent branch name change from master to rawhide/main — some assumption in rpkg's test suite seems to clash with running the tests inside a git repository that does not have a branch named master. i tested with another package, rubygem-jekyll, which completed successfully (in f33 branch matching my environment).

I apologize for repeatedly submitting faulty code. I hope everything is alright now.

I should also note at this point that the path to getting to relatively meager goal of simplifying .gitignore rules has been more convoluted than I initially expected. In case problems continue to surface, I am not at all offended if the decision is to drop this pull request due to weak cost benefit ratio.

Edited 3 years ago by oturpe

rebased onto b1881be387c998069ff0fe11eb7bb91ed6d9b242

3 years ago

lsedlar commented 3 years ago

I would like to have this option available.

Due to a lot of variety in how specfiles are worked with, there is unfortunately a lot of potential for conflicts and edge cases. While I think it would be a good change to enable by default (in fedpkg), maybe it would be nice to first ship it disabled, and let users know how to enable this to test it. That way feedback could be gathered from wider audience and possible bugs fixed.

oturpe commented 3 years ago

Hi @lsedlar ,

I am happy to hear that you think it is a good feature. And yes, the idea is that it is opt-in. fedpkg default configuration in Fedora should set it to root so current behavior is retained. Also, the code it written in such a way that if config file does not have anything set, root is the default. The later, discussion about switching the default config to subdir, if users like the new style.