PR#191: Fix rpm command to get changelog from SPEC - rpkg

rpkg

#191 Fix rpm command to get changelog from SPEC

Merged 7 years ago by cqi. Opened 7 years ago by cqi.

cqi/rpkg fix-bug-1412224 into master

Fix rpm command to get changelog from SPEC

Chenxiong Qi • 7 years ago

72cf760

CHANGELOG.rst

file modified

		`@@ -1,6 +1,11 @@`
		`ChangeLog`
		`=========`

		`+ NEXT`
		`+ ----`
		`+`
		`+ - Fix rpm command to get changelog from SPEC - rhbz#1412224 (cqi)`
		`+`
		`v1.49 (2017-02-22)`
		`------------------`

pyrpkg/__init__.py

file modified

+4 -2

		`@@ -1981,8 +1981,10 @@`
		`"""Write the latest spec changelog entry to a clog file"""`

		`spec_file = os.path.join(self.path, self.spec)`
		`- cmd = ['rpm', '-q', '--qf', '%{CHANGELOGTEXT}\n', '--specfile', spec_file]`
		`- proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)`
		`+ cmd = ['rpm'] + self.rpmdefines + ['-q', '--qf', '"%{CHANGELOGTEXT}\n"',`
		`+ '--specfile', '"%s"' % spec_file]`
		`+ proc = subprocess.Popen(' '.join(cmd), shell=True,`
		`+ stdout=subprocess.PIPE, stderr=subprocess.PIPE)`
		`stdout, stderr = proc.communicate()`
		`if proc.returncode > 0:`
		`raise rpkgError(stderr.strip())`

tests/test_cli.py

file modified

+14 -4

		`@@ -72,6 +72,7 @@`

		`def setUp(self):`
		`super(TestClog, self).setUp()`
		`+ self.checkout_branch(git.Repo(self.cloned_repo_path), 'eng-rhel-6')`

		`def cli_clog(self):`
		`"""Run clog command"""`
		`@@ -86,8 +87,7 @@`

		`clog_file = os.path.join(self.cloned_repo_path, 'clog')`
		`self.assertTrue(os.path.exists(clog_file))`
		`- with open(clog_file, 'r') as f:`
		`- clog = f.read().strip()`
		`+ clog = self.read_file(clog_file).strip()`
		`self.assertEqual('Initial version', clog)`

		`def test_raw_clog(self):`
		`@@ -98,15 +98,25 @@`

		`clog_file = os.path.join(self.cloned_repo_path, 'clog')`
		`self.assertTrue(os.path.exists(clog_file))`
		`- with open(clog_file, 'r') as f:`
		`- clog = f.read().strip()`
		`+ clog = self.read_file(clog_file).strip()`
		`self.assertEqual('- Initial version', clog)`

		`+ def test_reference_source_files_in_spec_should_not_break_clog(self):`
		`+ """SPEC containing Source0 or Patch0 should not break clog`
		`+`
		`+ This case is reported in bug 1412224`
		`+ """`
		`+ spec_file = os.path.join(self.cloned_repo_path, self.spec_file)`
		`+ spec = self.read_file(spec_file)`
		`+ self.write_file(spec_file, spec.replace('#Source0:', 'Source0: extrafile.txt'))`
		`+ self.test_raw_clog()`
		`+`

		`class TestCommit(CliTestCase):`

		`def setUp(self):`
		`super(TestCommit, self).setUp()`
		`+ self.checkout_branch(git.Repo(self.cloned_repo_path), 'eng-rhel-6')`
		`self.make_changes()`

		`def get_last_commit_message(self):`

tests/test_commands.py

file modified

+6 -4

		`@@ -306,7 +306,7 @@`
		`def setUp(self):`
		`super(ClogTest, self).setUp()`

		`- with open(os.path.join(self.repo_path, self.spec_file), 'w') as specfile:`
		`+ with open(os.path.join(self.cloned_repo_path, self.spec_file), 'w') as specfile:`
		`specfile.write('''`
		`Summary: package demo`
		`Name: pkgtool`
		`@@ -325,12 +325,14 @@`
		`- initial`
		`''')`

		`- self.cmd = self.make_commands(self.repo_path)`
		`+ self.clog_file = os.path.join(self.cloned_repo_path, 'clog')`
		`+ self.cmd = self.make_commands()`
		`+ self.checkout_branch(self.cmd.repo, 'eng-rhel-6')`

		`def test_clog(self):`
		`self.cmd.clog()`

		`- with open(os.path.join(self.repo_path, 'clog'), 'r') as clog:`
		`+ with open(self.clog_file, 'r') as clog:`
		`clog_lines = clog.readlines()`

		`expected_lines = ['add %changelog section\n',`
		`@@ -340,7 +342,7 @@`
		`def test_raw_clog(self):`
		`self.cmd.clog(raw=True)`

		`- with open(os.path.join(self.repo_path, 'clog'), 'r') as clog:`
		`+ with open(self.clog_file, 'r') as clog:`
		`clog_lines = clog.readlines()`

		`expected_lines = ['- add %changelog section\n',`

tests/utils.py

file modified

		`@@ -27,6 +27,8 @@`
		`Version: 1.2`
		`Release: 2%{dist}`
		`License: GPL`
		`+ #Source0:`
		`+ #Patch0:`
		`Group: Applications/Productivity`
		`BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)`
		`%description`

cqi commented 7 years ago

Commands.rpmdefines was not added to rpm command to get changelog from
SPEC. This causes SPEC cannot be parsed correctly if it contains SourceN
or PatchN. Like other methods in Commands class that runs rpm,
Commands.rpmdefines should be always added to the rpm command.

Resolves: rhbz#1412224

Signed-off-by: Chenxiong Qi cqi@redhat.com

rebased

7 years ago

lsedlar commented on line 9 of pyrpkg/__init__.py 7 years ago

This will break if If the path to spec file contains a space. Maybe splitting rpmdefines on first space to create a list like ['--define', '_sourcedir …', …] would be safer?

rebased

7 years ago

cqi commented 7 years ago

This will break if If the path to spec file contains a space. Maybe splitting rpmdefines on first space to create a list like ['--define', '_sourcedir …', …] would be safer?

Hi @lsedlar, fixed this in the same way of load_nameverrel. Thanks for catching this bug.

lsedlar commented 7 years ago

Hmm, it still does not work with a path with space. For example in the systemd example from original bug report (after renaming parent directory from systemd to break me) I'm getting malformed %include statement. This is failing at rpm level and I can't find a combination of quotes that would fix that. I say let's not worry about it and get the fix out the way it is now.

Edited 7 years ago by lsedlar

cqi commented 7 years ago

Okay. I have no idea what %include does. I run the raw rpm by quoting all paths, like --define '_sourcedir "/home/cqi/packages/my fedora packages/systemd"', it still breaks the parse.

cqi commented 7 years ago

It could probably be caused by rpm itself. From line 462, rpm does not handle space within path.

lsedlar commented 7 years ago

Yeah, could be. The %include statement should just take another file and put it into the spec file. A use case I know of is to move long changelog to a separate file.

The fix is good from my point of view :thumbsup: