Red Hat Bugzilla – Bug 249235
Review Request: pidgin-knotify - KNotify plugin for Pidgin
Last modified: 2007-11-30 17:12:11 EST
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.
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)
Fixed. Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/pidgin-knotify.spec SRPM URL: http://crystalsanctuary.rpgsource.net/packages/source/pidgin-knotify-0.1-2.fc7.src.rpm
I'd be happy to review this. Look for a full review in a bit.
Wrong flag. Fixing.
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)
Okay, updated a few things. Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/pidgin-knotify.spec SRPM URL: http://crystalsanctuary.rpgsource.net/packages/source/pidgin-knotify-0.1-3.fc7.src.rpm
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.
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.
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...
Done. Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/pidgin-knotify.spec SRPM URL: http://crystalsanctuary.rpgsource.net/packages/source/pidgin-knotify-0.1-5.fc7.src.rpm
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.
New Package CVS Request ======================= Package Name: pidgin-knotify Short Description: Pidgin plugin to use KNotify Owners: lightsolphoenix@gmail.com Branches: FC-6 F-7 InitialCC: Cvsextras Commits: yes
cvs done.
This package appears to have been imported and built. Closing it now.