Merge pull request #2769 from coolo/split_source_checker

Move remains of check_source.pl into python

CONTENTS.md

@@ -263,7 +263,7 @@ Inspects built RPMs from staging projects.
 Checks for usual mistakes and problems in the source packages submitted by users. Used also as
 review bot that assigns reviews (?).
 
-* Sources: [check_source.py](check_source.py) and [check_source.pl](check_source.pl)
+* Sources: [check_source.py](check_source.py)
 * Documentation: [docs/check_source.asciidoc](docs/check_source.asciidoc)
 * Package: openSUSE-release-tools-check-source
 * Usage: [gocd](https://github.com/openSUSE/openSUSE-release-tools/search?q=path%3A%2Fgocd+check_source)

check_source.pl

@@ -1,138 +0,0 @@
-#! /usr/bin/perl
-
-use File::Basename;
-use File::Temp qw/ tempdir  /;
-use Cwd;
-use Text::Diff;
-BEGIN {
-    unshift @INC, ($::ENV{'BUILD_DIR'} || '/usr/lib/build');
-}
-use Build;
-
-my $ret = 0;
-
-my $old = $ARGV[0];
-my $dir = $ARGV[1];
-my $bname = basename($dir);
-
-my %patches = ();
-
-
-my $odir = getcwd();
-
-chdir($dir) || die "chdir $dir failed";
-for my $patch (glob("*.diff *.patch *.dif")) {
-    $patches{$patch} = 'current';
-}
-chdir($odir) || die "chdir $odir failed";
-
-if (-d "$old") {
-
-    chdir($old) || die "chdir $old failed";
-    my $cf = Build::read_config("x86_64", "/usr/lib/build/configs/default.conf");
-
-    my %thash = ();
-    for my $spec (glob("*.spec")) {
-        my $ps = Build::Rpm::parse($cf, $spec);
-
-        while (my ($k, $v) = each %$ps) {
-            if ($k =~ m/^source/) {
-                $thash{$v} = 1;
-            }
-        }
-    }
-    for my $patch (glob("*.diff *.patch *.dif")) {
-        if ($patches{$patch}) {
-            delete $patches{$patch};
-        }
-        else {
-            $patches{$patch} = 'old';
-        }
-    }
-
-    chdir($odir) || die "chdir $odir failed";
-    chdir($dir) || die "chdir $dir failed";
-    for my $spec (glob("*.spec")) {
-        my $ps = Build::Rpm::parse($cf, $spec);
-        open(OSPEC, "$spec");
-        open(NSPEC, ">$spec.new");
-        while (<OSPEC>) {
-            chomp;
-            if (m/^Source/) {
-                my $line = $_;
-                $line =~ s/^(Source[0-9]*)\s*:\s*//;
-                if ($patches{$line}) {
-                   delete $patches{$line};
-                }
-                my $prefix = $1;
-                my $parsedline = $ps->{lc $prefix};
-                if (defined $thash{$parsedline}) {
-                    my $file = $line;
-                    my $bname = basename($file);
-                    print NSPEC "$prefix: $bname\n";
-                }
-                else {
-                    print NSPEC "$_\n";
-                }
-            }
-            else {
-                print NSPEC "$_\n";
-            }
-        }
-        close(OSPEC);
-        close(NSPEC);
-        system(("cp", "$spec", "$spec.beforeurlstrip"));
-        rename("$spec.new", "$spec") || die "rename failed";
-    }
-
-    chdir($dir);
-    my @changes = glob("*.changes");
-    chdir($odir);
-
-    if (%patches) {
-        # parse changes
-        for my $changes (@changes) {
-            my $diff = "";
-            if (! -e "$old/$changes") {
-                $diff = diff "/dev/null", "$dir/$changes";
-            }
-            else {
-                $diff = diff "$old/$changes", "$dir/$changes";
-            }
-            for my $line (split(/\n/, $diff)) {
-                # Check if the line mentions a patch being added (starts with +)
-                # or removed (starts with -)
-                next unless $line =~ m/^[+-]/;
-                # In any of those cases, remove the patch from the list
-                $line =~ s/^[+-]//;
-                for my $patch (keys %patches) {
-                    if (index($line, $patch) != -1) {
-                        delete $patches{$patch};
-                    }
-                }
-            }
-        }
-    }
-    # still some left?
-    if (%patches) {
-        $ret = 1;
-        for my $patch (keys %patches) {
-            # wording stolen from Raymond's declines :)
-            if ($patches{$patch} eq 'current') {
-                print "A patch ($patch) is being added without this addition being mentioned in the changelog.\n";
-            }
-            else {
-                print "A patch ($patch) is being deleted without this removal being mentioned in the changelog.\n";
-            }
-        }
-    }
-}
-
-my $tmpdir = tempdir("obs-XXXXXXX", TMPDIR => 1, CLEANUP => 1);
-chdir($dir) || die 'tempdir failed';
-if (system("/usr/lib/obs/service/download_files","--enforceupstream", "yes", "--enforcelocal", "yes", "--outdir", $tmpdir)) {
-    print "Source URLs are not valid. Try \"osc service runall download_files\".\n";
-    $ret = 2;
-}
-
-exit($ret);

check_source.py

@@ -1,17 +1,18 @@
 #!/usr/bin/python3
 
+import difflib
 import glob
 import os
 import re
 import shutil
 import subprocess
 import sys
+import tempfile
 
 from lxml import etree as ET
 
 import osc.conf
 import osc.core
-from osc.util.helper import decode_list
 from osclib.conf import Config
 from osclib.core import devel_project_get
 from osclib.core import devel_project_fallback
@@ -182,13 +183,11 @@ class CheckSource(ReviewBot.ReviewBot):
         os.makedirs(dir)
         os.chdir(dir)
 
-        old_info = {'version': None}
         try:
             CheckSource.checkout_package(self.apiurl, target_project, target_package, pathname=dir,
                                          server_service_files=True, expand_link=True)
             shutil.rmtree(os.path.join(target_package, '.osc'))
             os.rename(target_package, '_old')
-            old_info = self.package_source_parse(target_project, target_package)
         except HTTPError as e:
             if e.code == 404:
                 self.logger.info('target package does not exist %s/%s' % (target_project, target_package))
@@ -225,39 +224,18 @@ class CheckSource(ReviewBot.ReviewBot):
         if not self.run_source_validator('_old', target_package):
             return False
 
-        # Run check_source.pl script and interpret output.
-        source_checker = os.path.join(CheckSource.SCRIPT_PATH, 'check_source.pl')
-        civs = ''
-        new_version = None
-        if old_info['version'] and old_info['version'] != new_info['version']:
-            new_version = new_info['version']
-            civs += "NEW_VERSION='{}' ".format(new_version)
-        civs += 'LC_ALL=C perl %s _old %s 2>&1' % (source_checker, target_package)
-        p = subprocess.Popen(civs, shell=True, stdout=subprocess.PIPE, close_fds=True)
-        ret = os.waitpid(p.pid, 0)[1]
-        checked = decode_list(p.stdout.readlines())
+        if not self.detect_mentioned_patches('_old', target_package, specs):
+            return False
 
-        output = '  '.join(checked).replace('\033', '')
-        os.chdir('/tmp')
-
-        # ret = 0 : Good
-        # ret = 1 : Bad
-        # ret = 2 : Bad but can be non-fatal in some cases
-        if ret > 1 and target_project.startswith('openSUSE:Leap:') and (source_project.startswith('SUSE:SLE-15:') or
-                                                                        source_project.startswith('openSUSE:Factory')):
-            pass
-        elif ret != 0:
-            shutil.rmtree(dir)
-            self.review_messages['declined'] = "Output of check script:\n" + output
+        if not self.check_urls('_old', target_package, specs):
             return False
 
         shutil.rmtree(dir)
         self.review_messages['accepted'] = 'Check script succeeded'
 
-        if len(checked):
-            self.review_messages['accepted'] += "\n\nOutput of check script (non-fatal):\n" + output
+        if self.skip_add_reviews:
+            return True
 
-        if not self.skip_add_reviews:
         if self.add_review_team and self.review_team is not None:
             self.add_review(self.request, by_group=self.review_team, msg='Please review sources')
 
@@ -574,6 +552,128 @@ class CheckSource(ReviewBot.ReviewBot):
 
         return True
 
+    def _snipe_out_existing_urls(self, old, directory, specs):
+        if not os.path.isdir(old):
+            return
+        oldsources = self._mentioned_sources(old, specs)
+        for spec in specs:
+            specfn = os.path.join(directory, spec)
+            nspecfn = specfn + '.new'
+            wf = open(nspecfn, 'w')
+            with open(specfn) as rf:
+                for line in rf:
+                    m = re.match(r'(Source[0-9]*\s*):\s*(.*)$', line)
+                    print(line, m)
+                    if m and m.group(2) in oldsources:
+                        wf.write(m.group(1) + ":" + os.path.basename(m.group(2)) + "\n")
+                        continue
+                    wf.write(line)
+            wf.close()
+            os.rename(nspecfn, specfn)
+
+    def check_urls(self, old, directory, specs):
+        self._snipe_out_existing_urls(old, directory, specs)
+        oldcwd = os.getcwd()
+        with tempfile.TemporaryDirectory() as tmpdir:
+            os.chdir(directory)
+            res = subprocess.run(["/usr/lib/obs/service/download_files", "--enforceupstream",
+                                  "yes", "--enforcelocal", "yes", "--outdir", tmpdir], stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+            if res.returncode:
+                self.review_messages['declined'] = "Source URLs are not valid. Try `osc service runall download_files`.\n" + \
+                    res.stdout.decode('utf-8')
+                os.chdir(oldcwd)
+                return False
+        os.chdir(oldcwd)
+        return True
+
+    def difflines(self, oldf, newf):
+        with open(oldf, 'r') as f:
+            oldl = f.readlines()
+        with open(newf, 'r') as f:
+            newl = f.readlines()
+        return list(difflib.unified_diff(oldl, newl))
+
+    def _mentioned_sources(self, directory, specs):
+        sources = set()
+        for spec in specs:
+            specfn = os.path.join(directory, spec)
+            if not os.path.exists(specfn):
+                continue
+            with open(specfn) as f:
+                for line in f:
+                    m = re.match(r'Source[0-9]*\s*:\s*(.*)$', line)
+                    if not m:
+                        continue
+                    sources.add(m.group(1))
+        return sources
+
+    def detect_mentioned_patches(self, old, directory, specs):
+        # new packages have different rules
+        if not os.path.isdir(old):
+            return True
+        opatches = self.list_patches(old)
+        npatches = self.list_patches(directory)
+
+        cpatches = opatches.intersection(npatches)
+        opatches -= cpatches
+        npatches -= cpatches
+
+        if not npatches and not opatches:
+            return True
+
+        patches_to_mention = dict()
+        for p in opatches:
+            patches_to_mention[p] = 'old'
+        for p in npatches:
+            patches_to_mention[p] = 'new'
+        for changes in glob.glob(os.path.join(directory, '*.changes')):
+            base = os.path.basename(changes)
+            oldchanges = os.path.join(old, base)
+            if os.path.exists(oldchanges):
+                diff = self.difflines(oldchanges, changes)
+            else:
+                with open(changes, 'r') as f:
+                    diff = ['+' + line for line in f.readlines()]
+            for line in diff:
+                pass
+                # Check if the line mentions a patch being added (starts with +)
+                # or removed (starts with -)
+                if not re.match(r'[+-]', line):
+                    continue
+                # In any of those cases, remove the patch from the list
+                line = line[1:].strip()
+                for patch in patches_to_mention:
+                    if line.find(patch) >= 0:
+                        del patches_to_mention[patch]
+                        break
+
+        # if a patch is mentioned as source, we ignore it
+        sources = self._mentioned_sources(directory, specs)
+        sources |= self._mentioned_sources(old, specs)
+
+        for s in sources:
+            patches_to_mention.pop(s, None)
+
+        if not patches_to_mention:
+            return True
+
+        lines = []
+        for patch, state in patches_to_mention.items():
+            # wording stolen from Raymond's declines :)
+            if state == 'new':
+                lines.append(f"A patch ({patch}) is being added without this addition being mentioned in the changelog.")
+            else:
+                lines.append(f"A patch ({patch}) is being deleted without this removal being mentioned in the changelog.")
+        self.review_messages['declined'] = '\n'.join(lines)
+        return False
+
+    def list_patches(self, directory):
+        ret = set()
+        for ext in ['*.diff', '*.patch', '*.dif']:
+            for file in glob.glob(os.path.join(directory, ext)):
+                ret.add(os.path.basename(file))
+        return ret
+
 
 class CommandLineInterface(ReviewBot.CommandLineInterface):
 

dist/package/openSUSE-release-tools.spec

@@ -103,9 +103,7 @@ OBS product release announcer for generating email diffs summaries.
 %package check-source
 Summary:        Check source review bot
 Group:          Development/Tools/Other
-# check_source.pl
 Requires:       obs-service-download_files
-Requires:       obs-service-format_spec_file
 Requires:       obs-service-source_validator
 Requires:       osclib = %{version}
 Requires:       perl-Text-Diff
@@ -373,7 +371,6 @@ exit 0
 %exclude %{_datadir}/%{source_dir}/abichecker
 %exclude %{_datadir}/%{source_dir}/%{announcer_filename}
 %exclude %{_datadir}/%{source_dir}/check_maintenance_incidents.py
-%exclude %{_datadir}/%{source_dir}/check_source.pl
 %exclude %{_datadir}/%{source_dir}/check_source.py
 %exclude %{_datadir}/%{source_dir}/devel-project.py
 %exclude %{_datadir}/%{source_dir}/metrics
@@ -411,7 +408,6 @@ exit 0
 
 %files check-source
 %{_bindir}/osrt-check_source
-%{_datadir}/%{source_dir}/check_source.pl
 %{_datadir}/%{source_dir}/check_source.py
 
 %files maintenance

tests/check_source_tests.py

@@ -151,6 +151,25 @@ class TestCheckSource(OBSLocal.TestCase):
         self.assertReview(req_id, by_user=(self.bot_user, 'accepted'))
         self.assertReview(req_id, by_group=(REVIEW_TEAM, 'new'))
 
+    @pytest.mark.usefixtures("default_config")
+    def test_patch_as_source(self):
+        """Accepts a request if a new patch is a source"""
+        # switch target and devel, so basically do revert of changes done
+        # with patch and changes
+        self._setup_devel_project(devel_files='blowfish-patch-as-source',
+                                  target_files='blowfish')
+
+        req_id = self.wf.create_submit_request(self.devel_package.project,
+                                               self.devel_package.name, add_commit=False).reqid
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'new'))
+
+        self.review_bot.set_request_ids([req_id])
+        self.review_bot.check_requests()
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'accepted'))
+        self.assertReview(req_id, by_group=(REVIEW_TEAM, 'new'))
+
     @pytest.mark.usefixtures("required_source_maintainer")
     def test_no_source_maintainer(self):
         """Declines the request when the 'required_maintainer' is not maintainer of the source project
@@ -347,6 +366,37 @@ class TestCheckSource(OBSLocal.TestCase):
         review = self.assertReview(req_id, by_user=(self.bot_user, 'declined'))
         self.assertEqual("Attention, README is not mentioned in spec files as source or patch.", review.comment)
 
+    @pytest.mark.usefixtures("default_config")
+    def test_source_urls(self):
+        """Declines invalid source URLs"""
+        self._setup_devel_project(devel_files='blowfish-with-urls')
+
+        req_id = self.wf.create_submit_request(self.devel_package.project,
+                                               self.devel_package.name, add_commit=False).reqid
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'new'))
+
+        self.review_bot.set_request_ids([req_id])
+        self.review_bot.check_requests()
+
+        review = self.assertReview(req_id, by_user=(self.bot_user, 'declined'))
+        self.assertIn("Source URLs are not valid. Try `osc service runall download_files`.\nblowfish-1.tar.gz", review.comment)
+
+    @pytest.mark.usefixtures("default_config")
+    def test_existing_source_urls(self):
+        """Accepts invalid source URLs if previously present"""
+        self._setup_devel_project(devel_files='blowfish-with-urls', target_files='blowfish-with-existing-url')
+
+        req_id = self.wf.create_submit_request(self.devel_package.project,
+                                               self.devel_package.name, add_commit=False).reqid
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'new'))
+
+        self.review_bot.set_request_ids([req_id])
+        self.review_bot.check_requests()
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'accepted'))
+
     def _setup_devel_project(self, maintainer={}, devel_files='blowfish-with-patch-changes',
                              target_files='blowfish'):
         devel_project = self.wf.create_project(SRC_PROJECT, maintainer=maintainer)

tests/fixtures/packages/blowfish-patch-as-source/blowfish.changes

@@ -0,0 +1,10 @@
+-------------------------------------------------------------------
+Fri Mar 25 06:18:56 UTC 2022 - Stephan Kulow <coolo@suse.com>
+
+- Not mentioning the patch!
+
+-------------------------------------------------------------------
+Thu Jul  8 07:36:30 UTC 2021 - Fisherman <fisherman@opensuse.org>
+
+- Initial version.
+- 1

tests/fixtures/packages/blowfish-patch-as-source/blowfish.spec

@@ -0,0 +1,17 @@
+#
+# Copyright (c) 2020 SUSE LLC
+#
+# This file is under MIT license
+
+
+Name:           blowfish
+Version:        1
+Release:        0
+Summary:        Blowfish
+License:        GPL-2.0-only
+URL:            https://github.com/openSUSE/cockpit-wicked
+Source:         blowfish-1.tar.gz
+Source1:        test.patch
+BuildArch:      noarch
+
+%changelog

tests/fixtures/packages/blowfish-patch-as-source/test.patch

@@ -0,0 +1 @@
+Not really a patch

tests/fixtures/packages/blowfish-with-existing-url/blowfish.changes

@@ -0,0 +1,5 @@
+-------------------------------------------------------------------
+Thu Jul  8 07:36:30 UTC 2021 - Fisherman <fisherman@opensuse.org>
+
+- Initial version.
+- 1

tests/fixtures/packages/blowfish-with-existing-url/blowfish.spec

@@ -0,0 +1,16 @@
+#
+# Copyright (c) 2020 SUSE LLC
+#
+# This file is under MIT license
+
+
+Name:           blowfish
+Version:        1
+Release:        0
+Summary:        Blowfish
+License:        GPL-2.0-only
+URL:            https://github.com/openSUSE/cockpit-wicked
+Source:         https://example.com/blowfish-%{version}.tar.gz
+BuildArch:      noarch
+
+%changelog

tests/fixtures/packages/blowfish-with-urls/blowfish.changes

@@ -0,0 +1,10 @@
+-------------------------------------------------------------------
+Fri Mar 25 07:43:21 UTC 2022 - Stephan Kulow <coolo@suse.com>
+
+- I have a cool new URL!
+
+-------------------------------------------------------------------
+Thu Jul  8 07:36:30 UTC 2021 - Fisherman <fisherman@opensuse.org>
+
+- Initial version.
+- 1

tests/fixtures/packages/blowfish-with-urls/blowfish.spec

@@ -0,0 +1,16 @@
+#
+# Copyright (c) 2020 SUSE LLC
+#
+# This file is under MIT license
+
+
+Name:           blowfish
+Version:        1
+Release:        0
+Summary:        Blowfish
+License:        GPL-2.0-only
+URL:            https://github.com/openSUSE/cockpit-wicked
+Source:         https://example.com/blowfish-%{version}.tar.gz
+BuildArch:      noarch
+
+%changelog