fedora-copr / copr

RPM build system - upstream for https://copr.fedorainfracloud.org/
113 stars 62 forks source link

The tooltip for *skipped* build state is confusing #2900

Closed hroncok closed 1 year ago

hroncok commented 1 year ago

Hello.

In https://copr.fedorainfracloud.org/coprs/churchyard/pytest-7.4.1/build/6376075/ the build was skipped.

In case this build is already deleted, you can reproduce by:

$ COPR=xxx
$ copr create $COPR --chroot fedora-rawhide-x86_64 --delete-after-days 30
$ copr add-package-distgit $COPR --webhook-rebuild off --commit 5694d64439a6fc73a223acd369fe2ef781216356 --name python-ast-monitor
$ copr build-package $COPR --nowait --name python-ast-monitor

It took me a while to figure out what happened.

The package has:

ExclusiveArch: %{qt6_qtwebengine_arches} noarch

The builder-live.log.gz has:

Output: ['python-ast-monitor-0.4.0-3.src.rpm']
Running SRPMResults tool
Package info: {
    "name": "python-ast-monitor",
    "epoch": null,
    "version": "0.4.0",
    "release": "3",
    "exclusivearch": [
        "%{qt6_qtwebengine_arches}",
        "noarch"
    ],
    "excludearch": []
}
SRPMResults finished

Apparently, the OS that builds SRPMs in Copr does not know the value of %{qt6_qtwebengine_arches} -- hence the value is interpreted literally and the architecture (x86_64) is skipped.

This is inconvenient but somehow understandable.

However, when hovering over the skipped state, the tooltip says:

This package has already been built previously.

That's not true at all.

screenshot

FrostyX commented 1 year ago

Thank you for the report @hroncok. Skipping chroots based on ExclusiveArch was introduced in the latest release https://docs.pagure.org/copr.copr/release-notes/2023-08-16.html#skip-chroots-based-on-excludearch-and-exclusivearch

You are right about the "skipped" state tooltip, I forgot to update it.

praiskup commented 1 year ago

I'm curious whether we could provide, aside the build_chroot.status field, also provide something like .status_reason[str] field.

praiskup commented 1 year ago

Triage time: this seems like a bug; the exclusivearch macro should be expanded in the build-live.log:

"exclusivearch": [
   "%{qt6_qtwebengine_arches}",
hroncok commented 1 year ago

%{qt6_qtwebengine_arches} is only defined (in the default buildroot) on Fedora 39+

praiskup commented 1 year ago

Uhm, what's the way out of this, then? @hroncok any suggestion?

hroncok commented 1 year ago

Building SRPMs on target chroot.

praiskup commented 1 year ago

That's not an option I'm afraid, users may build for XY target chroots. At the point we are making the decision whether we should or shouldn't build the chroot we don't actually need SRPM (we have it, but by accident and history).

praiskup commented 1 year ago

Copr is in a situation with (a) specfile in hand, (b) matrix of target chroots and (c) host system (currently Fedora 37). And we need to decide whether to allocate builder for particular chroots, or skip some of the chroot builds..

hroncok commented 1 year ago

For the record, I opened this issue to report the confusing tooltip, not to redesign the way copr works :)

praiskup commented 1 year ago

I know, but the situation is not ideal and worth brainstorming! :-) so thank you for your ideas and the original report.

praiskup commented 1 year ago

Sorry, I wasn't completely accurate. Jakub moved the logic parsing spec files onto our builder -> so it is Fedora 38. Still, the builder parses the spec file just once for the whole chroot matrix.

We could take an unexpanded macro in exclu*arch statements as an excuse to try the build everywhere. The worst thing that might happen is that we will end-up with a build failure... still better than status quo IMVHO where we actually did not start a potentially successful build.

Though, I fear this is still sub-optimal. What if Fedora 38 vs. Fedora 39 says:

- %qt6_qtwebengine_arches x86_64 noarch
+ %qt6_qtwebengine_arches x86_64 s390x noarch

This is not easily solvable.

FrostyX commented 1 year ago

However, when hovering over the skipped state, the tooltip says:

This package has already been built previously.

That's not true at all.

@praiskup do we still use the skipped status for this purpose? We AFAIK don't. I think we could keep this tooltip for build status and change the tooltip for chroots to

This chroot was skipped because of ExcludeArch or ExclusiveArch

To be sure maybe change the tooltip only for such chroots where the chroot is skipped but the build itself isn't. What do you think?

I am trying to avoid having to add the status_reason column. But we can if this easyfix won't work.

praiskup commented 1 year ago

I am trying to avoid having to add the status_reason column.

Having the status_reason field long-term seems like a good idea to me.

hroncok commented 1 year ago

That's not an option I'm afraid, users may build for XY target chroots. At the point we are making the decision whether we should or shouldn't build the chroot we don't actually need SRPM (we have it, but by accident and history).

What would fix this problem for me is to have the ability to specify the SPRM chroot used for a particular project/package/build. That way, when we do rawhide impact checks in copr, we would configure the SRPM chroot to be rawhide as well.