Bug 510402 - Review Request: znc - An advanced IRC bouncer
Review Request: znc - An advanced IRC bouncer
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: David Nalley
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-09 00:09 EDT by Nick Bebout
Modified: 2009-08-04 16:28 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-22 23:47:17 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
david: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nick Bebout 2009-07-09 00:09:23 EDT
Spec URL: http://nb.fedorapeople.org/znc.spec
SRPM URL: http://nb.fedorapeople.org/znc-0.070-1.fc11.src.rpm
Description:

ZNC is an IRC bounce with many advanced features like detaching,
multiple users, per channel playback buffer, SSL, IPv6, transparent
DCC bouncing, Perl and C++ module support to name a few.
Comment 1 Nick Bebout 2009-07-09 00:16:35 EDT
Below is the rpmlint output.  

The no-documentation warning is because the documentation is all under the main znc package.  

[nb@nb SPECS]$ rpmlint znc.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

[nb@nb SRPMS]$ rpmlint znc-0.070-1.fc11.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[nb@nb result]$ rpmlint *.rpm
znc-admin.i586: W: no-documentation
znc-autoattach.i586: W: no-documentation
znc-autocycle.i586: W: no-documentation
znc-autoop.i586: W: no-documentation
znc-away.i586: W: no-documentation
znc-awaynick.i586: W: no-documentation
znc-awayping.i586: W: no-documentation
znc-chansaver.i586: W: no-documentation
znc-crypt.i586: W: no-documentation
znc-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/znc-0.070/modules/q.cpp
znc-devel.i586: W: no-documentation
znc-email.i586: W: no-documentation
znc-fail2ban.i586: W: no-documentation
znc-fixfreenode.i586: W: no-documentation
znc-imapauth.i586: W: no-documentation
znc-keepnick.i586: W: no-documentation
znc-kickrejoin.i586: W: no-documentation
znc-log.i586: W: no-documentation
znc-modperl.i586: W: no-documentation
znc-nickserv.i586: W: no-documentation
znc-partyline.i586: W: no-documentation
znc-perform.i586: W: no-documentation
znc-q.i586: W: no-documentation
znc-raw.i586: W: no-documentation
znc-sample.i586: W: no-documentation
znc-saslauth.i586: W: no-documentation
znc-savebuff.i586: W: no-documentation
znc-schat.i586: W: no-documentation
znc-shell.i586: W: no-documentation
znc-simple-away.i586: W: no-documentation
znc-stickychan.i586: W: no-documentation
znc-watch.i586: W: no-documentation
znc-webadmin.i586: W: no-documentation
35 packages and 0 specfiles checked; 0 errors, 33 warnings.
Comment 2 Nick Bebout 2009-07-09 23:58:45 EDT
As requested on IRC, here is a new spec and SRPM with all the modules being packaged in one.

SPEC: http://nb.fedorapeople.org/znc.spec
SRPM: http://nb.fedorapeople.org/znc-0.070-2.fc11.src.rpm

[nb@nb SPECS]$ rpmlint znc.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[nb@nb SRPMS]$ rpmlint znc-0.070-2.fc11.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[nb@nb i586]$ rpmlint *
znc-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/znc-0.070/modules/q.cpp
znc-devel.i586: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 2 warnings.
Comment 3 David Nalley 2009-07-09 23:59:44 EDT
Had this conversation with nb in irc re the 22 subpackages. 

23:27 < ke4qqq> np - mind answerin a question or two?
23:28 < ke4qqq> nb ^^ /me can't type apparently
23:28 < nb> sure
23:29 < ke4qqq> so can you tell me why so many subpackages - are they really
all necessary to subpackage? esp since so many of them seem to be a 
                single file. 
23:30 < nb> hrmm
23:30  * nb was doing like the old spec i based them off of
23:31 < nb> i think the idea was some people may not want all the modules
23:31 < nb> although i could see making it all one package since they arent
that big
23:31 < nb> and you have to load them anyway
23:31 < nb> either via webadmin or the config or once you are on irc
23:31 < nb> either via webadmin or the config or once you are on irc
23:32 < ke4qqq> so you can interactively/programmatically load the modules -
ie, it's not a memory issue with all of them being loaded ?
23:32 < nb> yeah, they dont get loaded unless you tell it to
23:34 < ke4qqq> give me just a minute
23:41 < ke4qqq> so it looks like it would build 22 sub-packages - and while I
can't really find anything that specifically talks about what qualifies 
                something as a package, I think it greatly complicates things
without a lot of advantages - I could see saving the ssl stuff, perl, 
                sasl etc as subpackages. 
23:41 < ke4qqq> but you may also want to seek another opinion other than mine
as well
23:42 < mujahid> its been a while.;
23:42 < nb> i could see that
23:42 < mujahid> lol how so?
23:42  * nb can put the rest besides perl sasl and ssl in the main package
23:43 < nb> iirc the modules arent that big of files
23:43 < nb> or would it be ok just to BuildRequires: everything and put
everything in the one package?
23:44 < ke4qqq> that would strike me as ok as well - perhaps even logical - not
many systems are going to be without perl or ssl, sasl might not be as 
                common, but it's a small dependency
23:45 < ke4qqq> where did you get the old spec? 
23:46 < nb> let me get the link
23:47 < nb> http://home.ircnet.de/cru/znc/sources/znc-0.052-4.cru.src.rpm
23:47 < nb> its a old version
23:48 < ke4qqq> did it have a changelog? 
23:48 < nb> no
23:48  * nb is building a version with all in 1 package
23:48 < ke4qqq> weird - ok I have a few other comments as well that I'll add to
the review. 
23:48 < nb> ok




Also - the other sources should probably be noted as to where they came from. 
For instance - that you can get the znc-log code from here: 
http://cnu.dieplz.net/code/znc/log/znc_log-0.002.tar.bz2
(it's listed as a separate source file.) 

Look more at this shortly.
Comment 4 David Nalley 2009-07-10 00:11:33 EDT
You probably also want to strip this from the description:

Available rebuild switches:
  --without ipv6   Build without IPv6 connection support.
  --with debug     Build debugging binaries.


The rpms you are providing don't really have that option (they could rebuild the rpms, but not helpful in the package description really.
Comment 5 Susi Lehtola 2009-07-10 12:10:24 EDT
What are those extra modules? I find putting 3rd party stuff in a bit questionable..
Comment 6 Nick Bebout 2009-07-10 23:28:20 EDT
As requested, here is a new spec and SRPM with only the modules from the regular ZNC package.

SPEC: http://nb.fedorapeople.org/znc.spec
SRPM: http://nb.fedorapeople.org/znc-0.070-3.fc11.src.rpm
Comment 7 David Nalley 2009-07-11 17:39:59 EDT
So I think that you can also drop the logic about whether or not to be include debug. It should be included by default.
Comment 8 Susi Lehtola 2009-07-11 18:24:54 EDT
- The source URL site is incorrect, instead of dl.sourceforge.net it should be downloads.sourceforge.net

- Instead of
 %{_libdir}/znc/*.so
 %{_libdir}/znc/modperl.pm
 %{_datadir}/znc/webadmin/skins/*
the main package should own
 %{_libdir}/znc/
 %{_datadir}/znc/

- Instead of
 %{_includedir}/znc/*
the devel package should own
 %{_includedir}/znc/
Comment 9 Nick Bebout 2009-07-11 19:41:33 EDT
As requested, here is a new spec and SRPM with the debug logic removed (having it included by default, and with the updated %files section

SPEC: http://nb.fedorapeople.org/znc.spec
SRPM: http://nb.fedorapeople.org/znc-0.070-3.fc11.src.rpm
Comment 10 David Nalley 2009-07-12 00:28:01 EDT
Note - SRPM link above is or a previous version - actual version is:
http://nb.fedorapeople.org/znc-0.070-4.fc11.src.rpm

New Note 16

Package Review Guidelines

FIX: rpmlint must be run on every package. The output should be posted in the review.

[ke4qqq@nalleyt61 SPECS]$ rpmlint ./znc.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[ke4qqq@nalleyt61 SPECS]$ rpmlint ../SRPMS/znc-0.070-4.fc11.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[ke4qqq@nalleyt61 SPECS]$ rpmlint ../RPMS/i586/znc*
znc-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/znc-0.070/modules/q.cpp
znc-devel.i586: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

Please take care of the spurious executable perm warning

OK: The package must be named according to the Package Naming Guidelines .
OK: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. 
OK: The package must meet the Packaging Guidelines .
OK: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
GPLv2  
OK: The License field in the package spec file must match the actual license.
Code is a mixture of GPLv2 (bulk) with MD5.cpp being GPLv2+ - so actual package is GPLv2. 

FIX: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc

There is also a LICENSE.openssl that needs to be included in %doc

OK: The spec file must be written in American English.
OK: The spec file for the package MUST be legible. 
OK: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines 

[ke4qqq@nalleyt61 SOURCES]$ md5sum znc-0.070.tar.gz*
18bb813cb350c6db014a0d82ecdf85fe  znc-0.070.tar.gz
18bb813cb350c6db014a0d82ecdf85fe  znc-0.070.tar.gz.1

OK: The package MUST successfully compile and build into binary rpms on at least one primary architecture. 
works at least on x86

NA: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. 
OK: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
NA: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
NA: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. 
NA: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. 
OK: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 
OK: A Fedora package must not list a file more than once in the spec file's %files listings. 
OK: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. 
OK: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
OK: Each package must consistently use macros. 
OK: The package must contain code, or permissable content. 
NA: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). 
OK: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. 
OK: Header files must be in a -devel package. 
NA: Static libraries must be in a -static package. 
OK: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). 
NA: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. 
OK: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} 
OK: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
OK: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. 
OK: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. 
OK: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
OK: All filenames in rpm packages must be valid UTF-8. 


This is pretty close - should be just a couple of quick fixes here
Comment 11 Nick Bebout 2009-07-12 00:53:53 EDT
These have been fixed.

SPEC: http://nb.fedorapeople.org/znc.spec
SRPM: http://nb.fedorapeople.org/znc-0.070-5.fc11.src.rpm  

[nb@nb SPECS]$ rpmlint znc.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

[nb@nb SRPMS]$ rpmlint znc-0.070-5.fc11.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[nb@nb result]$ rpmlint znc*.rpm
znc-devel.i586: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 1 warnings.
Comment 12 David Nalley 2009-07-12 01:03:11 EDT
Sources no longer match upstream:

[ke4qqq@nalleyt61 SOURCES]$ md5sum ./znc-0.070.tar.gz*
def618c8d74017908b90c91f38047836  ./znc-0.070.tar.gz
18bb813cb350c6db014a0d82ecdf85fe  ./znc-0.070.tar.gz.1


That coupled with the fact that the perms error is gone suggests that you changed source by chmod -x the offending file, and then committed that source to your SRPM. Please take care of the chmod in %prep, otherwise no one else will know of the changes that you made - eg, essentially you just patched source (albeit EXTREMELY minimally)  but no patch file and no record of it.
Comment 14 Susi Lehtola 2009-07-12 04:41:32 EDT
.. except that OpenSSL is incompatible with GPL
http://fedoraproject.org/wiki/Licensing#Bad_Licenses

so the license tag is incorrect..?
Comment 15 Nick Bebout 2009-07-12 16:13:38 EDT
Changed license to be "GPLv2 with exceptions"

SPEC: http://nb.fedorapeople.org/znc.spec
SRPM: http://nb.fedorapeople.org/znc-0.070-7.fc11.src.rpm  

[nb@nb rpmbuild]$ rpmlint SPECS/znc.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[nb@nb rpmbuild]$ rpmlint SRPMS/znc-0.070-7.fc11.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[nb@nb result]$ rpmlint *.rpm
znc-devel.i586: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 1 warnings.
Comment 16 David Nalley 2009-07-12 19:11:20 EDT
I believe that fixes all of the outstanding issues: 


APPROVED
Comment 17 Nick Bebout 2009-07-12 22:04:20 EDT
New Package CVS Request
=======================
Package Name: znc
Short Description: An advanced IRC bouncer
Owners: nb
Branches: F-10 F-11 EL-5
InitialCC: nb
Comment 18 Jason Tibbitts 2009-07-12 22:49:00 EDT
CVS done.
Comment 19 Fedora Update System 2009-07-12 23:22:01 EDT
znc-0.070-7.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/znc-0.070-7.fc11
Comment 20 Fedora Update System 2009-07-19 06:06:25 EDT
znc-0.070-7.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update znc'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-7694
Comment 21 Fedora Update System 2009-07-19 06:12:22 EDT
znc-0.070-7.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update znc'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-7721
Comment 22 Fedora Update System 2009-07-22 21:29:11 EDT
znc-0.072-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/znc-0.072-1.fc11
Comment 23 Fedora Update System 2009-07-22 21:30:11 EDT
znc-0.072-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/znc-0.072-1.fc10
Comment 24 Fedora Update System 2009-07-22 22:07:50 EDT
znc-0.072-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/znc-0.072-2.fc10
Comment 25 Fedora Update System 2009-07-22 22:09:43 EDT
znc-0.072-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/znc-0.072-2.fc11
Comment 26 Nick Bebout 2009-08-04 00:14:29 EDT
Package Change Request
======================
Package Name: znc
New Branches:  EL-4
Owners: nb
Comment 27 Jason Tibbitts 2009-08-04 16:28:20 EDT
CVS done.

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