Bug 817306 - Review Request: libircclient - C library to create IRC clients
Summary: Review Request: libircclient - C library to create IRC clients
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Thomas Spura
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: libircclient (view as bug list)
Depends On:
Blocks: 771885 817315
TreeView+ depends on / blocked
 
Reported: 2012-04-28 20:01 UTC by Paulo Andrade
Modified: 2012-08-05 21:24 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2012-08-05 21:19:52 UTC
Type: ---
Embargoed:
tomspur: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Paulo Andrade 2012-04-28 20:01:50 UTC
Spec URL: http://kenobi.mandriva.com/~pcpa/libircclient.spec
SRPM URL: http://kenobi.mandriva.com/~pcpa/libircclient-1.6-1.fc16.src.rpm
Description: C library to create IRC clients

This is a small package required to build yet another package
I would like to contrib to fedora.

Comment 1 Thomas Spura 2012-04-29 15:10:08 UTC
- # Prefer static build because it only generates an unversioned .so
  %configure --disable-shared --enable-openssl --enable-ipv6

Why not building a versioned .so then?

That would make it way easier to update this package, without updating metaglest each time...

When you have a static library, this MUST go into a -static subpackage. So the -devel must provide it in this case, but I highly prefer building a versioned .so and ask upstream to do so too...

In both cases the devel package needs to require the main package (if existent), e.g. in your libminiupnpc package.

- License is wrong:
src/ and include/ is LGPLv3+
in cocoa/ are BSD/MIT/LGBLv2+ which are all GPL compat according to:
http://fedoraproject.org/wiki/Licensing:Main
but unused in the build.

To be on the save side, it'd be best to "rm -rvf cocoa" them in %prep...

-->> License is LGPLv3+

- Are the patches send upstream?
  Please make a note on that in the spec file

- Development/C is a non-standard-group, I'd use Development/Libraries here too.

Comment 2 Paulo Andrade 2012-04-30 17:36:12 UTC
Thanks for the review. I will attempt to upload a newer
package for review later, removing the cocoa subdir and
correcting the package group.

I just opened a bug report in the sourceforge tracker
for libircclient, uploaded the patches and suggested a
shared library build. See

https://sourceforge.net/tracker/?func=detail&aid=3522604&group_id=118640&atid=681658

I was unsure about naming conventions, so did look at
what was available, and noticed that, at least based
on descriptions, static libraries are install in plain
-devel packages.

Comment 3 Thomas Spura 2012-04-30 19:56:58 UTC
(In reply to comment #2)
> I was unsure about naming conventions, so did look at
> what was available, and noticed that, at least based
> on descriptions, static libraries are install in plain
> -devel packages.

When that's the case, that packages are violating the guidelines, as those -devel packages MUST provide a -static package in that case (and I bet they do too).
If you find a package, where it's not the case, please open a bug report against that package.

When having the shared library, like you suggested in the sf tracker and when the library is properly packaged like described in [1], I'll sponsor you to the packager group.

Your FAS account name is also pcpa, is that right?


[1] http://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries and above/below it

Comment 4 Paulo Andrade 2012-05-01 18:03:42 UTC
Thanks again for the review.

I uploaded updated spec and srpm on top of the
previous ones.

Yes, my FAS account is pcpa.

I opened a generic bug report about inconsistency
with static library packages and guidelines at

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

but besides there are only 3 packages that are
either in error or are a special case, I did not
open one per package, as in the same bug report
I listed inconsistency in summary of a large
amount of packages and mix of -devel and
-devel-static.

About the package, I actually implemented the
suggestion I made in the upstream bug report,
and attached it to the report, so that now it
by default creates a shared library.

I also rebuilt the megaglest package to ensure
it still works correctly. Actually, with a shared
libircclient, The megaglest patch
megaglest-3.6.0.3-openssl.patch is no longer
required, as now libircclient "pulls" the equivalent
of "-lssl -lcrypto"

Comment 5 Thomas Spura 2012-05-01 19:10:27 UTC
(In reply to comment #4)
> Thanks again for the review.
> 
> I uploaded updated spec and srpm on top of the
> previous ones.

Please always bump the release, add a changelog entry and describe what you have changed and publish a new src.rpm with the new release entry.
Then link to the new spec/src.rpm in the new comment, so you don't need to search where the spec/src.rpm is etc. I promise, it will make sense and is not just a nitpick ;)

> Yes, my FAS account is pcpa.

Thanks.

> I opened a generic bug report about inconsistency
> with static library packages and guidelines at
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=817888

I'm afraid, there needs to be a bug for all packages, that don't comply with the guidelines as Bill writes there... :(

> About the package, I actually implemented the
> suggestion I made in the upstream bug report,
> and attached it to the report, so that now it
> by default creates a shared library.

Great.

> I also rebuilt the megaglest package to ensure
> it still works correctly. Actually, with a shared
> libircclient, The megaglest patch
> megaglest-3.6.0.3-openssl.patch is no longer
> required, as now libircclient "pulls" the equivalent
> of "-lssl -lcrypto"

Great^2 :)


Continuing the review:
- cocoa is still there
- license correct now
- It doesn't compile, as you seem to have an old libirc library installed, when building, but when not having it installed, the new build library isn't found:
gcc -o spammer spammer.o -L../src/ -lircclient -lpthread -lssl -lcrypto  -lnsl
/usr/bin/ld: cannot find -lircclient
collect2: error: ld returned 1 exit status

Doing a "ln -s libircclient.so.0 src/libircclient.so" helps to build it.
--> A "ln -s libircclient.so.$(MAJOR) libircclient.so" is missing in the patch.

- rpmlint issues:

$ rpmlint /home/tomspur/rpmbuild/SRPMS/libircclient-1.6-1.fc17.src.rpm /home/tomspur/rpmbuild/RPMS/x86_64/libircclient-1.6-1.fc17.x86_64.rpm /home/tomspur/rpmbuild/RPMS/x86_64/libircclient-devel-1.6-1.fc17.x86_64.rpm /home/tomspur/rpmbuild/RPMS/x86_64/libircclient-debuginfo-1.6-1.fc17.x86_64.rpm
libircclient.src: I: enchant-dictionary-not-found en_US
libircclient.src:30: W: mixed-use-of-spaces-and-tabs (spaces: line 30, tab: line 1)
libircclient.x86_64: W: no-documentation
libircclient-devel.x86_64: W: summary-not-capitalized C libircclient development files.
libircclient-devel.x86_64: W: summary-ended-with-dot C libircclient development files.
libircclient-devel.x86_64: W: no-documentation
libircclient-debuginfo.x86_64: E: empty-debuginfo-package
4 packages and 0 specfiles checked; 1 errors, 5 warnings.

Please correct mixed-use-of-spaces-and-tabs, summary-not-capitalized, summary-ended-with-dot.

The empty-debuginfo-packages is normally a sign, when a package didn't apply the optflags, but that seems to be the case.
In this case, you are building the library with "-s", which strips the library. Please remove that.
More information about that here:
http://fedoraproject.org/wiki/Packaging:Debuginfo

Comment 6 Paulo Andrade 2012-05-01 19:51:33 UTC
  Sorry for not properly testing in a clean environment
to check that it was linking to the installed libircclient.
I now validated it with koji --scratch builds.

  The cocoa not being removed was a typo, sorry.

  I now also added the irc rfc that was in the sources,
as well as the pre generated doxygen files to %doc in
the -devel package.

  I updated the patch in the srpm and in the upstream trac
to create the symlink earlier to find -lirclient in -L../src
and removed the -s to not strip the library. Actually I was
in doubt about -s earlier, but left it that way as upstream
should have had some reason for it.

Comment 7 Paulo Andrade 2012-05-05 00:34:52 UTC
Was forgetting to bump release, update changelog, etc.

Updated spec and srpm:

Spec URL: http://kenobi.mandriva.com/~pcpa/libircclient.spec
SRPM URL: http://kenobi.mandriva.com/~pcpa/libircclient-1.6-2.fc16.src.rpm

Comment 8 Thomas Spura 2012-05-05 08:26:42 UTC
- source matches upstream:
  eb6a2c4e91862cc10de3b13b198cfa23  libircclient-1.6.tar.gz
- lib correctly installed
- R ok

One last thing:
Please add Changelog LICENSE THANKS to %doc in the main %files section.
(At least adding LICENSE is a MUST.)

Looks fine otherwise.

###################################################

APPROVED

###################################################

Welcome to the packager group.
Next steps:
- SCM admin request:
  http://fedoraproject.org/wiki/Package_SCM_admin_requests
- add the package to git & build (& ship updates via bodhi):
  http://fedoraproject.org/wiki/Using_Fedora_GIT
  http://admin.fedoraproject.org/updates

Comment 9 Paulo Andrade 2012-05-05 16:10:08 UTC
Uploaded new spec and srpm to include documentation in the main package.

Spec URL: http://kenobi.mandriva.com/~pcpa/libircclient.spec
SRPM URL: http://kenobi.mandriva.com/~pcpa/libircclient-1.6-3.fc16.src.rpm

Comment 10 Paulo Andrade 2012-05-05 16:14:29 UTC
New Package SCM Request
=======================
Package Name: libircclient
Short Description: C library to create IRC clients
Owners: pcpa
Branches: f16 f17
InitialCC:

Comment 11 Gwyn Ciesla 2012-05-06 21:02:24 UTC
Git done (by process-git-requests).

Comment 12 Paulo Andrade 2012-05-19 19:29:34 UTC
*** Bug 700818 has been marked as a duplicate of this bug. ***

Comment 13 Paulo Andrade 2012-07-02 19:21:26 UTC
Package is available in rawhide for quite some time already.

Comment 14 Itamar Reis Peixoto 2012-07-18 00:44:34 UTC
why you dont have submited a update for f16 ?

Comment 15 Paulo Andrade 2012-07-18 15:00:50 UTC
(In reply to comment #14)
> why you dont have submited a update for f16 ?

If you have need of this package then I can submit an update,
otherwise, I am waiting for another review request, that this
package is build requires
https://bugzilla.redhat.com/show_bug.cgi?id=828544 approved,
and the real one that I still would like to see in fc16
https://bugzilla.redhat.com/show_bug.cgi?id=817315

Comment 16 Itamar Reis Peixoto 2012-07-18 15:04:59 UTC
if you have requested a branch for f16 and f17 then you should submit a update.

Comment 17 Itamar Reis Peixoto 2012-07-18 15:07:42 UTC
please submit it into stable for f16 and f17

Comment 18 Paulo Andrade 2012-07-18 15:33:35 UTC
Ok. I think I need some help. From several review requests, this package
was the one I got sponsored, so I am still new to fedora. I had earlier
built it for f16 and f17:

$ koji buildinfo libircclient-1.6-3.fc16
BUILD: libircclient-1.6-3.fc16 [317898]
State: COMPLETE
Built by: pcpa
Volume: DEFAULT
Task: 4061632 build (f16-candidate, /libircclient:f6db33f527018aa2ca9a4523f76fd3970ed6a35e)
Finished: Tue, 08 May 2012 00:23:38 EDT
Tags: trashcan
RPMs:
/mnt/koji/packages/libircclient/1.6/3.fc16/i686/libircclient-1.6-3.fc16.i686.rpm
/mnt/koji/packages/libircclient/1.6/3.fc16/i686/libircclient-debuginfo-1.6-3.fc16.i686.rpm
/mnt/koji/packages/libircclient/1.6/3.fc16/i686/libircclient-devel-1.6-3.fc16.i686.rpm
/mnt/koji/packages/libircclient/1.6/3.fc16/x86_64/libircclient-debuginfo-1.6-3.fc16.x86_64.rpm
/mnt/koji/packages/libircclient/1.6/3.fc16/x86_64/libircclient-1.6-3.fc16.x86_64.rpm
/mnt/koji/packages/libircclient/1.6/3.fc16/x86_64/libircclient-devel-1.6-3.fc16.x86_64.rpm
/mnt/koji/packages/libircclient/1.6/3.fc16/src/libircclient-1.6-3.fc16.src.rpm

and requesting an update, after editing the template file I just see:
$ fedpkg switch-branch f16
Switched to branch 'f16'
$ fedpkg update
Creating a new update for  libircclient-1.6-3.fc16 
Password for pcpa: 
Creating a new update for  libircclient-1.6-3.fc16 
libircclient-1.6-3.fc16 not tagged as an update candidate

Guess I should somehow change the
Tags: trashcan
line

Comment 19 Itamar Reis Peixoto 2012-07-18 16:04:08 UTC
try via updates webpage

https://admin.fedoraproject.org/updates

Comment 20 Thomas Spura 2012-07-18 16:07:03 UTC
koji package link:
http://koji.fedoraproject.org/koji/packageinfo?packageID=13846

trashcan means garbage collecting and deleting, if you don't retag it and ship as an update:
http://fedoraproject.org/wiki/Koji/GarbageCollection

(You should have gotten an mail about this from koji.)

To resolve this, this needs to be retagged manually, which requires admin permissions in koji, so a ticket at rel-eng [1] or live help in #fedora-devel is needed. (I'm unsure, if you also have the proper permission, when the build was your own, or if you always need an admin.)

In this case, Dennis Gilmore was so kind to fix it, so you should be able to do the updates now.


[1] https://fedorahosted.org/rel-eng/

Comment 21 Paulo Andrade 2012-07-18 16:28:54 UTC
Thanks!
I suppose I should also (attempt to) update miniupnpc:
http://koji.fedoraproject.org/koji/packageinfo?packageID=14014

Just reopened
https://bugzilla.redhat.com/show_bug.cgi?id=817311

Comment 22 Fedora Update System 2012-07-30 21:51:19 UTC
libircclient-1.6-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/libircclient-1.6-3.fc17

Comment 23 Fedora Update System 2012-07-30 21:51:33 UTC
libircclient-1.6-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/libircclient-1.6-3.fc16

Comment 24 Fedora Update System 2012-08-01 18:23:02 UTC
Package libircclient-1.6-3.fc16:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing libircclient-1.6-3.fc16'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-11354/libircclient-1.6-3.fc16
then log in and leave karma (feedback).

Comment 25 Fedora Update System 2012-08-05 21:19:52 UTC
libircclient-1.6-3.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 26 Fedora Update System 2012-08-05 21:24:10 UTC
libircclient-1.6-3.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, 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.