Bug 2092965 - Review Request: libkrunfw - A dynamic library bundling the guest payload consumed by libkrun
Summary: Review Request: libkrunfw - A dynamic library bundling the guest payload cons...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Cole Robinson
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-06-02 16:03 UTC by Sergio Lopez
Modified: 2022-06-17 11:38 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2022-06-17 11:38:03 UTC
Type: ---
Embargoed:
crobinso: fedora-review+


Attachments (Terms of Use)

Comment 1 Cole Robinson 2022-06-05 18:49:52 UTC
Before the kernel `Source:` line can you add a comment that bundling of the kernel package was approved by fedora kernel maintainers, and link to the discussion: https://lists.fedorahosted.org/archives/list/kernel@lists.fedoraproject.org/thread/2TMXPCE2VWF7USZA7OHQ3P2SBJAEGCSX/

* Add a comment explaining the exclusivearch. `package only buildable for $ARCHES` is enough IMO

* Typo 'pacakge'
* Use %make_install PREFIX=%{_prefix}

BuildRequires: I see the list is copied from kernel distgit but a lot of that is redundant and possibly not even used with your libkrunfw config. First you can remove everything that's explicitly listed in the buildroot: https://pagure.io/fedora-comps/blob/main/f/comps-f37.xml.in#_324  For a f36 x86_64 build I got it down to:

# libkrunfw + packaging requirements
BuildRequires:  gcc
BuildRequires:  git-core
BuildRequires:  make
BuildRequires:  python3-pyelftools

# kernel build requirements
BuildRequires:  bison
BuildRequires:  flex
BuildRequires:  bc
BuildRequires:  elfutils-devel

But no runtime testing, and I didn't try dropping bison+flex+bc, I just saw they were in build output.

Comment 2 Sergio Lopez 2022-06-06 11:34:51 UTC
(In reply to Cole Robinson from comment #1)
> Before the kernel `Source:` line can you add a comment that bundling of the
> kernel package was approved by fedora kernel maintainers, and link to the
> discussion:
> https://lists.fedorahosted.org/archives/list/kernel@lists.fedoraproject.org/
> thread/2TMXPCE2VWF7USZA7OHQ3P2SBJAEGCSX/
> 
> * Add a comment explaining the exclusivearch. `package only buildable for
> $ARCHES` is enough IMO
> 
> * Typo 'pacakge'
> * Use %make_install PREFIX=%{_prefix}
> 
> BuildRequires: I see the list is copied from kernel distgit but a lot of
> that is redundant and possibly not even used with your libkrunfw config.
> First you can remove everything that's explicitly listed in the buildroot:
> https://pagure.io/fedora-comps/blob/main/f/comps-f37.xml.in#_324  For a f36
> x86_64 build I got it down to:
> 
> # libkrunfw + packaging requirements
> BuildRequires:  gcc
> BuildRequires:  git-core
> BuildRequires:  make
> BuildRequires:  python3-pyelftools
> 
> # kernel build requirements
> BuildRequires:  bison
> BuildRequires:  flex
> BuildRequires:  bc
> BuildRequires:  elfutils-devel
> 
> But no runtime testing, and I didn't try dropping bison+flex+bc, I just saw
> they were in build output.

You nailed the required packages, I've tried removing "bison", "flex" and "bc" individually and all of them are required. We also need "perl-interpreter" on aarch64.

I've produced a new COPR build with the new spec. I think I've applied all your suggestions, please let me know if I missed something:

- https://download.copr.fedorainfracloud.org/results/slp/libkrunfw/fedora-rawhide-x86_64/04501528-libkrunfw/libkrunfw.spec
- https://download.copr.fedorainfracloud.org/results/slp/libkrunfw/fedora-rawhide-x86_64/04501528-libkrunfw/libkrunfw-2.1.1-1.fc37.src.rpm

Thanks!

Comment 3 Cole Robinson 2022-06-06 14:56:40 UTC
Looks good to me now, setting fedora-review+

Comment 4 Gwyn Ciesla 2022-06-06 19:57:14 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/libkrunfw

Comment 5 Sergio Lopez 2022-06-17 11:38:03 UTC
The package has hit rawhide, thanks Cole and Gwyn!


Note You need to log in before you can comment on or make changes to this bug.