Move download URL check to check_source.py

And with that, check_source.pl is gone

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,78 +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 $odir = getcwd();
-
-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;
-            }
-        }
-    }
-   
-    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*//;
-                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";
-    }
-
-}
-
-my $tmpdir = tempdir("obs-XXXXXXX", TMPDIR => 1, CLEANUP => 1);
-chdir($odir) || die "chdir $odir failed";
-chdir($dir) || die "chdir $dir 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

@@ -7,12 +7,12 @@ 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
@@ -183,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))
@@ -229,51 +227,27 @@ class CheckSource(ReviewBot.ReviewBot):
         if not self.detect_mentioned_patches('_old', target_package, specs):
             return False
 
-        # Run check_source.pl script and interpret output.
+        if not self.check_urls('_old', target_package, specs):
-        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())
-
-        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
             return False
 
         shutil.rmtree(dir)
         self.review_messages['accepted'] = 'Check script succeeded'
 
-        if len(checked):
+        if self.skip_add_reviews:
-            self.review_messages['accepted'] += "\n\nOutput of check script (non-fatal):\n" + output
+            return True
 
-        if not self.skip_add_reviews:
+        if self.add_review_team and self.review_team is not None:
-            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')
-                self.add_review(self.request, by_group=self.review_team, msg='Please review sources')
 
-            if self.only_changes():
+        if self.only_changes():
-                self.logger.debug('only .changes modifications')
+            self.logger.debug('only .changes modifications')
-                if self.staging_group and self.review_user in group_members(self.apiurl, self.staging_group):
+            if self.staging_group and self.review_user in group_members(self.apiurl, self.staging_group):
-                    if not self.dryrun:
+                if not self.dryrun:
-                        osc.core.change_review_state(self.apiurl, str(self.request.reqid), 'accepted',
+                    osc.core.change_review_state(self.apiurl, str(self.request.reqid), 'accepted',
-                                                     by_group=self.staging_group,
+                                                 by_group=self.staging_group,
-                                                     message='skipping the staging process since only .changes modifications')
+                                                 message='skipping the staging process since only .changes modifications')
-                else:
+            else:
-                    self.logger.debug('unable to skip staging review since not a member of staging group')
+                self.logger.debug('unable to skip staging review since not a member of staging group')
 
         return True
 
@@ -578,6 +552,40 @@ 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()
@@ -588,7 +596,10 @@ class CheckSource(ReviewBot.ReviewBot):
     def _mentioned_sources(self, directory, specs):
         sources = set()
         for spec in specs:
-            with open(os.path.join(directory, spec)) as f:
+            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:
@@ -622,7 +633,7 @@ class CheckSource(ReviewBot.ReviewBot):
                 diff = self.difflines(oldchanges, changes)
             else:
                 with open(changes, 'r') as f:
-                    diff = ['+' + l for l in f.readlines()]
+                    diff = ['+' + line for line in f.readlines()]
             for line in diff:
                 pass
                 # Check if the line mentions a patch being added (starts with +)

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

@@ -366,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-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