diff --git a/CONTENTS.md b/CONTENTS.md index 905aba5f..af6266c7 100644 --- a/CONTENTS.md +++ b/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) diff --git a/check_source.pl b/check_source.pl deleted file mode 100644 index 14e933d3..00000000 --- a/check_source.pl +++ /dev/null @@ -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 () { - 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); diff --git a/check_source.py b/check_source.py index 97c337c0..a9eceebe 100755 --- a/check_source.py +++ b/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. - 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 + 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') + 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') - if self.only_changes(): - self.logger.debug('only .changes modifications') - if self.staging_group and self.review_user in group_members(self.apiurl, self.staging_group): - if not self.dryrun: - osc.core.change_review_state(self.apiurl, str(self.request.reqid), 'accepted', - by_group=self.staging_group, - message='skipping the staging process since only .changes modifications') - else: - self.logger.debug('unable to skip staging review since not a member of staging group') + if self.only_changes(): + self.logger.debug('only .changes modifications') + if self.staging_group and self.review_user in group_members(self.apiurl, self.staging_group): + if not self.dryrun: + osc.core.change_review_state(self.apiurl, str(self.request.reqid), 'accepted', + by_group=self.staging_group, + message='skipping the staging process since only .changes modifications') + else: + 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 +) diff --git a/dist/package/openSUSE-release-tools.spec b/dist/package/openSUSE-release-tools.spec index 212b1bcb..ca586098 100644 --- a/dist/package/openSUSE-release-tools.spec +++ b/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 diff --git a/tests/check_source_tests.py b/tests/check_source_tests.py index ea6303a4..9e51e9dd 100644 --- a/tests/check_source_tests.py +++ b/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) diff --git a/tests/fixtures/packages/blowfish-with-existing-url/blowfish-1.tar.gz b/tests/fixtures/packages/blowfish-with-existing-url/blowfish-1.tar.gz new file mode 100644 index 00000000..94fff08b Binary files /dev/null and b/tests/fixtures/packages/blowfish-with-existing-url/blowfish-1.tar.gz differ diff --git a/tests/fixtures/packages/blowfish-with-existing-url/blowfish.changes b/tests/fixtures/packages/blowfish-with-existing-url/blowfish.changes new file mode 100644 index 00000000..45f636c9 --- /dev/null +++ b/tests/fixtures/packages/blowfish-with-existing-url/blowfish.changes @@ -0,0 +1,5 @@ +------------------------------------------------------------------- +Thu Jul 8 07:36:30 UTC 2021 - Fisherman + +- Initial version. +- 1 diff --git a/tests/fixtures/packages/blowfish-with-existing-url/blowfish.spec b/tests/fixtures/packages/blowfish-with-existing-url/blowfish.spec new file mode 100644 index 00000000..2ffcffea --- /dev/null +++ b/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 diff --git a/tests/fixtures/packages/blowfish-with-urls/blowfish-1.tar.gz b/tests/fixtures/packages/blowfish-with-urls/blowfish-1.tar.gz new file mode 100644 index 00000000..94fff08b Binary files /dev/null and b/tests/fixtures/packages/blowfish-with-urls/blowfish-1.tar.gz differ diff --git a/tests/fixtures/packages/blowfish-with-urls/blowfish.changes b/tests/fixtures/packages/blowfish-with-urls/blowfish.changes new file mode 100644 index 00000000..ecb7b430 --- /dev/null +++ b/tests/fixtures/packages/blowfish-with-urls/blowfish.changes @@ -0,0 +1,10 @@ +------------------------------------------------------------------- +Fri Mar 25 07:43:21 UTC 2022 - Stephan Kulow + +- I have a cool new URL! + +------------------------------------------------------------------- +Thu Jul 8 07:36:30 UTC 2021 - Fisherman + +- Initial version. +- 1 diff --git a/tests/fixtures/packages/blowfish-with-urls/blowfish.spec b/tests/fixtures/packages/blowfish-with-urls/blowfish.spec new file mode 100644 index 00000000..2ffcffea --- /dev/null +++ b/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