Bug 1943526 - Review Request: libuev - Simple event loop for Linux
Summary: Review Request: libuev - Simple event loop for Linux
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Otto Liljalaakso
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2021-03-26 11:08 UTC by Alessio
Modified: 2021-10-01 06:51 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-05-12 05:41:41 UTC
Type: ---
Embargoed:
otto.liljalaakso: fedora-review+


Attachments (Terms of Use)

Description Alessio 2021-03-26 11:08:00 UTC
Spec URL: https://alciregi.fedorapeople.org/uredir/libuev.spec
SRPM URL: https://alciregi.fedorapeople.org/uredir/libuev-2.3.2-1.fc34.src.rpm
Description: libuEv is a small event loop that wraps the Linux epoll() family
of APIs. It is similar to the more established libevent, libev 
and the venerable Xt(3) event loop. The µ in the name refers to 
both its limited feature set and the size impact of the library.
Fedora Account System Username: alciregi

Comment 1 Alessio 2021-03-26 11:09:10 UTC
It is needed in order to build uredir (https://github.com/troglobit/uredir), an UDP port redirector.

Comment 2 Alessio 2021-03-26 12:31:29 UTC
Koji scratch build 
https://koji.fedoraproject.org/koji/taskinfo?taskID=64631903

Comment 3 Otto Liljalaakso 2021-03-30 20:57:00 UTC
I will review. This looks simple enough, but is the first time I encounter libtool and pkgconfig, so please be patient if I need a bit of time to understand the build system.

Issues found by running fedora-review and inspecting output follow. This is just the first look at the package, so there may still be other issues that automation cannot detect:

- Libtool archives (.la) should not be packaged
  Note: libuev : /usr/lib64/libuev.la
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries

- Development (unversioned) .so files should be in the -devel subpackage.
  Note: Unversioned so-files directly in %_libdir.
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages

- Static library libuev.a should not be included at all, the shared object
  should be used by all dependencies. If the static library is really required
  for some reason, it should go to -static subpackage.
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries

- Rpmlint reveals the following problems:

    libuev.x86_64: W: incoherent-version-in-changelog 3.3-1 ['2.3.2-1.fc35', '2.3.2-1']
    libuev.src:33: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 33)

- Complains about the use of AM_CONFIG_HEADER found in: libuev-2.3.2/configure.ac:7
  This is just an extra item, however it would be nice to contact upstream about using its replacement.
  See: https://ftp.gnu.org/old-gnu/Manuals/automake-1.7.2/html_chapter/automake_5.html#SEC23

Also I noticed a strange thing: the rpm both Requires and Provides libuev.so.2()(64bit). I need to look deeper into that, how that happened and is that a problem.

Comment 4 Alessio 2021-03-31 10:07:00 UTC
Thank you very much.

New files:

Spec URL: https://alciregi.fedorapeople.org/uredir/libuev.spec
SRPM URL: https://alciregi.fedorapeople.org/uredir/libuev-2.3.2-1.fc34.src.rpm

Comment 5 Otto Liljalaakso 2021-03-31 21:32:43 UTC
Great, it is getting better. Another set of findings still:

- Main package documentation could be added: AUTHORS, ChangeLog, README

- Documentation for -devel:
    - There is %{_pkgdocdir} which resolves to %{_docdir}/%{name}
    - But do not do that if also %doc is used, reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation
    - At the moment, part of the material goes to /usr/share/doc/libuev, other part to /usr/share/doc/libuev-devel. Everything marked as -devel package docs should go to -devel directory. I think using %doc only would correct this.
    - In examples, .gitignore is not useful and should not be included. Probably
    the same also holds for Makefiles

- Directories without known owners: /usr/share/doc/libuev
  I think this comes from the problem above where -devel installs docs to main package's directory. You could run fedora-review after the changes and see if the error remains. Or submit the new files and I will run it as usual.
  Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_directory_ownership

I slightly worry over licensing and libuev-2.3.2/src/bench.c which has different license and copyright owner than the rest. But, it seem to be built separately from the library and not installed, so it does not require License entry for binary rpm, and includes the license in the file itself, so source rpm licensing is ok as well. So, I think it can be like it is.

Apart from these, everything looks good. fedora-review was unable to download Source, but perhaps that was a transient failure. I will ignore that for now and see how it goes for the next version.

Comment 6 Alessio 2021-04-01 09:55:06 UTC
Ok.

I have a doubt. If I don't put LICENSE under %doc, I get
    Installed (but unpackaged) file(s) found:
   /usr/share/doc/libuev/LICENSE

So, %license macro is still necessary? Or is it fine to have the LICENSE file under /usr/share/doc/libuev/LICENSE and under /usr/share/licenses/libuev/LICENSE?

New files:
Spec URL: https://alciregi.fedorapeople.org/uredir/libuev.spec
SRPM URL: https://alciregi.fedorapeople.org/uredir/libuev-2.3.2-1.fc34.src.rpm

Comment 7 Otto Liljalaakso 2021-04-01 11:28:33 UTC
%license must be always used, it puts the license to standard license file location in Fedora, which is /usr/share/licenses. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/

The complain from unstalled but unpackaged files appears, because upstream build script installs the LICENSE to buildroot (to docs it seems), then rpm detects that there is a file installed in buildroot that is not handled in %files. There are at least two ways to fix this: Copy the license to docs as you were already doing. The it appears both in /usr/share/licenses and /usr/share/doc. That is kind of redundant, but not a big deal. Or get rid of the file with 'rm /unwanted/file' in the end of %install section, which avoids the duplication.

The warning can also be silenced by using %exclude in %files, but apparently it will stop working in the future: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/RWUGI5HAPH6FTM46QUC534ZE4HLMMOSG/

I will take a look at the new files later.

Comment 8 Alessio 2021-04-01 17:04:11 UTC
Great. Thank you for the explanation.

Comment 9 Otto Liljalaakso 2021-04-02 11:53:35 UTC
I found one important issue still. I apologize for not bringing this up earlier, I simply missed it before now:

- Shared object should not be installed with glob %{_libdir}/%{name}.so.*, %{_libdir}/%{name}.so.2* would be better to avoid accidentally making an abi breaking update. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files

An then two minor items I just note but will not insist on:

- -devel package description mentions static libary files, but none are installed. Perhaps that part can be omitted.

- API.md would fit -devel package docs better, since it is interesting only for development. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation say this:

> For example API documentation belongs in the -devel subpackage, not the main one.

Comment 10 Robert-André Mauchin 🐧 2021-04-02 15:11:32 UTC
 - This disable default ldflags:

LDFLAGS="-fPIE" --disable-static

Use LDFLAGS="%build_ldflags -fPIE" --disable-static

 - In order to avoid unintentional soname bump, we recommend not globbing the major soname vension, be more specific instead:

 %{_libdir}/%{name}.so.*

 - Own %{_includedir}/uev/ or remove the glob

 - explicitly BR make

Comment 11 Alessio 2021-04-02 20:55:30 UTC
Since there are two so files (libuev.so.2.2.0 and libuev.so.2), using %{_libdir}/%{name}.so.2* is ok?
Thanks.

Comment 12 Alessio 2021-04-02 20:58:14 UTC
(In reply to Robert-André Mauchin 🐧 from comment #10)
> Use LDFLAGS="%build_ldflags -fPIE" --disable-static

Yes. Right.
But if I use this, checks fails with:

/usr/bin/ld: active.o: relocation R_X86_64_32 against `.rodata' can not be used when making a PIE object; recompile with -fPIE

Comment 13 Alessio 2021-04-02 23:13:56 UTC
(In reply to Alessio from comment #12)
> (In reply to Robert-André Mauchin 🐧 from comment #10)
> > Use LDFLAGS="%build_ldflags -fPIE" --disable-static
> 
> Yes. Right.
> But if I use this, checks fails with:
> 
> /usr/bin/ld: active.o: relocation R_X86_64_32 against `.rodata' can not be
> used when making a PIE object; recompile with -fPIE

Ah.
This is because in tests/Makefile the CFLAGS variable is "hard coded".

tests/Makefile.am contains 
CFLAGS          = -W -Wall -Wextra -Wno-unused-result -Wno-unused-parameter

So, after running autogen, tests/Makefile.in will contain such stuff, instead of "CFLAGS = @CFLAGS@"

Comment 14 Alessio 2021-04-02 23:48:53 UTC
Thank you for your patience and your advises.

These are the new files:
Spec URL: https://alciregi.fedorapeople.org/uredir/libuev.spec
SRPM URL: https://alciregi.fedorapeople.org/uredir/libuev-2.3.2-1.fc34.src.rpm

Comment 15 Robert-André Mauchin 🐧 2021-04-06 20:15:20 UTC
(In reply to Alessio from comment #11)
> Since there are two so files (libuev.so.2.2.0 and libuev.so.2), using
> %{_libdir}/%{name}.so.2* is ok?
> Thanks.

Yes it is ok.

Comment 16 Otto Liljalaakso 2021-04-06 21:57:12 UTC
(In reply to Alessio from comment #14)
> Thank you for your patience and your advises.
> 
> These are the new files:
> Spec URL: https://alciregi.fedorapeople.org/uredir/libuev.spec
> SRPM URL:
> https://alciregi.fedorapeople.org/uredir/libuev-2.3.2-1.fc34.src.rpm

Looks good. Only two points brought up by  Robert-André Mauchin 🐧  are still unresolved:

- Own %{_includedir}/uev/ or remove the glob. So you could do in %files simply this and get both directory ownership and the contents installed (reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_directory_ownership):

    %{_includedir}/uev 

- explicitly BR make:

    BuildRequires: make

Comment 17 Alessio 2021-04-07 07:53:05 UTC
Ok.

...
BuildRequires:  make
...
%files devel
...
%{_includedir}/uev


These are the new files:
Spec URL: https://alciregi.fedorapeople.org/uredir/libuev.spec
SRPM URL: https://alciregi.fedorapeople.org/uredir/libuev-2.3.2-1.fc34.src.rpm

Comment 18 Otto Liljalaakso 2021-04-07 12:02:34 UTC
Great! Everything seems to be in order now, review passed. You may now request a repository for this package as explained at https://fedoraproject.org/wiki/Package_Review_Process#Contributor

Thank you for contributing to Fedora!

Comment 19 Alessio 2021-04-07 12:10:07 UTC
Yay! Thank you for your assistance.

Comment 20 Gwyn Ciesla 2021-04-07 13:15:57 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/libuev

Comment 21 Fedora Update System 2021-04-07 14:37:35 UTC
FEDORA-2021-6d7cab8d70 has been submitted as an update to Fedora 34. https://bodhi.fedoraproject.org/updates/FEDORA-2021-6d7cab8d70

Comment 22 Fedora Update System 2021-04-07 18:16:30 UTC
FEDORA-2021-6d7cab8d70 has been pushed to the Fedora 34 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-6d7cab8d70 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-6d7cab8d70

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 23 Otto Liljalaakso 2021-05-11 18:42:02 UTC
Should the Fedora 34 update be pushed to stable so that libuev would be available in Fedora 34 or has the plan been changed to only include this in Fedora 35? I am asking because a) a build for Fedora 34 has been submitted, but not pushed to stable, and b) bug 1954212, which, as far as I can tell, would be closed by pushing this.

Comment 24 Fedora Update System 2021-05-12 05:41:41 UTC
FEDORA-2021-6d7cab8d70 has been pushed to the Fedora 34 stable repository.
If problem still persists, please make note of it in this bug report.


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