Merge pull request #172 from aplanas/master

A draft for the local cache (not properly tested) and some tests

osc-check_repo.py

@@ -29,15 +29,11 @@ from osc.core import Request
 # Expand sys.path to search modules inside the pluging directory
 _plugin_dir = os.path.expanduser('~/.osc-plugins')
 sys.path.append(_plugin_dir)
-from osclib.checkrepo import CheckRepo
+from osclib.checkrepo import CheckRepo, DOWNLOADS
 from osclib.cycle import CycleDetector
 from osclib.memoize import CACHEDIR
 
 
-# Directory where download binary packages.
-DOWNLOADS = os.path.expanduser('~/co/downloads')
-
-
 def _check_repo_find_submit_request(self, opts, project, package):
     xpath = "(action/target/@project='%s' and "\
             "action/target/@package='%s' and "\
@@ -94,48 +90,19 @@ def _check_repo_get_binary(self, apiurl, prj, repo, arch, package, file, target,
     get_binary_file(apiurl, prj, repo, arch, file, package=package, target_filename=target)
 
 
-def _get_verifymd5(self, request, rev):
-    try:
-        url = makeurl(self.get_api_url(), ['source', request.src_project, request.src_package, '?view=info&rev=%s' % rev])
-        root = ET.parse(http_GET(url)).getroot()
-    except urllib2.HTTPError, e:
-        print 'ERROR in URL %s [%s]' % (url, e)
-        return []
-    return root.attrib['verifymd5']
-
-
-def _checker_compare_disturl(self, disturl, request):
-    distmd5 = os.path.basename(disturl).split('-')[0]
-    if distmd5 == request.srcmd5:
-        return True
-
-    vrev1 = self._get_verifymd5(request, request.srcmd5)
-    vrev2 = self._get_verifymd5(request, distmd5)
-    if vrev1 == vrev2:
-        return True
-    print 'ERROR Revision missmatch: %s, %s' % (vrev1, vrev2)
-    return False
-
-
 def _download_and_check_disturl(self, request, todownload, opts):
     for _project, _repo, arch, fn, mt in todownload:
         repodir = os.path.join(DOWNLOADS, request.src_package, _project, _repo)
         if not os.path.exists(repodir):
             os.makedirs(repodir)
         t = os.path.join(repodir, fn)
-        # print 'Downloading ...', _project, _repo, arch, request.src_package, fn, t, mt
         self._check_repo_get_binary(opts.apiurl, _project, _repo,
                                     arch, request.src_package, fn, t, mt)
 
         request.downloads[_repo].append(t)
         if fn.endswith('.rpm'):
-            pid = subprocess.Popen(['rpm', '--nosignature', '--queryformat', '%{DISTURL}', '-qp', t],
+            if not self.checkrepo.check_disturl(request, t):
-                                   stdout=subprocess.PIPE, close_fds=True)
+                request.error = '[#%s] DISTURL does not match revision %s' % (request.request_id, request.srcmd5)
-            os.waitpid(pid.pid, 0)[1]
-            disturl = pid.stdout.readlines()[0]
-
-            if not self._checker_compare_disturl(disturl, request):
-                request.error = '[%s] %s does not match revision %s' % (request, disturl, request.srcmd5)
 
 
 def _check_repo_download(self, request, opts):
@@ -324,8 +291,9 @@ def _check_repo_group(self, id_, requests, opts):
                 execution_plan[_repo].append((rq, _repo, rq.downloads[_repo]))
             else:
                 _other_repo = [r for r in rq.downloads if r != _repo]
-                _other_repo = _other_repo[0]  # XXX TODO - Recurse here to create combinations
+                if _other_repo:
-                execution_plan[_repo].append((rq, _other_repo, rq.downloads[_other_repo]))
+                    _other_repo = _other_repo[0]  # XXX TODO - Recurse here to create combinations
+                    execution_plan[_repo].append((rq, _other_repo, rq.downloads[_other_repo]))
 
     repo_checker_error = ''
     for _repo, dirstolink in execution_plan.items():
@@ -402,6 +370,12 @@ def mirror_full(plugin_dir, repo_dir):
     os.system(script)
 
 
+def _print_request_and_specs(self, request_and_specs):
+    print request_and_specs[0]
+    for spec in request_and_specs[1:]:
+        print ' * ', spec
+
+
 @cmdln.alias('check', 'cr')
 @cmdln.option('-s', '--skip', action='store_true', help='skip review')
 def do_check_repo(self, subcmd, opts, *args):
@@ -442,11 +416,15 @@ def do_check_repo(self, subcmd, opts, *args):
     if not ids:
         # Return a list, we flat here with .extend()
         for request in self.checkrepo.pending_requests():
-            requests.extend(self.checkrepo.check_specs(request=request))
+            request_and_specs = self.checkrepo.check_specs(request=request)
+            self._print_request_and_specs(request_and_specs)
+            requests.extend(request_and_specs)
     else:
         # We have a list, use them.
         for request_id in ids:
-            requests.extend(self.checkrepo.check_specs(request_id=request_id))
+            request_and_specs = self.checkrepo.check_specs(request_id=request_id)
+            self._print_request_and_specs(request_and_specs)
+            requests.extend(request_and_specs)
 
     # Order the packs before grouping
     requests = sorted(requests, key=lambda p: p.request_id, reverse=True)

osclib/checkrepo.py

@@ -14,6 +14,8 @@
 # with this program; if not, write to the Free Software Foundation, Inc.,
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 
+import os
+import subprocess
 from urllib import quote_plus
 import urllib2
 from xml.etree import cElementTree as ET
@@ -25,6 +27,10 @@ from osclib.stagingapi import StagingAPI
 from osclib.memoize import memoize
 
 
+# Directory where download binary packages.
+DOWNLOADS = os.path.expanduser('~/co/downloads')
+
+
 class Request(object):
     """Simple request container."""
 
@@ -70,11 +76,11 @@ class Request(object):
         self.missings = []
 
     def __repr__(self):
-        return 'SUBMIT(%s) %s/%s -> %s/%s' % (self.request_id,
+        return '#%s %s/%s -> %s/%s' % (self.request_id,
-                                              self.src_project,
+                                       self.src_project,
-                                              self.src_package,
+                                       self.src_package,
-                                              self.tgt_project,
+                                       self.tgt_project,
-                                              self.tgt_package)
+                                       self.tgt_package)
 
 
 class CheckRepo(object):
@@ -120,7 +126,7 @@ class CheckRepo(object):
         }
 
         code = 404
-        url = makeurl(self.apiurl, ['request', str(request_id)], query=query)
+        url = makeurl(self.apiurl, ('request', str(request_id)), query=query)
         try:
             root = ET.parse(http_POST(url, data=message)).getroot()
             code = root.attrib['code']
@@ -152,7 +158,7 @@ class CheckRepo(object):
         return requests
 
     @memoize()
-    def build(self, repository, project, package, arch):
+    def build(self, project, repository, arch, package):
         """Return the build XML document from OBS."""
         xml = ''
         try:
@@ -280,16 +286,19 @@ class CheckRepo(object):
             return requests
 
         rq = Request(element=request)
-        print rq
         rq.group = self.grouped.get(request_id, request_id)
         requests.append(rq)
 
         # Get source information about the SR:
         #   - Source MD5
         #   - Entries (.tar.gz, .changes, .spec ...) and MD5
+        query = {
+            'rev': rq.revision,
+            'expand': 1
+        }
         try:
             url = makeurl(self.apiurl, ['source', rq.src_project, rq.src_package],
-                          {'rev': rq.revision, 'expand': 1})
+                          query=query)
             root = ET.parse(http_GET(url)).getroot()
         except urllib2.HTTPError, e:
             print 'ERROR in URL %s [%s]' % (url, e)
@@ -323,8 +332,13 @@ class CheckRepo(object):
         #  - with the name of the .spec file.
 
         for spec in specs:
-            spec_info = self.staging.get_package_information(rq.src_project,
+            try:
-                                                             spec)
+                spec_info = self.staging.get_package_information(rq.src_project,
+                                                                 spec)
+            except urllib2.HTTPError as e:
+                print "Can't gather package information for (%s, %s)" % (rq.src_project, spec)
+                rq.updated = True
+                continue
 
             if (spec_info['project'] != rq.src_project
                or spec_info['package'] != rq.src_package) and not rq.updated:
@@ -384,15 +398,71 @@ class CheckRepo(object):
 
         return repos_to_check
 
-    def is_binary(self, repository, project, package, arch):
+    def is_binary(self, project, repository, arch, package):
         """Return True if is a binary package."""
-        root_xml = self.build(repository, project, package, arch)
+        root_xml = self.build(project, repository, arch, package)
         root = ET.fromstring(root_xml)
         for binary in root.findall('binary'):
             # If there are binaries, we're out.
             return False
         return True
 
+    def _disturl(self, filename):
+        """Get the DISTURL from a RPM file."""
+        pid = subprocess.Popen(('rpm', '--nosignature', '--queryformat', '%{DISTURL}', '-qp', filename),
+                               stdout=subprocess.PIPE, close_fds=True)
+        os.waitpid(pid.pid, 0)[1]
+        disturl = pid.stdout.readlines()[0]
+        return disturl
+        return os.path.basename(disturl).split('-')[0]
+
+    def _md5_disturl(self, disturl):
+        """Get the md5 from the DISTURL from a RPM file."""
+        return os.path.basename(disturl).split('-')[0]
+
+    def _get_verifymd5(self, request, revision):
+        """Return the verifymd5 attribute from a request."""
+        query = {
+            'view': 'info',
+            'rev': revision,
+        }
+        try:
+            url = makeurl(self.apiurl, ('source', request.src_project, request.src_package),
+                          query=query)
+            root = ET.parse(http_GET(url)).getroot()
+        except urllib2.HTTPError, e:
+            print 'ERROR in URL %s [%s]' % (url, e)
+            return []
+        return root.attrib['verifymd5']
+
+    def check_disturl(self, request, filename):
+        """Try to match the srcmd5 of a request with the one in the RPM package."""
+        disturl = self._disturl(filename)
+        md5_disturl = self._md5_disturl(disturl)
+
+        if md5_disturl == request.srcmd5:
+            return True
+
+        vrev1 = self._get_verifymd5(request, request.srcmd5)
+        vrev2 = self._get_verifymd5(request, md5_disturl)
+        if vrev1 == vrev2:
+            return True
+
+        return False
+
+    def is_request_cached(self, request):
+        """Search the request in the local cache."""
+        result = False
+
+        package_dir = os.path.join(DOWNLOADS, request.src_package)
+        rpm_packages = []
+        for dirpath, dirnames, filenames in os.walk(package_dir):
+            rpm_packages.extend(os.path.join(dirpath, f) for f in filenames if f.endswith('.rpm'))
+
+        result = any(self.check_disturl(request, rpm) for rpm in rpm_packages)
+
+        return result
+
     def is_buildsuccess(self, request):
         """Return True if the request is correctly build
 
@@ -433,10 +503,10 @@ class CheckRepo(object):
                 if 'missing' in arch.attrib:
                     for package in arch.attrib['missing'].split(','):
                         if not self.is_binary(
-                                repository.attrib['name'],
                                 request.src_project,
-                                package,
+                                repository.attrib['name'],
-                                arch.attrib['arch']):
+                                arch.attrib['arch'],
+                                package):
                             missings[package] = 1
                 if arch.attrib['result'] not in ('succeeded', 'excluded'):
                     isgood = False
@@ -484,13 +554,18 @@ class CheckRepo(object):
             # Next line not needed, but for documentation
             request.updated = True
             return False
+
         if foundbuilding:
             msg = '%s is still building for repository %s' % (request.src_package, foundbuilding)
             print msg
-            self.change_review_state(request.request_id, 'new', message=msg)
+            if self.is_request_cached(request):
-            # Next line not needed, but for documentation
+                print 'Found cached version.'
-            request.updated = True
+            else:
-            return False
+                self.change_review_state(request.request_id, 'new', message=msg)
+                # Next line not needed, but for documentation
+                request.updated = True
+                return False
+
         if foundfailed:
             msg = '%s failed to build in repository %s - not accepting' % (request.src_package, foundfailed)
             # failures might be temporary, so don't autoreject but wait for a human to check

tests/checkrepo_tests.py

@@ -0,0 +1,66 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+#
+# Copyright (C) 2014 SUSE Linux Products GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+import unittest
+
+from obs import APIURL
+from obs import OBS
+from osclib.checkrepo import CheckRepo
+
+
+class TestCheckRepoCalls(unittest.TestCase):
+    """Tests for various check repo calls."""
+
+    def setUp(self):
+        """Initialize the configuration."""
+
+        self.obs = OBS()
+        self.checkrepo = CheckRepo(APIURL)
+
+    def test_packages_grouping(self):
+        """Validate the creation of the groups."""
+        grouped = {
+            1000: 'openSUSE:Factory:Staging:J',
+            1001: 'openSUSE:Factory:Staging:J',
+            501: 'openSUSE:Factory:Staging:C',
+            502: 'openSUSE:Factory:Staging:C',
+            333: 'openSUSE:Factory:Staging:B'
+        }
+        groups = {
+            'openSUSE:Factory:Staging:J': [1000, 1001],
+            'openSUSE:Factory:Staging:C': [501, 502],
+            'openSUSE:Factory:Staging:B': [333]
+        }
+        self.assertEqual(self.checkrepo.grouped, grouped)
+        self.assertEqual(self.checkrepo.groups, groups)
+
+    def test_pending_request(self):
+        """Test CheckRepo.get_request."""
+        self.assertEqual(len(self.checkrepo.pending_requests()), 2)
+
+    def test_check_specs(self):
+        """Test CheckRepo.check_specs."""
+        for request in self.checkrepo.pending_requests():
+            request_and_specs = self.checkrepo.check_specs(request=request)
+            self.assertEqual(len(request_and_specs), 1)
+            self.assertTrue(request_and_specs[0].request_id in (1000, 1001))
+        for request_id in (1000, 1001):
+            request_and_specs = self.checkrepo.check_specs(request_id=request_id)
+            self.assertEqual(len(request_and_specs), 1)
+            self.assertEqual(request_and_specs[0].request_id, request_id)

tests/fixtures/request/1000

@@ -0,0 +1 @@
+template_request.xml

tests/fixtures/request/1001

@@ -0,0 +1 @@
+template_request.xml

tests/fixtures/source/home:Admin/apparmor

@@ -1 +1 @@
-wine
+source.xml

tests/fixtures/source/home:Admin/emacs

@@ -0,0 +1 @@
+source_expanded.xml

tests/fixtures/source/home:Admin/gcc

@@ -1 +1 @@
-wine
+source.xml

tests/fixtures/source/home:Admin/mariadb

@@ -1 +1 @@
-wine
+source.xml

tests/fixtures/source/home:Admin/puppet

@@ -1 +1 @@
-wine
+source.xml

tests/fixtures/source/home:Admin/python

@@ -0,0 +1 @@
+source_expanded.xml

tests/fixtures/source/home:Admin/source.xml

@@ -0,0 +1 @@
+<directory name="${name}" rev="${rev}" vrev="${vrev}" srcmd5="${srcmd5}"/>

tests/fixtures/source/home:Admin/source_expanded.xml

@@ -0,0 +1,4 @@
+<directory name="${name}" rev="${rev}" vrev="${vrev}" srcmd5="${srcmd5}">
+  <linkinfo project="openSUSE:Factory" package="${name}" srcmd5="" baserev="" lsrcmd5=""/>
+  <entry name="${name}.spec" md5="" size="" mtime=""/>
+</directory>

tests/fixtures/source/home:Admin/wine

@@ -1 +0,0 @@
-<directory name="${name}" rev="${rev}" vrev="${vrev}" srcmd5="${srcmd5}"/>

tests/fixtures/source/home:Admin/wine

@@ -0,0 +1 @@
+source.xml

tests/fixtures/source/openSUSE:Factory:Staging:J/_meta

@@ -0,0 +1 @@
+../openSUSE:Factory:Staging:A/_meta

tests/fixtures/source/openSUSE:Factory:Staging:J/_project

@@ -0,0 +1 @@
+../openSUSE:Factory:Staging:A/_project

tests/fixtures/source/openSUSE:Factory:Staging:J/emacs

@@ -0,0 +1 @@
+../linksource.xml

tests/fixtures/source/openSUSE:Factory:Staging:J/python

@@ -0,0 +1 @@
+../linksource.xml

tests/obs.py

@@ -159,6 +159,24 @@ class OBS(object):
                 'by_who': 'openSUSE:Factory:Staging:C',
                 'package': 'mariadb',
             },
+            '1000': {
+                'request': 'review',
+                'review': 'new',
+                'who': 'Admin',
+                'by': 'user',
+                'id': '1000',
+                'by_who': 'factory-repo-checker',
+                'package': 'emacs',
+            },
+            '1001': {
+                'request': 'review',
+                'review': 'new',
+                'who': 'Admin',
+                'by': 'user',
+                'id': '1001',
+                'by_who': 'factory-repo-checker',
+                'package': 'python',
+            },
         }
 
         self.staging_project = {
@@ -180,8 +198,14 @@ class OBS(object):
             'C': {
                 'project': 'openSUSE:Factory:Staging:C',
                 'title': 'A project ready to be accepted',
-                'description': ("requests:\n - {id: 501, package: apparmor, author: Admin}\n"
+                'description': ('requests:\n- {id: 501, package: apparmor, author: Admin}\n'
-                                " - {id: 502, package: mariadb, author: Admin}"),
+                                '- {id: 502, package: mariadb, author: Admin}'),
+            },
+            'J': {
+                'project': 'openSUSE:Factory:Staging:J',
+                'title': 'A project to be checked',
+                'description': ('requests:\n- {id: 1000, package: emacs, author: Admin}\n'
+                                '- {id: 1001, package: python, author: Admin}'),
             },
         }
 
@@ -201,6 +225,16 @@ class OBS(object):
                 'pkg': 'mariadb',
                 'devprj': 'home:Admin',
             },
+            'openSUSE:Factory:Staging:J/emacs': {
+                'prj': 'openSUSE:Factory:Staging:J',
+                'pkg': 'emacs',
+                'devprj': 'home:Admin',
+            },
+            'openSUSE:Factory:Staging:J/python': {
+                'prj': 'openSUSE:Factory:Staging:J',
+                'pkg': 'python',
+                'devprj': 'home:Admin',
+            },
         }
 
         self.meta = {}
@@ -272,6 +306,18 @@ class OBS(object):
                 'name': 'mariadb',
                 'srcmd5': 'de7a9f5e3bedb01980465f3be3d236cb',
             },
+            'home:Admin/emacs': {
+                'rev': '1',
+                'vrev': '1',
+                'name': 'emacs',
+                'srcmd5': 'de7a9f5e3bedb01980465f3be3d236cb',
+            },
+            'home:Admin/python': {
+                'rev': '1',
+                'vrev': '1',
+                'name': 'python',
+                'srcmd5': 'de7a9f5e3bedb01980465f3be3d236cb',
+            },
         }
 
         self.comments = {
@@ -295,6 +341,7 @@ class OBS(object):
                              " * Request#502 for package mariadb submitted by Admin\n")
                 }
             ],
+            'openSUSE:Factory:Staging:J': [],
         }
 
         # To track comments created during test execution, even if they have
@@ -397,7 +444,7 @@ class OBS(object):
     # /source/
     #
 
-    @GET(re.compile(r'/source/openSUSE:Factory:Staging:[A|B|C]/_project'))
+    @GET(re.compile(r'/source/openSUSE:Factory:Staging:[A|B|C|J]/_project'))
     def source_staging_project_project(self, request, uri, headers):
         """Return the _project information for a staging project."""
         # Load the proper fixture and adjust mtime according the
@@ -419,7 +466,7 @@ class OBS(object):
 
         return response
 
-    @GET(re.compile(r'/source/openSUSE:Factory:Staging:[A|B|C|U](/\w+)?/_meta'))
+    @GET(re.compile(r'/source/openSUSE:Factory:Staging:[A|B|C|U|J](/\w+)?/_meta'))
     def source_staging_project_meta(self, request, uri, headers):
         """Return the _meta information for a staging project."""
         key = re.search(r'openSUSE:Factory:Staging:(\w(?:/\w+)?)/_meta', uri).group(1)
@@ -441,7 +488,7 @@ class OBS(object):
 
         return response
 
-    @PUT(re.compile(r'/source/openSUSE:Factory:Staging:[A|B|C|U](/\w+)?/_meta'))
+    @PUT(re.compile(r'/source/openSUSE:Factory:Staging:[A|B|C|U|J](/\w+)?/_meta'))
     def put_source_staging_project_meta(self, request, uri, headers):
         """Set the _meta information for a staging project."""
         key = re.search(r'openSUSE:Factory:Staging:(\w(?:/\w+)?)/_meta', uri).group(1)
@@ -490,7 +537,7 @@ class OBS(object):
 
         return response
 
-    @GET(re.compile(r'/source/home:Admin/\w+'))
+    @GET(re.compile(r'/source/home:Admin/\w+(\?rev=\w+&expand=1)?'))
     def source_project(self, request, uri, headers):
         """Return information of a source package."""
         package = re.search(r'/source/([\w:]+/\w+)', uri).group(1)
@@ -590,15 +637,21 @@ class OBS(object):
     def search_request(self, request, uri, headers):
         """Return a search result for /search/request."""
         query = urlparse.urlparse(uri).query
-        assert query == "match=state/@name='review'+and+review[@by_group='factory-staging'+and+@state='new']"
+        assert query in (
+            "match=state/@name='review'+and+review[@by_group='factory-staging'+and+@state='new']",
+            "match=state/@name='review'+and+review[@by_user='factory-repo-checker'+and+@state='new']"
+        )
 
         response = (404, headers, '<result>Not found</result>')
 
+        by, by_who = re.search(r"@by_(user|group)='([-\w]+)'", query).groups()
+        state = re.search(r"@state='(\w+)'", query).group(1)
+
         requests = [rq for rq in self.requests.values()
                     if rq['request'] == 'review'
-                    and rq['review'] == 'new'
+                    and rq['review'] == state
-                    and rq['by'] == 'group'
+                    and rq['by'] == by
-                    and rq['by_who'] == 'factory-staging']
+                    and rq['by_who'] == by_who]
 
         try:
             _requests = '\n'.join(self._request(rq['id']) for rq in requests)