Bug 194481 - Review Request: eggdrop
Review Request: eggdrop
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael J Knox
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-06-08 09:47 EDT by Robert Scheck
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-06-26 16:56:35 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Robert Scheck 2006-06-08 09:47:06 EDT
Spec URL: http://labs.linuxnetz.de/bugzilla/eggdrop.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/eggdrop-1.6.17-3.src.rpm
Description: Eggdrop is the world's most popular Open Source IRC bot,
designed for flexibility and ease of use. It is extendable with Tcl
scripts and/or C modules, has support for the big five IRC networks
and is able to form botnets, share partylines and userfiles between
bots.
Comment 1 Jima 2006-06-14 10:50:49 EDT
Looks like this came in right before the last backup before the disk crash. I'm
re-adding the CCs I saw added to this bug; I'm quite interested in seeing this
package get into Extras (I maintain a copy on the side).

To summarize what happened (to any roving sponsors), Christopher Stone suggested
a few changes, and Robert applied them.

I suspect you'd be well-off removing the Requires: line, as it's technically
redundant. Rpmbuild should (and, ultimately, is) able to resolve what the end
package's dependencies are. I spun a test built without it and the resultant
package depended on (among others):
libdns.so.21 (which is provided by bind-lib)
libtcl8.4.so (tcl)
libz.so.1 (zlib)

Anything else I see...nothing non-trivial. I'd personally change line 37 to "#
Move modules into /usr/lib*" for honesty's sake. ;-)

Not a full review, but then, I'm not a sponsor (sorry).
Comment 2 Robert Scheck 2006-06-14 11:05:30 EDT
Thanks, your suggestions were also applied as they are reasonable.

Any non-x86_32 rpmlint outputs (especially of x86_64) would be very interesting 
to me, to see whether there are any rpath issues. On x86_32, I've got no rpmlint 
outputs any longer...
Comment 3 Jima 2006-06-14 11:56:39 EDT
The binary packages on ppc and sparc have no rpmlint output. I was going to tell
you about the rpmlint output from the debuginfo RPMs, but it looks like you
added src/mod/transfer.mod/*.c to your chmod -x line, which should resolve that.
One step ahead of me on that. :-)

Throwing the current incarnation into my Plague system... *taps foot for a
while* ...done. No rpmlint errors.
Comment 4 Hans de Goede 2006-06-17 08:43:43 EDT
Notice that Robert has been sponsored now, so anyone who wants to see this
package into FE can help getting it there be doing a (full) review.
Comment 5 Michael J Knox 2006-06-17 18:46:12 EDT
Builds fine on i386 development with mock and its minimal build. 

Review for release 3:
* RPM name is OK
* Source eggdrop1.6.17.tar.gz is the same as upstream
* This is the latest version
* Builds fine in mock
* rpmlint looks OK
* File list looks OK

However.. I installed it try and use it, now I might be a complete nub, but I
get this error:

[mjk@localhost ~]$ eggdrop -m eggdrop.conf
[10:52] LANG: No lang files found for section core.

Eggdrop v1.6.17 (C) 1997 Robey Pointer (C) 2004 Eggheads
[10:52] --- Loading eggdrop v1.6.17 (Sun Jun 18 2006)
[10:52] * Please make sure you edit your config file completely.
[mjk@localhost ~]$

I have, addlang "English", in my config. so I am not sure if this is a broken
package or a broken config. 




Comment 6 Michael J Knox 2006-06-17 18:54:24 EDT
OK, you are probably going to need to have a wrapper script for executing
eggdrop itself, as this needs to be done:

[mjk@localhost ~]$ EGG_LANGDIR=/usr/share/eggdrop/language eggdrop 

To get eggdrop to start working as expected. 
Comment 7 Robert Scheck 2006-06-17 18:56:22 EDT
Yepp, you're right. But I'll have a look into this, maybe a default directory 
can be specified in the configuration file.
Comment 8 Robert Scheck 2006-06-17 19:15:27 EDT
Unfortunately the language path is hardcoded, the issue could be resolved this 
way (when accepted). Simultaneously I could open an upstream bug report to get 
something like a lang-path directive...

--- eggdrop1.6.17/src/eggdrop.h       2004-07-25 13:17:34.000000000 +0200
+++ eggdrop1.6.17/src/eggdrop.h.rsc   2006-06-18 01:19:23.000000000 +0200
@@ -75,7 +75,7 @@


 /* Language stuff */
-#define LANGDIR  "./language" /* language file directory                   */
+#define LANGDIR  "/usr/share/eggdrop/language"  /* language file directory */
 #define BASELANG "english"    /* language which always gets loaded before
                                  all other languages. You do not want to
                                  change this.                              */
Comment 9 Michael J Knox 2006-06-17 19:20:02 EDT
I am happy for that patch to be used, it should not create any issues. Not sure
if upstream will go for it, but there is no harm in trying :) 

If you want to package it up again, I will do a final review. 
Comment 10 Robert Scheck 2006-06-17 20:19:30 EDT
Package was updated...
Comment 11 Michael J Knox 2006-06-17 23:29:01 EDT
I don't see a release 4 making use of the patch. 
Comment 12 Robert Scheck 2006-06-18 10:00:12 EDT
I didn't bump version last night, but was updated anyway:

-rw-r--r-- 1 robert fedora 1028579 Jun 18 02:00 eggdrop-1.6.17-3.src.rpm
-rw-r--r-- 1 robert fedora    2344 Jun 18 02:00 eggdrop.spec
Comment 13 Michael J Knox 2006-06-18 20:01:10 EDT
But you have not added the patch to make eggdrop function out of the box with
the language path being /usr/share/eggdrop/language. 
Comment 14 Robert Scheck 2006-06-18 20:05:52 EDT
Sorry Michael, but the patch *was* added to eggdrop-1.6.17-conf.patch...
Comment 15 Michael J Knox 2006-06-18 20:08:12 EDT
Contents of Patch0:

Patch by Robert Scheck <robert@fedoraproject.org> for eggdrop >= 1.6.17,
which sets the correct path to the eggdrop executable in the example file.

--- eggdrop1.6.17/eggdrop.conf		2004-08-22 00:43:27.000000000 +0200
+++ eggdrop1.6.17/eggdrop.conf.conf	2006-01-22 00:16:16.000000000 +0100
@@ -1,5 +1,4 @@
-#! /path/to/executable/eggdrop
-# ^- This should contain a fully qualified path to your Eggdrop executable.
+#!/usr/bin/eggdrop
 #
 # $Id: eggdrop.conf,v 1.40 2004/08/21 22:43:27 wcc Exp $
 #


This does contain the diff as you showed in comment 8. 
Comment 16 Robert Scheck 2006-06-18 20:22:12 EDT
You must be joking - verify the md5sums, please. And now I did only for you a 
rebuild and ensured that there *is* the whole patch: eggdrop-1.6.17-3.1.src.rpm
Comment 17 Michael J Knox 2006-06-18 20:35:42 EDT
Robert. Firstly, I would not "joke" like that in a review. in release 3, the
patch is incorrect. 

Secondly, you should review the package naming guidelines, as it clearly states
in there this:

----

Package Release

In the past, Fedora.us used 0.fdr as a release prefix to identify Fedora.us
packages. In Fedora, this repository "tagging" is unnecessary, and should not be
used. The release number (referred to in some older documentation as a "vepoch")
is how the maintainer marks build revisions, starting from 1. When a minor
change (spec file changed, patch added/removed) occurs, or a package is rebuilt
to use newer headers or libraries, the release number should be incremented. If
a major change (new version of the software being packaged) occurs, the version
number should be changed to reflect the new software version, and the release
number should be reset to 1. 

----

Following this practise is required when a package is accepted in to Extras, a
review should be considered no different. 

Thirdly, you have not updated the changelog in the SPEC to reflect that you have
made a patch to the source. Accurate changelogs are important. 

Lastly, the patch file's name incorrectly represents the change now that you
have added a source code patch to it. 

My suggestion is that you make a eggdrop-1.6.17-4.src.rpm, with a changelog
entry reflecting the new patch and you rename the patch so it does not mislead
other people doing a quick overview as to what the patch is doing. 

Comment 18 Robert Scheck 2006-06-18 20:58:16 EDT
eggdrop-1.6.17-4.src.rpm
Comment 19 Michael J Knox 2006-06-21 18:23:12 EDT
Thank you. You can consider this APPROVED. 

Please close this bug report with NEXTRELEASE once imported into CVS.
Comment 20 Robert Scheck 2006-06-26 16:56:35 EDT
11618 (eggdrop): Build on target fedora-development-extras succeeded.
11617 (eggdrop): Build on target fedora-5-extras succeeded.
11616 (eggdrop): Build on target fedora-4-extras succeeded.

as per http://fedoraproject.org/wiki/Extras/Contributors I'll close this
bug report with NEXTRELEASE now. 

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