Bug 433925 - Review Request: libyahoo2 - Library for the Yahoo! Messenger Protocol
Review Request: libyahoo2 - Library for the Yahoo! Messenger Protocol
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-22 00:33 EST by Ray Van Dolson
Modified: 2008-06-01 18:37 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-06-01 18:37:25 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lemenkov: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ray Van Dolson 2008-02-22 00:33:35 EST
Spec URL: http://www.bludgeon.org/~rayvd/rpms/libyahoo2/libyahoo2.spec
SRPM URL: http://www.bludgeon.org/~rayvd/rpms/libyahoo2/libyahoo2-0.7.5-4.src.rpm
Description: libyahoo2 is a C library interface to the new Yahoo! Messenger protocol. It supports almost all current features of the protocol.
Comment 1 Peter Lemenkov 2008-03-24 16:25:16 EDT
I'll review it.
Comment 2 Ray Van Dolson 2008-04-03 13:56:36 EDT
FYI, new spec and SRPM as a result of April 2 protocol bump by Yahoo:

http://www.bludgeon.org/~rayvd/rpms/libyahoo2/libyahoo2.spec
http://www.bludgeon.org/~rayvd/rpms/libyahoo2/libyahoo2-0.7.5-5.src.rpm
Comment 3 Peter Lemenkov 2008-04-03 14:26:01 EDT
Sorry for the delay, Ray.

Some remarks before review.

* You must specify full path to source's tarball, not only
%{name}-%{version}.tar.bz2. For SF-hosted projects we have special requirements
for %Source field - take a look at this page (at the bottom):

http://fedoraproject.org/wiki/Packaging/SourceURL

* You should change your BuildRoot to something like 

BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

* Change these two lines:

iconv -c -t UTF8 AUTHORS > AUTHORS.utf8 || :
iconv -c -t UTF8 NEWS > NEWS.utf8
%{__mv} AUTHORS.utf8 AUTHORS
%{__mv} NEWS.utf8 NEWS

I recommend you to use something like:

iconv -c -t UTF8 AUTHORS > AUTHORS.utf8 && %{__mv} AUTHORS.utf8 AUTHORS

because if iconv fails you won't destroy all contents of your original file. Not
a very probable scenario but still...

* Create a backups of patched files by using key -b. For example:

%patch0 -p1 -b .fh
%patch1 -p1 -b .protocol

Other things looks sane.

Comment 4 Peter Lemenkov 2008-04-05 03:45:52 EDT
BTW well-known instant messenger, Miranda-IM, includes internal version of
libyahoo2. Are there any interesting patches which could be incorporated into
source shipped with this package?
Comment 5 Ray Van Dolson 2008-04-06 12:26:21 EDT
Thanks Peter -- I'll hopefully make the suggested adjustments sometime today.  I
haven't looked at Miranda IM before, but I'm open to including anything
interesting.  Do they base off of 0.7.5 as well?

I've actually gotten commit access to upstream and am encouraging them to do a
0.7.6 release as a result of the April 2nd protocol bump.  libyahoo 0.7.5 is
useless for connecting without this fix which I'm happy to include as a patch,
but would prefer to use upstream's sources if possible of course.  Maybe I'll
just use SVN source here.
Comment 6 Ray Van Dolson 2008-04-06 12:59:54 EDT
One strange issue I am trying to clear up -- this also explains why I was doing:

iconv -c -t UTF8 AUTHORS > AUTHORS.utf8 || :

For some reason, iconv returns false on the above command (without the || :). 
Initially I assumed this was because the file was already in UTF8 format, but
it's not -- rpmlint complains if I don't convert it.

rpmbuild stops however with a non-zero return code from any function.

--verbose wasn't helpful either, so I'll probably do a gdb on the process, but
may just return to forcing a successful return code so that rpmbuild continues
and rpmlint won't complain about the non-UTF8 file.

Give it a try if you like -- just run iconv on the AUTHORS file by hand and echo
$? afterwards.

Comment 7 Ray Van Dolson 2008-04-28 02:11:21 EDT
Upstream (me in this case) released 0.7.6.  I've updated my SRPM and spec file:

http://www.bludgeon.org/~rayvd/rpms/libyahoo2/libyahoo2.spec
http://www.bludgeon.org/~rayvd/rpms/libyahoo2/libyahoo2-0.7.6-1.src.rpm

iconv issue I was experiencing above no longer occurs.
Comment 8 Peter Lemenkov 2008-04-28 03:19:52 EDT
Good news. There is only one issue left:

*Full path to source tarball still missing. You must specify full path to
source's tarball, not only %{name}-%{version}.tar.bz2. For SF-hosted projects we
have special requirements for %Source field - take a look at this page (at the
bottom):

http://fedoraproject.org/wiki/Packaging/SourceURL

Please fix it and I'll review asap.
Comment 9 Ray Van Dolson 2008-04-29 11:57:02 EDT
(In reply to comment #8)

> *Full path to source tarball still missing. You must specify full path to
> source's tarball, not only %{name}-%{version}.tar.bz2. For SF-hosted projects we
> have special requirements for %Source field - take a look at this page (at the
> bottom):

Argh, meant to fix that last time. :-)  Updated SRPM here:

http://www.bludgeon.org/~rayvd/rpms/libyahoo2/libyahoo2-0.7.6-2.src.rpm

SPEC URL is the same.  Appreciate the catch on that.
Comment 10 Peter Lemenkov 2008-05-05 07:00:11 EDT
Remarks :

* You should remove *.a and *.la files from devel.
* UTF-8 conversion is wrong (located in rpmlint logs). Proper string will be:

   iconv -f iso8859-1 -t UTF8 AUTHORS > AUTHORS.utf8 && %{__mv} AUTHORS.utf8 AUTHORS
   iconv -f iso8859-1 -t UTF8 NEWS > NEWS.utf8 && %{__mv} NEWS.utf8 NEWS


Other things are looks OK.

So fix these remaining issues and I'll review it.
Comment 11 Ray Van Dolson 2008-05-13 20:06:53 EDT
Updated SRPM:

http://www.bludgeon.org/~rayvd/rpms/libyahoo2/libyahoo2-0.7.6-3.src.rpm
Comment 12 Peter Lemenkov 2008-05-14 05:43:46 EDT
Last minute fix - please add missing BuildRequiires: gtk2-devel

REVIEW

MUST Items:

+ rpmlint is silent.
+ The package is named according to the Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meet the Licensing
Guidelines.
+ The License field in the package spec file matches the actual license.
+ File, containing the text of the license(s) for the package is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.


- The sources used to build the package must match the upstream source.

[petro@localhost SOURCES]$ md5sum libyahoo2-0.7.6.tar.bz2*
9cb9a037506196bc370ba8d48698c4d8  libyahoo2-0.7.6.tar.bz2
3679c2e6a03a57c6f06414ca82b386eb  libyahoo2-0.7.6.tar.bz2.from_srpm

Please, use only upstreamed source from SF.

+ The package is successfully compiled and built into binary rpms on at least
one supported architecture.
+ All build dependencies (except of gtk2-devel, see above) are listed in
BuildRequires.
+ Package calls ldconfig in %post and %postun.
+ A package owns all directories that it creates. Personally, I'd like to
explicitly add as %dir in files-section for devel-package this line:

dir %{_includedir}/%{name}

and change this one 

%{_includedir}/%{name}/*

to that

%{_includedir}/*

but this only my personal favour, not a blocker.

+ A package does not contain any duplicate files in the %files listing.
+ Permissions on files are be set properly.
+ A package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
+ A package consistently uses macros, as described in the macros section of
Packaging Guidelines.
+ The package contains code, or permissable content.
+ Everything, a package includes as %doc, does not affect the runtime of the
application. 
+ Header files are in a -devel package.
+ This packages containing pkgconfig(.pc) files and it 'Requires: pkgconfig' 
+ A package contains library files with a suffix (e.g. libfoo.so.1.1), and
library files that end in .so (without suffix) are in a -devel package.
+ Devel package requires the base package using a fully versioned dependency:
Requires: %{name} = %{version}-%{release} 
+ Package is NOT contains any .la libtool archives.
+ Package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or
$RPM_BUILD_ROOT). See Prepping BuildRoot For %install for details.
+ All filenames in rpm packages are valid UTF-8.

SHOULD Items:

+ The pkgconfig(.pc) file is placed in a -devel pkg.

OK, please add missing BuildRequire, use exact tarball from SF.net, and this
package is

============
= APPROVED =
============

Don't forget to raise fedora-cvs flag to '?' then you'll be ready.
Comment 13 Peter Lemenkov 2008-05-24 14:45:40 EDT
Ping.
Comment 14 Ray Van Dolson 2008-05-27 10:07:06 EDT
I've updated the spec file, however I am confirming on the requirement of
gtk2-devel.  This one doesn't make sense to me. :-)  Going to try in mock.

Thanks.
Comment 15 Ray Van Dolson 2008-05-31 02:32:27 EDT
New Package CVS Request
=======================
Package Name: libyahoo2
Short Description: Library for the Yahoo! Messenger Protocol
Owners: rayvd
Branches: F-8 F-9 EL-4 EL-5
InitialCC:
Cvsextras Commits: yes
Comment 16 Kevin Fenzi 2008-05-31 19:16:25 EDT
cvs done.

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