Bug 480103

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

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.

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

I will do a full review soon but be aware I can't sponsor you.

https://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

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

> 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.

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

> -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.

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.

* 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

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.

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!

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.

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"...]

What is the status of this bug?

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.

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

* 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.

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.

Any update?

Waiting to hear back from upstream about possible fixes/changes.  Will definitely update the bug when I hear something new.

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.

tibbs it's been 6 months, I am going to clear FE-NEEDSPONSOR (and the whiteboard) and add FE-DEADREVIEW and close this ticket.