Spec URL: https://fedoraproject.org/w/uploads/a/ac/Libservicelog.spec SRPM URL: https://fedoraproject.org/w/uploads/0/03/Libservicelog-1.0.1-1.fc11.src.rpm Description: The libservicelgo package contains a library to create and maintain a database for storing events related to system service. This database allows for the logging of serviceable and informational events, and for the logging of service procedures that have been performed upon the system.
First check: I'm a bit worried about the simple groupadd in post and simple groupdel in postun ... service is quite common name for group and I could imagine that group being used for "service" guys on some machines... by simple removal of libservicelog package, you will delete that group without warning - imho too dangerous... I remember case of Amanda (as user) and troubles which were caused by amanda package...
Also, what's the separate user used for - there's nothing obvious in this package that explains why it needs to be this way.
Additionally groupdel in postun would mean deleting that group even on update (as postun is run after post). Other checks: + package builds in mock (development i386). + rpmlint is silent for SRPM + rpmlint is not silent for RPM. - mostly could be ignored: - .so is in lib<package> - servicelog.db file strange permissions are recommended in usptream installation procedure, so I guess required - dangerous groupdel in postun mentioned earlier + source files match upstream. e3280d47a16a29beef586d5b6068a562 libservicelog-1.0.1.tar.gz + package meets naming and packaging guidelines. + specfile is properly named, is cleanly written + Spec file is written in American English. + Spec file is legible. + dist tag is present. + build root is correct. + license is open source-compatible. + License text is included in package. + %doc files present. + BuildRequires are proper. + defattr usage is correct. + %clean is present. + package installed properly. + Macro use appears rather consistent. + Package contains code. + no static libraries. - .pc file present (in devel subpackage). - I guess it is necessary for that package - Requires: pkgconfig is missing for devel subpackage + no .la files. + no translations are available. - Does NOT owns the directories it creates. /var/lib/servicelog has to be owned + no duplicates in %files. + file permissions are appropriate. + Not a GUI app. Summary of things to fix: 1) groupdel in post (consider complete removal) 2) var/lib/servicelog has to be owned 3) Requires: pkgconfig is missing for devel subpackage
https://fedoraproject.org/w/uploads/a/ac/Libservicelog.spec spec file updated. Asked upstream about group service. Waiting to answer...
I recommend to also add - otherwise pre scriptlet will fail for no shadow-utils or for existing service group: Requires(pre): shadow-utils %pre getent group news >/dev/null || /usr/sbin/groupadd service
Sorry, I meant: %post getent group service >/dev/null || /usr/sbin/groupadd service cut & paste is evil ;)
updated
Ok, looks sane for me now (except those pending things [strange mode of servicelog.db and group service existence] now in clarification process with upstream - however those things are not review blockers). APPROVED...
New Package CVS Request ======================= Package Name: libservicelog Short Description: Servicelog Database and Library Owners: rrakus Branches: InitialCC:
Shoudln't this be using a defined system uid/gid, rather than blindly calling groupadd?
If defined system gid, then some of the existing or created via https://fedoraproject.org/wiki/PackageUserRegistry and https://fedoraproject.org/wiki/PackageUserCreation - as there are no free slots under 100 in setup uidgid reservation file. Need for service group is currently questioned on upstream mailing list, so it is possible that it will be removed completely. But IMHO not a review blocker anyway...
I don't see the "Requires(pre): shadow-utils". Did you add that?
Good point - I mentioned that off bugzilla too and AFAIK Roman changed this locally - so I guess he only forgot to update spec file on fedoraproject upload.
ok. cvs done.
Service group will be able to write into database. Every needed program will be in this group. Next question is database file permissions. I will push them as they are now (with executable bit)
libservicelog-1.0.1-1.fc11 just built... Trust I can close this bug...