Bug 249235 - Review Request: pidgin-knotify - KNotify plugin for Pidgin
Summary: Review Request: pidgin-knotify - KNotify plugin for Pidgin
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-07-23 05:53 UTC by Kelly Miller
Modified: 2007-11-30 22:12 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2007-08-28 04:02:32 UTC
Type: ---
Embargoed:
kevin: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Kelly Miller 2007-07-23 05:53:52 UTC
Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/pidgin-knotify.spec
SRPM URL: http://crystalsanctuary.rpgsource.net/packages/source/pidgin-knotify-0.1-1.fc7.src.rpm
Description: Pidgin-KNotify is a plugin for Pidgin that uses KNotify to do notifications. This is for those who like pidgin but don't like the lack of KDE integration.

Comment 1 manuel wolfshant 2007-07-23 09:05:26 UTC
snip from a mock build:

Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.40708
+ umask 022
+ cd /builddir/build/BUILD
+ cd pidgin-knotify-0.1
+ LANG=C
+ export LANG
+ unset DISPLAY
+ make
gcc -o knotify-plugin.so -shared knotify-plugin.c `pkg-config pidgin --cflags
--libs` -g -Wall
/bin/sh: pkg-config: command not found
knotify-plugin.c:16:18: error: glib.h: No such file or directory
knotify-plugin.c:17:26: error: glib/gprintf.h: No such file or directory
knotify-plugin.c:21:24: error: connection.h: No such file or directory
knotify-plugin.c:22:26: error: conversation.h: No such file or directory
knotify-plugin.c:23:18: error: core.h: No such file or directory
knotify-plugin.c:24:21: error: signals.h: No such file or directory
knotify-plugin.c:25:21: error: version.h: No such file or directory
knotify-plugin.c:26:20: error: status.h: No such file or directory
knotify-plugin.c:31: error: expected '=', ',', ';', 'asm' or '__attribute__'
before 'dcopmessage'
knotify-plugin.c:34: error: expected ')' before '*' token
knotify-plugin.c:49: error: expected ')' before '*' token
knotify-plugin.c:64: error: expected ')' before '*' token
knotify-plugin.c:71: error: expected '=', ',', ';', 'asm' or '__attribute__'
before 'plugin_load'
knotify-plugin.c:84: error: expected '=', ',', ';', 'asm' or '__attribute__'
before 'info'
knotify-plugin.c:115: error: expected ')' before '*' token
knotify-plugin.c:118: warning: data definition has no type or storage class
knotify-plugin.c:118: warning: type defaults to 'int' in declaration of
'PURPLE_INIT_PLUGIN'
knotify-plugin.c:118: warning: parameter names (without types) in function
declaration
make: *** [knotify-plugin.so] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.40708 (%build)



Comment 3 Kevin Fenzi 2007-08-05 23:34:05 UTC
I'd be happy to review this. Look for a full review in a bit. 

Comment 4 Kevin Fenzi 2007-08-05 23:35:36 UTC
Wrong flag. Fixing. 

Comment 5 Kevin Fenzi 2007-08-06 00:28:27 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License(LGPLv2)
See below - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
00c9a07cbe529e1375d2dc5ad7a3b1fb  knotify-plugin_0.1.tar.gz
00c9a07cbe529e1375d2dc5ad7a3b1fb  knotify-plugin_0.1.tar.gz.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
Ok - Package has a correct %clean section.
Ok - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock.
See below - Should build on all supported archs
OK - Should have dist tag
OK - Should package latest version

Issues:

1. You might make the summary a bit less harsh, perhaps just remove        
the second sentence, or just change it to "This is for KDE pidgin users".
Not a blocker, just a comment.

2. According to the new license rules the License tag here should be:
LGPLv2. You could ask upstream to include a copy in the package.

3. In your patch, you might make the cp do a 'cp -a' to preserve timestamps
on the plugin as it was built.

4. rpmlint says:
W: pidgin-knotify no-documentation

Can ignore.

W: pidgin-knotify invalid-license LGPL
W: pidgin-knotify invalid-license LGPL
W: pidgin-knotify-debuginfo invalid-license LGPL

Should be LGPLv2.

W: pidgin-knotify mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 10)

Fix if you get a chance, not a blocker.

5. Doesn't build here on x86_64... I get:

+ make -j2
gcc -o knotify-plugin.so -shared knotify-plugin.c `pkg-config pidgin --cflags
--libs` -g -Wall
/usr/bin/ld: /tmp/ccfkilnZ.o: relocation R_X86_64_32 against `a local symbol'
can not be used when making a shared object; recompile with -fPIC
/tmp/ccfkilnZ.o: could not read symbols: Bad value
collect2: ld returned 1 exit status
make: *** [knotify-plugin.so] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.66121 (%build)


Comment 7 Kevin Fenzi 2007-08-06 03:52:21 UTC
1. looks good. ok. 
2. looks good. ok. 
3. looks good. ok. 
4. looks good. ok. 
5. It still doesn't build on x86_64, but for a different reason: 
error: Installed (but unpackaged) file(s) found:
   /usr/lib/pidgin/knotify-plugin.so

Your patch has a hard coded /usr/lib in it. 

Looking at the compile options you should patch it to use rpm's normal optflags
if at all possible. 


Comment 8 Kelly Miller 2007-08-06 04:32:28 UTC
Updated.

Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/pidgin-knotify.spec
SRPM URL:
http://crystalsanctuary.rpgsource.net/packages/source/pidgin-knotify-0.1-4.fc7.src.rpm

I changed it to use a define at the beginning to find pidgin, which relies on
the libdir macro.  It should work fine, but this way it can be quickly fixed if
Pidgin is installed in another directory.

Comment 9 Kevin Fenzi 2007-08-06 05:20:33 UTC
ok, the package from comment #8 builds ok on x86_64. 
You still do need to make it honor the rpm optflags when compiling however. 
See: 
http://fedoraproject.org/wiki/Packaging/Guidelines#head-8b14098227aebff1cf6188939e9d0877295ac448

Thats the last blocker I see...

Comment 11 Kevin Fenzi 2007-08-09 02:06:36 UTC
ok, that looks good. I see no further blockers, so this package is APPROVED. 
Don't forget to close this request once it's been imported and built. 

Also, do consider reviewing another waiting package and helping out with the
reviewing load. 


Comment 12 Kelly Miller 2007-08-09 02:27:27 UTC
New Package CVS Request
=======================
Package Name: pidgin-knotify
Short Description: Pidgin plugin to use KNotify
Owners: lightsolphoenix
Branches: FC-6 F-7
InitialCC:
Cvsextras Commits: yes

Comment 13 Kevin Fenzi 2007-08-09 03:26:07 UTC
cvs done.

Comment 14 Kevin Fenzi 2007-08-28 04:02:32 UTC
This package appears to have been imported and built. 
Closing it now. 



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