Bug 1766157 - Review Request: liburing - Linux-native io_uring I/O access library
Summary: Review Request: liburing - Linux-native io_uring I/O access library
Keywords:
Status: CLOSED CURRENTRELEASE
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: 2019-10-28 13:07 UTC by Stefan Hajnoczi
Modified: 2020-04-07 18:38 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-04-07 18:38:28 UTC
Type: ---
crobinso: fedora-review+


Attachments (Terms of Use)

Description Stefan Hajnoczi 2019-10-28 13:07:25 UTC
Spec URL: https://vmsplice.net/~stefan/liburing.spec
SRPM URL: https://vmsplice.net/~stefan/liburing-0.2-1.src.rpm
Description: Provides native async IO for the Linux kernel, in a fast and efficient manner, for both buffered and O_DIRECT.
Fedora Account System Username: stefanha

Comment 1 Stefan Hajnoczi 2019-10-28 13:10:17 UTC
Applications using the new Linux io_uring system call API use liburing for the userspace ring setup and I/O submission/completion.

liburing 0.2 was recently released and is now in a shape where it can be consumed by applications in Fedora.  QEMU is close to merging io_uring support and depends on liburing.  Therefore I am proposing this package.

Comment 2 Cole Robinson 2019-10-28 15:40:28 UTC
The package doesn't build in 'mock' because it's missing BuildRequires: gcc. Do 'mock liburing-0.2-1.src.rpm' to reproduce, there may be other missing build deps.

* Release should be Release: 1%{?dist}   so the .fcXX bits get appended to the version string
* Source: should be a pointer to the upstream URL that hosts the release. In this case I think it should be https://github.com/axboe/liburing/archive/%{name}-%{version}.tar.gz#%{name}-%{name}-%{version}.tar.gz   , the ending weirdness is due to github renaming the archive strangely. You might need to pass '-n %{name}-%{name}-%{version}' to %setup/%autosetup to tell it what the extracted archive name is
* The %defattr lines should be removed: https://pagure.io/packaging-committee/issue/77
* The Group: lines should be removed
* All the BuildRoot and RPM_BUILD_ROOT lines should be removed. %clean should be removed
* The ./configure line should be replaced with just %configure
* The 'make' call should be %make_build
* The 'make install' call should be %make_install
* The %pre and %post sections can be entirely removed, ldconfig is done automatically: https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets
* The devel package 'Requires: liburing' should instead be: Requires: %{name} = %{version}-%{release}
* The devel package should also have Requires: pkgconfig
* I think all the %attr usage can be entirely removed, unless they are doing something that the build system isn't doing.
* The Provides: liburing.so.1 shouldn't be necessary, I'm pretty sure RPM automatically adds annotations like this
* Replace %setup with %autosetup, which will automatically apply any listed Patch: in the spec if anything is backported in the future. It's a small maintenace optimization

Comment 3 Fabio Valentini 2019-10-28 15:49:15 UTC
(In reply to Cole Robinson from comment #2)
> The package doesn't build in 'mock' because it's missing BuildRequires: gcc.
> Do 'mock liburing-0.2-1.src.rpm' to reproduce, there may be other missing
> build deps.
> 
> * Release should be Release: 1%{?dist}   so the .fcXX bits get appended to
> the version string
> * Source: should be a pointer to the upstream URL that hosts the release. In
> this case I think it should be
> https://github.com/axboe/liburing/archive/%{name}-%{version}.tar.gz#%{name}-
> %{name}-%{version}.tar.gz   , the ending weirdness is due to github renaming
> the archive strangely. You might need to pass '-n
> %{name}-%{name}-%{version}' to %setup/%autosetup to tell it what the
> extracted archive name is

Please don't do the HTML anchor hacks anymore, they haven't been necessary for years. See the SourceURL page in the packaging guidelines how to correctly and nicely handle GitHub tarballs:

https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags

> * The %defattr lines should be removed:
> https://pagure.io/packaging-committee/issue/77
> * The Group: lines should be removed
> * All the BuildRoot and RPM_BUILD_ROOT lines should be removed. %clean
> should be removed
> * The ./configure line should be replaced with just %configure
> * The 'make' call should be %make_build
> * The 'make install' call should be %make_install
> * The %pre and %post sections can be entirely removed, ldconfig is done
> automatically:
> https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets
> * The devel package 'Requires: liburing' should instead be: Requires:
> %{name} = %{version}-%{release}
> * The devel package should also have Requires: pkgconfig
> * I think all the %attr usage can be entirely removed, unless they are doing
> something that the build system isn't doing.
> * The Provides: liburing.so.1 shouldn't be necessary, I'm pretty sure RPM
> automatically adds annotations like this
> * Replace %setup with %autosetup, which will automatically apply any listed
> Patch: in the spec if anything is backported in the future. It's a small
> maintenace optimization

Comment 4 Jeff Moyer 2019-10-31 19:18:22 UTC
Stefan, I hope you don't mind, but I went ahead and made all of the changes requested:

(In reply to Cole Robinson from comment #2)
> The package doesn't build in 'mock' because it's missing BuildRequires: gcc.
> Do 'mock liburing-0.2-1.src.rpm' to reproduce, there may be other missing
> build deps.
> 
> * Release should be Release: 1%{?dist}   so the .fcXX bits get appended to
> the version string
> * Source: should be a pointer to the upstream URL that hosts the release. In
> this case I think it should be
> https://github.com/axboe/liburing/archive/%{name}-%{version}.tar.gz#%{name}-
> %{name}-%{version}.tar.gz   , the ending weirdness is due to github renaming
> the archive strangely. You might need to pass '-n
> %{name}-%{name}-%{version}' to %setup/%autosetup to tell it what the
> extracted archive name is
> * The %defattr lines should be removed:
> https://pagure.io/packaging-committee/issue/77
> * The Group: lines should be removed
> * All the BuildRoot and RPM_BUILD_ROOT lines should be removed. %clean
> should be removed

All done.

> * The ./configure line should be replaced with just %configure

Unfortunately, this configure script only supports the 4 options used in the spec file I'm linking to below, so I had to continue to call ./configure by hand.

> * The 'make' call should be %make_build
> * The 'make install' call should be %make_install
> * The %pre and %post sections can be entirely removed, ldconfig is done
> automatically:
> https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets
> * The devel package 'Requires: liburing' should instead be: Requires:
> %{name} = %{version}-%{release}
> * The devel package should also have Requires: pkgconfig
> * I think all the %attr usage can be entirely removed, unless they are doing
> something that the build system isn't doing.

I double checked, and the build system takes care of installing with the correct permissions.

> * The Provides: liburing.so.1 shouldn't be necessary, I'm pretty sure RPM
> automatically adds annotations like this

I removed the line, and this is what I see:

$ rpm -qp --provides RPMS/x86_64/liburing-0.2-1.el8.x86_64.rpm 
liburing = 0.2-1.el8
liburing(x86-64) = 0.2-1.el8
liburing.so.1()(64bit)
liburing.so.1(LIBURING_0.1)(64bit)
liburing.so.1(LIBURING_0.2)(64bit)

so confirmed.

> * Replace %setup with %autosetup, which will automatically apply any listed
> Patch: in the spec if anything is backported in the future. It's a small
> maintenace optimization

Done.

All good feedback, thanks!

(In reply to Fabio Valentini from comment #3)

> Please don't do the HTML anchor hacks anymore, they haven't been necessary
> for years. See the SourceURL page in the packaging guidelines how to
> correctly and nicely handle GitHub tarballs:
> 
> https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
> #_git_tags

Noted.  Here's the updated URL line, based on those guidelines:

URL: https://github.com/axboe/liburing/archive/%{name}-%{version}/%{name}-%{version}.tar.gz

The tag is "liburing-0.2", which means that the tarball is named "liburing-liburing-0.2.tar.gz".  That's unfortunate, but as Cole noted, we can just pass that name to %autosetup.

You can find the resulting spec file and package here:

http://people.redhat.com/jmoyer/liburing/liburing.spec
http://people.redhat.com/jmoyer/liburing/liburing-0.2-1.el8.src.rpm

Thanks again for the feedback!

-Jeff

Comment 5 Jeff Moyer 2019-10-31 19:25:32 UTC
NOTE: I did not try to do a mock build.

Comment 6 Jeff Moyer 2019-10-31 19:59:54 UTC
Ah, Jens hosts release tarballs here:
  http://brick.kernel.dk/snaps/

So we can get rid of the liburing-liburing-<ver>.tar.gz nonsense.  \o/

Comment 7 Stefan Hajnoczi 2019-11-04 12:30:36 UTC
Excellent, Jeff.  Thanks for making the changes!

The /usr/lib64/pkgconfig/liburing.pc version number is incorrect (0.1).  The .spec file (where the version number is extracted by the makefile) wasn't updated when 0.2 was tagged.

I have sent a patch series with your changes as well as the version number fix:
https://lore.kernel.org/linux-block/20191104120532.32839-1-stefanha@redhat.com/T/#t

Here is the latest version of this package (it includes 1 patch to fix liburing.pc's version number):
https://vmsplice.net/~stefan/liburing.spec
https://vmsplice.net/~stefan/liburing-0.2-1.fc31.src.rpm

Comment 8 Cole Robinson 2019-11-11 16:51:53 UTC
Since %configure can't be used, I think you'll want to do

  %set_build_flags
  ./configure ...

That's what the %configure macro does essentially. See rpm --eval "%configure" or the
macro definition in /usr/lib/rpm/macros

Things flagged by fedora-review:

* Use '%license COPYING' instead of %doc

* The package ships .a static libraries. Are these required? If not they should be removed.
  I think typically ./configure scripts will have a way to opt in to building these, but
  just rm'ing them before %make_install is probably sufficient, or using %exclude.
  https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries

* Source: should be the full URL format that URL: has. URL: should point to the webpage for
  the project, or git repo if it doesn't have one.

* The devel package dep needs to be tweaked to:
  Requires: %{name}%{?_isa} = %{version}-%{release}

I think that's it

Comment 9 Stefan Hajnoczi 2019-12-13 10:09:37 UTC
(In reply to Cole Robinson from comment #8)
> Since %configure can't be used, I think you'll want to do
> 
>   %set_build_flags
>   ./configure ...
> 
> That's what the %configure macro does essentially. See rpm --eval
> "%configure" or the
> macro definition in /usr/lib/rpm/macros
> 
> Things flagged by fedora-review:
> 
> * Use '%license COPYING' instead of %doc
> 
> * The package ships .a static libraries. Are these required? If not they
> should be removed.
>   I think typically ./configure scripts will have a way to opt in to
> building these, but
>   just rm'ing them before %make_install is probably sufficient, or using
> %exclude.
>  
> https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-
> libraries
> 
> * Source: should be the full URL format that URL: has. URL: should point to
> the webpage for
>   the project, or git repo if it doesn't have one.
> 
> * The devel package dep needs to be tweaked to:
>   Requires: %{name}%{?_isa} = %{version}-%{release}
> 
> I think that's it

Thanks for the feedback!  I have updated the spec and rebuilt the RPMs.

https://vmsplice.net/~stefan/liburing.spec
https://vmsplice.net/~stefan/liburing-0.2-1.fc31.src.rpm

Comment 10 Cole Robinson 2019-12-15 20:15:10 UTC
Looks good, setting fedora-review+

Comment 11 Gwyn Ciesla 2020-01-06 21:43:16 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/liburing

Comment 12 Cole Robinson 2020-04-07 18:38:28 UTC
This has been built for f31+


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