Bug 480103 - Review Request: bnIRC - An ncurses based IRC client and modular IRC framework.(Need Sponsorship. First time Packager)
Review Request: bnIRC - An ncurses based IRC client and modular IRC framework...
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2009-01-14 20:53 EST by Tom Wisniewski
Modified: 2012-06-29 18:31 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-04-09 23:49:41 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
rpmlint output (8.43 KB, text/plain)
2009-01-19 17:40 EST, Fabian Affolter
no flags Details

  None (edit)
Description Tom Wisniewski 2009-01-14 20:53:55 EST
Spec URL: http://dev.zerogin.com/bnIRC.spec
SRPM URL: http://dev.zerogin.com/bnIRC-1.1.1-2.fc10.src.rpm
Description: Hi.  This is the first time I'm trying to contribute an RPM to Fedora and would love for someone to sponsor me and review my rpm.  All criticism is welcome :).  

bnIRC is a ncurses based IRC client as well as a modular IRC framework.  It can easily be extended through the creation of plugins written in Python.
Comment 1 Fabian Affolter 2009-01-15 06:54:17 EST
Just some quick comments on your spec file.

- There is no need for '%define name bnIRC' and '%define version 1.1.1' because 'Name:' and 'Version:' can be used as macros later.
  https://fedoraproject.org/wiki/PackageMaintainers/CreatingPackageHowTo#Macros

- Source0: should point to the upstream location of the tarball.
  https://fedoraproject.org/wiki/Packaging/SourceURL

- 'BuildRoot:' please use on of the examples in the guidelines
  https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

- Your %description is too long.  Didn't rpmlint complain about this?

- Please preserve the time stamps in your %install section if possible
  make install DESTDIR=%{buildroot} INSTALL="install -p"

- You are using '%post -p /sbin/ldconfig' and '%postun -p /sbin/ldconfig'.  Aren't 'Requires(post):   /sbin/ldconfig' and 'Requires(postun): /sbin/ldconfig' missing?

- Please use one of the formating style from the guidelines for your %changelog entry
  https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs
Comment 2 Tom Wisniewski 2009-01-15 20:49:00 EST
Thanks for your input.  Much appreciated.  I went ahead and made the appropriate changes and uploaded a new set of rpm's.  I have to admit, the %changelog thing took me a while to figure out.  I kept staring at it and staring at it, but not seeing what's wrong.  I then noticed that I was missing the version/release number :)

I had a look through the Fedora Guidelines for the %post ldconfig sections but didn't see any reference to 'Requires(post): /sbin/ldconfig.  I then did a quick google search and came across the following bugzilla entry which mentions that '%post -p /sbin/ldconfig' notation automatically mentions 'Requires(post): /sbin/ldconfig'.  So I left 'Requires(post):' out.  If it's really needed, please let me know and I'll add it in.

https://bugzilla.redhat.com/show_bug.cgi?id=426599


When running rpmlint against the RPM, I do have errors.  They seem to have something to do with ownership of directories.  Here is a sample:
bnIRC.i386: E: standard-dir-owned-by-package /usr/share
Not quite sure what I can do differently in my spec file to fix this.  I can only assume I would have to make changes to my %files section and/or %defattr, but I have no idea what to change. 

Once again, any and all feedback/guidance is really appreciated.

Thanks.
Comment 3 Tom Wisniewski 2009-01-16 23:25:55 EST
Forgot to provide the new links. 

SRC RPM: http://dev.zerogin.com/bnIRC-1.1.1-3.fc10.src.rpm
SPEC FILE: http://dev.zerogin.com/bnIRC.spec
Comment 4 Fabian Affolter 2009-01-19 05:47:14 EST
I will do a full review soon but be aware I can't sponsor you.

https://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored
Comment 5 Fabian Affolter 2009-01-19 17:40:49 EST
Created attachment 329407 [details]
rpmlint output

There are still some issues.

- From my point of view, the name should be bnirc.spec
  https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Case_Sensitivity
  https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Spec_file_name
- One line per BR would be nice
- The %file section needs some work
   - duplicates
   - ownership
- You need to make a devel subpackage
- *.la files must be deleted
  https://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries


The rpmlint output

[fab@laptop024 i386]$ rpmlint bnIRC*
bnIRC.i386: W: devel-file-in-non-devel-package /usr/src/debug/bnIRC-1.1.1/plugins/server_strings/server_strings.c
....
....
....
bnIRC.i386: W: unstripped-binary-or-object /usr/lib/debug/usr/bin/bnirc.debug
bnIRC.i386: E: statically-linked-binary /usr/lib/debug/usr/bin/bnirc.debug
2 packages and 0 specfiles checked; 22 errors, 75 warnings.

see attachment for full details
Comment 6 Michael Schwendt 2009-02-09 15:41:10 EST
> Aren't 'Requires(post):   /sbin/ldconfig' and 'Requires(postun):
> /sbin/ldconfig' missing?

No, they are automatic if /sbin/ldconfig is set as scriptlet processor via option -p.

[...]

* The entire /usr tree is mispackaged: Package must not include directories 

  /usr/include
  /usr/lib
  /usr/lib/debug
  /usr/share
  /usr/share/man
  /usr/share/man/man1
  /usr/src
  /usr/src/debug

and no files below /usr/src and /usr/lib/debug either.

Where files below /usr/include and /usr/lib are needed (in the -devel subpackage), prefix the paths with %_includedir and %_libdir.

Use %_mandir as prefix for files below /usr/share/man

* It must not include /usr/lib/debug/ as those files are automatically put into the -debuginfo subpackage.

* It must not include /usr/share and not anything in /usr/src either, which is another side-effect of using %_prefix/* as a bad catch-all for all files below /usr

* Including static libs as plugins makes no sense. It likely loads the *.so or *.so.0 files. Perhaps the *.la, but not the *.a libs.

* The %doc file "INSTALL" is irrelevant to your package users.

* The %doc file "NEWS" is empty. You can remove it for now and add a guard in %prep which exists if NEWS is larger than zero. Then you can include it.

* rpmlint also reports an executable .spec file.

* Including the "config.h" autoheader file in the public API is dangerous. Values in it bear the risk of conflicting with any API-user that uses an own config.h file.
Comment 7 Tom Wisniewski 2009-02-13 15:57:14 EST
Alright.  Finally found some free time to work on this again.  I have split the rpm into two packages, the rpm and a devel rpm.  The links are below. Please have a look and let me know what else needs to be changed.  Thanks again for all suggestions/feedback.

Spec URL:      http://dev.zerogin.com/bnIRC.spec
RPM URL:       http://dev.zerogin.com/bnIRC-1.1.1-5.fc10.i386.rpm
SRPM URL:      http://dev.zerogin.com/bnIRC-1.1.1-5.fc10.src.rpm
DEVEL RPM URL: http://dev.zerogin.com/bnIRC-devel-1.1.1-5.fc10.i386.rpm
Comment 8 Michael Schwendt 2009-03-07 05:08:32 EST
> -License:       GPLv2+
> +License:       GPLv2

Confirmed. The source files explicitly say "LICENSE: GPL Version 2".


> +%package devel

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

> +Group:         Applications/Internet

That sounds wrong for the bnIRC-devel package. More likely the group is "Development/Libraries". Even if the package contained just a plugin API, there would not be a more accurate RPM Group.


> -%post -p /sbin/ldconfig
> -
> -%postun -p /sbin/ldconfig

Deleting them is not right. The previous .spec file was correct. Put them back.


> -%{_datadir}/%{name}-%{version}
> +%{_datadir}/%{name}-%{version}/*

With this change, the directory is not included. Please revert.
https://fedoraproject.org/wiki/Packaging/UnownedDirectories


> +%files devel
> +%defattr(-,root,root,-)
> +%{_prefix}/src/*

Don't include %_prefix/src. These are included in the automatically generated -debuginfo package. If that doesn't work for you, install the "redhat-rpm-config".


> +%{_libdir}/bnIRC/*

These are the application's plugins. They belong into the main package.


> +%{_libdir}/libbnirc.a
> +%{_libdir}/libbnirc.la

These are not needed and must not be included. You can %exclude it or remove it in the %install section.
http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries


> +%{_libdir}/libbnirc.so

This is the softlink that really belongs into the -devel package. It is needed when compiling/linking with -lbnirc


> +%{_libdir}/libbnirc.so.0
> +%{_libdir}/libbnirc.so.0.0.0

These two belong into the main application package.


Your %changelog doesn't comment on several of the spec changes between release 3 and 5. It is good practise to document and explain non-trivial modifications.
Comment 9 Tom Wisniewski 2009-03-13 16:40:36 EDT
Alright, I made some changes based on the feedback and they're all available at the links below

SPEC: http://dev.zerogin.com/bnIRC.spec
RPM:  http://dev.zerogin.com/bnIRC-1.1.1-6.fc10.i386.rpm
SRPM: http://dev.zerogin.com/bnIRC-1.1.1-6.fc10.src.rpm
DEVEL RPM: http://dev.zerogin.com/bnIRC-devel-1.1.1-6.fc10.i386.rpm

eed9f0123b0695c63072eeeb37a66114 bnIRC-1.1.1-6.fc10.i386.rpm
a836f791a84132e0cdc280ddf7ea8867 bnIRC-1.1.1-6.fc10.src.rpm
8f22430b1299a368a1ebc93ef75bbeb0 bnIRC-devel-1.1.1-6.fc10.i386.rpm

Now that I added some of the libs to the main package I get warnings when I run rpmlint. Since they're plugins, they do belong in the main package like mentioned in the previous comment made by Michael Schwendt.  Not sure if I need to do something differently to get rid of the warnings or if they can be left alone.  

Any and all comments are welcome.  thanks.
Comment 10 Michael Schwendt 2009-03-13 17:43:12 EDT
* The plugin loader evaluates the libtool .la files and dlopen()s the library with the file name found in the "dlname=" parameter, e.g. libdcc.so.0, which in turn is a symlink to libdcc.so.0.0.0

The statically linked plugins 'lib*.a' are not needed as they cannot be loaded at run-time.

The plugin symlinks 'lib*.so' are not needed either.

The program could be patched to simply name the plugins 'lib*.so' and dlopen() them directly instead of looking at the .la files.


* Please look at "rpm --query --provides bnIRC". Currently, the plugin libraries produce several automatic SONAME Provides, which bear the risk of causing conflicts with other packages during dependency resolving:

libctcp.so.0  
libdcc.so.0  
libdebug.so.0  
libhello.so.0  
libio_ncurses.so.0  
libirc_input.so.0  
libpython.so.0  
librserver.so.0  
libserver_strings.so.0  

This is a blocker, even if one could show that no other Fedora package currently provides libraries with the same SONAMEs. I haven't tried that, but I could imagine packages such as "libdcc", "libctcp", "librserver", for example, with similar library sonames.

The package also contains automatic "Requires" for the same library SONAMEs. The least thing that could be done is to filter these self-Provides and self-Requires out. Various docs exist, in the Wiki and on Google,
http://fedoraproject.org/wiki/PackagingDrafts/FilteringAutomaticDependencies

but disabling rpmbuild's internal dependency generator is dangerous, and one must carefully examine the results.

It would be good, if upstream could use a unique namespace for these plugins, e.g. like libbnirc_plugin_foo.so.0


> %post devel -p /sbin/ldconfig
> %postun devel -p /sbin/ldconfig

These are a no-op and can be deleted. The scriptlets in the main pkg are the ones that are correct and needed.


> %{_libdir}/bnIRC/*

Directory %{_libdir}/bnIRC is not included.
https://fedoraproject.org/wiki/Packaging/UnownedDirectories
Comment 11 Tom Wisniewski 2009-03-14 11:34:10 EDT
Thanks for the quick reply.
I'll make sure to remove the post and postun sections from the devel package.  I'll also remove the asterisk from the %{_libdir}/bnIRC/* line.

You say that the library names are still a major blocker for this package.  I just want to make sure I understand your suggestion.  If I were to talk to upstream and have them rename the libraries, that is all that would be needed for me to get this package approved?  Or would I still have to jump through some hoops to have the libraries accepted?

Please let me know and I'll try talking to upstream about your suggestions.

Thanks again.
Comment 12 Michael Schwendt 2009-03-15 06:34:25 EDT
If all the plugin libraries (and specifically their library SONAME values) were renamed to put them into a namespace that is much more specific to this application, that would make it unnecessary to filter rpmbuild's automatic Provides/Requires. The risk that any other library package would introduce a shared library with a SONAME like libbnirc_plugin_SOMETHING.so.0 would be very low. And as such I would approve that as a valid work-around.

[...]

Here's a run-time error:

RegisterTab called
added python tab hook
Traceback (most recent call last):

  File "/usr/share/bnIRC-1.1.1/scripts/toc.py", line 15, in <module>

    
import whrandom

ImportError
: 
No module named whrandom


script error!
Comment 13 Tom Wisniewski 2009-03-31 08:41:57 EDT
So as I wait for upstream to make the necessary changes, I have a quick question about package naming.  Since upstream is making changes to the code, they will most likely release it as 1.1.2.  How should I deal with this in my rpm?  Right now my rpm is called bnIRC-1.1.1-6.fc10.src.rpm.  Once the newer version of source is out, would I name my rpm bnIRC-1.1.2-7.fc10.src.rpm, basically changing the software version number and incrementing the release number by 1, or do I have to start the release numbering from scratch since it's a new upstream version number?  So the new name should be bnIRC-1.1.2-1.fc10.src.rpm.

Just trying to prepare for when upstream makes the required changes.

thanks.
Comment 14 Michael Schwendt 2009-03-31 14:03:30 EDT
If you increase the %version, you can and should reset %release to 1:
http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Release

[Basically, package 1.1.2-1.fc10 means "the 1st release/build of version 1.1.2 for Fedora 10"...]
Comment 15 Mamoru TASAKA 2009-05-22 04:31:19 EDT
What is the status of this bug?
Comment 16 Tom Wisniewski 2009-05-22 09:20:33 EDT
Currently waiting for the dev of this program to make some changes to the names of certain libraries.  I have confirmation from the dev that he's working on it but life has been getting in the way and delaying the changes.  As soon as I have a new version of the software, I'll make a new RPM and upload it for review.
Comment 17 Tom Wisniewski 2009-07-03 16:10:47 EDT
Good news.  Upstream made requested changes(renaming libraries) so I had a chance to create new packages.  Please review and let me know if there is anything else that needs changing.

SPEC: http://dev.zerogin.com/bnIRC.spec
RPM: http://dev.zerogin.com/bnIRC-1.1.2-1.fc10.i386.rpm
SRPM: http://dev.zerogin.com/bnIRC-1.1.2-1.fc10.src.rpm
DEVEL: http://dev.zerogin.com/bnIRC-devel-1.1.2-1.fc10.i386.rpm


830c2a3d2ac694ac23900f35805e8ff4 bnIRC-1.1.2-1.fc10.i386.rpm

050865e2fcf07c2bc9c8e210392231fe bnIRC-1.1.2-1.fc10.src.rpm

cfb5e3af3f1f2f5403c1b4ba0381e68b bnIRC-devel-1.1.1-2.fc10.i386.rpm
Comment 18 Michael Schwendt 2009-07-09 02:39:18 EDT
* It fails to build on Fedora 11:

| channel.c:146: warning: conflicting types for built-in function 'log'
| user.c: In function 'users_in_channel':
| user.c:299: warning: passing argument 4 of 'qsort' from incompatible pointer type
/usr/include/stdlib.h:710: note: expected '__compar_fn_t' but argument is of type 'int (*)(void *, void *)'
| regex.c:34: error: static declaration of 'strndup' follows non-static declaration
| make[1]: *** [regex.lo] Error 1

Indeed, regex.c includes <string.h> and declares its own one just a few lines further down in the file.


* It doesn't adhere to the compiler flags guidelines:
  https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

The flags you should see in the build log are those printed by "rpm --eval %{optflags}". Since %configure exports them (see "rpm --eval %configure"), but the bnirc source tarball doesn't accept the variables passed in from the outside, it may be necessary to apply a patch.


* Issues pointed out in bottom of comment 10 are not fixed yet.
Comment 19 Tom Wisniewski 2009-07-09 15:41:01 EDT
hmm...I was sure I fixed the suggestions about ldconfig and the libdir.  Must have got my spec files confused.  Guess that's what happens when you don't use version control :/

I'll check with upstream to see what can be done about the compiler flags and I'll start testing in F11.

Thanks for the review.
Comment 20 Jason Tibbitts 2009-09-22 19:46:51 EDT
Any update?
Comment 21 Tom Wisniewski 2009-09-23 19:49:39 EDT
Waiting to hear back from upstream about possible fixes/changes.  Will definitely update the bug when I hear something new.
Comment 22 Jason Tibbitts 2010-11-02 17:15:30 EDT
Well, it's been well over a year now.  Should this just be closed now?  In any case it does not build properly on current rawhide:

regex.c:34:14: error: static declaration of 'strndup' follows non-static declaration

so I'll mark this as not building.  Please clear the whiteboard if providing a version that builds.
Comment 23 David Nalley 2011-04-09 23:49:41 EDT
tibbs it's been 6 months, I am going to clear FE-NEEDSPONSOR (and the whiteboard) and add FE-DEADREVIEW and close this ticket.

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