Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip excluded architectures #2769

Merged
merged 5 commits into from
Jun 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions backend/copr_backend/background_worker_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@

MAX_HOST_ATTEMPTS = 3
MAX_SSH_ATTEMPTS = 5
MIN_BUILDER_VERSION = "0.54.1.dev"
MIN_BUILDER_VERSION = "0.68.dev"
CANCEL_CHECK_PERIOD = 5

MESSAGES = {
Expand Down Expand Up @@ -291,10 +291,6 @@ def _parse_results(self):
"""
Parse `results.json` and update the `self.job` object.
"""
if self.job.chroot == "srpm-builds":
# We care only about final RPMs
return

path = os.path.join(self.job.results_dir, "results.json")
if not os.path.exists(path):
raise BackendError("results.json file not found in resultdir")
Expand Down
2 changes: 1 addition & 1 deletion backend/copr_backend/frontend.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from copr_backend.exceptions import FrontendClientException

# The frontend counterpart is in `backend_general:send_frontend_version`
MIN_FE_BE_API = 5
MIN_FE_BE_API = 6

class FrontendClient:
"""
Expand Down
13 changes: 9 additions & 4 deletions backend/tests/test_background_worker_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,8 @@ def test_prev_build_backup(f_build_rpm_case):
assert len(glob.glob(os.path.join(prev_results, rsync_patt))) == 1


def test_full_srpm_build(f_build_srpm):
@_patch_bwbuild_object("BuildBackgroundWorker._parse_results")
def test_full_srpm_build(_parse_results, f_build_srpm):
worker = f_build_srpm.bw
worker.process()
assert worker.job.pkg_name == "example"
Expand All @@ -326,7 +327,9 @@ def test_full_srpm_build(f_build_srpm):


@mock.patch("copr_backend.background_worker_build.find_spec_file")
def test_full_srpm_build_without_specfile(mock_find_spec_file, f_build_srpm):
@_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
Expand Down Expand Up @@ -784,7 +787,8 @@ def test_createrepo_failure(mc_call_copr_repo, f_build_rpm_case, caplog):
], caplog)

@_patch_bwbuild_object("pkg_name_evr")
def test_pkg_collect_failure(mc_pkg_evr, f_build_srpm, caplog):
@_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
Expand Down Expand Up @@ -868,7 +872,8 @@ def test_unable_to_start_builder(f_build_srpm, caplog):
assert_logs_dont_exist(["Retry"], caplog)

@_patch_bwbuild_object("time.sleep", mock.MagicMock())
def test_retry_vm_factory_take(f_build_srpm, caplog):
@_patch_bwbuild_object("BuildBackgroundWorker._parse_results")
def test_retry_vm_factory_take(_parse_results, f_build_srpm, caplog):
config = f_build_srpm
rhf = config.resalloc_host_factory
host = config.host
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#! /bin/bash

# Include Beaker environment
. /usr/share/beakerlib/beakerlib.sh || exit 1

# Load config settings
HERE=$(dirname "$(realpath "$0")")
source "$HERE/config"
source "$HERE/helpers"


rlJournalStart
rlPhaseStartSetup
setup_checks
rlAssertRpm "jq"
rlPhaseEnd

rlPhaseStartTest
chroots=""
chroots+=" --chroot fedora-$FEDORA_VERSION-x86_64"
chroots+=" --chroot fedora-$FEDORA_VERSION-aarch64"
chroots+=" --chroot fedora-$FEDORA_VERSION-ppc64le"
chroots+=" --chroot fedora-$FEDORA_VERSION-s390x"

OUTPUT=`mktemp`

# Test ExclusiveArch
rlRun "copr-cli create ${NAME_PREFIX}ExclusiveArch $chroots"
rlRun "copr-cli build-distgit ${NAME_PREFIX}ExclusiveArch --name biosdevname --commit $BRANCH"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this testing both excludearch and exclusivearch? It would be nice to have both tested

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was only for ExclusivAarch. I added another one for ExcludeArch

rlRun "copr monitor ${NAME_PREFIX}ExclusiveArch > $OUTPUT"
rlAssertEquals "Skipped chroots" `cat $OUTPUT |grep "skipped" |wc -l` 3
rlAssertEquals "Succeeded chroots" `cat $OUTPUT |grep "succeeded" |wc -l` 1

# This is a more complicated package with `BuildArch: noarch` and
# ExclusiveArch for subpackages. Test that we don't fail while parsing it
rlRun "copr-cli build-distgit ${NAME_PREFIX}ExclusiveArch --name procyon"

# Test ExcludeArch
rlRun "copr-cli create ${NAME_PREFIX}ExcludeArch $chroots"
rlRun "copr-cli build-distgit ${NAME_PREFIX}ExcludeArch --name python-giacpy"
rlRun "copr monitor ${NAME_PREFIX}ExcludeArch > $OUTPUT"
rlAssertEquals "Skipped chroots" `cat $OUTPUT |grep "skipped" |wc -l` 3
rlAssertEquals "Succeeded chroots" `cat $OUTPUT |grep "succeeded" |wc -l` 1
rlPhaseEnd

rlPhaseStartCleanup
rlRun "copr-cli delete ${NAME_PREFIX}ExclusiveArch"
rlRun "copr-cli delete ${NAME_PREFIX}ExcludeArch"
rlPhaseEnd
rlJournalPrintText
rlJournalEnd
23 changes: 19 additions & 4 deletions frontend/coprs_frontend/coprs/logic/builds_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -1011,19 +1011,34 @@ def update_state_from_dict(cls, build, upd_dict):
buildchroot.status = chroot_status
db.session.add(buildchroot)

def finish(chroot, status):
chroot.status = status
chroot.ended_on = upd_dict.get("ended_on") or time.time()
chroot.started_on = upd_dict.get("started_on", chroot.ended_on)
db.session.add(chroot)

build.source_status = new_status
if new_status == StatusEnum("failed") or \
new_status == StatusEnum("skipped"):
for ch in build.build_chroots:
ch.status = new_status
ch.ended_on = upd_dict.get("ended_on") or time.time()
ch.started_on = upd_dict.get("started_on", ch.ended_on)
db.session.add(ch)
finish(ch, new_status)

if new_status == StatusEnum("failed"):
build.fail_type = FailTypeEnum("srpm_build_error")
BuildsLogic.delete_local_source(build)

# Skip excluded architectures
if upd_dict.get("chroot") == "srpm-builds" and upd_dict.get("results"):
for chroot in build.build_chroots:
arch = chroot.mock_chroot.arch

exclusivearch = upd_dict["results"]["exclusivearch"]
if exclusivearch and arch not in exclusivearch:
finish(chroot, StatusEnum("skipped"))

if arch in upd_dict["results"]["excludearch"]:
finish(chroot, StatusEnum("skipped"))

cls.process_update_callback(build)
db.session.add(build)
return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def send_frontend_version(response):
setup the version according to our needs.
For the backend counterpart, see the `MIN_FE_BE_API` constant.
"""
response.headers['Copr-FE-BE-API-Version'] = '5'
response.headers['Copr-FE-BE-API-Version'] = '6'
return response


Expand Down Expand Up @@ -90,10 +90,12 @@ def dist_git_upload_completed():
for branch, git_hash in flask.request.json.get("branch_commits", {}).items():
branch_chroots = BuildsLogic.get_buildchroots_by_build_id_and_branch(build_id, branch)
for ch in branch_chroots:
collected_branch_chroots.append((ch.task_id))
if ch.status == StatusEnum("skipped"):
continue
ch.status = StatusEnum("pending")
ch.git_hash = git_hash
db.session.add(ch)
collected_branch_chroots.append((ch.task_id))

final_source_status = StatusEnum("succeeded")
for ch in build.build_chroots:
Expand Down
2 changes: 2 additions & 0 deletions rpmbuild/copr-rpmbuild.spec
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ BuildRequires: %{python_pfx}-munch
BuildRequires: %{python}-requests
BuildRequires: %{python_pfx}-jinja2
BuildRequires: %{python_pfx}-simplejson
BuildRequires: %{python_pfx}-specfile
BuildRequires: python3-backoff >= 1.9.0

%if 0%{?fedora} || 0%{?rhel} > 7
Expand All @@ -66,6 +67,7 @@ Requires: %{python_pfx}-jinja2
Requires: %{python_pfx}-munch
Requires: %{python}-requests
Requires: %{python_pfx}-simplejson
Requires: %{python_pfx}-specfile
Requires: python3-backoff >= 1.9.0

Requires: mock >= 2.0
Expand Down
3 changes: 2 additions & 1 deletion rpmbuild/copr_rpmbuild/automation/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"""

from copr_rpmbuild.automation.fedora_review import FedoraReview
from copr_rpmbuild.automation.srpm_results import SRPMResults
from copr_rpmbuild.automation.rpm_results import RPMResults


Expand All @@ -12,7 +13,7 @@ def run_automation_tools(task, resultdir, mock_config_file, log):
Iterate over all supported post-build tools (e.g. `fedora-review`,
`rpmlint`, etc) and run the desired ones for a given task.
"""
tools = [FedoraReview, RPMResults]
tools = [FedoraReview, SRPMResults, RPMResults]
for _class in tools:
tool = _class(task, resultdir, mock_config_file, log)
if not tool.enabled:
Expand Down
6 changes: 5 additions & 1 deletion rpmbuild/copr_rpmbuild/automation/fedora_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ def enabled(self):
Do we want to run `fedora-review` tool for this particular task?
Depends on the project settings and the chroot that we build in.
"""
return self.chroot.startswith("fedora-") and self.fedora_review_enabled
if not self.chroot:
return False
if not self.chroot.startswith("fedora-"):
return False
return self.fedora_review_enabled

def run(self):
"""
Expand Down
19 changes: 4 additions & 15 deletions rpmbuild/copr_rpmbuild/automation/rpm_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
"""

import os
import rpm
import simplejson
from copr_rpmbuild.automation.base import AutomationTool
from copr_rpmbuild.helpers import get_rpm_header


class RPMResults(AutomationTool):
Expand All @@ -16,9 +16,9 @@ class RPMResults(AutomationTool):
@property
def enabled(self):
"""
Do this for every build
Do this for every RPM build
"""
return True
return self.chroot is not None

def run(self):
"""
Expand Down Expand Up @@ -53,7 +53,7 @@ def get_nevra_dict(cls, path):
msg = "File name doesn't end with '.rpm': {}".format(path)
raise ValueError(msg)

hdr = cls.get_rpm_header(path)
hdr = get_rpm_header(path)
arch = "src" if filename.endswith(".src.rpm") else hdr["arch"]
return {
"name": hdr["name"],
Expand All @@ -62,14 +62,3 @@ def get_nevra_dict(cls, path):
"release": hdr["release"],
"arch": arch,
}

@staticmethod
def get_rpm_header(path):
"""
Examine a RPM package file and return its header
See docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html/RPM_Guide/ch16s04.html
"""
ts = rpm.TransactionSet()
with open(path, "r") as f:
hdr = ts.hdrFromFdno(f.fileno())
return hdr
39 changes: 39 additions & 0 deletions rpmbuild/copr_rpmbuild/automation/srpm_results.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
"""
Create `results.json` file for SRPM builds
"""

import os
import simplejson
from copr_rpmbuild.automation.base import AutomationTool
from copr_rpmbuild.helpers import locate_spec, Spec


class SRPMResults(AutomationTool):
"""
Create `results.json` for SRPM builds
"""

@property
def enabled(self):
"""
Do this for every RPM build
"""
return not self.chroot

def run(self):
"""
Create `results.json`
"""
data = self.get_package_info()
path = os.path.join(self.resultdir, "results.json")
with open(path, "w", encoding="utf-8") as dst:
simplejson.dump(data, dst, indent=4)

def get_package_info(self):
"""
Return ``dict`` with interesting package metadata
"""
spec_path = locate_spec(self.resultdir)
spec = Spec(spec_path)
keys = ["name", "exclusivearch", "excludearch"]
return {key: getattr(spec, key) for key in keys}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exclusive arch is a bit different, I think it can be specified per subpackage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a tip for any package that does either of those per subpackage, please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

binutils has ExcludeArch for sub-packages, but it's hidden behind the --with crossbuilds flag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lammps package also has this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

procyon package has ExclusiveArch for its sub-packages, so that should give you test cases for both ExcludeArch and ExclusiveArch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, procyon is even more specific - that's the variant of package with BuildArch: noarch with ExclusiveArch for sub-packages. https://bugzilla.redhat.com/show_bug.cgi?id=1394853

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @tstellar, and @praiskup.
I added one more test for procyon just to make sure the parsing doesn't fail on it.
The PR can IMHO be merged now.

59 changes: 59 additions & 0 deletions rpmbuild/copr_rpmbuild/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import backoff
import rpm
import munch
from specfile import Specfile

from six.moves.urllib.parse import urlparse
from copr_common.enums import BuildSourceEnum
Expand Down Expand Up @@ -101,6 +102,17 @@ def locate_srpm(dirpath):
return srpm_path


def get_rpm_header(path):
"""
Examine a RPM package file and return its header
See docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html/RPM_Guide/ch16s04.html
"""
ts = rpm.TransactionSet()
with open(path, "rb") as f:
hdr = ts.hdrFromFdno(f.fileno())
return hdr


def get_package_name(spec_path):
"""
Obtain name of a package described by spec
Expand Down Expand Up @@ -366,3 +378,50 @@ def is_srpm_build(task):
Return `True` if the `self.source_dict` belongs to a SRPM build task
"""
return task.get("source_type") is not None


class Spec:
"""
Wrapper around `specfile.Specfile` to easily access attributes that we need
"""

def __init__(self, path):
try:
# TODO We want to loop over all name-version chroots and parse the
# spec values for each of them. For that, we will set dist macros.
# It expect tuples like this: [("fedora", "39"), ("foo", "bar")]
macros = []
self.spec = Specfile(path, macros=macros)
except TypeError as ex:
raise RuntimeError("No .spec file") from ex
except OSError as ex:
raise RuntimeError(ex) from ex

self.path = path
self.tags = self.spec.tags(self.spec.parsed_sections.package).content

def __getattr__(self, name):
return getattr(self.spec, name)

@property
def exclusivearch(self):
"""
Evaluated %{exclusivearch} as a list
"""
return self.safe_attr("exclusivearch").split()

@property
def excludearch(self):
"""
Evaluated %{excludearch} as a list
"""
return self.safe_attr("excludearch").split()

def safe_attr(self, name):
"""
Return evaluated spec file attribute or empty string
"""
try:
return getattr(self.tags, name).expanded_value
except AttributeError:
return ""
Loading