[rpkg] A new format for 'sources' files

[Mathieu Bridon]

As part of the move away from MD5 for the source files hashes in the lookaside
cache, we have decided to change the format of the 'sources' files, so it
includes the hash type, to make it easier to identify which hash was used.

We agreed on using the BSD-style format, as returned on Linux by passing the
extra --tag option to commands like md5sum, sha512sumn, etc.

In addition, we decided to forbid mixing hash types in a single sources file,
forcing maintainers to use `fedpkg new-sources` the first time they upload an
additional source file.

This 4 patches series implements all that, with loads of unit tests, and some
code cleanup.

 src/pyrpkg/__init__.py |  60 ++++-----
 src/pyrpkg/sources.py  | 123 ++++++++++++------
 test/test_sources.py   | 334 ++++++++++++++++++++++++++++++++++--------------
 3 files changed, 359 insertions(+), 158 deletions(-)

https://fedorahosted.org/rel-eng/ticket/5846

[PATCH 1/4] Rewrite the sources module

[Mathieu Bridon]

From: Mathieu Bridon <bochecha(a)daitauha.fr&gt;

The API was awkward to use, to test, and hard to extend to support the
new file format we are targetting.

The new code is fully tested, and offers a simpler object API which
should be easier to work with.
---
 src/pyrpkg/__init__.py |  45 ++++-----
 src/pyrpkg/sources.py  |  92 +++++++++++-------
 test/test_sources.py   | 255 +++++++++++++++++++++++++++++++------------------
 3 files changed, 234 insertions(+), 158 deletions(-)

diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
index 5c4dbc5..7ecc8b4 100644
--- a/src/pyrpkg/__init__.py
+++ b/src/pyrpkg/__init__.py
@@ -38,7 +38,7 @@ try:
 except ImportError:
     pass
 
-import pyrpkg.sources
+from pyrpkg.sources import SourcesFile
 
 
 # Define our own error class
@@ -1568,16 +1568,20 @@ class Commands(object):
         # Default to putting the files where the module is
         if not outdir:
             outdir = self.path
-        for (csum, file) in self._read_sources():
+
+        sourcesf = SourcesFile(self.sources_filename)
+
+        for entry in sourcesf.entries:
             # See if we already have a valid copy downloaded
-            outfile = os.path.join(outdir, file)
+            outfile = os.path.join(outdir, entry.file)
             if os.path.exists(outfile):
-                if self._verify_file(outfile, csum, self.lookasidehash):
+                if self._verify_file(outfile, entry.hash, entry.hashtype):
                     continue
-            self.log.info("Downloading %s" % (file))
+            self.log.info("Downloading %s" % (entry.file))
             url = '%s/%s/%s/%s/%s' % (self.lookaside, self.module_name,
-                                      file.replace(' ', '%20'),
-                                      csum, file.replace(' ', '%20'))
+                                      entry.file.replace(' ', '%20'),
+                                      entry.hash,
+                                      entry.file.replace(' ', '%20'))
             # These options came from Makefile.common.
             # Probably need to support wget as well
             command = ['curl', '-H', 'Pragma:', '-o',
outfile, '-R', '-S',
@@ -1586,8 +1590,8 @@ class Commands(object):
                 command.append('-s')
             command.append(url)
             self._run_command(command)
-            if not self._verify_file(outfile, csum, self.lookasidehash):
-                raise rpkgError('%s failed checksum' % file)
+            if not self._verify_file(outfile, entry.hash, entry.hashtype):
+                raise rpkgError('%s failed checksum' % entry.file)
         return
 
     def switch_branch(self, branch, fetch=True):
@@ -2226,18 +2230,6 @@ class Commands(object):
                            config_dir)
             self._cleanup_tmp_dir(config_dir)
 
-    def _read_sources(self):
-        """Parses 'sources' file"""
-        with open(self.sources_filename, 'rb') as sources_fp:
-            reader = pyrpkg.sources.reader(sources_fp)
-            return [a for a in reader]
-
-    def _write_sources(self, rows):
-        """Writes 'sources' file"""
-        with open(self.sources_filename, 'wb') as sources_fp:
-            writer = pyrpkg.sources.writer(sources_fp)
-            writer.writerows(rows)
-
     def upload(self, files, replace=False):
         """Upload source file(s) in the lookaside cache
 
@@ -2247,11 +2239,7 @@ class Commands(object):
         oldpath = os.getcwd()
         os.chdir(self.path)
 
-        # Decide to overwrite or append to sources:
-        if replace or not os.path.exists(self.sources_filename):
-            sources = []
-        else:
-            sources = self._read_sources()
+        sourcesf = SourcesFile(self.sources_filename, replace=replace)
 
         # Will add new sources to .gitignore if they are not already there.
         gitignore = GitIgnore(os.path.join(self.path, '.gitignore'))
@@ -2262,8 +2250,7 @@ class Commands(object):
             file_hash = self._hash_file(f, self.lookasidehash)
             self.log.info("Uploading: %s  %s" % (file_hash, f))
             file_basename = os.path.basename(f)
-            if (file_hash, file_basename) not in sources:
-                sources.append((file_hash, file_basename))
+            sourcesf.add_entry(self.lookasidehash, file_basename, file_hash)
 
             # Add this file to .gitignore if it's not already there:
             if not gitignore.match(file_basename):
@@ -2278,7 +2265,7 @@ class Commands(object):
                 self._do_curl(file_hash, f)
                 uploaded.append(file_basename)
 
-        self._write_sources(sources)
+        sourcesf.write()
 
         # Write .gitignore with the new sources if anything changed:
         gitignore.write()
diff --git a/src/pyrpkg/sources.py b/src/pyrpkg/sources.py
index c4263ad..e987fa6 100644
--- a/src/pyrpkg/sources.py
+++ b/src/pyrpkg/sources.py
@@ -1,55 +1,75 @@
 """
-Our so-called sources file is simple text-based line-oriented file format. Each
-line represents one file and has two fields: file hash and base name of the
-file. Field separator is two spaces and Unix end-of-lines.
+Our so-called sources file is simple text-based line-oriented file format.
 
-This sources module implements API similar to csv module from standard library
-to read and write data in sources file format.
+Each line represents one source file and is in the same format as the output
+of commands like `md5sum filename`:
+
+    hash  filename
+
+This module implements a simple API to read these files, parse lines into
+entries, and write these entries to the file in the proper format.
 """
 
 
-class Reader(object):
-    def __init__(self, sourcesfile):
-        self.sourcesfile = sourcesfile
-        self._sourcesiter = None
+import os
+import re
 
-    def __iter__(self):
-        for entry in self.sourcesfile:
-            yield _parse_line(entry)
 
+class MalformedLineError(Exception):
+    pass
 
-class Writer(object):
-    def __init__(self, sourcesfile):
+
+class SourcesFile(object):
+    def __init__(self, sourcesfile, replace=False):
         self.sourcesfile = sourcesfile
+        self.entries = []
+
+        if not replace:
+            if not os.path.exists(sourcesfile):
+                return
+
+            with open(sourcesfile) as f:
+                for line in f:
+                    entry = self.parse_line(line)
+
+                    if entry and entry not in self.entries:
+                        self.entries.append(entry)
+
+    def parse_line(self, line):
+        stripped = line.strip()
+
+        if not stripped:
+            return
 
-    def writerow(self, row):
-        self.sourcesfile.write("%s\n" % _format_line(row))
+        try:
+            hash, file = stripped.split('  ', 1)
 
-    def writerows(self, rows):
-        for row in rows:
-            self.writerow(row)
+        except ValueError:
+            raise MalformedLineError(line)
 
+        return SourceFileEntry('md5', file, hash)
 
-def reader(sourcesfile):
-    return Reader(sourcesfile)
+    def add_entry(self, hashtype, file, hash):
+        entry = SourceFileEntry(hashtype, file, hash)
 
+        if entry not in self.entries:
+            self.entries.append(entry)
 
-def writer(sourcesfile):
-    return Writer(sourcesfile)
+    def write(self):
+        with open(self.sourcesfile, 'w') as f:
+            for entry in self.entries:
+                f.write(str(entry))
 
 
-def _parse_line(line):
-    stripped_line = line.strip()
-    if not stripped_line:
-        return []
-    entries = stripped_line.split('  ', 1)
-    if len(entries) != 2:
-        raise ValueError("Malformed line: %r." % line)
-    return entries
+class SourceFileEntry(object):
+    def __init__(self, hashtype, file, hash):
+            self.hashtype = hashtype.lower()
+            self.hash = hash
+            self.file = file
 
+    def __str__(self):
+        return '%s  %s\n' % (self.hash, self.file)
 
-def _format_line(entry):
-    if len(entry) != 0 and len(entry) != 2:
-        raise ValueError("Incorrect number of fields for entry: %r."
-                         % (entry,))
-    return "  ".join(entry)
+    def __eq__(self, other):
+        return ((self.hashtype, self.hash, self.file) ==
+                (other.hashtype, other.hash, other.file))
diff --git a/test/test_sources.py b/test/test_sources.py
index 997ab83..2b281e7 100644
--- a/test/test_sources.py
+++ b/test/test_sources.py
@@ -1,6 +1,9 @@
 import os
+import random
+import shutil
+import string
 import sys
-import StringIO
+import tempfile
 import unittest
 
 old_path = list(sys.path)
@@ -10,98 +13,164 @@ from pyrpkg import sources
 sys.path = old_path
 
 
-class formatLineTestCase(unittest.TestCase):
-    def test_wrong_number_of_fields(self):
-        WRONG_ENTRIES = [
-            ('foo'),
-            ('foo', 'bar', 'foo'),
-        ]
-        for entry in WRONG_ENTRIES:
-            self.assertRaises(ValueError, sources._format_line, entry)
-
-    def test_empty_entry(self):
-        self.assertEqual('', sources._format_line(()))
-
-    def test_correct_entry(self):
-        CORRECT_ENTRIES = [
-            (['foo', 'bar'], ('foo  bar')),
-        ]
-        for entry, line in CORRECT_ENTRIES:
-            self.assertEqual(line,
-                             sources._format_line(entry))
-
-
-class parseLineTestCase(unittest.TestCase):
-    def test_wrong_number_of_parts(self):
-        WRONG_LINES = [
-            'foo\n',
-            'foo  \n',
-            'foo bar\n',
-        ]
-        for line in WRONG_LINES:
-            self.assertRaises(ValueError, sources._parse_line, line)
-
-    def test_empty_line(self):
-        EMPTY_LINES = [
-            '',
-            '\n',
-            '  \n',
-        ]
-        for line in EMPTY_LINES:
-            self.assertEqual([], sources._parse_line(line))
-
-    def test_correct_line(self):
-        CORRECT_LINES = [
-            ('foo  bar\n', ['foo', 'bar']),
-            ('foo   bar\n', ['foo', ' bar'])
-        ]
-        for line, entry in CORRECT_LINES:
-            self.assertEqual(entry, sources._parse_line(line))
-
-
-class ReaderTestCase(unittest.TestCase):
-    def test_empty_sources(self):
-        EMPTY_SOURCES = [
-            ('', []),
-            ('\n', [[]]),
-            (' \n', [[]]),
-            ('\n\n', [[], []]),
-            (' \n ', [[], []]),
-        ]
-        for buffer, entries in EMPTY_SOURCES:
-            fp = StringIO.StringIO(buffer)
-            reader = sources.Reader(fp)
-            self.assertEqual(entries, [a for a in reader])
-            fp.close()
-
-    def test_correct_sources(self):
-        CORRECT_SOURCES = [
-            ('foo  bar\n', [['foo', 'bar']]),
-            ('foo  bar\nfooo  baaar\n', [['foo', 'bar'],
-                                         ['fooo', 'baaar'],
-                                         ]),
-        ]
-        for buffer, entries in CORRECT_SOURCES:
-            fp = StringIO.StringIO(buffer)
-            reader = sources.Reader(fp)
-            self.assertEqual(entries, [a for a in reader])
-            fp.close()
-
-
-class WriterTestCase(unittest.TestCase):
-    def test_writerows(self):
-        CORRECT_SOURCES = [
-            ([['foo', 'bar']], 'foo  bar\n'),
-            ([['foo', 'bar'],
-              ['fooo', 'baaar'],
-              ], 'foo  bar\nfooo  baaar\n'),
-        ]
-        for entries, buffer in CORRECT_SOURCES:
-            fp = StringIO.StringIO()
-            writer = sources.Writer(fp)
-            writer.writerows(entries)
-            self.assertEqual(fp.getvalue(), buffer)
-            fp.close()
+class SourceFileEntryTestCase(unittest.TestCase):
+    def test_entry(self):
+        e = sources.SourceFileEntry('md5', 'afile', 'ahash')
+        expected = 'ahash  afile\n'
+        self.assertEqual(str(e), expected)
+
+
+class SourcesFileTestCase(unittest.TestCase):
+    def setUp(self):
+        self.workdir = tempfile.mkdtemp(prefix='rpkg-tests.')
+        self.sourcesfile = os.path.join(self.workdir, self._testMethodName)
+
+    def tearDown(self):
+        shutil.rmtree(self.workdir)
+
+    def test_parse_empty_line(self):
+        s = sources.SourcesFile(self.sourcesfile)
+        entry = s.parse_line('')
+        self.assertIsNone(entry)
+
+    def test_parse_eol_line(self):
+        s = sources.SourcesFile(self.sourcesfile)
+        entry = s.parse_line('\n')
+        self.assertIsNone(entry)
+
+    def test_parse_whitespace_line(self):
+        s = sources.SourcesFile(self.sourcesfile)
+        entry = s.parse_line('    \n')
+        self.assertIsNone(entry)
+
+    def test_parse_entry_line(self):
+        s = sources.SourcesFile(self.sourcesfile)
+
+        line = 'ahash  afile\n'
+        entry = s.parse_line(line)
+
+        self.assertTrue(isinstance(entry, sources.SourceFileEntry))
+        self.assertEqual(entry.hashtype, 'md5')
+        self.assertEqual(entry.hash, 'ahash')
+        self.assertEqual(entry.file, 'afile')
+        self.assertEqual(str(entry), line)
+
+    def test_parse_wrong_lines(self):
+        s = sources.SourcesFile(self.sourcesfile)
+
+        lines = ['ahash',
+                 'ahash  ',
+                 'ahash afile',
+                 ]
+
+        for line in lines:
+            with self.assertRaises(sources.MalformedLineError):
+                s.parse_line(line)
+
+    def test_open_new_file(self):
+        s = sources.SourcesFile(self.sourcesfile)
+        self.assertEqual(len(s.entries), 0)
+
+    def test_open_empty_file(self):
+        open(self.sourcesfile, 'w').write('')
+        s = sources.SourcesFile(self.sourcesfile)
+        self.assertEqual(len(s.entries), 0)
+
+    def test_open_existing_file(self):
+        lines = ['ahash  afile\n', 'anotherhash  anotherfile\n']
+
+        with open(self.sourcesfile, 'w') as f:
+            for line in lines:
+                f.write(line)
+
+        s = sources.SourcesFile(self.sourcesfile)
+
+        for i, entry in enumerate(s.entries):
+            self.assertTrue(isinstance(entry, sources.SourceFileEntry))
+            self.assertEqual(str(entry), lines[i])
+
+    def test_open_existing_file_with_wrong_line(self):
+        line = 'some garbage here\n'
+
+        with open(self.sourcesfile, 'w') as f:
+            f.write(line)
+
+        with self.assertRaises(sources.MalformedLineError):
+            return sources.SourcesFile(self.sourcesfile)
+
+    def test_add_entry(self):
+        s = sources.SourcesFile(self.sourcesfile)
+        self.assertEqual(len(s.entries), 0)
+
+        s.add_entry('md5', 'afile', 'ahash')
+        self.assertEqual(len(s.entries), 1)
+        self.assertEqual(str(s.entries[-1]), 'ahash  afile\n')
+
+        s.add_entry('md5', 'anotherfile', 'anotherhash')
+        self.assertEqual(len(s.entries), 2)
+        self.assertEqual(str(s.entries[-1]), 'anotherhash  anotherfile\n')
+
+    def test_add_entry_twice(self):
+        s = sources.SourcesFile(self.sourcesfile)
+        self.assertEqual(len(s.entries), 0)
+
+        s.add_entry('md5', 'afile', 'ahash')
+        self.assertEqual(len(s.entries), 1)
+        self.assertEqual(str(s.entries[-1]), 'ahash  afile\n')
+
+        s.add_entry('md5', 'afile', 'ahash')
+        self.assertEqual(len(s.entries), 1)
+
+    def test_write_new_file(self):
+        s = sources.SourcesFile(self.sourcesfile)
+        self.assertEqual(len(s.entries), 0)
+
+        s.add_entry('md5', 'afile', 'ahash')
+        s.add_entry('md5', 'anotherfile', 'anotherhash')
+        s.write()
+
+        with open(self.sourcesfile) as f:
+             lines = f.readlines()
+
+        self.assertEqual(len(lines), 2)
+        self.assertEqual(lines[0], 'ahash  afile\n')
+        self.assertEqual(lines[1], 'anotherhash  anotherfile\n')
+
+    def test_write_adding_a_line(self):
+        lines = ['ahash  afile\n', 'anotherhash  anotherfile\n']
+
+        with open(self.sourcesfile, 'w') as f:
+            for line in lines:
+                f.write(line)
+
+        s = sources.SourcesFile(self.sourcesfile)
+        s.add_entry('md5', 'thirdfile', 'thirdhash')
+        s.write()
+
+        with open(self.sourcesfile) as f:
+             lines = f.readlines()
+
+        self.assertEqual(len(lines), 3)
+        self.assertEqual(lines[0], 'ahash  afile\n')
+        self.assertEqual(lines[1], 'anotherhash  anotherfile\n')
+        self.assertEqual(lines[2], 'thirdhash  thirdfile\n')
+
+    def test_write_over(self):
+        lines = ['ahash  afile\n', 'anotherhash  anotherfile\n']
+
+        with open(self.sourcesfile, 'w') as f:
+            for line in lines:
+                f.write(line)
+
+        s = sources.SourcesFile(self.sourcesfile, replace=True)
+        s.add_entry('md5', 'thirdfile', 'thirdhash')
+        s.write()
+
+        with open(self.sourcesfile) as f:
+             lines = f.readlines()
+
+        self.assertEqual(len(lines), 1)
+        self.assertEqual(lines[0], 'thirdhash  thirdfile\n')
 
 
 if __name__ == '__main__':
-- 
2.1.0

[PATCH 2/4] sources: Move to the new file format

[Mathieu Bridon]

From: Mathieu Bridon <bochecha(a)daitauha.fr&gt;

We are eventually going to move away from md5 for the sources. However,
in order to make the migration (and future ones) easier, we want to
indicate on each line of the 'sources' file what is the hash function
used to compute the hash of the file.

Fortunately, the md5sum/sha512sum/... utilities support two file formats
as their inputs and outputs:

  * the current format:
    `ahash  afile`

  * the BSD-style format, obtained with the `--tag` option:
    `HASHTYPE (afile) = ahash

This second format is perfect for us, so this commit moves our 'sources'
file handling to it.

A couple of notes:

* we preserve compatibility with existing files, so lines in the old
  format are still read
* we now only ever write lines in the new format, which means that when
  reading an existing file, all currently present lines will be converted to
  the new format when writing back to the file
---
 src/pyrpkg/sources.py | 21 ++++++++++--
 test/test_sources.py  | 92 ++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 99 insertions(+), 14 deletions(-)

diff --git a/src/pyrpkg/sources.py b/src/pyrpkg/sources.py
index e987fa6..340c3e1 100644
--- a/src/pyrpkg/sources.py
+++ b/src/pyrpkg/sources.py
@@ -2,7 +2,12 @@
 Our so-called sources file is simple text-based line-oriented file format.
 
 Each line represents one source file and is in the same format as the output
-of commands like `md5sum filename`:
+of commands like `md5sum --tag filename`:
+
+    hashtype (filename) = hash
+
+To preserve backwards compatibility, lines can also be in the older format,
+which corresponds to the output of commands like `md5sum filename`:
 
     hash  filename
 
@@ -15,6 +20,10 @@ import os
 import re
 
 
+LINE_PATTERN = re.compile(
+    r'^(?P<hashtype>[^ ]+?) \((?P<file>[^ )]+?)\) = (?P<hash>[^
]+?)$')
+
+
 class MalformedLineError(Exception):
     pass
 
@@ -41,6 +50,12 @@ class SourcesFile(object):
         if not stripped:
             return
 
+        m = LINE_PATTERN.match(stripped)
+        if m is not None:
+            return SourceFileEntry(m.group('hashtype'), m.group('file'),
+                                   m.group('hash'))
+
+        # Try falling back on the old format
         try:
             hash, file = stripped.split('  ', 1)
 
@@ -68,7 +83,9 @@ class SourceFileEntry(object):
             self.file = file
 
     def __str__(self):
-        return '%s  %s\n' % (self.hash, self.file)
+        return '%s (%s) = %s\n' % (self.hashtype.upper(), self.file,
+                                   self.hash)
+
 
     def __eq__(self, other):
         return ((self.hashtype, self.hash, self.file) ==
diff --git a/test/test_sources.py b/test/test_sources.py
index 2b281e7..fb91b3e 100644
--- a/test/test_sources.py
+++ b/test/test_sources.py
@@ -16,7 +16,7 @@ sys.path = old_path
 class SourceFileEntryTestCase(unittest.TestCase):
     def test_entry(self):
         e = sources.SourceFileEntry('md5', 'afile', 'ahash')
-        expected = 'ahash  afile\n'
+        expected = 'MD5 (afile) = ahash\n'
         self.assertEqual(str(e), expected)
 
 
@@ -43,10 +43,23 @@ class SourcesFileTestCase(unittest.TestCase):
         entry = s.parse_line('    \n')
         self.assertIsNone(entry)
 
-    def test_parse_entry_line(self):
+    def test_parse_old_style_line(self):
         s = sources.SourcesFile(self.sourcesfile)
 
         line = 'ahash  afile\n'
+        newline = 'MD5 (afile) = ahash\n'
+        entry = s.parse_line(line)
+
+        self.assertTrue(isinstance(entry, sources.SourceFileEntry))
+        self.assertEqual(entry.hashtype, 'md5')
+        self.assertEqual(entry.hash, 'ahash')
+        self.assertEqual(entry.file, 'afile')
+        self.assertEqual(str(entry), newline)
+
+    def test_parse_entry_line(self):
+        s = sources.SourcesFile(self.sourcesfile)
+
+        line = 'MD5 (afile) = ahash\n'
         entry = s.parse_line(line)
 
         self.assertTrue(isinstance(entry, sources.SourceFileEntry))
@@ -61,6 +74,8 @@ class SourcesFileTestCase(unittest.TestCase):
         lines = ['ahash',
                  'ahash  ',
                  'ahash afile',
+                 'SHA512 (afile) = ahash garbage',
+                 'MD5 SHA512 (afile) = ahash',
                  ]
 
         for line in lines:
@@ -76,8 +91,23 @@ class SourcesFileTestCase(unittest.TestCase):
         s = sources.SourcesFile(self.sourcesfile)
         self.assertEqual(len(s.entries), 0)
 
-    def test_open_existing_file(self):
+    def test_open_existing_file_with_old_style_lines(self):
         lines = ['ahash  afile\n', 'anotherhash  anotherfile\n']
+        newlines = ['MD5 (afile) = ahash\n',
+                    'MD5 (anotherfile) = anotherhash\n']
+
+        with open(self.sourcesfile, 'w') as f:
+            for line in lines:
+                f.write(line)
+
+        s = sources.SourcesFile(self.sourcesfile)
+
+        for i, entry in enumerate(s.entries):
+            self.assertTrue(isinstance(entry, sources.SourceFileEntry))
+            self.assertEqual(str(entry), newlines[i])
+
+    def test_open_existing_file(self):
+        lines = ['MD5 (afile) = ahash\n', 'MD5 (anotherfile) =
anotherhash\n']
 
         with open(self.sourcesfile, 'w') as f:
             for line in lines:
@@ -89,6 +119,44 @@ class SourcesFileTestCase(unittest.TestCase):
             self.assertTrue(isinstance(entry, sources.SourceFileEntry))
             self.assertEqual(str(entry), lines[i])
 
+    def test_open_existing_file_with_mixed_lines(self):
+        lines = ['ahash  afile\n',
+                 'anotherhash  anotherfile\n',
+                 'MD5 (thirdfile) = thirdhash\n',
+                 ]
+        expected = [
+            'MD5 (afile) = ahash\n',
+            'MD5 (anotherfile) = anotherhash\n',
+            'MD5 (thirdfile) = thirdhash\n',
+            ]
+
+        with open(self.sourcesfile, 'w') as f:
+            for line in lines:
+                f.write(line)
+
+        s = sources.SourcesFile(self.sourcesfile)
+
+        for i, entry in enumerate(s.entries):
+            self.assertTrue(isinstance(entry, sources.SourceFileEntry))
+            self.assertEqual(str(entry), expected[i])
+
+    def test_open_existing_file_with_identical_entries_old_and_new(self):
+        lines = ['ahash  afile\n',
+                 'MD5 (afile) = ahash\n',
+                 ]
+
+        with open(self.sourcesfile, 'w') as f:
+            for line in lines:
+                f.write(line)
+
+        s = sources.SourcesFile(self.sourcesfile)
+
+        self.assertEqual(len(s.entries), 1)
+        self.assertEqual(s.entries[0].hashtype, 'md5')
+        self.assertEqual(s.entries[0].file, 'afile')
+        self.assertEqual(s.entries[0].hash, 'ahash')
+        self.assertEqual(str(s.entries[0]), lines[-1])
+
     def test_open_existing_file_with_wrong_line(self):
         line = 'some garbage here\n'
 
@@ -104,11 +172,11 @@ class SourcesFileTestCase(unittest.TestCase):
 
         s.add_entry('md5', 'afile', 'ahash')
         self.assertEqual(len(s.entries), 1)
-        self.assertEqual(str(s.entries[-1]), 'ahash  afile\n')
+        self.assertEqual(str(s.entries[-1]), 'MD5 (afile) = ahash\n')
 
         s.add_entry('md5', 'anotherfile', 'anotherhash')
         self.assertEqual(len(s.entries), 2)
-        self.assertEqual(str(s.entries[-1]), 'anotherhash  anotherfile\n')
+        self.assertEqual(str(s.entries[-1]), 'MD5 (anotherfile) =
anotherhash\n')
 
     def test_add_entry_twice(self):
         s = sources.SourcesFile(self.sourcesfile)
@@ -116,7 +184,7 @@ class SourcesFileTestCase(unittest.TestCase):
 
         s.add_entry('md5', 'afile', 'ahash')
         self.assertEqual(len(s.entries), 1)
-        self.assertEqual(str(s.entries[-1]), 'ahash  afile\n')
+        self.assertEqual(str(s.entries[-1]), 'MD5 (afile) = ahash\n')
 
         s.add_entry('md5', 'afile', 'ahash')
         self.assertEqual(len(s.entries), 1)
@@ -133,8 +201,8 @@ class SourcesFileTestCase(unittest.TestCase):
              lines = f.readlines()
 
         self.assertEqual(len(lines), 2)
-        self.assertEqual(lines[0], 'ahash  afile\n')
-        self.assertEqual(lines[1], 'anotherhash  anotherfile\n')
+        self.assertEqual(lines[0], 'MD5 (afile) = ahash\n')
+        self.assertEqual(lines[1], 'MD5 (anotherfile) = anotherhash\n')
 
     def test_write_adding_a_line(self):
         lines = ['ahash  afile\n', 'anotherhash  anotherfile\n']
@@ -151,9 +219,9 @@ class SourcesFileTestCase(unittest.TestCase):
              lines = f.readlines()
 
         self.assertEqual(len(lines), 3)
-        self.assertEqual(lines[0], 'ahash  afile\n')
-        self.assertEqual(lines[1], 'anotherhash  anotherfile\n')
-        self.assertEqual(lines[2], 'thirdhash  thirdfile\n')
+        self.assertEqual(lines[0], 'MD5 (afile) = ahash\n')
+        self.assertEqual(lines[1], 'MD5 (anotherfile) = anotherhash\n')
+        self.assertEqual(lines[2], 'MD5 (thirdfile) = thirdhash\n')
 
     def test_write_over(self):
         lines = ['ahash  afile\n', 'anotherhash  anotherfile\n']
@@ -170,7 +238,7 @@ class SourcesFileTestCase(unittest.TestCase):
              lines = f.readlines()
 
         self.assertEqual(len(lines), 1)
-        self.assertEqual(lines[0], 'thirdhash  thirdfile\n')
+        self.assertEqual(lines[0], 'MD5 (thirdfile) = thirdhash\n')
 
 
 if __name__ == '__main__':
-- 
2.1.0

[PATCH 3/4] sources: Forbid mixing hash types

[Mathieu Bridon]

From: Mathieu Bridon <bochecha(a)daitauha.fr&gt;

There theoretically isn't any problem with mixing various hashes, as far
as rpkg is concerned.

However, taking for example the following file:

    $ cat sources
    MD5 (pygit2-0.22.0.tar.gz) = 31dc65b3cbe6e39d75a39db86dd7cd8c
    SHA512 (pygit2-0.22.0.tar.gz) =2079adc3285bf08fc7227b412b9d3dc[...]

When someone tries verifying it with `md5sum -c` they would get:

    $ md5sum -c sources
    pygit2-0.22.0.tar.gz: OK
    md5sum: WARNING: 1 line is improperly formatted

They'd need to also to check the file with `sha512sum -c`, but then they
would get:

    $ sha512sum -c sources
    pygit2-0.22.0.tar.gz: OK
    sha512sum: WARNING: 1 line is improperly formatted

This is clearly not ideal, and it might cause confusion.

Mixing might happen when a file contains lines with the old hash, then
after the migration a packager tries to upload an additional source
file.

This commit prevents mixing, by forcing packagers to use `new-sources`
and redo the source file entirely with the new hash.

It's a one time annoyance, but in addition to reducing future confusion,
it helps moving towards the goal of completely dropping MD5.
---
 src/pyrpkg/__init__.py | 20 ++++++++++++++++++--
 src/pyrpkg/sources.py  | 18 ++++++++++++++++--
 test/test_sources.py   | 11 +++++++++++
 3 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
index 7ecc8b4..57d9b75 100644
--- a/src/pyrpkg/__init__.py
+++ b/src/pyrpkg/__init__.py
@@ -38,7 +38,7 @@ try:
 except ImportError:
     pass
 
-from pyrpkg.sources import SourcesFile
+from pyrpkg.sources import HashtypeMixingError, SourcesFile
 
 
 # Define our own error class
@@ -2250,7 +2250,23 @@ class Commands(object):
             file_hash = self._hash_file(f, self.lookasidehash)
             self.log.info("Uploading: %s  %s" % (file_hash, f))
             file_basename = os.path.basename(f)
-            sourcesf.add_entry(self.lookasidehash, file_basename, file_hash)
+
+            try:
+                sourcesf.add_entry(self.lookasidehash, file_basename,
+                                   file_hash)
+            except HashtypeMixingError as e:
+                msg = '\n'.join([
+                    'Can not upload a new source file with a %(newhash)s '
+                    'hash, as the "%(sources)s" file contains at least one
'
+                    'line with a %(existinghash)s hash.', '',
+                    'Please redo the whole "%(sources)s" file using:',
+                    '    `%(arg0)s new-sources file1 file2 ...`']) % {
+                        'newhash': e.new_hashtype,
+                        'existinghash': e.existing_hashtype,
+                        'sources': self.sources_filename,
+                        'arg0': sys.argv[0],
+                    }
+                raise rpkgError(msg)
 
             # Add this file to .gitignore if it's not already there:
             if not gitignore.match(file_basename):
diff --git a/src/pyrpkg/sources.py b/src/pyrpkg/sources.py
index 340c3e1..9c3b22e 100644
--- a/src/pyrpkg/sources.py
+++ b/src/pyrpkg/sources.py
@@ -24,6 +24,14 @@ LINE_PATTERN = re.compile(
     r'^(?P<hashtype>[^ ]+?) \((?P<file>[^ )]+?)\) = (?P<hash>[^
]+?)$')
 
 
+class HashtypeMixingError(Exception):
+    def __init__(self, existing_hashtype, new_hashtype):
+        super(HashtypeMixingError, self).__init__()
+
+        self.existing_hashtype = existing_hashtype
+        self.new_hashtype = new_hashtype
+
+
 class MalformedLineError(Exception):
     pass
 
@@ -67,8 +75,14 @@ class SourcesFile(object):
     def add_entry(self, hashtype, file, hash):
         entry = SourceFileEntry(hashtype, file, hash)
 
-        if entry not in self.entries:
-            self.entries.append(entry)
+        for e in self.entries:
+            if entry.hashtype != e.hashtype:
+                raise HashtypeMixingError(e.hashtype, entry.hashtype)
+
+            if entry == e:
+                return
+
+        self.entries.append(entry)
 
     def write(self):
         with open(self.sourcesfile, 'w') as f:
diff --git a/test/test_sources.py b/test/test_sources.py
index fb91b3e..ba90427 100644
--- a/test/test_sources.py
+++ b/test/test_sources.py
@@ -189,6 +189,17 @@ class SourcesFileTestCase(unittest.TestCase):
         s.add_entry('md5', 'afile', 'ahash')
         self.assertEqual(len(s.entries), 1)
 
+    def test_add_entry_mixing_hashtypes(self):
+        s = sources.SourcesFile(self.sourcesfile)
+        self.assertEqual(len(s.entries), 0)
+
+        s.add_entry('md5', 'afile', 'ahash')
+        self.assertEqual(len(s.entries), 1)
+        self.assertEqual(str(s.entries[-1]), 'MD5 (afile) = ahash\n')
+
+        with self.assertRaises(sources.HashtypeMixingError):
+            s.add_entry('sha512', 'anotherfile', 'anotherhash')
+
     def test_write_new_file(self):
         s = sources.SourcesFile(self.sourcesfile)
         self.assertEqual(len(s.entries), 0)
-- 
2.1.0

[PATCH 4/4] Don't do several times the same thing

[Mathieu Bridon]

From: Mathieu Bridon <bochecha(a)daitauha.fr&gt;

---
 src/pyrpkg/__init__.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
index 57d9b75..a5f02ea 100644
--- a/src/pyrpkg/__init__.py
+++ b/src/pyrpkg/__init__.py
@@ -1578,10 +1578,9 @@ class Commands(object):
                 if self._verify_file(outfile, entry.hash, entry.hashtype):
                     continue
             self.log.info("Downloading %s" % (entry.file))
+            urled_file = entry.file.replace(' ', '%20')
             url = '%s/%s/%s/%s/%s' % (self.lookaside, self.module_name,
-                                      entry.file.replace(' ', '%20'),
-                                      entry.hash,
-                                      entry.file.replace(' ', '%20'))
+                                      urled_file, entry.hash, urled_file)
             # These options came from Makefile.common.
             # Probably need to support wget as well
             command = ['curl', '-H', 'Pragma:', '-o',
outfile, '-R', '-S',
-- 
2.1.0

[PATCH 4/4] Don't do several times the same thing

[Pavol Babincak]

All patches looked ok so I've merged them in upstream and released 
rpkg-1.31. I've rebuilt rpkg in Fedora too: rpkg-1.31-1.fc23 and 
rpkg-1.31-1.fc22 (by now).

Thanks!
-- 
Pavol Babincak
Release Engineering, Red Hat

[PATCH 4/4] Don't do several times the same thing

[Mathieu Bridon]

On Fri, 2015-03-06 at 17:31 +0100, Pavol Babincak wrote:
>  All patches looked ok so I've merged them in upstream and
> released 
>  rpkg-1.31. I've rebuilt rpkg in Fedora too: rpkg-1.31-1.fc23 and 
>  rpkg-1.31-1.fc22 (by now). 
This might be problematic.

Say I run this new rpkg on Fedora 22, and upload a new source tarball in
the f21 branch.

The sources file will be in the new format.

In Koji, when building the package, inside the mock buildroot, the
previous rpkg gets installed, which won't be able to read the sources
file in the new format.

You should push it to all Fedora branches at the same time.


-- 
Mathieu

[PATCH 4/4] Don't do several times the same thing

[Pavol Babincak]

On 03/06/2015 06:13 PM, Mathieu Bridon wrote:
>  On Fri, 2015-03-06 at 17:31 +0100, Pavol Babincak wrote:
> > All patches looked ok so I've merged them in upstream and released
> > rpkg-1.31. I've rebuilt rpkg in Fedora too: rpkg-1.31-1.fc23 and
> > rpkg-1.31-1.fc22 (by now).
>
>  This might be problematic.
>
>  Say I run this new rpkg on Fedora 22, and upload a new source tarball in
>  the f21 branch.
>
>  The sources file will be in the new format.
>
>  In Koji, when building the package, inside the mock buildroot, the
>  previous rpkg gets installed, which won't be able to read the sources
>  file in the new format.
>
>  You should push it to all Fedora branches at the same time. 
Thanks for pointing
this out. I realized this might break building 
packages on custom buildsystems if they aren't prepared for this. 
Package for fetching sources in buildroots of a build system needs to 
know the new (BSD-like) sources format before developers start to commit 
sources in this format. People might have an alternative implementation 
for fetching the sources in their buildroots similar to Fedora's 
fedpkg-minimal. So upgrading to new rpkg might not be a proper solution 
for them.

We'll need to take different approach then. By default pyrpkg will be 
backwards compatible and write sources in old format with md5 hash. 
pyrpkg's clients (e.g. fedpkg) will have a way to switch to new sources 
file format if their buildsystem infrastructure is prepared for that.

In Fedora this means I need to finish my implementation of new sources 
format in fedpkg-minimal and only after this package will be in all 
supported buildroots we can switch fedpkg's default behaviour.

I've untagged rpkg build from rawhide so developers won't rewrite their 
sources files in the way which Koji buildroots doesn't understand yet.
-- 
Pavol Babincak
Release Engineering, Red Hat

[PATCH 4/4] Don't do several times the same thing

[Mathieu Bridon]

On Fri, 2015-03-06 at 19:35 +0100, Pavol Babincak wrote:
>  We'll need to take different approach then. By default pyrpkg
> will be 
>  backwards compatible and write sources in old format with md5 hash. 
>  pyrpkg's clients (e.g. fedpkg) will have a way to switch to new sources 
>  file format if their buildsystem infrastructure is prepared for that.
>  
>  In Fedora this means I need to finish my implementation of new sources 
>  format in fedpkg-minimal and only after this package will be in all 
>  supported buildroots we can switch fedpkg's default behaviour.
>  
>  I've untagged rpkg build from rawhide so developers won't rewrite their 
>  sources files in the way which Koji buildroots doesn't understand yet. 
Seems like the best for now.

I'll send new patches that implement what you described above.


-- 
Mathieu

[PATCH 1/3] sources: Reindent code

[Mathieu Bridon]

From: Mathieu Bridon <bochecha(a)daitauha.fr&gt;

Why did I submit this like that in the first place? :x
---
 src/pyrpkg/sources.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/pyrpkg/sources.py b/src/pyrpkg/sources.py
index 9c3b22e..ea49ea6 100644
--- a/src/pyrpkg/sources.py
+++ b/src/pyrpkg/sources.py
@@ -92,9 +92,9 @@ class SourcesFile(object):
 
 class SourceFileEntry(object):
     def __init__(self, hashtype, file, hash):
-            self.hashtype = hashtype.lower()
-            self.hash = hash
-            self.file = file
+        self.hashtype = hashtype.lower()
+        self.hash = hash
+        self.file = file
 
     def __str__(self):
         return '%s (%s) = %s\n' % (self.hashtype.upper(), self.file,
-- 
2.1.0

[PATCH 2/3] sources: Support writing in either the old or new format

[Mathieu Bridon]

From: Mathieu Bridon <bochecha(a)daitauha.fr&gt;

Not all rpkg-based tools will be migrating to the new sources file
format at the same time, if ever.

For example, if rpkg was updated only in Rawhide, then a package
maintainer who uses Rawhide would commit sources file in the new format,
and another maintainer (or Koji!) on an older Fedora version would not
be able to read the new file.

So we would need to upgrade rpkg in all versions of Fedora at the same
time.

But then all Fedora downstreams would also need to have the new rpkg at
the same time, or at least before maintainers start pushing new-format
files.

This kind of lock-in can't happen at the rpkg level.

Instead, this new patch:

* makes rpkg able to write files in either format
* makes rpkg write files in the old format by default (to preserve
  compatibility)
* allows rpkg-based tools to specify whether they want to use the new
  format.

This means we can upgrade rpkg more easily everywhere, taking our time.
Eventually, Fedora can decide to move to the new format simply by
overriding pyrpkg.Commands.source_entry_type to 'bsd' in fedpkg.Commands
---
 src/pyrpkg/__init__.py |  7 +++++--
 src/pyrpkg/sources.py  | 20 +++++++++++-------
 test/test_sources.py   | 55 +++++++++++++++++++++++++++++++++-----------------
 3 files changed, 54 insertions(+), 28 deletions(-)

diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
index a5f02ea..60c1a6e 100644
--- a/src/pyrpkg/__init__.py
+++ b/src/pyrpkg/__init__.py
@@ -158,6 +158,8 @@ class Commands(object):
         self._branch_remote = None
         # Name of default remote to be used for new clone
         self.default_branch_remote = 'origin'
+        # Default sources file output format type
+        self.source_entry_type = 'old'
 
     # Define properties here
     # Properties allow us to "lazy load" various attributes, which also means
@@ -1569,7 +1571,7 @@ class Commands(object):
         if not outdir:
             outdir = self.path
 
-        sourcesf = SourcesFile(self.sources_filename)
+        sourcesf = SourcesFile(self.sources_filename, self.source_entry_type)
 
         for entry in sourcesf.entries:
             # See if we already have a valid copy downloaded
@@ -2238,7 +2240,8 @@ class Commands(object):
         oldpath = os.getcwd()
         os.chdir(self.path)
 
-        sourcesf = SourcesFile(self.sources_filename, replace=replace)
+        sourcesf = SourcesFile(self.sources_filename, self.source_entry_type,
+                               replace=replace)
 
         # Will add new sources to .gitignore if they are not already there.
         gitignore = GitIgnore(os.path.join(self.path, '.gitignore'))
diff --git a/src/pyrpkg/sources.py b/src/pyrpkg/sources.py
index ea49ea6..a3ff1b1 100644
--- a/src/pyrpkg/sources.py
+++ b/src/pyrpkg/sources.py
@@ -37,8 +37,10 @@ class MalformedLineError(Exception):
 
 
 class SourcesFile(object):
-    def __init__(self, sourcesfile, replace=False):
+    def __init__(self, sourcesfile, entry_type, replace=False):
         self.sourcesfile = sourcesfile
+        self.entry_type = {'old': SourceFileEntry,
+                           'bsd': BSDSourceFileEntry}[entry_type]
         self.entries = []
 
         if not replace:
@@ -60,7 +62,7 @@ class SourcesFile(object):
 
         m = LINE_PATTERN.match(stripped)
         if m is not None:
-            return SourceFileEntry(m.group('hashtype'), m.group('file'),
+            return self.entry_type(m.group('hashtype'), m.group('file'),
                                    m.group('hash'))
 
         # Try falling back on the old format
@@ -70,10 +72,10 @@ class SourcesFile(object):
         except ValueError:
             raise MalformedLineError(line)
 
-        return SourceFileEntry('md5', file, hash)
+        return self.entry_type('md5', file, hash)
 
     def add_entry(self, hashtype, file, hash):
-        entry = SourceFileEntry(hashtype, file, hash)
+        entry = self.entry_type(hashtype, file, hash)
 
         for e in self.entries:
             if entry.hashtype != e.hashtype:
@@ -97,10 +99,14 @@ class SourceFileEntry(object):
         self.file = file
 
     def __str__(self):
-        return '%s (%s) = %s\n' % (self.hashtype.upper(), self.file,
-                                   self.hash)
-
+        return '%s  %s\n' % (self.hash, self.file)
 
     def __eq__(self, other):
         return ((self.hashtype, self.hash, self.file) ==
                 (other.hashtype, other.hash, other.file))
+
+
+class BSDSourceFileEntry(SourceFileEntry):
+    def __str__(self):
+        return '%s (%s) = %s\n' % (self.hashtype.upper(), self.file,
+                                   self.hash)
diff --git a/test/test_sources.py b/test/test_sources.py
index 13f7c06..576bd2c 100644
--- a/test/test_sources.py
+++ b/test/test_sources.py
@@ -14,6 +14,11 @@ sys.path = old_path
 class SourceFileEntryTestCase(unittest.TestCase):
     def test_entry(self):
         e = sources.SourceFileEntry('md5', 'afile', 'ahash')
+        expected = 'ahash  afile\n'
+        self.assertEqual(str(e), expected)
+
+    def test_bsd_style_entry(self):
+        e = sources.BSDSourceFileEntry('md5', 'afile', 'ahash')
         expected = 'MD5 (afile) = ahash\n'
         self.assertEqual(str(e), expected)
 
@@ -27,22 +32,34 @@ class SourcesFileTestCase(unittest.TestCase):
         shutil.rmtree(self.workdir)
 
     def test_parse_empty_line(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
         entry = s.parse_line('')
         self.assertIsNone(entry)
 
     def test_parse_eol_line(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
         entry = s.parse_line('\n')
         self.assertIsNone(entry)
 
     def test_parse_whitespace_line(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
         entry = s.parse_line('    \n')
         self.assertIsNone(entry)
 
     def test_parse_old_style_line(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'old')
+
+        line = 'ahash  afile\n'
+        entry = s.parse_line(line)
+
+        self.assertTrue(isinstance(entry, sources.SourceFileEntry))
+        self.assertEqual(entry.hashtype, 'md5')
+        self.assertEqual(entry.hash, 'ahash')
+        self.assertEqual(entry.file, 'afile')
+        self.assertEqual(str(entry), line)
+
+    def test_migrate_old_style_line(self):
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
 
         line = 'ahash  afile\n'
         newline = 'MD5 (afile) = ahash\n'
@@ -55,7 +72,7 @@ class SourcesFileTestCase(unittest.TestCase):
         self.assertEqual(str(entry), newline)
 
     def test_parse_entry_line(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
 
         line = 'MD5 (afile) = ahash\n'
         entry = s.parse_line(line)
@@ -67,7 +84,7 @@ class SourcesFileTestCase(unittest.TestCase):
         self.assertEqual(str(entry), line)
 
     def test_parse_wrong_lines(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
 
         lines = ['ahash',
                  'ahash  ',
@@ -81,12 +98,12 @@ class SourcesFileTestCase(unittest.TestCase):
                 s.parse_line(line)
 
     def test_open_new_file(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
         self.assertEqual(len(s.entries), 0)
 
     def test_open_empty_file(self):
         open(self.sourcesfile, 'w').write('')
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
         self.assertEqual(len(s.entries), 0)
 
     def test_open_existing_file_with_old_style_lines(self):
@@ -98,7 +115,7 @@ class SourcesFileTestCase(unittest.TestCase):
             for line in lines:
                 f.write(line)
 
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
 
         for i, entry in enumerate(s.entries):
             self.assertTrue(isinstance(entry, sources.SourceFileEntry))
@@ -111,7 +128,7 @@ class SourcesFileTestCase(unittest.TestCase):
             for line in lines:
                 f.write(line)
 
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
 
         for i, entry in enumerate(s.entries):
             self.assertTrue(isinstance(entry, sources.SourceFileEntry))
@@ -132,7 +149,7 @@ class SourcesFileTestCase(unittest.TestCase):
             for line in lines:
                 f.write(line)
 
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
 
         for i, entry in enumerate(s.entries):
             self.assertTrue(isinstance(entry, sources.SourceFileEntry))
@@ -147,7 +164,7 @@ class SourcesFileTestCase(unittest.TestCase):
             for line in lines:
                 f.write(line)
 
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
 
         self.assertEqual(len(s.entries), 1)
         self.assertEqual(s.entries[0].hashtype, 'md5')
@@ -162,10 +179,10 @@ class SourcesFileTestCase(unittest.TestCase):
             f.write(line)
 
         with self.assertRaises(sources.MalformedLineError):
-            return sources.SourcesFile(self.sourcesfile)
+            return sources.SourcesFile(self.sourcesfile, 'bsd')
 
     def test_add_entry(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
         self.assertEqual(len(s.entries), 0)
 
         s.add_entry('md5', 'afile', 'ahash')
@@ -177,7 +194,7 @@ class SourcesFileTestCase(unittest.TestCase):
         self.assertEqual(str(s.entries[-1]), 'MD5 (anotherfile) =
anotherhash\n')
 
     def test_add_entry_twice(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
         self.assertEqual(len(s.entries), 0)
 
         s.add_entry('md5', 'afile', 'ahash')
@@ -188,7 +205,7 @@ class SourcesFileTestCase(unittest.TestCase):
         self.assertEqual(len(s.entries), 1)
 
     def test_add_entry_mixing_hashtypes(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
         self.assertEqual(len(s.entries), 0)
 
         s.add_entry('md5', 'afile', 'ahash')
@@ -199,7 +216,7 @@ class SourcesFileTestCase(unittest.TestCase):
             s.add_entry('sha512', 'anotherfile', 'anotherhash')
 
     def test_write_new_file(self):
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
         self.assertEqual(len(s.entries), 0)
 
         s.add_entry('md5', 'afile', 'ahash')
@@ -220,7 +237,7 @@ class SourcesFileTestCase(unittest.TestCase):
             for line in lines:
                 f.write(line)
 
-        s = sources.SourcesFile(self.sourcesfile)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd')
         s.add_entry('md5', 'thirdfile', 'thirdhash')
         s.write()
 
@@ -239,7 +256,7 @@ class SourcesFileTestCase(unittest.TestCase):
             for line in lines:
                 f.write(line)
 
-        s = sources.SourcesFile(self.sourcesfile, replace=True)
+        s = sources.SourcesFile(self.sourcesfile, 'bsd', replace=True)
         s.add_entry('md5', 'thirdfile', 'thirdhash')
         s.write()
 
-- 
2.1.0

[PATCH 3/3] tests: Properly open/close the file

[Mathieu Bridon]

From: Mathieu Bridon <bochecha(a)daitauha.fr&gt;

In addition to being just cleaner, this removes the only warning when
running the tests for the pyrpkg.sources module with Python3.

Not that we're anywhere ready to have pyrpkg run with Python3, but any
little step in that direction is a good thing.

Plus it means pyrpkg.sources works fine with both Python2 and Python3!
---
 test/test_sources.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/test/test_sources.py b/test/test_sources.py
index 576bd2c..747abe2 100644
--- a/test/test_sources.py
+++ b/test/test_sources.py
@@ -102,7 +102,9 @@ class SourcesFileTestCase(unittest.TestCase):
         self.assertEqual(len(s.entries), 0)
 
     def test_open_empty_file(self):
-        open(self.sourcesfile, 'w').write('')
+        with open(self.sourcesfile, 'w') as f:
+            f.write('')
+
         s = sources.SourcesFile(self.sourcesfile, 'bsd')
         self.assertEqual(len(s.entries), 0)
 
-- 
2.1.0

[PATCH 4/4] Don't do several times the same thing

[Mathieu Bridon]

On Sat, 2015-03-07 at 16:20 +0800, Mathieu Bridon wrote:
>  On Fri, 2015-03-06 at 19:35 +0100, Pavol Babincak wrote:
>  > We'll need to take different approach then. By default pyrpkg will be 
>  > backwards compatible and write sources in old format with md5 hash. 
>  > pyrpkg's clients (e.g. fedpkg) will have a way to switch to new sources 
>  > file format if their buildsystem infrastructure is prepared for that.
>  > 
>  > In Fedora this means I need to finish my implementation of new sources 
>  > format in fedpkg-minimal and only after this package will be in all 
>  > supported buildroots we can switch fedpkg's default behaviour.
>  > 
>  > I've untagged rpkg build from rawhide so developers won't rewrite their 
>  > sources files in the way which Koji buildroots doesn't understand yet.
>  
>  Seems like the best for now.
>  
>  I'll send new patches that implement what you described above. 
Patch 2/3 in the new series I just sent does just that:

 - pyrpkg.sources can read/write sources files both in the
   current format and the new (BSD-style) format
 - pyrpkg defaults to writing sources files in the current
   format, to preserve compatibility
 - pyrpkg-based applications (like fedpkg) can override this
   easily if they want to move to writing sources files in
   the new (BSD-style) format

I believe with these patches merged you could make a new release of
rpkg, build it and update it for Fedora in all branches (including
EPEL), and nothing would change... except we'd now have the possibility
to eventually move to the new format whenever we decided to.

We should even be able to only move to the new format in newer Fedora
only if we wanted to, but it could be confusing, as the sources file
would be written in a given format based on the Fedora release the
packager is running, not the target Fedora branch for the build.

But it would all work nevertheless, as long as pyrpkg had been updated
in all branches.


-- 
Mathieu

[PATCH 4/4] Don't do several times the same thing

[Pavol Babincak]

On 03/19/2015 12:16 PM, Mathieu Bridon wrote:
>  On Sat, 2015-03-07 at 16:20 +0800, Mathieu Bridon wrote:
> > On Fri, 2015-03-06 at 19:35 +0100, Pavol Babincak wrote:
> >> We'll need to take different approach then. By default pyrpkg will be
> >> backwards compatible and write sources in old format with md5 hash.
> >> pyrpkg's clients (e.g. fedpkg) will have a way to switch to new sources
> >> file format if their buildsystem infrastructure is prepared for that.
> >>
> >> In Fedora this means I need to finish my implementation of new sources
> >> format in fedpkg-minimal and only after this package will be in all
> >> supported buildroots we can switch fedpkg's default behaviour.
> >>
> >> I've untagged rpkg build from rawhide so developers won't rewrite their
> >> sources files in the way which Koji buildroots doesn't understand yet.
> >
> > Seems like the best for now.
> >
> > I'll send new patches that implement what you described above.
>
>  Patch 2/3 in the new series I just sent does just that:
>
>    - pyrpkg.sources can read/write sources files both in the
>      current format and the new (BSD-style) format
>    - pyrpkg defaults to writing sources files in the current
>      format, to preserve compatibility
>    - pyrpkg-based applications (like fedpkg) can override this
>      easily if they want to move to writing sources files in
>      the new (BSD-style) format 
Patches looked fine and tests passed so merged and
released new rpkg  - 
1.32.

rpkg built in rawhide by now. I'll do the rest later. Hopefully this week.
-- 
Pavol Babincak
Release Engineering, Red Hat

[Mike McLean]

On 02/16/2015 10:33 PM, Mathieu Bridon wrote:
>  As part of the move away from MD5 for the source files hashes in the
> lookaside
>  cache, we have decided to change the format of the 'sources' files, so it
>  includes the hash type, to make it easier to identify which hash was used.
>
>  We agreed on using the BSD-style format, as returned on Linux by passing the
>  extra --tag option to commands like md5sum, sha512sumn, etc.
>
>  In addition, we decided to forbid mixing hash types in a single sources file,
>  forcing maintainers to use `fedpkg new-sources` the first time they upload an
>  additional source file. 
are these changes backwards compatible?

>  This 4 patches series implements all that, with loads of unit tests,
> and some
>  code cleanup.
>
>    src/pyrpkg/__init__.py |  60 ++++-----
>    src/pyrpkg/sources.py  | 123 ++++++++++++------
>    test/test_sources.py   | 334 ++++++++++++++++++++++++++++++++++--------------
>    3 files changed, 359 insertions(+), 158 deletions(-)
>
>  https://fedorahosted.org/rel-eng/ticket/5846
>
>  --
>  buildsys mailing list
>  buildsys(a)lists.fedoraproject.org
>  https://admin.fedoraproject.org/mailman/listinfo/buildsys
>

[Mathieu Bridon]

On Thu, 2015-02-19 at 19:21 -0500, Mike McLean wrote:
>  On 02/16/2015 10:33 PM, Mathieu Bridon wrote:
>  > As part of the move away from MD5 for the source files hashes in the lookaside
>  > cache, we have decided to change the format of the 'sources' files, so it
>  > includes the hash type, to make it easier to identify which hash was used.
>  >
>  > We agreed on using the BSD-style format, as returned on Linux by passing the
>  > extra --tag option to commands like md5sum, sha512sumn, etc.
>  >
>  > In addition, we decided to forbid mixing hash types in a single sources file,
>  > forcing maintainers to use `fedpkg new-sources` the first time they upload an
>  > additional source file.
>  
>  are these changes backwards compatible? 
I always get confused with the meaning of "backwards" vs "forward"
compatible, so let me state both. :)

Older rpkg will not work with files in the new format, they will just
raise exceptions trying to read them.

However, after being patched, rpkg still will be able to read files in
the old format. In fact, that's how files get converted to the new
format.

Does that answer your question?


-- 
Mathieu