From e2922c715c1957c31fb44bf97aa45c1ed58b8f9d Mon Sep 17 00:00:00 2001 From: Jakub Kadlcik Date: Sun, 2 Jul 2023 00:44:56 +0200 Subject: [PATCH 1/3] backend, rpmbuild: move package NEVR parsing to builder Fix #2790 --- backend/copr-backend.spec | 2 - .../copr_backend/background_worker_build.py | 17 ++++-- backend/copr_backend/helpers.py | 58 +------------------ backend/tests/test_background_worker_build.py | 39 ++----------- backend/tests/testlib/__init__.py | 7 ++- .../runtest-build-results-json.sh | 10 ++++ .../copr_rpmbuild/automation/srpm_results.py | 3 +- 7 files changed, 32 insertions(+), 104 deletions(-) diff --git a/backend/copr-backend.spec b/backend/copr-backend.spec index c7641fee3..a75ba78f6 100644 --- a/backend/copr-backend.spec +++ b/backend/copr-backend.spec @@ -56,7 +56,6 @@ BuildRequires: python3-retask BuildRequires: python3-setproctitle BuildRequires: python3-sphinx BuildRequires: python3-tabulate -BuildRequires: python3-specfile BuildRequires: modulemd-tools >= 0.6 BuildRequires: prunerepo >= %prunerepo_version BuildRequires: dnf @@ -95,7 +94,6 @@ Requires: python3-retask Requires: python3-setproctitle Requires: python3-tabulate Requires: python3-boto3 -Requires: python3-specfile Requires: redis Requires: rpm-sign Requires: rsync diff --git a/backend/copr_backend/background_worker_build.py b/backend/copr_backend/background_worker_build.py index e9309047a..6c488a165 100644 --- a/backend/copr_backend/background_worker_build.py +++ b/backend/copr_backend/background_worker_build.py @@ -24,7 +24,7 @@ FrontendClientException, ) from copr_backend.helpers import ( - call_copr_repo, pkg_name_evr, run_cmd, register_build_result, find_spec_file, + call_copr_repo, run_cmd, register_build_result, format_evr, ) from copr_backend.job import BuildJob from copr_backend.msgbus import MessageSender @@ -628,15 +628,20 @@ def _do_createrepo(self): def _get_srpm_build_details(self, job): build_details = {'srpm_url': ''} - self.log.info("Retrieving srpm URL from %s", job.results_dir) + self.log.info("Retrieving SRPM info from %s", job.results_dir) + + results_path = os.path.join(job.results_dir, "results.json") + with open(results_path, "r", encoding="utf-8") as fp: + results = json.load(fp) + + build_details["pkg_name"] = results["name"] + build_details["pkg_version"] = format_evr( + results["epoch"], results["version"], results["release"]) + pattern = os.path.join(job.results_dir, '*.src.rpm') srpm_file = glob.glob(pattern)[0] srpm_name = os.path.basename(srpm_file) srpm_url = os.path.join(job.results_dir_url, srpm_name) - spec_file_path = find_spec_file(job.results_dir, self.log) - - build_details['pkg_name'], build_details['pkg_version'] = \ - pkg_name_evr(spec_file_path, srpm_file, self.log) build_details['srpm_url'] = srpm_url self.log.info("SRPM URL: %s", srpm_url) return build_details diff --git a/backend/copr_backend/helpers.py b/backend/copr_backend/helpers.py index bdccc4c01..f74f2aa4d 100644 --- a/backend/copr_backend/helpers.py +++ b/backend/copr_backend/helpers.py @@ -27,13 +27,12 @@ import munch from munch import Munch -from specfile import Specfile from copr_common.redis_helpers import get_redis_connection from copr.v3 import Client from copr_backend.constants import DEF_BUILD_USER, DEF_BUILD_TIMEOUT, DEF_CONSECUTIVE_FAILURE_THRESHOLD, \ CONSECUTIVE_FAILURE_REDIS_KEY, default_log_format -from copr_backend.exceptions import CoprBackendError, CoprBackendSrpmError +from copr_backend.exceptions import CoprBackendError from . import constants @@ -633,22 +632,6 @@ def get_chroot_arch(chroot): return chroot.rsplit("-", 2)[2] -def find_spec_file(folder, logger): - """ - Returns spec file path or None - """ - spec_pattern = os.path.join(folder, "*.spec") - result = glob.glob(spec_pattern) - if not result: - return None - - if len(result) > 1: - logger.error("More than one spec file found! Using the first %s from %s", - str(result[0]), str(result)) - - return result[0] - - def format_evr(epoch, version, release): """ Return evr in format (epoch:)version-release @@ -659,45 +642,6 @@ def format_evr(epoch, version, release): return f"{version}-{release}" -def pkg_name_evr(spec_file_path, srpm_path, logger): - """ - Queries a SRPM or spec file for its name and evr (epoch:version-release) - """ - try: - spec = Specfile(spec_file_path) - return spec.expanded_name, format_evr(spec.expanded_epoch, - spec.expanded_version, - spec.expanded_release) - except Exception: # pylint: disable=broad-exception-caught - # Specfile library raises too many exception to name the in except block - logger.exception("Exception appeared during handling spec file: %s", spec_file_path) - return pkg_name_evr_from_srpm(srpm_path, logger) - - - -def pkg_name_evr_from_srpm(srpm_path, logger): - """ - Queries a package for its name and evr (epoch:version-release) - """ - cmd = ['rpm', '-qp', '--nosignature', '--qf', - '%{NAME} %{EPOCH} %{VERSION} %{RELEASE}', srpm_path] - - try: - result = run_cmd(cmd, logger=logger) - except OSError as e: - raise CoprBackendSrpmError(str(e)) from e - - if result.returncode != 0: - raise CoprBackendSrpmError('Error querying SRPM') - - try: - name, epoch, version, release = result.stdout.split(" ") - except ValueError as e: - raise CoprBackendSrpmError(str(e)) from e - - return name, format_evr(epoch, version, release) - - def format_filename(name, version, release, epoch, arch, zero_epoch=False): if not epoch.isdigit() and zero_epoch: epoch = "0" diff --git a/backend/tests/test_background_worker_build.py b/backend/tests/test_background_worker_build.py index 94d39def4..f5524395d 100644 --- a/backend/tests/test_background_worker_build.py +++ b/backend/tests/test_background_worker_build.py @@ -27,7 +27,6 @@ from copr_backend.vm_alloc import ResallocHost, RemoteHostAllocationTerminated from copr_backend.background_worker_build import COMMANDS, MIN_BUILDER_VERSION from copr_backend.sshcmd import SSHConnectionError -from copr_backend.exceptions import CoprBackendSrpmError import testlib from testlib import assert_logs_exist, assert_logs_dont_exist @@ -169,6 +168,7 @@ def f_build_srpm(f_build_something): to build RPM. """ config = f_build_something + config.ssh.resultdir = "srpm-builds/02914697" config.fe_client.return_value.get.return_value = _get_srpm_job() config.ssh.set_command( "copr-rpmbuild --verbose --drop-resultdir --srpm --task-url " @@ -315,32 +315,15 @@ def test_prev_build_backup(f_build_rpm_case): def test_full_srpm_build(_parse_results, f_build_srpm): worker = f_build_srpm.bw worker.process() - assert worker.job.pkg_name == "example" + assert worker.job.pkg_name == "hello" # TODO: fix this is ugly pkg_version testament assert worker.job.pkg_version is None - assert worker.job.__dict__["pkg_version"] == "1.0.14-1" + assert worker.job.__dict__["pkg_version"] == "2.8-1" assert worker.job.srpm_url == ( "https://example.com/results/@copr/PROJECT_2/srpm-builds/" - "00855954/example-1.0.14-1.fc30.src.rpm") - - -@mock.patch("copr_backend.background_worker_build.find_spec_file") -@_patch_bwbuild_object("BuildBackgroundWorker._parse_results") -def test_full_srpm_build_without_specfile(_parse_results, mock_find_spec_file, - f_build_srpm): - mock_find_spec_file.return_value = None - - worker = f_build_srpm.bw - worker.process() - - assert worker.job.pkg_name == "example" - assert worker.job.pkg_version is None - assert worker.job.__dict__["pkg_version"] == "1.0.14-1.fc30" - assert worker.job.srpm_url == ( - "https://example.com/results/@copr/PROJECT_2/srpm-builds/" - "00855954/example-1.0.14-1.fc30.src.rpm") + "00855954/hello-2.8-1.src.rpm") @mock.patch("copr_backend.sign.SIGN_BINARY", "tests/fake-bin-sign") @@ -786,20 +769,6 @@ def test_createrepo_failure(mc_call_copr_repo, f_build_rpm_case, caplog): "Finished build: id=848963 failed=True ", ], caplog) -@_patch_bwbuild_object("pkg_name_evr") -@_patch_bwbuild_object("BuildBackgroundWorker._parse_results") -def test_pkg_collect_failure(_parse_results, mc_pkg_evr, f_build_srpm, caplog): - mc_pkg_evr.side_effect = CoprBackendSrpmError("srpm error") - config = f_build_srpm - worker = config.bw - worker.process() - assert_logs_exist([ - "Error while collecting built packages", - "Worker failed build, took ", - "Finished build: id=855954 failed=True ", - ], caplog) - assert worker.job.status == 0 # fail - @_patch_bwbuild_object("BuildBackgroundWorker._parse_results") def test_existing_compressed_file(_parse_results, f_build_rpm_case, caplog): config = f_build_rpm_case diff --git a/backend/tests/testlib/__init__.py b/backend/tests/testlib/__init__.py index 781a78a8a..037ea854c 100644 --- a/backend/tests/testlib/__init__.py +++ b/backend/tests/testlib/__init__.py @@ -134,6 +134,7 @@ def __init__(self, user=None, host=None, config_file=None, log=None): 0, "", "") self.set_command("copr-rpmbuild-log", 0, "build log stdout\n", "build log stderr\n") + self.resultdir = "fedora-30-x86_64/00848963-example" def set_command(self, cmd, exit_code, stdout, stderr, action=None, return_action=None): @@ -177,8 +178,8 @@ def _full_source_path(self, src): def rsync_download(self, src, dest, logfile=None, max_retries=0): data = os.environ["TEST_DATA_DIRECTORY"] trail_slash = src.endswith("/") - src = os.path.join(data, "build_results", "fedora-30-x86_64", - "00848963-example") + src = os.path.join(data, "build_results", self.resultdir) + if trail_slash: src = src + "/" @@ -193,7 +194,7 @@ def rsync_download(self, src, dest, logfile=None, max_retries=0): if self.unlink_success: os.unlink(os.path.join(dest, "success")) - if "PROJECT_2" in dest: + if "PROJECT_2" in dest and "fedora-30-x86_64" in dest: os.unlink(os.path.join(dest, "example-1.0.14-1.fc30.x86_64.rpm")) def assert_logs_exist(messages, caplog): diff --git a/beaker-tests/Sanity/copr-cli-basic-operations/runtest-build-results-json.sh b/beaker-tests/Sanity/copr-cli-basic-operations/runtest-build-results-json.sh index eb1442560..3a60e27f6 100755 --- a/beaker-tests/Sanity/copr-cli-basic-operations/runtest-build-results-json.sh +++ b/beaker-tests/Sanity/copr-cli-basic-operations/runtest-build-results-json.sh @@ -22,6 +22,16 @@ rlJournalStart rlRun "parse_build_id" rlRun "copr watch-build $BUILD_ID" + # Check the SRPM results.json file that is stored on backend + URL_PATH="results/${NAME_PREFIX}TestResultsJson/srpm-builds/$(build_id_with_leading_zeroes)/results.json" + path="$RESULTDIR/results.json" + rlRun "curl $BACKEND_URL/$URL_PATH > $path" + rlRun "cat $path |jq -e '.name == \"hello\"'" + rlRun "cat $path |jq -e '.epoch == null'" + rlRun "cat $path |jq -e '.version == \"2.8\"'" + rlRun "cat $path |jq -e '.release == \"1\"'" + + # Check results.json for the final chroots checkResults() { orig_file=$1 diff --git a/rpmbuild/copr_rpmbuild/automation/srpm_results.py b/rpmbuild/copr_rpmbuild/automation/srpm_results.py index a95bdfbc9..8d2b353ea 100644 --- a/rpmbuild/copr_rpmbuild/automation/srpm_results.py +++ b/rpmbuild/copr_rpmbuild/automation/srpm_results.py @@ -35,5 +35,6 @@ def get_package_info(self): """ spec_path = locate_spec(self.resultdir) spec = Spec(spec_path) - keys = ["name", "exclusivearch", "excludearch"] + keys = ["name", "epoch", "version", "release", + "exclusivearch", "excludearch"] return {key: getattr(spec, key) for key in keys} From 54ff96f5a6ca6e574b893af6cb506e13fc3d244a Mon Sep 17 00:00:00 2001 From: Jakub Kadlcik Date: Sun, 2 Jul 2023 01:05:23 +0200 Subject: [PATCH 2/3] rpmbuild: depend on new, much faster python3-specfile Fix #2777 --- rpmbuild/copr-rpmbuild.spec | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rpmbuild/copr-rpmbuild.spec b/rpmbuild/copr-rpmbuild.spec index b077c8724..72a4ed4d9 100644 --- a/rpmbuild/copr-rpmbuild.spec +++ b/rpmbuild/copr-rpmbuild.spec @@ -40,7 +40,7 @@ BuildRequires: %{python_pfx}-munch BuildRequires: %{python}-requests BuildRequires: %{python_pfx}-jinja2 BuildRequires: %{python_pfx}-simplejson -BuildRequires: %{python_pfx}-specfile +BuildRequires: %{python_pfx}-specfile >= 0.19.0 BuildRequires: python3-backoff >= 1.9.0 BuildRequires: /usr/bin/argparse-manpage @@ -58,7 +58,7 @@ Requires: %{python_pfx}-jinja2 Requires: %{python_pfx}-munch Requires: %{python}-requests Requires: %{python_pfx}-simplejson -Requires: %{python_pfx}-specfile +Requires: %{python_pfx}-specfile >= 0.19.0 Requires: python3-backoff >= 1.9.0 Requires: mock >= 2.0 From e970e2ca325f42bacdd2fce29f40351f250df714 Mon Sep 17 00:00:00 2001 From: Jakub Kadlcik Date: Sun, 6 Aug 2023 16:23:41 +0200 Subject: [PATCH 3/3] rpmbuild: fallback to querying NEVRA from SRPM package header --- .../copr_rpmbuild/automation/srpm_results.py | 25 ++++++++++++++++--- rpmbuild/copr_rpmbuild/helpers.py | 5 ++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/rpmbuild/copr_rpmbuild/automation/srpm_results.py b/rpmbuild/copr_rpmbuild/automation/srpm_results.py index 8d2b353ea..be4d75741 100644 --- a/rpmbuild/copr_rpmbuild/automation/srpm_results.py +++ b/rpmbuild/copr_rpmbuild/automation/srpm_results.py @@ -5,7 +5,12 @@ import os import simplejson from copr_rpmbuild.automation.base import AutomationTool -from copr_rpmbuild.helpers import locate_spec, Spec +from copr_rpmbuild.helpers import ( + get_rpm_header, + locate_srpm, + locate_spec, + Spec, +) class SRPMResults(AutomationTool): @@ -33,8 +38,20 @@ def get_package_info(self): """ Return ``dict`` with interesting package metadata """ - spec_path = locate_spec(self.resultdir) - spec = Spec(spec_path) keys = ["name", "epoch", "version", "release", "exclusivearch", "excludearch"] - return {key: getattr(spec, key) for key in keys} + try: + path = locate_spec(self.resultdir) + spec = Spec(path) + return {key: getattr(spec, key) for key in keys} + + except Exception: # pylint: disable=broad-exception-caught + # Specfile library raises too many exception to name the + # in except block + msg = "Exception appeared during handling spec file: {0}".format(path) + self.log.exception(msg) + + # Fallback to querying NEVRA from the SRPM package header + path = locate_srpm(self.resultdir) + hdr = get_rpm_header(path) + return {key: hdr[key] for key in keys} diff --git a/rpmbuild/copr_rpmbuild/helpers.py b/rpmbuild/copr_rpmbuild/helpers.py index 247446622..aa445d1ee 100644 --- a/rpmbuild/copr_rpmbuild/helpers.py +++ b/rpmbuild/copr_rpmbuild/helpers.py @@ -108,6 +108,11 @@ def get_rpm_header(path): See docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html/RPM_Guide/ch16s04.html """ ts = rpm.TransactionSet() + # I don't want to copy-paste the value of the protected variable. + # IMHO there is only a low chance it will get removed and even if it gets, + # we have tests to catch it anyway + # pylint: disable=protected-access + ts.setVSFlags(rpm._RPMVSF_NOSIGNATURES) with open(path, "rb") as f: hdr = ts.hdrFromFdno(f.fileno()) return hdr