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

Conversation

FrostyX
Copy link
Member

@FrostyX FrostyX commented Jun 11, 2023

Fix #2137

I am updating the existing tools to still run only for RPM builds, so
this commit changes nothing, only allows for creating automation that
can be hooked after SRPM builds.
srpm = locate_srpm(self.resultdir)
hdr = get_rpm_header(srpm)
keys = ["name", "exclusivearch", "excludearch"]
return {key: hdr[key] for key in keys}
Copy link
Member Author

Choose a reason for hiding this comment

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

I know we agreed on using python3-specfile but I tried it and couldn't easily get the exclusivearch from it so I used python3-rpm instead. We already use it on some places, so it was trivial to use here.

IMHO there is no advantage in using one over the other, so I would just pick whatever is easiest to use. But if we really want specfile, I can try to figure out how to use it.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like OK for the first step at least. specfile could resolve some some issues like

%if 0%{?fedora} > 39
ExcludeEarch: x86_64
%endif

Now thinking again, are such spec files actually resolvable on the single builder that did source build?

Copy link
Member

Choose a reason for hiding this comment

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

@nforro can you provide a guidance to @FrostyX please?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO there is no advantage in using one over the other, so I would just pick whatever is easiest to use.

With specfile you can get ExclusiveArch like this:

spec = Specfile(...)
with spec.tags() as tags:
    try:
        exclusivearch = tags.exclusivearch.value
    except AttributeError:
        exclusivearch = None

or (to get all of them):

spec = Specfile(...)
with spec.tags() as tags:
    exclusivearches = [t.value for t in tags if t.normalized_name == "Exclusivearch"]

IMHO there is no advantage in using one over the other, so I would just pick whatever is easiest to use.

There is a difference, with specfile you can access all tags, even those that are enclosed in conditions that would resolve to false on the system where the code is running, while rpm gives you only resolved tags.

BTW, you can use specfile and still access the underlying rpm.spec object with Specfile.rpm_spec property.

Copy link
Member

Choose a reason for hiding this comment

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

Can I affect the conditions, to get the answer to question like "What is the ExclusiveArch value for Fedora 38+"?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, expanded_value is fine, but that doesn't answer the question; we want expanded_value on specific distribution (e.g. RHEL 9). Can we use python-specfile for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. You can specify macros you want to be defined before every expansion (you can't delete macros loaded from macro files, but you can redefine them):

spec = Specfile("python-specfile.spec", macros=[("fedora", "39"), ("rhel", "")])

# explicitly specify parsed preamble section here, so that macros and conditions are resolved
# be aware though that if you make any changes the original section will be overwritten
with spec.tags(spec.parsed_sections.package) as tags:
    try:
        exclusivearch = tags.exclusivearch.expanded_value
    except AttributeError:
        exclusivearch = None

# for read-only access it's probably better to avoid the context manager
tags = spec.tags(spec.parsed_sections.package).content
try:
    exclusivearch = tags.exclusivearch.expanded_value
except AttributeError:
    exclusivearch = None

You can do the same thing with rpm, but it would be rather cumbersome.

Copy link
Member

Choose a reason for hiding this comment

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

This way of working with spec files is quite revolutionary, thanks.

Copy link
Member Author

@FrostyX FrostyX Jun 13, 2023

Choose a reason for hiding this comment

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

Thank you for the code example @nforro, worked for me perfectly.
I updated the PR.

Copy link
Member

Choose a reason for hiding this comment

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

I submitted a documentation request packit/specfile#243

@FrostyX
Copy link
Member Author

FrostyX commented Jun 11, 2023

The #1701 added the python-specfile into the copr-backend.spec, and I don't quite feel this is ideal.
Would you @FrostyX mind moving that (spec / SRPM analysis from backend to builder, and create the
results.json or alike for SRPM builds? (similar to what we have for BuildChroot handling)

I agree, and I have no problem to add it to this PR or to create a follow-up PR for it. But first, let's figure out the exclude-arch part.

This is needed because otherwise backend can set some chroots as
skipped after the SRPM build is completed but then DistGit imports the
package and sets all (even the skipped) chroots as pending.
@FrostyX
Copy link
Member Author

FrostyX commented Jun 12, 2023

@tstellar, if the results for your package looked like this, would that be okay?

Screenshot_2023-06-12_20-51-26

@tstellar
Copy link
Contributor

The UI looks good, what will the json returned by the monitor API call look like?

@FrostyX
Copy link
Member Author

FrostyX commented Jun 12, 2023

Like this:

[
    {
        "build_id": 2914549,
        "chroot": "fedora-37-ppc64le",
        "name": "biosdevname",
        "pkg_version": "0.7.3-11",
        "state": "skipped"
    }
    ,
    {
        "build_id": 2914549,
        "chroot": "fedora-37-x86_64",
        "name": "biosdevname",
        "pkg_version": "0.7.3-11",
        "state": "succeeded"
    }
    ,
    {
        "build_id": 2914549,
        "chroot": "fedora-37-aarch64",
        "name": "biosdevname",
        "pkg_version": "0.7.3-11",
        "state": "skipped"
    }
    ,
    {
        "build_id": 2914549,
        "chroot": "fedora-37-s390x",
        "name": "biosdevname",
        "pkg_version": "0.7.3-11",
        "state": "skipped"
    }
    ,
    {
        "build_id": 2914549,
        "chroot": "fedora-37-armhfp",
        "name": "biosdevname",
        "pkg_version": "0.7.3-11",
        "state": "skipped"
    }
]

Please ignore the build ID difference from the screenshot.

@tstellar
Copy link
Contributor

@FrostyX That looks great. Thank You!

# PURPOSE. See the GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see http://www.gnu.org/licenses/.
Copy link
Member

Choose a reason for hiding this comment

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

Copyright year does not match. And the header is too long. Can you either omit it or use the two-liner from https://github.com/david-a-wheeler/spdx-tutorial#spdx-license-expressions-in-source-code-files ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

chroots+=" --chroot fedora-$FEDORA_VERSION-s390x"

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

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.


def __init__(self, path):
try:
macros = [("fedora", "39"), ("rhel", "")]
Copy link
Member Author

Choose a reason for hiding this comment

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

We should loop over all chroots in the future. For now, we should have macros = [] and put a TODO into the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@praiskup praiskup merged commit 48c4d99 into fedora-copr:main Jun 27, 2023
9 checks passed
@FrostyX
Copy link
Member Author

FrostyX commented Jul 17, 2023

I finally created an issue for rpm.org, to have the macros documented for sub-packages
rpm-software-management/rpm-web#43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Builds on excluded architectures should be given a result other than 'Failed'
5 participants